Re: [PATCH bpf-next v3 4/6] bpf: Introduce bpf_per_cpu_ptr()

Hi, Andrii,

Thanks for taking a look. Sorry for the late reply. Spent some time on rebasing and fixing a build issue in my development environment that started happening in v5.9.

On Mon, Sep 21, 2020 at 11:09 AM Andrii Nakryiko andrii.nakryiko@gmail.com wrote:

On Wed, Sep 16, 2020 at 3:39 PM Hao Luo haoluo@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.

Acked-by: Andrii Nakryiko andriin@fb.com Signed-off-by: Hao Luo haoluo@google.com

---

include/linux/bpf.h | 4 +++ include/linux/btf.h | 11 ++++++ include/uapi/linux/bpf.h | 18 ++++++++++ kernel/bpf/btf.c | 10 ------ kernel/bpf/helpers.c | 18 ++++++++++ kernel/bpf/verifier.c | 64 ++++++++++++++++++++++++++++++++-- kernel/trace/bpf_trace.c | 2 ++ tools/include/uapi/linux/bpf.h | 18 ++++++++++ 8 files changed, 132 insertions(+), 13 deletions(-)

I already acked this, but see my concern about O(N) look up for .data..percpu. Feel free to follow up on this with a separate patch. Thanks!

[...]

...

@@ -4003,6 +4008,15 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, if (type != expected_type) goto err_type; }

•   } 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 invalid btf_id in R%d\n", regno);
  

•                   return -EACCES;
  

•           }
  

•           meta->ret_btf_id = reg->btf_id;
  

FYI, this will conflict with Lorenz's refactoring, so you might need to rebase and solve the conflicts if his patch set lands first.

Indeed. Do hit this while rebasing but managed to resolve it. Please take a look and let me know if you have comments there in v4

...

@@ -7413,6 +7451,7 @@ static int check_ld_imm(struct bpf_verifier_env *env, struct bpf_insn *insn) dst_reg->mem_size = aux->btf_var.mem_size; break; case PTR_TO_BTF_ID:

•           case PTR_TO_PERCPU_BTF_ID:
                    dst_reg->btf_id = aux->btf_var.btf_id;
                    break;
            default:
  

@@ -9313,10 +9352,14 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env, struct bpf_insn *insn, struct bpf_insn_aux_data *aux) {

•   u32 type, id = insn->imm;
  

•   u32 datasec_id, type, id = insn->imm;
  

•   const struct btf_var_secinfo *vsi;
  

•   const struct btf_type *datasec;
    const struct btf_type *t;
    const char *sym_name;
  

•   bool percpu = false;
    u64 addr;
  

•   int i;
  
    if (!btf_vmlinux) {
            verbose(env, "kernel is missing BTF, make sure CONFIG_DEBUG_INFO_BTF=y is specified in Kconfig.\n");
  

@@ -9348,12 +9391,27 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env, return -ENOENT; }

•   datasec_id = btf_find_by_name_kind(btf_vmlinux, ".data..percpu",
  

•                                      BTF_KIND_DATASEC);
  

this is a relatively expensive O(N) operation, it probably makes sense to cache it (there are about 80'000 types now in BTF for my typical kernel config, so iterating that much for every single ldimm64 for ksym is kind of expensive.

ACK. This currently works. I can do it in another patch.

Hao