diff mbox series

[bpf-next,v2,5/6] bpf: Introduce bpf_this_cpu_ptr()

Message ID 20200903223332.881541-6-haoluo@google.com
State New
Headers show
Series None | expand

Commit Message

Hao Luo Sept. 3, 2020, 10:33 p.m. UTC
Add bpf_this_cpu_ptr() to help access percpu var on this cpu. This
helper always returns a valid pointer, therefore no need to check
returned value for NULL. Also note that all programs run with
preemption disabled, which means that the returned pointer is stable
during all the execution of the program.

Signed-off-by: Hao Luo <haoluo@google.com>
---
 include/linux/bpf.h            |  1 +
 include/uapi/linux/bpf.h       | 14 ++++++++++++++
 kernel/bpf/verifier.c          | 10 +++++++---
 kernel/trace/bpf_trace.c       | 14 ++++++++++++++
 tools/include/uapi/linux/bpf.h | 14 ++++++++++++++
 5 files changed, 50 insertions(+), 3 deletions(-)

Comments

Andrii Nakryiko Sept. 4, 2020, 8:09 p.m. UTC | #1
On Thu, Sep 3, 2020 at 3:35 PM Hao Luo <haoluo@google.com> wrote:
>
> Add bpf_this_cpu_ptr() to help access percpu var on this cpu. This
> helper always returns a valid pointer, therefore no need to check
> returned value for NULL. Also note that all programs run with
> preemption disabled, which means that the returned pointer is stable
> during all the execution of the program.
>
> Signed-off-by: Hao Luo <haoluo@google.com>
> ---

looks good, few small things, but otherwise:

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  include/linux/bpf.h            |  1 +
>  include/uapi/linux/bpf.h       | 14 ++++++++++++++
>  kernel/bpf/verifier.c          | 10 +++++++---
>  kernel/trace/bpf_trace.c       | 14 ++++++++++++++
>  tools/include/uapi/linux/bpf.h | 14 ++++++++++++++
>  5 files changed, 50 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 6b2034f7665e..506fdd5d0463 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -307,6 +307,7 @@ enum bpf_return_type {
>         RET_PTR_TO_ALLOC_MEM_OR_NULL,   /* returns a pointer to dynamically allocated memory or NULL */
>         RET_PTR_TO_BTF_ID_OR_NULL,      /* returns a pointer to a btf_id or NULL */
>         RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL, /* returns a pointer to a valid memory or a btf_id or NULL */
> +       RET_PTR_TO_MEM_OR_BTF_ID,       /* returns a pointer to a valid memory or a btf_id */
>  };
>
>  /* eBPF function prototype used by verifier to allow BPF_CALLs from eBPF programs
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index d0ec94d5bdbf..e7ca91c697ed 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3612,6 +3612,19 @@ union bpf_attr {
>   *             bpf_per_cpu_ptr() must check the returned value.
>   *     Return
>   *             A generic pointer pointing to the kernel percpu variable on *cpu*.
> + *
> + * void *bpf_this_cpu_ptr(const void *percpu_ptr)
> + *     Description
> + *             Take a pointer to a percpu ksym, *percpu_ptr*, and return a
> + *             pointer to the percpu kernel variable on this cpu. See the
> + *             description of 'ksym' in **bpf_per_cpu_ptr**\ ().
> + *
> + *             bpf_this_cpu_ptr() has the same semantic as this_cpu_ptr() in
> + *             the kernel. Different from **bpf_per_cpu_ptr**\ (), it would
> + *             never return NULL.
> + *     Return
> + *             A generic pointer pointing to the kernel percpu variable on

what's "a generic pointer"? is it as opposed to sk_buff pointer or something?

> + *             this cpu.
>   */
>  #define __BPF_FUNC_MAPPER(FN)          \
>         FN(unspec),                     \
> @@ -3764,6 +3777,7 @@ union bpf_attr {
>         FN(d_path),                     \
>         FN(copy_from_user),             \
>         FN(bpf_per_cpu_ptr),            \
> +       FN(bpf_this_cpu_ptr),           \
>         /* */
>
>  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a702600ff581..e070d2abc405 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5016,8 +5016,10 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
>                 regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL;
>                 regs[BPF_REG_0].id = ++env->id_gen;
>                 regs[BPF_REG_0].mem_size = meta.mem_size;
> -       } else if (fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL) {
> +       } else if (fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL ||
> +                  fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID) {
>                 const struct btf_type *t;
> +               bool not_null = fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID;

nit: this is fine, but I'd inline it below

>
>                 mark_reg_known_zero(env, regs, BPF_REG_0);
>                 t = btf_type_skip_modifiers(btf_vmlinux, meta.ret_btf_id, NULL);
> @@ -5034,10 +5036,12 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
>                                         tname, PTR_ERR(ret));
>                                 return -EINVAL;
>                         }
> -                       regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL;
> +                       regs[BPF_REG_0].type = not_null ?
> +                               PTR_TO_MEM : PTR_TO_MEM_OR_NULL;
>                         regs[BPF_REG_0].mem_size = tsize;
>                 } else {
> -                       regs[BPF_REG_0].type = PTR_TO_BTF_ID_OR_NULL;
> +                       regs[BPF_REG_0].type = not_null ?
> +                               PTR_TO_BTF_ID : PTR_TO_BTF_ID_OR_NULL;
>                         regs[BPF_REG_0].btf_id = meta.ret_btf_id;
>                 }
>         } else if (fn->ret_type == RET_PTR_TO_BTF_ID_OR_NULL) {
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index d474c1530f87..466acf82a9c7 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1160,6 +1160,18 @@ static const struct bpf_func_proto bpf_per_cpu_ptr_proto = {
>         .arg2_type      = ARG_ANYTHING,
>  };
>
> +BPF_CALL_1(bpf_this_cpu_ptr, const void *, percpu_ptr)
> +{
> +       return (u64)this_cpu_ptr(percpu_ptr);

see previous comment, this might trigger unnecessary compilation
warnings on 32-bit arches

> +}
> +
> +static const struct bpf_func_proto bpf_this_cpu_ptr_proto = {
> +       .func           = bpf_this_cpu_ptr,
> +       .gpl_only       = false,
> +       .ret_type       = RET_PTR_TO_MEM_OR_BTF_ID,
> +       .arg1_type      = ARG_PTR_TO_PERCPU_BTF_ID,
> +};
> +

[...]
Hao Luo Sept. 14, 2020, 5:04 a.m. UTC | #2
Thanks for taking a look!

On Fri, Sep 4, 2020 at 1:09 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Sep 3, 2020 at 3:35 PM Hao Luo <haoluo@google.com> wrote:
> >
> > Add bpf_this_cpu_ptr() to help access percpu var on this cpu. This
> > helper always returns a valid pointer, therefore no need to check
> > returned value for NULL. Also note that all programs run with
> > preemption disabled, which means that the returned pointer is stable
> > during all the execution of the program.
> >
> > Signed-off-by: Hao Luo <haoluo@google.com>
> > ---
>
> looks good, few small things, but otherwise:
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
>
[...]
> >
> >  /* eBPF function prototype used by verifier to allow BPF_CALLs from eBPF programs
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index d0ec94d5bdbf..e7ca91c697ed 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -3612,6 +3612,19 @@ union bpf_attr {
> >   *             bpf_per_cpu_ptr() must check the returned value.
> >   *     Return
> >   *             A generic pointer pointing to the kernel percpu variable on *cpu*.
> > + *
> > + * void *bpf_this_cpu_ptr(const void *percpu_ptr)
> > + *     Description
> > + *             Take a pointer to a percpu ksym, *percpu_ptr*, and return a
> > + *             pointer to the percpu kernel variable on this cpu. See the
> > + *             description of 'ksym' in **bpf_per_cpu_ptr**\ ().
> > + *
> > + *             bpf_this_cpu_ptr() has the same semantic as this_cpu_ptr() in
> > + *             the kernel. Different from **bpf_per_cpu_ptr**\ (), it would
> > + *             never return NULL.
> > + *     Return
> > + *             A generic pointer pointing to the kernel percpu variable on
>
> what's "a generic pointer"? is it as opposed to sk_buff pointer or something?
>

Ack. "A pointer" should be good enough. I wrote "generic pointer"
because the per_cpu_ptr() in kernel code is a macro, whose returned
value is a typed pointer, IIUC. But here we are missing the type. This
is another difference between this helper and per_cpu_ptr(). But this
may not matter.

> >  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index a702600ff581..e070d2abc405 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5016,8 +5016,10 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
> >                 regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL;
> >                 regs[BPF_REG_0].id = ++env->id_gen;
> >                 regs[BPF_REG_0].mem_size = meta.mem_size;
> > -       } else if (fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL) {
> > +       } else if (fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL ||
> > +                  fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID) {
> >                 const struct btf_type *t;
> > +               bool not_null = fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID;
>
> nit: this is fine, but I'd inline it below
>

Ack.

> >
> >                 mark_reg_known_zero(env, regs, BPF_REG_0);
> >                 t = btf_type_skip_modifiers(btf_vmlinux, meta.ret_btf_id, NULL);
> > @@ -5034,10 +5036,12 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
> >                                         tname, PTR_ERR(ret));
> >                                 return -EINVAL;
> >                         }
> > -                       regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL;
> > +                       regs[BPF_REG_0].type = not_null ?
> > +                               PTR_TO_MEM : PTR_TO_MEM_OR_NULL;
> >                         regs[BPF_REG_0].mem_size = tsize;
> >                 } else {
> > -                       regs[BPF_REG_0].type = PTR_TO_BTF_ID_OR_NULL;
> > +                       regs[BPF_REG_0].type = not_null ?
> > +                               PTR_TO_BTF_ID : PTR_TO_BTF_ID_OR_NULL;
> >                         regs[BPF_REG_0].btf_id = meta.ret_btf_id;
> >                 }
> >         } else if (fn->ret_type == RET_PTR_TO_BTF_ID_OR_NULL) {
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index d474c1530f87..466acf82a9c7 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -1160,6 +1160,18 @@ static const struct bpf_func_proto bpf_per_cpu_ptr_proto = {
> >         .arg2_type      = ARG_ANYTHING,
> >  };
> >
> > +BPF_CALL_1(bpf_this_cpu_ptr, const void *, percpu_ptr)
> > +{
> > +       return (u64)this_cpu_ptr(percpu_ptr);
>
> see previous comment, this might trigger unnecessary compilation
> warnings on 32-bit arches
>

Ack. Will cast to "unsigned long". Thanks for catching this!


> > +}
> > +
> > +static const struct bpf_func_proto bpf_this_cpu_ptr_proto = {
> > +       .func           = bpf_this_cpu_ptr,
> > +       .gpl_only       = false,
> > +       .ret_type       = RET_PTR_TO_MEM_OR_BTF_ID,
> > +       .arg1_type      = ARG_PTR_TO_PERCPU_BTF_ID,
> > +};
> > +
>
> [...]
diff mbox series

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 6b2034f7665e..506fdd5d0463 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -307,6 +307,7 @@  enum bpf_return_type {
 	RET_PTR_TO_ALLOC_MEM_OR_NULL,	/* returns a pointer to dynamically allocated memory or NULL */
 	RET_PTR_TO_BTF_ID_OR_NULL,	/* returns a pointer to a btf_id or NULL */
 	RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL, /* returns a pointer to a valid memory or a btf_id or NULL */
+	RET_PTR_TO_MEM_OR_BTF_ID,	/* returns a pointer to a valid memory or a btf_id */
 };
 
 /* eBPF function prototype used by verifier to allow BPF_CALLs from eBPF programs
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index d0ec94d5bdbf..e7ca91c697ed 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3612,6 +3612,19 @@  union bpf_attr {
  *             bpf_per_cpu_ptr() must check the returned value.
  *     Return
  *             A generic pointer pointing to the kernel percpu variable on *cpu*.
+ *
+ * void *bpf_this_cpu_ptr(const void *percpu_ptr)
+ *	Description
+ *		Take a pointer to a percpu ksym, *percpu_ptr*, and return a
+ *		pointer to the percpu kernel variable on this cpu. See the
+ *		description of 'ksym' in **bpf_per_cpu_ptr**\ ().
+ *
+ *		bpf_this_cpu_ptr() has the same semantic as this_cpu_ptr() in
+ *		the kernel. Different from **bpf_per_cpu_ptr**\ (), it would
+ *		never return NULL.
+ *	Return
+ *		A generic pointer pointing to the kernel percpu variable on
+ *		this cpu.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3764,6 +3777,7 @@  union bpf_attr {
 	FN(d_path),			\
 	FN(copy_from_user),		\
 	FN(bpf_per_cpu_ptr),            \
+	FN(bpf_this_cpu_ptr),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a702600ff581..e070d2abc405 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5016,8 +5016,10 @@  static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
 		regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL;
 		regs[BPF_REG_0].id = ++env->id_gen;
 		regs[BPF_REG_0].mem_size = meta.mem_size;
-	} else if (fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL) {
+	} else if (fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL ||
+		   fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID) {
 		const struct btf_type *t;
+		bool not_null = fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID;
 
 		mark_reg_known_zero(env, regs, BPF_REG_0);
 		t = btf_type_skip_modifiers(btf_vmlinux, meta.ret_btf_id, NULL);
@@ -5034,10 +5036,12 @@  static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
 					tname, PTR_ERR(ret));
 				return -EINVAL;
 			}
-			regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL;
+			regs[BPF_REG_0].type = not_null ?
+				PTR_TO_MEM : PTR_TO_MEM_OR_NULL;
 			regs[BPF_REG_0].mem_size = tsize;
 		} else {
-			regs[BPF_REG_0].type = PTR_TO_BTF_ID_OR_NULL;
+			regs[BPF_REG_0].type = not_null ?
+				PTR_TO_BTF_ID : PTR_TO_BTF_ID_OR_NULL;
 			regs[BPF_REG_0].btf_id = meta.ret_btf_id;
 		}
 	} else if (fn->ret_type == RET_PTR_TO_BTF_ID_OR_NULL) {
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index d474c1530f87..466acf82a9c7 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1160,6 +1160,18 @@  static const struct bpf_func_proto bpf_per_cpu_ptr_proto = {
 	.arg2_type	= ARG_ANYTHING,
 };
 
+BPF_CALL_1(bpf_this_cpu_ptr, const void *, percpu_ptr)
+{
+	return (u64)this_cpu_ptr(percpu_ptr);
+}
+
+static const struct bpf_func_proto bpf_this_cpu_ptr_proto = {
+	.func		= bpf_this_cpu_ptr,
+	.gpl_only	= false,
+	.ret_type	= RET_PTR_TO_MEM_OR_BTF_ID,
+	.arg1_type	= ARG_PTR_TO_PERCPU_BTF_ID,
+};
+
 const struct bpf_func_proto *
 bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
@@ -1248,6 +1260,8 @@  bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return prog->aux->sleepable ? &bpf_copy_from_user_proto : NULL;
 	case BPF_FUNC_bpf_per_cpu_ptr:
 		return &bpf_per_cpu_ptr_proto;
+	case BPF_FUNC_bpf_this_cpu_ptr:
+		return &bpf_this_cpu_ptr_proto;
 	default:
 		return NULL;
 	}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index d0ec94d5bdbf..e7ca91c697ed 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3612,6 +3612,19 @@  union bpf_attr {
  *             bpf_per_cpu_ptr() must check the returned value.
  *     Return
  *             A generic pointer pointing to the kernel percpu variable on *cpu*.
+ *
+ * void *bpf_this_cpu_ptr(const void *percpu_ptr)
+ *	Description
+ *		Take a pointer to a percpu ksym, *percpu_ptr*, and return a
+ *		pointer to the percpu kernel variable on this cpu. See the
+ *		description of 'ksym' in **bpf_per_cpu_ptr**\ ().
+ *
+ *		bpf_this_cpu_ptr() has the same semantic as this_cpu_ptr() in
+ *		the kernel. Different from **bpf_per_cpu_ptr**\ (), it would
+ *		never return NULL.
+ *	Return
+ *		A generic pointer pointing to the kernel percpu variable on
+ *		this cpu.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3764,6 +3777,7 @@  union bpf_attr {
 	FN(d_path),			\
 	FN(copy_from_user),		\
 	FN(bpf_per_cpu_ptr),            \
+	FN(bpf_this_cpu_ptr),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper