diff mbox series

[bpf-next,v2,05/15] bpf: Allow storing percpu kptr in map

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

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1452 this patch: 1452
netdev/cc_maintainers warning 6 maintainers not CCed: netdev@vger.kernel.org songliubraving@fb.com yhs@fb.com john.fastabend@gmail.com kafai@fb.com kpsingh@kernel.org
netdev/build_clang success Errors and warnings before: 172 this patch: 172
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1469 this patch: 1469
netdev/checkpatch warning WARNING: line length of 92 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next success VM_Test
bpf/vmtest-bpf-next-PR success PR summary

Commit Message

Kumar Kartikeya Dwivedi March 17, 2022, 11:59 a.m. UTC
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(-)

Comments

Alexei Starovoitov March 19, 2022, 6:30 p.m. UTC | #1
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?
Kumar Kartikeya Dwivedi March 19, 2022, 7:04 p.m. UTC | #2
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
Alexei Starovoitov March 19, 2022, 9:26 p.m. UTC | #3
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.
Kumar Kartikeya Dwivedi March 19, 2022, 9:45 p.m. UTC | #4
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
Alexei Starovoitov March 19, 2022, 11:01 p.m. UTC | #5
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 mbox series

Patch

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