| Message ID | 20230713060729.390027-1-yhs@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Support new insns from cpu v4 | expand |
On Wed, 2023-07-12 at 23:07 -0700, Yonghong Song wrote: > > In uapi bpf.h, there are two ctx structures which contain > > signed members. Without cpu v4, such signed members will > > be loaded with unsigned load and the compiler will generate > > proper left and right shifts to get the proper final value. > > > > With sign-extension load, however, left/right shifts are gone, > > we need to ensure these signed members are properly handled, > > with signed loads or other means. The following are list of > > signed ctx members and how they are handled. This is not a generic approach, in theory any field could be cast as a signed integer. Do we want to support this? If so, then it should be handled in convert_ctx_access() by generating additional sign extension instructions. > > > > (1). > > struct bpf_sock { > > ... > > __s32 rx_queue_mapping; > > } > > > > The corresponding kernel fields are > > struct sock_common { > > ... > > unsigned short skc_rx_queue_mapping; > > ... > > } > > > > Current ctx rewriter uses unsigned load for the kernel field > > which is correct and does not need further handling. > > > > (2). > > struct bpf_sockopt { > > ... > > __s32 level; > > __s32 optname; > > __s32 optlen; > > __s32 retval; > > } > > The level/optname/optlen are from struct bpf_sockopt_kern > > struct bpf_sockopt_kern { > > ... > > s32 level; > > s32 optname; > > s32 optlen; > > ... > > } > > and the 'retval' is from struct bpf_cg_run_ctx > > struct bpf_cg_run_ctx { > > ... > > int retval; > > } > > Current the above four fields are loaded with unsigned load. > > Let us modify the read macro for bpf_sockopt which use > > the same signedness for the original insn. > > > > Signed-off-by: Yonghong Song <yhs@fb.com> > > --- > > kernel/bpf/cgroup.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c > > index 5b2741aa0d9b..29e3606ff6f4 100644 > > --- a/kernel/bpf/cgroup.c > > +++ b/kernel/bpf/cgroup.c > > @@ -2397,9 +2397,10 @@ static bool cg_sockopt_is_valid_access(int off, int size, > > } > > > > #define CG_SOCKOPT_READ_FIELD(F) \ > > - BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct bpf_sockopt_kern, F), \ > > + BPF_RAW_INSN((BPF_FIELD_SIZEOF(struct bpf_sockopt_kern, F) | \ > > + BPF_MODE(si->code) | BPF_CLASS(si->code)), \ > > si->dst_reg, si->src_reg, \ > > - offsetof(struct bpf_sockopt_kern, F)) > > + offsetof(struct bpf_sockopt_kern, F), si->imm) > > > > #define CG_SOCKOPT_WRITE_FIELD(F) \ > > BPF_RAW_INSN((BPF_FIELD_SIZEOF(struct bpf_sockopt_kern, F) | \ > > @@ -2456,7 +2457,7 @@ static u32 cg_sockopt_convert_ctx_access(enum bpf_access_type type, > > *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct task_struct, bpf_ctx), > > treg, treg, > > offsetof(struct task_struct, bpf_ctx)); > > - *insn++ = BPF_RAW_INSN(BPF_CLASS(si->code) | BPF_MEM | > > + *insn++ = BPF_RAW_INSN(BPF_CLASS(si->code) | BPF_MODE(si->code) | > > BPF_FIELD_SIZEOF(struct bpf_cg_run_ctx, retval), > > treg, si->src_reg, > > offsetof(struct bpf_cg_run_ctx, retval), > > @@ -2470,9 +2471,10 @@ static u32 cg_sockopt_convert_ctx_access(enum bpf_access_type type, > > *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct task_struct, bpf_ctx), > > si->dst_reg, si->dst_reg, > > offsetof(struct task_struct, bpf_ctx)); > > - *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct bpf_cg_run_ctx, retval), > > - si->dst_reg, si->dst_reg, > > - offsetof(struct bpf_cg_run_ctx, retval)); > > + *insn++ = BPF_RAW_INSN((BPF_FIELD_SIZEOF(struct bpf_cg_run_ctx, retval) | > > + BPF_MODE(si->code) | BPF_CLASS(si->code)), > > + si->dst_reg, si->dst_reg, > > + offsetof(struct bpf_cg_run_ctx, retval), si->imm); > > } > > break; > > case offsetof(struct bpf_sockopt, optval):
On 7/16/23 6:40 PM, Eduard Zingerman wrote: > On Wed, 2023-07-12 at 23:07 -0700, Yonghong Song wrote: >>> In uapi bpf.h, there are two ctx structures which contain >>> signed members. Without cpu v4, such signed members will >>> be loaded with unsigned load and the compiler will generate >>> proper left and right shifts to get the proper final value. >>> >>> With sign-extension load, however, left/right shifts are gone, >>> we need to ensure these signed members are properly handled, >>> with signed loads or other means. The following are list of >>> signed ctx members and how they are handled. > > This is not a generic approach, in theory any field could > be cast as a signed integer. Do we want to support this? > If so, then it should be handled in convert_ctx_access() > by generating additional sign extension instructions. Good point. I will make change in verifier.c which is more generic and cover more cases. > >>> >>> (1). >>> struct bpf_sock { >>> ... >>> __s32 rx_queue_mapping; >>> } >>> >>> The corresponding kernel fields are >>> struct sock_common { >>> ... >>> unsigned short skc_rx_queue_mapping; >>> ... >>> } >>> >>> Current ctx rewriter uses unsigned load for the kernel field >>> which is correct and does not need further handling. >>> >>> (2). >>> struct bpf_sockopt { >>> ... >>> __s32 level; >>> __s32 optname; >>> __s32 optlen; >>> __s32 retval; >>> } >>> The level/optname/optlen are from struct bpf_sockopt_kern >>> struct bpf_sockopt_kern { >>> ... >>> s32 level; >>> s32 optname; >>> s32 optlen; >>> ... >>> } >>> and the 'retval' is from struct bpf_cg_run_ctx >>> struct bpf_cg_run_ctx { >>> ... >>> int retval; >>> } >>> Current the above four fields are loaded with unsigned load. >>> Let us modify the read macro for bpf_sockopt which use >>> the same signedness for the original insn. >>> >>> Signed-off-by: Yonghong Song <yhs@fb.com> >>> --- >>> kernel/bpf/cgroup.c | 14 ++++++++------ >>> 1 file changed, 8 insertions(+), 6 deletions(-) >>> >>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c >>> index 5b2741aa0d9b..29e3606ff6f4 100644 >>> --- a/kernel/bpf/cgroup.c >>> +++ b/kernel/bpf/cgroup.c >>> @@ -2397,9 +2397,10 @@ static bool cg_sockopt_is_valid_access(int off, int size, >>> } >>> >>> #define CG_SOCKOPT_READ_FIELD(F) \ >>> - BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct bpf_sockopt_kern, F), \ >>> + BPF_RAW_INSN((BPF_FIELD_SIZEOF(struct bpf_sockopt_kern, F) | \ >>> + BPF_MODE(si->code) | BPF_CLASS(si->code)), \ >>> si->dst_reg, si->src_reg, \ >>> - offsetof(struct bpf_sockopt_kern, F)) >>> + offsetof(struct bpf_sockopt_kern, F), si->imm) >>> >>> #define CG_SOCKOPT_WRITE_FIELD(F) \ >>> BPF_RAW_INSN((BPF_FIELD_SIZEOF(struct bpf_sockopt_kern, F) | \ >>> @@ -2456,7 +2457,7 @@ static u32 cg_sockopt_convert_ctx_access(enum bpf_access_type type, >>> *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct task_struct, bpf_ctx), >>> treg, treg, >>> offsetof(struct task_struct, bpf_ctx)); >>> - *insn++ = BPF_RAW_INSN(BPF_CLASS(si->code) | BPF_MEM | >>> + *insn++ = BPF_RAW_INSN(BPF_CLASS(si->code) | BPF_MODE(si->code) | >>> BPF_FIELD_SIZEOF(struct bpf_cg_run_ctx, retval), >>> treg, si->src_reg, >>> offsetof(struct bpf_cg_run_ctx, retval), >>> @@ -2470,9 +2471,10 @@ static u32 cg_sockopt_convert_ctx_access(enum bpf_access_type type, >>> *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct task_struct, bpf_ctx), >>> si->dst_reg, si->dst_reg, >>> offsetof(struct task_struct, bpf_ctx)); >>> - *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct bpf_cg_run_ctx, retval), >>> - si->dst_reg, si->dst_reg, >>> - offsetof(struct bpf_cg_run_ctx, retval)); >>> + *insn++ = BPF_RAW_INSN((BPF_FIELD_SIZEOF(struct bpf_cg_run_ctx, retval) | >>> + BPF_MODE(si->code) | BPF_CLASS(si->code)), >>> + si->dst_reg, si->dst_reg, >>> + offsetof(struct bpf_cg_run_ctx, retval), si->imm); >>> } >>> break; >>> case offsetof(struct bpf_sockopt, optval): >
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c index 5b2741aa0d9b..29e3606ff6f4 100644 --- a/kernel/bpf/cgroup.c +++ b/kernel/bpf/cgroup.c @@ -2397,9 +2397,10 @@ static bool cg_sockopt_is_valid_access(int off, int size, } #define CG_SOCKOPT_READ_FIELD(F) \ - BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct bpf_sockopt_kern, F), \ + BPF_RAW_INSN((BPF_FIELD_SIZEOF(struct bpf_sockopt_kern, F) | \ + BPF_MODE(si->code) | BPF_CLASS(si->code)), \ si->dst_reg, si->src_reg, \ - offsetof(struct bpf_sockopt_kern, F)) + offsetof(struct bpf_sockopt_kern, F), si->imm) #define CG_SOCKOPT_WRITE_FIELD(F) \ BPF_RAW_INSN((BPF_FIELD_SIZEOF(struct bpf_sockopt_kern, F) | \ @@ -2456,7 +2457,7 @@ static u32 cg_sockopt_convert_ctx_access(enum bpf_access_type type, *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct task_struct, bpf_ctx), treg, treg, offsetof(struct task_struct, bpf_ctx)); - *insn++ = BPF_RAW_INSN(BPF_CLASS(si->code) | BPF_MEM | + *insn++ = BPF_RAW_INSN(BPF_CLASS(si->code) | BPF_MODE(si->code) | BPF_FIELD_SIZEOF(struct bpf_cg_run_ctx, retval), treg, si->src_reg, offsetof(struct bpf_cg_run_ctx, retval), @@ -2470,9 +2471,10 @@ static u32 cg_sockopt_convert_ctx_access(enum bpf_access_type type, *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct task_struct, bpf_ctx), si->dst_reg, si->dst_reg, offsetof(struct task_struct, bpf_ctx)); - *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct bpf_cg_run_ctx, retval), - si->dst_reg, si->dst_reg, - offsetof(struct bpf_cg_run_ctx, retval)); + *insn++ = BPF_RAW_INSN((BPF_FIELD_SIZEOF(struct bpf_cg_run_ctx, retval) | + BPF_MODE(si->code) | BPF_CLASS(si->code)), + si->dst_reg, si->dst_reg, + offsetof(struct bpf_cg_run_ctx, retval), si->imm); } break; case offsetof(struct bpf_sockopt, optval):
In uapi bpf.h, there are two ctx structures which contain signed members. Without cpu v4, such signed members will be loaded with unsigned load and the compiler will generate proper left and right shifts to get the proper final value. With sign-extension load, however, left/right shifts are gone, we need to ensure these signed members are properly handled, with signed loads or other means. The following are list of signed ctx members and how they are handled. (1). struct bpf_sock { ... __s32 rx_queue_mapping; } The corresponding kernel fields are struct sock_common { ... unsigned short skc_rx_queue_mapping; ... } Current ctx rewriter uses unsigned load for the kernel field which is correct and does not need further handling. (2). struct bpf_sockopt { ... __s32 level; __s32 optname; __s32 optlen; __s32 retval; } The level/optname/optlen are from struct bpf_sockopt_kern struct bpf_sockopt_kern { ... s32 level; s32 optname; s32 optlen; ... } and the 'retval' is from struct bpf_cg_run_ctx struct bpf_cg_run_ctx { ... int retval; } Current the above four fields are loaded with unsigned load. Let us modify the read macro for bpf_sockopt which use the same signedness for the original insn. Signed-off-by: Yonghong Song <yhs@fb.com> --- kernel/bpf/cgroup.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)