| Message ID | 20220317115957.3193097-6-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Introduce typed pointer support in BPF maps | expand |
On Thu, Mar 17, 2022 at 05:29:47PM +0530, Kumar Kartikeya Dwivedi wrote: > Make adjustments to the code to allow storing percpu PTR_TO_BTF_ID in a > map. Similar to 'kptr_ref' tag, a new 'kptr_percpu' allows tagging types > of pointers accepting stores of such register types. On load, verifier > marks destination register as having type PTR_TO_BTF_ID | MEM_PERCPU | > PTR_MAYBE_NULL. > > Cc: Hao Luo <haoluo@google.com> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > include/linux/bpf.h | 3 ++- > kernel/bpf/btf.c | 13 ++++++++++--- > kernel/bpf/verifier.c | 26 +++++++++++++++++++++----- > 3 files changed, 33 insertions(+), 9 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 702aa882e4a3..433f5cb161cf 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -161,7 +161,8 @@ enum { > }; > > enum { > - BPF_MAP_VALUE_OFF_F_REF = (1U << 0), > + BPF_MAP_VALUE_OFF_F_REF = (1U << 0), > + BPF_MAP_VALUE_OFF_F_PERCPU = (1U << 1), What is the use case for storing __percpu pointer into a map?
On Sun, Mar 20, 2022 at 12:00:28AM IST, Alexei Starovoitov wrote: > On Thu, Mar 17, 2022 at 05:29:47PM +0530, Kumar Kartikeya Dwivedi wrote: > > Make adjustments to the code to allow storing percpu PTR_TO_BTF_ID in a > > map. Similar to 'kptr_ref' tag, a new 'kptr_percpu' allows tagging types > > of pointers accepting stores of such register types. On load, verifier > > marks destination register as having type PTR_TO_BTF_ID | MEM_PERCPU | > > PTR_MAYBE_NULL. > > > > Cc: Hao Luo <haoluo@google.com> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > include/linux/bpf.h | 3 ++- > > kernel/bpf/btf.c | 13 ++++++++++--- > > kernel/bpf/verifier.c | 26 +++++++++++++++++++++----- > > 3 files changed, 33 insertions(+), 9 deletions(-) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index 702aa882e4a3..433f5cb161cf 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -161,7 +161,8 @@ enum { > > }; > > > > enum { > > - BPF_MAP_VALUE_OFF_F_REF = (1U << 0), > > + BPF_MAP_VALUE_OFF_F_REF = (1U << 0), > > + BPF_MAP_VALUE_OFF_F_PERCPU = (1U << 1), > > What is the use case for storing __percpu pointer into a map? No specific use case for me, just thought it would be useful, especially now that __percpu tag is understood by verifier for kernel BTF, so it may also refer to dynamically allocated per-CPU memory, not just global percpu variables. But fine with dropping both this and user kptr if you don't feel like keeping them. -- Kartikeya
On Sun, Mar 20, 2022 at 12:34:09AM +0530, Kumar Kartikeya Dwivedi wrote: > On Sun, Mar 20, 2022 at 12:00:28AM IST, Alexei Starovoitov wrote: > > On Thu, Mar 17, 2022 at 05:29:47PM +0530, Kumar Kartikeya Dwivedi wrote: > > > Make adjustments to the code to allow storing percpu PTR_TO_BTF_ID in a > > > map. Similar to 'kptr_ref' tag, a new 'kptr_percpu' allows tagging types > > > of pointers accepting stores of such register types. On load, verifier > > > marks destination register as having type PTR_TO_BTF_ID | MEM_PERCPU | > > > PTR_MAYBE_NULL. > > > > > > Cc: Hao Luo <haoluo@google.com> > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > --- > > > include/linux/bpf.h | 3 ++- > > > kernel/bpf/btf.c | 13 ++++++++++--- > > > kernel/bpf/verifier.c | 26 +++++++++++++++++++++----- > > > 3 files changed, 33 insertions(+), 9 deletions(-) > > > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > index 702aa882e4a3..433f5cb161cf 100644 > > > --- a/include/linux/bpf.h > > > +++ b/include/linux/bpf.h > > > @@ -161,7 +161,8 @@ enum { > > > }; > > > > > > enum { > > > - BPF_MAP_VALUE_OFF_F_REF = (1U << 0), > > > + BPF_MAP_VALUE_OFF_F_REF = (1U << 0), > > > + BPF_MAP_VALUE_OFF_F_PERCPU = (1U << 1), > > > > What is the use case for storing __percpu pointer into a map? > > No specific use case for me, just thought it would be useful, especially now > that __percpu tag is understood by verifier for kernel BTF, so it may also refer > to dynamically allocated per-CPU memory, not just global percpu variables. But > fine with dropping both this and user kptr if you don't feel like keeping them. I prefer to drop it for now. The patch is trivial but kptr_percpu tag would stay forever. Maybe we can allow storing percpu pointers in a map with just kptr tag. The verifier should be able to understand from btf whether that pointer is percpu or not.
On Sun, Mar 20, 2022 at 02:56:20AM IST, Alexei Starovoitov wrote: > On Sun, Mar 20, 2022 at 12:34:09AM +0530, Kumar Kartikeya Dwivedi wrote: > > On Sun, Mar 20, 2022 at 12:00:28AM IST, Alexei Starovoitov wrote: > > > On Thu, Mar 17, 2022 at 05:29:47PM +0530, Kumar Kartikeya Dwivedi wrote: > > > > Make adjustments to the code to allow storing percpu PTR_TO_BTF_ID in a > > > > map. Similar to 'kptr_ref' tag, a new 'kptr_percpu' allows tagging types > > > > of pointers accepting stores of such register types. On load, verifier > > > > marks destination register as having type PTR_TO_BTF_ID | MEM_PERCPU | > > > > PTR_MAYBE_NULL. > > > > > > > > Cc: Hao Luo <haoluo@google.com> > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > > --- > > > > include/linux/bpf.h | 3 ++- > > > > kernel/bpf/btf.c | 13 ++++++++++--- > > > > kernel/bpf/verifier.c | 26 +++++++++++++++++++++----- > > > > 3 files changed, 33 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > > index 702aa882e4a3..433f5cb161cf 100644 > > > > --- a/include/linux/bpf.h > > > > +++ b/include/linux/bpf.h > > > > @@ -161,7 +161,8 @@ enum { > > > > }; > > > > > > > > enum { > > > > - BPF_MAP_VALUE_OFF_F_REF = (1U << 0), > > > > + BPF_MAP_VALUE_OFF_F_REF = (1U << 0), > > > > + BPF_MAP_VALUE_OFF_F_PERCPU = (1U << 1), > > > > > > What is the use case for storing __percpu pointer into a map? > > > > No specific use case for me, just thought it would be useful, especially now > > that __percpu tag is understood by verifier for kernel BTF, so it may also refer > > to dynamically allocated per-CPU memory, not just global percpu variables. But > > fine with dropping both this and user kptr if you don't feel like keeping them. > > I prefer to drop it for now. > The patch is trivial but kptr_percpu tag would stay forever. Ok, I'll drop both this and user kptr for now. > Maybe we can allow storing percpu pointers in a map with just kptr tag. > The verifier should be able to understand from btf whether that pointer > is percpu or not. This won't work (unless I missed something), it is possible to see the type when a store is being done, but we cannot know whether the pointer was percpu or not when doing a load (which is needed to decide whether it will be marked with MEM_PERCPU, so that user has to call bpf_this_cpu_ptr or bpf_per_cpu_ptr to obtain actual pointer). So some extra tagging is needed. -- Kartikeya
On Sun, Mar 20, 2022 at 03:15:05AM +0530, Kumar Kartikeya Dwivedi wrote: > On Sun, Mar 20, 2022 at 02:56:20AM IST, Alexei Starovoitov wrote: > > On Sun, Mar 20, 2022 at 12:34:09AM +0530, Kumar Kartikeya Dwivedi wrote: > > > On Sun, Mar 20, 2022 at 12:00:28AM IST, Alexei Starovoitov wrote: > > > > On Thu, Mar 17, 2022 at 05:29:47PM +0530, Kumar Kartikeya Dwivedi wrote: > > > > > Make adjustments to the code to allow storing percpu PTR_TO_BTF_ID in a > > > > > map. Similar to 'kptr_ref' tag, a new 'kptr_percpu' allows tagging types > > > > > of pointers accepting stores of such register types. On load, verifier > > > > > marks destination register as having type PTR_TO_BTF_ID | MEM_PERCPU | > > > > > PTR_MAYBE_NULL. > > > > > > > > > > Cc: Hao Luo <haoluo@google.com> > > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > > > --- > > > > > include/linux/bpf.h | 3 ++- > > > > > kernel/bpf/btf.c | 13 ++++++++++--- > > > > > kernel/bpf/verifier.c | 26 +++++++++++++++++++++----- > > > > > 3 files changed, 33 insertions(+), 9 deletions(-) > > > > > > > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > > > index 702aa882e4a3..433f5cb161cf 100644 > > > > > --- a/include/linux/bpf.h > > > > > +++ b/include/linux/bpf.h > > > > > @@ -161,7 +161,8 @@ enum { > > > > > }; > > > > > > > > > > enum { > > > > > - BPF_MAP_VALUE_OFF_F_REF = (1U << 0), > > > > > + BPF_MAP_VALUE_OFF_F_REF = (1U << 0), > > > > > + BPF_MAP_VALUE_OFF_F_PERCPU = (1U << 1), > > > > > > > > What is the use case for storing __percpu pointer into a map? > > > > > > No specific use case for me, just thought it would be useful, especially now > > > that __percpu tag is understood by verifier for kernel BTF, so it may also refer > > > to dynamically allocated per-CPU memory, not just global percpu variables. But > > > fine with dropping both this and user kptr if you don't feel like keeping them. > > > > I prefer to drop it for now. > > The patch is trivial but kptr_percpu tag would stay forever. > > Ok, I'll drop both this and user kptr for now. > > > Maybe we can allow storing percpu pointers in a map with just kptr tag. > > The verifier should be able to understand from btf whether that pointer > > is percpu or not. > > This won't work (unless I missed something), it is possible to see the type when > a store is being done, but we cannot know whether the pointer was percpu or not > when doing a load (which is needed to decide whether it will be marked with > MEM_PERCPU, so that user has to call bpf_this_cpu_ptr or bpf_per_cpu_ptr to > obtain actual pointer). So some extra tagging is needed. The pointer in bpf program should probably be marked as normal __percpu then. So that types match during both store and load. It will be a combination of btf_tags __kptr and __percpu. Anyway let's table this discussion until main feature lands.
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 702aa882e4a3..433f5cb161cf 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -161,7 +161,8 @@ enum { }; enum { - BPF_MAP_VALUE_OFF_F_REF = (1U << 0), + BPF_MAP_VALUE_OFF_F_REF = (1U << 0), + BPF_MAP_VALUE_OFF_F_PERCPU = (1U << 1), }; struct bpf_map_value_off_desc { diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 7b4179667bf1..04d604931f59 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -3197,7 +3197,7 @@ static int btf_find_field_kptr(const struct btf *btf, const struct btf_type *t, u32 off, int sz, struct btf_field_info *info, int info_cnt, int idx) { - bool kptr_tag = false, kptr_ref_tag = false; + bool kptr_tag = false, kptr_ref_tag = false, kptr_percpu_tag = false; int tags; /* For PTR, sz is always == 8 */ @@ -3216,12 +3216,17 @@ static int btf_find_field_kptr(const struct btf *btf, const struct btf_type *t, if (kptr_ref_tag) return -EEXIST; kptr_ref_tag = true; + } else if (!strcmp("kptr_percpu", __btf_name_by_offset(btf, t->name_off))) { + /* repeated tag */ + if (kptr_percpu_tag) + return -EEXIST; + kptr_percpu_tag = true; } /* Look for next tag */ t = btf_type_by_id(btf, t->type); } - tags = kptr_tag + kptr_ref_tag; + tags = kptr_tag + kptr_ref_tag + kptr_percpu_tag; if (!tags) return BTF_FIELD_IGNORE; else if (tags > 1) @@ -3236,7 +3241,9 @@ static int btf_find_field_kptr(const struct btf *btf, const struct btf_type *t, if (idx >= info_cnt) return -E2BIG; - if (kptr_ref_tag) + if (kptr_percpu_tag) + info[idx].flags = BPF_MAP_VALUE_OFF_F_PERCPU; + else if (kptr_ref_tag) info[idx].flags = BPF_MAP_VALUE_OFF_F_REF; else info[idx].flags = 0; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index f8738054aa52..cc8f7250e43e 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -3517,11 +3517,19 @@ static int map_kptr_match_type(struct bpf_verifier_env *env, bool ref_ptr) { const char *targ_name = kernel_type_name(off_desc->btf, off_desc->btf_id); + enum bpf_reg_type reg_type; const char *reg_name = ""; bool fixed_off_ok = true; - if (reg->type != PTR_TO_BTF_ID && reg->type != PTR_TO_BTF_ID_OR_NULL) - goto bad_type; + if (off_desc->flags & BPF_MAP_VALUE_OFF_F_PERCPU) { + if (reg->type != (PTR_TO_BTF_ID | MEM_PERCPU) && + reg->type != (PTR_TO_BTF_ID | PTR_MAYBE_NULL | MEM_PERCPU)) + goto bad_type; + } else { /* referenced and unreferenced case */ + if (reg->type != PTR_TO_BTF_ID && + reg->type != (PTR_TO_BTF_ID | PTR_MAYBE_NULL)) + goto bad_type; + } if (!btf_is_kernel(reg->btf)) { verbose(env, "R%d must point to kernel BTF\n", regno); @@ -3557,9 +3565,13 @@ static int map_kptr_match_type(struct bpf_verifier_env *env, goto bad_type; return 0; bad_type: + if (off_desc->flags & BPF_MAP_VALUE_OFF_F_PERCPU) + reg_type = PTR_TO_BTF_ID | PTR_MAYBE_NULL | MEM_PERCPU; + else + reg_type = PTR_TO_BTF_ID | PTR_MAYBE_NULL; verbose(env, "invalid kptr access, R%d type=%s%s ", regno, reg_type_str(env, reg->type), reg_name); - verbose(env, "expected=%s%s\n", reg_type_str(env, PTR_TO_BTF_ID), targ_name); + verbose(env, "expected=%s%s\n", reg_type_str(env, reg_type), targ_name); return -EINVAL; } @@ -3572,10 +3584,11 @@ static int check_map_kptr_access(struct bpf_verifier_env *env, u32 regno, { struct bpf_reg_state *reg = reg_state(env, regno), *val_reg; struct bpf_insn *insn = &env->prog->insnsi[insn_idx]; + enum bpf_type_flag reg_flags = PTR_MAYBE_NULL; + bool ref_ptr = false, percpu_ptr = false; struct bpf_map_value_off_desc *off_desc; int insn_class = BPF_CLASS(insn->code); struct bpf_map *map = reg->map_ptr; - bool ref_ptr = false; /* Things we already checked for in check_map_access: * - Reject cases where variable offset may touch BTF ID pointer @@ -3601,6 +3614,9 @@ static int check_map_kptr_access(struct bpf_verifier_env *env, u32 regno, } ref_ptr = off_desc->flags & BPF_MAP_VALUE_OFF_F_REF; + percpu_ptr = off_desc->flags & BPF_MAP_VALUE_OFF_F_PERCPU; + if (percpu_ptr) + reg_flags |= MEM_PERCPU; if (insn_class == BPF_LDX) { if (WARN_ON_ONCE(value_regno < 0)) @@ -3614,7 +3630,7 @@ static int check_map_kptr_access(struct bpf_verifier_env *env, u32 regno, * value from map as PTR_TO_BTF_ID, with the correct type. */ mark_btf_ld_reg(env, cur_regs(env), value_regno, PTR_TO_BTF_ID, off_desc->btf, - off_desc->btf_id, PTR_MAYBE_NULL); + off_desc->btf_id, reg_flags); val_reg->id = ++env->id_gen; } else if (insn_class == BPF_STX) { if (WARN_ON_ONCE(value_regno < 0))
Make adjustments to the code to allow storing percpu PTR_TO_BTF_ID in a map. Similar to 'kptr_ref' tag, a new 'kptr_percpu' allows tagging types of pointers accepting stores of such register types. On load, verifier marks destination register as having type PTR_TO_BTF_ID | MEM_PERCPU | PTR_MAYBE_NULL. Cc: Hao Luo <haoluo@google.com> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- include/linux/bpf.h | 3 ++- kernel/bpf/btf.c | 13 ++++++++++--- kernel/bpf/verifier.c | 26 +++++++++++++++++++++----- 3 files changed, 33 insertions(+), 9 deletions(-)