On Wed, Aug 19, 2020 at 3:42 PM Hao Luo <hao...@google.com> 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 <hao...@google.com>
> ---
The logic looks correct, few naming nits, but otherwise:
Acked-by: Andrii Nakryiko <andr...@fb.com>
> 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;
[...]
