On Wed, Aug 19, 2020 at 3:42 PM Hao Luo <[email protected]> wrote:
>
> Add bpf_per_cpu_ptr() to help bpf programs access percpu vars.
> bpf_per_cpu_ptr() has the same semantic as per_cpu_ptr() in the kernel
> except that it may return NULL. This happens when the cpu parameter is
> out of range. So the caller must check the returned value.
>
> Signed-off-by: Hao Luo <[email protected]>
> ---
The logic looks correct, few naming nits, but otherwise:
Acked-by: Andrii Nakryiko <[email protected]>
> include/linux/bpf.h | 3 ++
> include/linux/btf.h | 11 +++++++
> include/uapi/linux/bpf.h | 14 +++++++++
> kernel/bpf/btf.c | 10 -------
> kernel/bpf/verifier.c | 64 ++++++++++++++++++++++++++++++++++++++--
> kernel/trace/bpf_trace.c | 18 +++++++++++
> 6 files changed, 107 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 55f694b63164..613404beab33 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -268,6 +268,7 @@ enum bpf_arg_type {
> ARG_PTR_TO_ALLOC_MEM, /* pointer to dynamically allocated memory */
> ARG_PTR_TO_ALLOC_MEM_OR_NULL, /* pointer to dynamically allocated memory or NULL */
> ARG_CONST_ALLOC_SIZE_OR_ZERO, /* number of allocated bytes requested */
> + ARG_PTR_TO_PERCPU_BTF_ID, /* pointer to in-kernel percpu type */
> };
>
> /* type of values returned from helper functions */
> @@ -281,6 +282,7 @@ enum bpf_return_type {
> RET_PTR_TO_SOCK_COMMON_OR_NULL, /* returns a pointer to a sock_common or NULL */
> 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_OR_NULL, /* returns a pointer to a valid memory or a btf_id or NULL */
I know it's already very long, but still let's use BTF_ID consistently
> };
>
> /* eBPF function prototype used by verifier to allow BPF_CALLs from eBPF programs
> @@ -360,6 +362,7 @@ enum bpf_reg_type {
> PTR_TO_RDONLY_BUF_OR_NULL, /* reg points to a readonly buffer or NULL */
> PTR_TO_RDWR_BUF, /* reg points to a read/write buffer */
> PTR_TO_RDWR_BUF_OR_NULL, /* reg points to a read/write buffer or NULL */
> + PTR_TO_PERCPU_BTF_ID, /* reg points to percpu kernel type */
> };
>
[...]
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 468376f2910b..c7e49a102ed2 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3415,6 +3415,19 @@ union bpf_attr {
> * A non-negative value equal to or less than *size* on success,
> * or a negative error in case of failure.
> *
> + * void *bpf_per_cpu_ptr(const void *ptr, u32 cpu)
> + * Description
> + * Take the address of a percpu ksym and return a pointer pointing
> + * to the variable on *cpu*. A ksym is an extern variable decorated
> + * with '__ksym'. A ksym is percpu if there is a global percpu var
> + * (either static or global) defined of the same name in the kernel.
The function signature has "ptr", not "ksym", but the description is
using "ksym". please make them consistent (might name param
"percpu_ptr")
> + *
> + * bpf_per_cpu_ptr() has the same semantic as per_cpu_ptr() in the
> + * kernel, except that bpf_per_cpu_ptr() may return NULL. This
> + * happens if *cpu* is larger than nr_cpu_ids. The caller of
> + * bpf_per_cpu_ptr() must check the returned value.
> + * Return
> + * A generic pointer pointing to the variable on *cpu*.
> */
[...]
> + } else if (arg_type == ARG_PTR_TO_PERCPU_BTF_ID) {
> + expected_type = PTR_TO_PERCPU_BTF_ID;
> + if (type != expected_type)
> + goto err_type;
> + if (!reg->btf_id) {
> + verbose(env, "Helper has zero btf_id in R%d\n", regno);
nit: "invalid btf_id"?
> + return -EACCES;
> + }
> + meta->ret_btf_id = reg->btf_id;
> } else if (arg_type == ARG_PTR_TO_BTF_ID) {
> expected_type = PTR_TO_BTF_ID;
> if (type != expected_type)
> @@ -4904,6 +4918,30 @@ 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;
[...]
On Fri, Aug 21, 2020 at 8:26 PM Andrii Nakryiko
<[email protected]> wrote:
>
> On Wed, Aug 19, 2020 at 3:42 PM Hao Luo <[email protected]> wrote:
> >
> > Add bpf_per_cpu_ptr() to help bpf programs access percpu vars.
> > bpf_per_cpu_ptr() has the same semantic as per_cpu_ptr() in the kernel
> > except that it may return NULL. This happens when the cpu parameter is
> > out of range. So the caller must check the returned value.
> >
> > Signed-off-by: Hao Luo <[email protected]>
> > ---
>
> The logic looks correct, few naming nits, but otherwise:
>
> Acked-by: Andrii Nakryiko <[email protected]>
>
> > include/linux/bpf.h | 3 ++
> > include/linux/btf.h | 11 +++++++
> > include/uapi/linux/bpf.h | 14 +++++++++
> > kernel/bpf/btf.c | 10 -------
> > kernel/bpf/verifier.c | 64 ++++++++++++++++++++++++++++++++++++++--
> > kernel/trace/bpf_trace.c | 18 +++++++++++
> > 6 files changed, 107 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 55f694b63164..613404beab33 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -268,6 +268,7 @@ enum bpf_arg_type {
> > ARG_PTR_TO_ALLOC_MEM, /* pointer to dynamically allocated memory */
> > ARG_PTR_TO_ALLOC_MEM_OR_NULL, /* pointer to dynamically allocated memory or NULL */
> > ARG_CONST_ALLOC_SIZE_OR_ZERO, /* number of allocated bytes requested */
> > + ARG_PTR_TO_PERCPU_BTF_ID, /* pointer to in-kernel percpu type */
> > };
> >
> > /* type of values returned from helper functions */
> > @@ -281,6 +282,7 @@ enum bpf_return_type {
> > RET_PTR_TO_SOCK_COMMON_OR_NULL, /* returns a pointer to a sock_common or NULL */
> > 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_OR_NULL, /* returns a pointer to a valid memory or a btf_id or NULL */
>
> I know it's already very long, but still let's use BTF_ID consistently
>
> > };
> >
> > /* eBPF function prototype used by verifier to allow BPF_CALLs from eBPF programs
> > @@ -360,6 +362,7 @@ enum bpf_reg_type {
> > PTR_TO_RDONLY_BUF_OR_NULL, /* reg points to a readonly buffer or NULL */
> > PTR_TO_RDWR_BUF, /* reg points to a read/write buffer */
> > PTR_TO_RDWR_BUF_OR_NULL, /* reg points to a read/write buffer or NULL */
> > + PTR_TO_PERCPU_BTF_ID, /* reg points to percpu kernel type */
> > };
> >
>
> [...]
>
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 468376f2910b..c7e49a102ed2 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -3415,6 +3415,19 @@ union bpf_attr {
> > * A non-negative value equal to or less than *size* on success,
> > * or a negative error in case of failure.
> > *
> > + * void *bpf_per_cpu_ptr(const void *ptr, u32 cpu)
btw, having bpf_this_cpu_ptr(const void *ptr) seems worthwhile as well, WDYT?
> > + * Description
> > + * Take the address of a percpu ksym and return a pointer pointing
> > + * to the variable on *cpu*. A ksym is an extern variable decorated
> > + * with '__ksym'. A ksym is percpu if there is a global percpu var
> > + * (either static or global) defined of the same name in the kernel.
>
> The function signature has "ptr", not "ksym", but the description is
> using "ksym". please make them consistent (might name param
> "percpu_ptr")
>
> > + *
> > + * bpf_per_cpu_ptr() has the same semantic as per_cpu_ptr() in the
> > + * kernel, except that bpf_per_cpu_ptr() may return NULL. This
> > + * happens if *cpu* is larger than nr_cpu_ids. The caller of
> > + * bpf_per_cpu_ptr() must check the returned value.
> > + * Return
> > + * A generic pointer pointing to the variable on *cpu*.
> > */
>
> [...]
>
> > + } else if (arg_type == ARG_PTR_TO_PERCPU_BTF_ID) {
> > + expected_type = PTR_TO_PERCPU_BTF_ID;
> > + if (type != expected_type)
> > + goto err_type;
> > + if (!reg->btf_id) {
> > + verbose(env, "Helper has zero btf_id in R%d\n", regno);
>
> nit: "invalid btf_id"?
>
> > + return -EACCES;
> > + }
> > + meta->ret_btf_id = reg->btf_id;
> > } else if (arg_type == ARG_PTR_TO_BTF_ID) {
> > expected_type = PTR_TO_BTF_ID;
> > if (type != expected_type)
> > @@ -4904,6 +4918,30 @@ 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;
>
> [...]
On Fri, Aug 21, 2020 at 8:31 PM Andrii Nakryiko
<[email protected]> wrote:
>
> On Fri, Aug 21, 2020 at 8:26 PM Andrii Nakryiko
> <[email protected]> wrote:
> >
> > On Wed, Aug 19, 2020 at 3:42 PM Hao Luo <[email protected]> wrote:
> > >
> > > Add bpf_per_cpu_ptr() to help bpf programs access percpu vars.
> > > bpf_per_cpu_ptr() has the same semantic as per_cpu_ptr() in the kernel
> > > except that it may return NULL. This happens when the cpu parameter is
> > > out of range. So the caller must check the returned value.
> > >
> > > Signed-off-by: Hao Luo <[email protected]>
> > > ---
> >
> > The logic looks correct, few naming nits, but otherwise:
> >
> > Acked-by: Andrii Nakryiko <[email protected]>
> >
> > > include/linux/bpf.h | 3 ++
> > > include/linux/btf.h | 11 +++++++
> > > include/uapi/linux/bpf.h | 14 +++++++++
> > > kernel/bpf/btf.c | 10 -------
> > > kernel/bpf/verifier.c | 64 ++++++++++++++++++++++++++++++++++++++--
> > > kernel/trace/bpf_trace.c | 18 +++++++++++
> > > 6 files changed, 107 insertions(+), 13 deletions(-)
[...]
>
> btw, having bpf_this_cpu_ptr(const void *ptr) seems worthwhile as well, WDYT?
>
It's probably not a good idea, IMHO. How does it interact with
preemption? Should we treat it as __this_cpu_ptr()? If so, I feel it's
easy to be misused, if the bpf program is called in a preemptible
context.
Btw, is bpf programs always called with preemption disabled? How about
interrupts? I haven't thought about these questions before but I think
they matter as we start to have more ways for bpf programs to interact
with the kernel.
Best,
Hao
On Sat, Aug 22, 2020 at 12:49 AM Hao Luo <[email protected]> wrote:
>
> On Fri, Aug 21, 2020 at 8:31 PM Andrii Nakryiko
> <[email protected]> wrote:
> >
> > On Fri, Aug 21, 2020 at 8:26 PM Andrii Nakryiko
> > <[email protected]> wrote:
> > >
> > > On Wed, Aug 19, 2020 at 3:42 PM Hao Luo <[email protected]> wrote:
> > > >
> > > > Add bpf_per_cpu_ptr() to help bpf programs access percpu vars.
> > > > bpf_per_cpu_ptr() has the same semantic as per_cpu_ptr() in the kernel
> > > > except that it may return NULL. This happens when the cpu parameter is
> > > > out of range. So the caller must check the returned value.
> > > >
> > > > Signed-off-by: Hao Luo <[email protected]>
> > > > ---
> > >
> > > The logic looks correct, few naming nits, but otherwise:
> > >
> > > Acked-by: Andrii Nakryiko <[email protected]>
> > >
> > > > include/linux/bpf.h | 3 ++
> > > > include/linux/btf.h | 11 +++++++
> > > > include/uapi/linux/bpf.h | 14 +++++++++
> > > > kernel/bpf/btf.c | 10 -------
> > > > kernel/bpf/verifier.c | 64 ++++++++++++++++++++++++++++++++++++++--
> > > > kernel/trace/bpf_trace.c | 18 +++++++++++
> > > > 6 files changed, 107 insertions(+), 13 deletions(-)
> [...]
> >
> > btw, having bpf_this_cpu_ptr(const void *ptr) seems worthwhile as well, WDYT?
> >
>
> It's probably not a good idea, IMHO. How does it interact with
> preemption? Should we treat it as __this_cpu_ptr()? If so, I feel it's
> easy to be misused, if the bpf program is called in a preemptible
> context.
>
> Btw, is bpf programs always called with preemption disabled? How about
> interrupts? I haven't thought about these questions before but I think
> they matter as we start to have more ways for bpf programs to interact
> with the kernel.
non-sleepable BPF is always disabling CPU migration, so there is no
problem with this_cpu_ptr. For sleepable not sure, but we can disable
this helper for sleepable BPF programs, if that's a problem.
>
> Best,
> Hao
On Sat, Aug 22, 2020 at 12:55 AM Andrii Nakryiko
<[email protected]> wrote:
>
> On Sat, Aug 22, 2020 at 12:49 AM Hao Luo <[email protected]> wrote:
> >
> > On Fri, Aug 21, 2020 at 8:31 PM Andrii Nakryiko
> > <[email protected]> wrote:
> > >
> > > On Fri, Aug 21, 2020 at 8:26 PM Andrii Nakryiko
> > > <[email protected]> wrote:
> > > >
> > > > On Wed, Aug 19, 2020 at 3:42 PM Hao Luo <[email protected]> wrote:
> > > > >
> > > > > Add bpf_per_cpu_ptr() to help bpf programs access percpu vars.
> > > > > bpf_per_cpu_ptr() has the same semantic as per_cpu_ptr() in the kernel
> > > > > except that it may return NULL. This happens when the cpu parameter is
> > > > > out of range. So the caller must check the returned value.
> > > > >
> > > > > Signed-off-by: Hao Luo <[email protected]>
> > > > > ---
> > > >
> > > > The logic looks correct, few naming nits, but otherwise:
> > > >
> > > > Acked-by: Andrii Nakryiko <[email protected]>
> > > >
> > > > > include/linux/bpf.h | 3 ++
> > > > > include/linux/btf.h | 11 +++++++
> > > > > include/uapi/linux/bpf.h | 14 +++++++++
> > > > > kernel/bpf/btf.c | 10 -------
> > > > > kernel/bpf/verifier.c | 64 ++++++++++++++++++++++++++++++++++++++--
> > > > > kernel/trace/bpf_trace.c | 18 +++++++++++
> > > > > 6 files changed, 107 insertions(+), 13 deletions(-)
> > [...]
> > >
> > > btw, having bpf_this_cpu_ptr(const void *ptr) seems worthwhile as well, WDYT?
> > >
> >
> > It's probably not a good idea, IMHO. How does it interact with
> > preemption? Should we treat it as __this_cpu_ptr()? If so, I feel it's
> > easy to be misused, if the bpf program is called in a preemptible
> > context.
> >
> > Btw, is bpf programs always called with preemption disabled? How about
> > interrupts? I haven't thought about these questions before but I think
> > they matter as we start to have more ways for bpf programs to interact
> > with the kernel.
>
> non-sleepable BPF is always disabling CPU migration, so there is no
> problem with this_cpu_ptr. For sleepable not sure, but we can disable
> this helper for sleepable BPF programs, if that's a problem.
>
Sounds good. I see there is bpf_get_smp_processor_id() and we are
already doing this. I can add this_cpu_ptr() in v2.
Hao