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@xxxxxxxxx> wrote:
>
> On Wed, Sep 16, 2020 at 3:39 PM Hao Luo <haoluo@xxxxxxxxxx> 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@xxxxxx>
> > Signed-off-by: Hao Luo <haoluo@xxxxxxxxxx>
> > ---
> > 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
