Re: [PATCH bpf-next v2 10/25] bpf: Introduce local kptrs

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Wed, Oct 19, 2022 at 10:45:22PM IST, Dave Marchevsky wrote:
> On 10/13/22 2:22 AM, Kumar Kartikeya Dwivedi wrote:
> > Introduce the idea of local kptrs, i.e. PTR_TO_BTF_ID that point to a
> > type in program BTF. This is indicated by the presence of MEM_TYPE_LOCAL
> > type tag in reg->type to avoid having to check btf_is_kernel when trying
> > to match argument types in helpers.
> >
> > For now, these local kptrs will always be referenced in verifier
> > context, hence ref_obj_id == 0 for them is a bug. It is allowed to write
> > to such objects, as long fields that are special are not touched
> > (support for which will be added in subsequent patches).
> >
> > No PROBE_MEM handling is hence done since they can never be in an
> > undefined state, and their lifetime will always be valid.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  include/linux/bpf.h              | 14 +++++++++++---
> >  include/linux/filter.h           |  4 +++-
> >  kernel/bpf/btf.c                 |  9 ++++++++-
> >  kernel/bpf/verifier.c            | 15 ++++++++++-----
> >  net/bpf/bpf_dummy_struct_ops.c   |  3 ++-
> >  net/core/filter.c                | 13 ++++++++-----
> >  net/ipv4/bpf_tcp_ca.c            |  3 ++-
> >  net/netfilter/nf_conntrack_bpf.c |  1 +
> >  8 files changed, 45 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 46330d871d4e..a2f4d3356cc8 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -526,6 +526,11 @@ enum bpf_type_flag {
> >  	/* Size is known at compile time. */
> >  	MEM_FIXED_SIZE		= BIT(10 + BPF_BASE_TYPE_BITS),
> >
> > +	/* MEM is of a type from program BTF, not kernel BTF. This is used to
> > +	 * tag PTR_TO_BTF_ID allocated using bpf_kptr_alloc.
> > +	 */
> > +	MEM_TYPE_LOCAL		= BIT(11 + BPF_BASE_TYPE_BITS),
> > +
> >  	__BPF_TYPE_FLAG_MAX,
> >  	__BPF_TYPE_LAST_FLAG	= __BPF_TYPE_FLAG_MAX - 1,
> >  };
> > @@ -774,6 +779,7 @@ struct bpf_prog_ops {
> >  			union bpf_attr __user *uattr);
> >  };
> >
> > +struct bpf_reg_state;
> >  struct bpf_verifier_ops {
> >  	/* return eBPF function prototype for verification */
> >  	const struct bpf_func_proto *
> > @@ -795,6 +801,7 @@ struct bpf_verifier_ops {
> >  				  struct bpf_insn *dst,
> >  				  struct bpf_prog *prog, u32 *target_size);
> >  	int (*btf_struct_access)(struct bpf_verifier_log *log,
> > +				 const struct bpf_reg_state *reg,
>
> Not that struct_ops API is meant to be stable, but would be good to note that
> this changes that API in the summary.
>

Ack, will do.

> On that note, maybe passing whole bpf_reg_state *reg can be avoided for now
> by making this a 'bool disallow_ptr_walk' or similar, since that's the only
> thing this patch is using it for.
>

I did this in the RFC version, with bool local_type, but Alexei asked me to drop
it and just pass the register in. But more on that below...

> >  				 const struct btf *btf,
> >  				 const struct btf_type *t, int off, int size,
> >  				 enum bpf_access_type atype,
> > @@ -2076,10 +2083,11 @@ static inline bool bpf_tracing_btf_ctx_access(int off, int size,
> >  	return btf_ctx_access(off, size, type, prog, info);
> >  }
> >
> > -int btf_struct_access(struct bpf_verifier_log *log, const struct btf *btf,
> > +int btf_struct_access(struct bpf_verifier_log *log,
> > +		      const struct bpf_reg_state *reg, const struct btf *btf,
> >  		      const struct btf_type *t, int off, int size,
> > -		      enum bpf_access_type atype,
> > -		      u32 *next_btf_id, enum bpf_type_flag *flag);
> > +		      enum bpf_access_type atype, u32 *next_btf_id,
> > +		      enum bpf_type_flag *flag);
> >  bool btf_struct_ids_match(struct bpf_verifier_log *log,
> >  			  const struct btf *btf, u32 id, int off,
> >  			  const struct btf *need_btf, u32 need_type_id,
> > diff --git a/include/linux/filter.h b/include/linux/filter.h
> > index efc42a6e3aed..9b94e24f90b9 100644
> > --- a/include/linux/filter.h
> > +++ b/include/linux/filter.h
> > @@ -568,7 +568,9 @@ struct sk_filter {
> >  DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
> >
> >  extern struct mutex nf_conn_btf_access_lock;
> > -extern int (*nfct_btf_struct_access)(struct bpf_verifier_log *log, const struct btf *btf,
> > +extern int (*nfct_btf_struct_access)(struct bpf_verifier_log *log,
> > +				     const struct bpf_reg_state *reg,
> > +				     const struct btf *btf,
> >  				     const struct btf_type *t, int off, int size,
> >  				     enum bpf_access_type atype, u32 *next_btf_id,
> >  				     enum bpf_type_flag *flag);
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 066984d73a8b..65f444405d9c 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6019,11 +6019,13 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
> >  	return -EINVAL;
> >  }
> >
> > -int btf_struct_access(struct bpf_verifier_log *log, const struct btf *btf,
> > +int btf_struct_access(struct bpf_verifier_log *log,
> > +		      const struct bpf_reg_state *reg, const struct btf *btf,
> >  		      const struct btf_type *t, int off, int size,
> >  		      enum bpf_access_type atype __maybe_unused,
> >  		      u32 *next_btf_id, enum bpf_type_flag *flag)
> >  {
> > +	bool local_type = reg && (type_flag(reg->type) & MEM_TYPE_LOCAL);
> >  	enum bpf_type_flag tmp_flag = 0;
> >  	int err;
> >  	u32 id;
> > @@ -6033,6 +6035,11 @@ int btf_struct_access(struct bpf_verifier_log *log, const struct btf *btf,
> >
> >  		switch (err) {
> >  		case WALK_PTR:
> > +			/* For local types, the destination register cannot
> > +			 * become a pointer again.
> > +			 */
> > +			if (local_type)
> > +				return SCALAR_VALUE;
> >  			/* If we found the pointer or scalar on t+off,
> >  			 * we're done.
> >  			 */
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 3c47cecda302..6ee8c06c2080 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -4522,16 +4522,20 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
> >  		return -EACCES;
> >  	}
> >
> > -	if (env->ops->btf_struct_access) {
> > -		ret = env->ops->btf_struct_access(&env->log, reg->btf, t,
> > +	if (env->ops->btf_struct_access && !(type_flag(reg->type) & MEM_TYPE_LOCAL)) {
> > +		WARN_ON_ONCE(!btf_is_kernel(reg->btf));
> > +		ret = env->ops->btf_struct_access(&env->log, reg, reg->btf, t,
> >  						  off, size, atype, &btf_id, &flag);
> >  	} else {
> > -		if (atype != BPF_READ) {
> > +		if (atype != BPF_READ && !(type_flag(reg->type) & MEM_TYPE_LOCAL)) {
> >  			verbose(env, "only read is supported\n");
> >  			return -EACCES;
> >  		}
> >
> > -		ret = btf_struct_access(&env->log, reg->btf, t, off, size,
> > +		if (reg->type & MEM_TYPE_LOCAL)
> > +			WARN_ON_ONCE(!reg->ref_obj_id);
>
> Can we instead verbose(env, ...) and return error? Then when someone tries to
> add local kptrs that don't set ref_obj_id in the future, it'll be more obvious
> that this wasn't explicitly supported and they need to check verifier logic
> carefully. Also rest of check_ptr_to_btf_access checks do verbose + err.
>
> Similar for btf_is_kernel WARN above.
>

Ack.

> > +
> > +		ret = btf_struct_access(&env->log, reg, reg->btf, t, off, size,
>
>
> more re: passing entire reg state to btf_struct access:
>
> In the next patch in the series ("bpf: Recognize bpf_{spin_lock,list_head,list_node} in local kptrs")
> you do btf_find_struct_meta(btf, reg->btf_id). I see why you couldn't use 't'
> that's passed in here / elsewhere since you need the btf_id for meta lookup.
> Perhaps 'btf_type *t' param can be changed to btf_id, eliminating the need
> to pass 'reg'.
>
> Alternatively, since we're already passing reg->btf and result of
> btf_type_by_id(reg->btf, reg->btf_id), seems like btf_struct_access
> maybe is tied closely enough to reg state that passing reg state
> directly and getting rid of extraneous args is cleaner.
>

So Alexei actually suggested dropping both btf and type arguments and simply
pass in the register and get it from there.

But one call site threw a wrench in the plan:

check_ptr_to_map_access -> btf_struct_access

Here, it passes it's own btf and type to simulate access to a map. Maybe I
should be creating a dummy register on stack and make it work like that for this
particular case? Otherwise all other callers pass in what they have from reg.