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@xxxxxx>
> > ---
> > 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):
