| Message ID | 20221013062303.896469-11-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Local kptrs, BPF linked lists | expand |
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. 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. > 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. > + > + 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. > atype, &btf_id, &flag); > } > > @@ -4596,7 +4600,7 @@ static int check_ptr_to_map_access(struct bpf_verifier_env *env, > return -EACCES; > } > > - ret = btf_struct_access(&env->log, btf_vmlinux, t, off, size, atype, &btf_id, &flag); > + ret = btf_struct_access(&env->log, NULL, btf_vmlinux, t, off, size, atype, &btf_id, &flag); > if (ret < 0) > return ret; > > @@ -5816,6 +5820,7 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, > * fixed offset. > */ > case PTR_TO_BTF_ID: > + case PTR_TO_BTF_ID | MEM_TYPE_LOCAL: > /* When referenced PTR_TO_BTF_ID is passed to release function, > * it's fixed offset must be 0. In the other cases, fixed offset > * can be non-zero. > diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c > index e78dadfc5829..d7aa636d90ce 100644 > --- a/net/bpf/bpf_dummy_struct_ops.c > +++ b/net/bpf/bpf_dummy_struct_ops.c > @@ -156,6 +156,7 @@ static bool bpf_dummy_ops_is_valid_access(int off, int size, > } > > static int bpf_dummy_ops_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, > @@ -177,7 +178,7 @@ static int bpf_dummy_ops_btf_struct_access(struct bpf_verifier_log *log, > return -EACCES; > } > > - err = btf_struct_access(log, btf, t, off, size, atype, next_btf_id, > + err = btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id, > flag); > if (err < 0) > return err; > diff --git a/net/core/filter.c b/net/core/filter.c > index bb0136e7a8e4..cc7af7be91d9 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -8647,13 +8647,15 @@ static bool tc_cls_act_is_valid_access(int off, int size, > DEFINE_MUTEX(nf_conn_btf_access_lock); > EXPORT_SYMBOL_GPL(nf_conn_btf_access_lock); > > -int (*nfct_btf_struct_access)(struct bpf_verifier_log *log, const struct btf *btf, > +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); > EXPORT_SYMBOL_GPL(nfct_btf_struct_access); > > static int tc_cls_act_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, > @@ -8663,12 +8665,12 @@ static int tc_cls_act_btf_struct_access(struct bpf_verifier_log *log, > int ret = -EACCES; > > if (atype == BPF_READ) > - return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, > + return btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id, > flag); > > mutex_lock(&nf_conn_btf_access_lock); > if (nfct_btf_struct_access) > - ret = nfct_btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag); > + ret = nfct_btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id, flag); > mutex_unlock(&nf_conn_btf_access_lock); > > return ret; > @@ -8734,6 +8736,7 @@ void bpf_warn_invalid_xdp_action(struct net_device *dev, struct bpf_prog *prog, > EXPORT_SYMBOL_GPL(bpf_warn_invalid_xdp_action); > > static int xdp_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, > @@ -8743,12 +8746,12 @@ static int xdp_btf_struct_access(struct bpf_verifier_log *log, > int ret = -EACCES; > > if (atype == BPF_READ) > - return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, > + return btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id, > flag); > > mutex_lock(&nf_conn_btf_access_lock); > if (nfct_btf_struct_access) > - ret = nfct_btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag); > + ret = nfct_btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id, flag); > mutex_unlock(&nf_conn_btf_access_lock); > > return ret; > diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c > index 6da16ae6a962..1fe3935c4260 100644 > --- a/net/ipv4/bpf_tcp_ca.c > +++ b/net/ipv4/bpf_tcp_ca.c > @@ -69,6 +69,7 @@ static bool bpf_tcp_ca_is_valid_access(int off, int size, > } > > static int bpf_tcp_ca_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, > @@ -78,7 +79,7 @@ static int bpf_tcp_ca_btf_struct_access(struct bpf_verifier_log *log, > size_t end; > > if (atype == BPF_READ) > - return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, > + return btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id, > flag); > > if (t != tcp_sock_type) { > diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c > index 8639e7efd0e2..f6036a84484b 100644 > --- a/net/netfilter/nf_conntrack_bpf.c > +++ b/net/netfilter/nf_conntrack_bpf.c > @@ -191,6 +191,7 @@ BTF_ID(struct, nf_conn___init) > > /* Check writes into `struct nf_conn` */ > static int _nf_conntrack_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,
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.
On 10/19/22 8:48 PM, Kumar Kartikeya Dwivedi wrote: > 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> >>> --- [...] >> >> >> 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. Ah, sorry for missing that. Personally I'm not a fan of dummy register on the stack. Then if btf_struct_access starts using some reg state that wasn't populated in the dummy reg it will be confusing.
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> > --- One nit unrelated to the other thread we have going for this patch. [...] > 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); Can you add a type_is_local_kptr helper (or similar name) to reduce this type_flag(reg->type) & MEM_TYPE_LOCAL repetition here and elsewhere in the patch? Some examples of repetition in verifier.c below. > 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); > + > + ret = btf_struct_access(&env->log, reg, reg->btf, t, off, size, > atype, &btf_id, &flag); > } > > @@ -4596,7 +4600,7 @@ static int check_ptr_to_map_access(struct bpf_verifier_env *env, > return -EACCES; > } > > - ret = btf_struct_access(&env->log, btf_vmlinux, t, off, size, atype, &btf_id, &flag); > + ret = btf_struct_access(&env->log, NULL, btf_vmlinux, t, off, size, atype, &btf_id, &flag); > if (ret < 0) > return ret; > > @@ -5816,6 +5820,7 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, > * fixed offset. > */ > case PTR_TO_BTF_ID: > + case PTR_TO_BTF_ID | MEM_TYPE_LOCAL: > /* When referenced PTR_TO_BTF_ID is passed to release function, > * it's fixed offset must be 0. In the other cases, fixed offset > * can be non-zero. [...]
On Tue, Oct 25, 2022 at 09:57:58PM IST, Dave Marchevsky wrote: > On 10/19/22 8:48 PM, Kumar Kartikeya Dwivedi wrote: > > 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> > >>> --- > > [...] > > >> > >> > >> 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. > > Ah, sorry for missing that. Personally I'm not a fan of dummy register on the > stack. Then if btf_struct_access starts using some reg state that wasn't > populated in the dummy reg it will be confusing. Well, it can be initialized the same way it's done for normal regs. memset to 0 and then mark_reg_known_zero, memset zeroes state untouched by mark_reg_known_zero (reg->parent, reg->live, etc.). which won't matter here. In general, your point is valid as well, but I think for this particular case we can get away with it, moreso because this is the only case needing such adjustment. Or maybe it can be split into two, with the inner call taking btf and btf_id, while the outer one passes them in. Then btf_struct_access uses reg to pass them in, while __btf_struct_access takes them directly.
On Tue, Oct 25, 2022 at 10:02:49PM 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> > > --- > > One nit unrelated to the other thread we have going for this patch. > > [...] > > > 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); > > Can you add a type_is_local_kptr helper (or similar name) to reduce this > type_flag(reg->type) & MEM_TYPE_LOCAL repetition here and elsewhere in the patch? > Some examples of repetition in verifier.c below. > Good point, it was there in RFC but for some reason I decided against it. I will include it in v3.
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, 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); + + ret = btf_struct_access(&env->log, reg, reg->btf, t, off, size, atype, &btf_id, &flag); } @@ -4596,7 +4600,7 @@ static int check_ptr_to_map_access(struct bpf_verifier_env *env, return -EACCES; } - ret = btf_struct_access(&env->log, btf_vmlinux, t, off, size, atype, &btf_id, &flag); + ret = btf_struct_access(&env->log, NULL, btf_vmlinux, t, off, size, atype, &btf_id, &flag); if (ret < 0) return ret; @@ -5816,6 +5820,7 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, * fixed offset. */ case PTR_TO_BTF_ID: + case PTR_TO_BTF_ID | MEM_TYPE_LOCAL: /* When referenced PTR_TO_BTF_ID is passed to release function, * it's fixed offset must be 0. In the other cases, fixed offset * can be non-zero. diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c index e78dadfc5829..d7aa636d90ce 100644 --- a/net/bpf/bpf_dummy_struct_ops.c +++ b/net/bpf/bpf_dummy_struct_ops.c @@ -156,6 +156,7 @@ static bool bpf_dummy_ops_is_valid_access(int off, int size, } static int bpf_dummy_ops_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, @@ -177,7 +178,7 @@ static int bpf_dummy_ops_btf_struct_access(struct bpf_verifier_log *log, return -EACCES; } - err = btf_struct_access(log, btf, t, off, size, atype, next_btf_id, + err = btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id, flag); if (err < 0) return err; diff --git a/net/core/filter.c b/net/core/filter.c index bb0136e7a8e4..cc7af7be91d9 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -8647,13 +8647,15 @@ static bool tc_cls_act_is_valid_access(int off, int size, DEFINE_MUTEX(nf_conn_btf_access_lock); EXPORT_SYMBOL_GPL(nf_conn_btf_access_lock); -int (*nfct_btf_struct_access)(struct bpf_verifier_log *log, const struct btf *btf, +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); EXPORT_SYMBOL_GPL(nfct_btf_struct_access); static int tc_cls_act_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, @@ -8663,12 +8665,12 @@ static int tc_cls_act_btf_struct_access(struct bpf_verifier_log *log, int ret = -EACCES; if (atype == BPF_READ) - return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, + return btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id, flag); mutex_lock(&nf_conn_btf_access_lock); if (nfct_btf_struct_access) - ret = nfct_btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag); + ret = nfct_btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id, flag); mutex_unlock(&nf_conn_btf_access_lock); return ret; @@ -8734,6 +8736,7 @@ void bpf_warn_invalid_xdp_action(struct net_device *dev, struct bpf_prog *prog, EXPORT_SYMBOL_GPL(bpf_warn_invalid_xdp_action); static int xdp_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, @@ -8743,12 +8746,12 @@ static int xdp_btf_struct_access(struct bpf_verifier_log *log, int ret = -EACCES; if (atype == BPF_READ) - return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, + return btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id, flag); mutex_lock(&nf_conn_btf_access_lock); if (nfct_btf_struct_access) - ret = nfct_btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag); + ret = nfct_btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id, flag); mutex_unlock(&nf_conn_btf_access_lock); return ret; diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c index 6da16ae6a962..1fe3935c4260 100644 --- a/net/ipv4/bpf_tcp_ca.c +++ b/net/ipv4/bpf_tcp_ca.c @@ -69,6 +69,7 @@ static bool bpf_tcp_ca_is_valid_access(int off, int size, } static int bpf_tcp_ca_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, @@ -78,7 +79,7 @@ static int bpf_tcp_ca_btf_struct_access(struct bpf_verifier_log *log, size_t end; if (atype == BPF_READ) - return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, + return btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id, flag); if (t != tcp_sock_type) { diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c index 8639e7efd0e2..f6036a84484b 100644 --- a/net/netfilter/nf_conntrack_bpf.c +++ b/net/netfilter/nf_conntrack_bpf.c @@ -191,6 +191,7 @@ BTF_ID(struct, nf_conn___init) /* Check writes into `struct nf_conn` */ static int _nf_conntrack_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,
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(-)