From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: Dave Marchevsky <davemarchevsky@meta.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
Andrii Nakryiko <andrii@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Delyan Kratunov <delyank@meta.com>
Subject: Re: [PATCH bpf-next v2 10/25] bpf: Introduce local kptrs
Date: Thu, 20 Oct 2022 06:18:37 +0530 [thread overview]
Message-ID: <20221020004837.qclzg6pgrqamcn7e@apollo> (raw)
In-Reply-To: <582912fa-3d32-7c5a-cf24-fc79899a2e31@meta.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.
next prev parent reply other threads:[~2022-10-20 0:48 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-13 6:22 [PATCH bpf-next v2 00/25] Local kptrs, BPF linked lists Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 01/25] bpf: Document UAPI details for special BPF types Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 02/25] bpf: Allow specifying volatile type modifier for kptrs Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 03/25] bpf: Clobber stack slot when writing over spilled PTR_TO_BTF_ID Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 04/25] bpf: Fix slot type check in check_stack_write_var_off Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 05/25] bpf: Drop reg_type_may_be_refcounted_or_null Kumar Kartikeya Dwivedi
2022-10-19 16:04 ` Dave Marchevsky
2022-10-13 6:22 ` [PATCH bpf-next v2 06/25] bpf: Refactor kptr_off_tab into fields_tab Kumar Kartikeya Dwivedi
2022-10-19 1:35 ` Alexei Starovoitov
2022-10-19 5:42 ` Kumar Kartikeya Dwivedi
2022-10-19 15:54 ` Alexei Starovoitov
2022-10-19 23:57 ` Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 07/25] bpf: Consolidate spin_lock, timer management " Kumar Kartikeya Dwivedi
2022-10-19 1:40 ` Alexei Starovoitov
2022-10-19 5:43 ` Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 08/25] bpf: Refactor map->off_arr handling Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 09/25] bpf: Support bpf_list_head in map values Kumar Kartikeya Dwivedi
2022-10-19 1:59 ` Alexei Starovoitov
2022-10-19 5:48 ` Kumar Kartikeya Dwivedi
2022-10-19 15:57 ` Alexei Starovoitov
2022-10-19 23:59 ` Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 10/25] bpf: Introduce local kptrs Kumar Kartikeya Dwivedi
2022-10-19 17:15 ` Dave Marchevsky
2022-10-20 0:48 ` Kumar Kartikeya Dwivedi [this message]
2022-10-25 16:27 ` Dave Marchevsky
2022-10-25 18:11 ` Kumar Kartikeya Dwivedi
2022-10-25 16:32 ` Dave Marchevsky
2022-10-25 18:11 ` Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 11/25] bpf: Recognize bpf_{spin_lock,list_head,list_node} in " Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 12/25] bpf: Verify ownership relationships for owning types Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 13/25] bpf: Support locking bpf_spin_lock in local kptr Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 14/25] bpf: Allow locking bpf_spin_lock global variables Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 15/25] bpf: Rewrite kfunc argument handling Kumar Kartikeya Dwivedi
2022-10-13 13:48 ` kernel test robot
2022-10-13 6:22 ` [PATCH bpf-next v2 16/25] bpf: Drop kfunc bits from btf_check_func_arg_match Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 17/25] bpf: Support constant scalar arguments for kfuncs Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 18/25] bpf: Teach verifier about non-size constant arguments Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 19/25] bpf: Introduce bpf_kptr_new Kumar Kartikeya Dwivedi
2022-10-19 2:31 ` Alexei Starovoitov
2022-10-19 5:58 ` Kumar Kartikeya Dwivedi
2022-10-19 16:31 ` Alexei Starovoitov
2022-10-20 0:44 ` Kumar Kartikeya Dwivedi
2022-10-20 1:11 ` Alexei Starovoitov
2022-10-13 6:22 ` [PATCH bpf-next v2 20/25] bpf: Introduce bpf_kptr_drop Kumar Kartikeya Dwivedi
2022-10-13 6:22 ` [PATCH bpf-next v2 21/25] bpf: Permit NULL checking pointer with non-zero fixed offset Kumar Kartikeya Dwivedi
2022-10-13 6:23 ` [PATCH bpf-next v2 22/25] bpf: Introduce single ownership BPF linked list API Kumar Kartikeya Dwivedi
2022-10-25 17:45 ` Dave Marchevsky
2022-10-25 19:00 ` Kumar Kartikeya Dwivedi
2022-10-13 6:23 ` [PATCH bpf-next v2 23/25] libbpf: Add support for private BSS map section Kumar Kartikeya Dwivedi
2022-10-18 4:03 ` Andrii Nakryiko
2022-10-13 6:23 ` [PATCH bpf-next v2 24/25] selftests/bpf: Add __contains macro to bpf_experimental.h Kumar Kartikeya Dwivedi
2022-10-13 6:23 ` [PATCH bpf-next v2 25/25] selftests/bpf: Add BPF linked list API tests Kumar Kartikeya Dwivedi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221020004837.qclzg6pgrqamcn7e@apollo \
--to=memxor@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davemarchevsky@meta.com \
--cc=delyank@meta.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox