Re: [PATCH bpf-next v2 02/15] bpf: Fix sign-extension ctx member accesses

From: Eduard Zingerman <eddyz87@gmail.com>
To: Yonghong Song <yhs@fb.com>, bpf@vger.kernel.org
Cc: Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	 Daniel Borkmann <daniel@iogearbox.net>,
	Fangrui Song <maskray@google.com>,
	kernel-team@fb.com
Subject: Re: [PATCH bpf-next v2 02/15] bpf: Fix sign-extension ctx member accesses
Date: Mon, 17 Jul 2023 04:40:07 +0300	[thread overview]
Message-ID: <a3fe0382a39a5ac462d071b5c6ca3415ade16939.camel@gmail.com> (raw)
In-Reply-To: <20230713060729.390027-1-yhs@fb.com>

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