| Message ID | 20210920141526.3940002-7-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Support kernel module function calls from eBPF | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next | fail | VM_Test |
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: john.fastabend@gmail.com kpsingh@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 86 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Mon, Sep 20, 2021 at 7:15 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > This patch adds libbpf support for kernel module function call support. > The fd_array parameter is used during BPF program load is used to pass > module BTFs referenced by the program. insn->off is set to index into > this array, but starts from 1, because insn->off as 0 is reserved for > btf_vmlinux. > > We try to use existing insn->off for a module, since the kernel limits > the maximum distinct module BTFs for kfuncs to 256, and also because > index must never exceed the maximum allowed value that can fit in > insn->off (INT16_MAX). In the future, if kernel interprets signed offset > as unsigned for kfunc calls, this limit can be increased to UINT16_MAX. > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > tools/lib/bpf/bpf.c | 1 + > tools/lib/bpf/libbpf.c | 58 +++++++++++++++++++++++++++++++-- > tools/lib/bpf/libbpf_internal.h | 1 + > 3 files changed, 57 insertions(+), 3 deletions(-) > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > index 2401fad090c5..7d1741ceaa32 100644 > --- a/tools/lib/bpf/bpf.c > +++ b/tools/lib/bpf/bpf.c > @@ -264,6 +264,7 @@ int libbpf__bpf_prog_load(const struct bpf_prog_load_params *load_attr) > attr.line_info_rec_size = load_attr->line_info_rec_size; > attr.line_info_cnt = load_attr->line_info_cnt; > attr.line_info = ptr_to_u64(load_attr->line_info); > + attr.fd_array = ptr_to_u64(load_attr->fd_array); > > if (load_attr->name) > memcpy(attr.prog_name, load_attr->name, > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index da65a1666a5e..3049dfc6088e 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -420,6 +420,12 @@ struct extern_desc { > > /* local btf_id of the ksym extern's type. */ > __u32 type_id; > + /* offset to be patched in for insn->off, > + * this is 0 for btf_vmlinux, and index + 1 What does "index + 1" mean here? Seems like kernel code is using the offset as is, without any -1 compensation. > + * for module BTF, where index is BTF index in > + * obj->fd_array > + */ > + __s16 offset; > } ksym; > }; > }; > @@ -516,6 +522,10 @@ struct bpf_object { > void *priv; > bpf_object_clear_priv_t clear_priv; > > + int *fd_array; > + size_t fd_cap_cnt; > + int nr_fds; > + > char path[]; > }; > #define obj_elf_valid(o) ((o)->efile.elf) > @@ -5357,6 +5367,7 @@ bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog) > ext = &obj->externs[relo->sym_off]; > insn[0].src_reg = BPF_PSEUDO_KFUNC_CALL; > insn[0].imm = ext->ksym.kernel_btf_id; > + insn[0].off = ext->ksym.offset; > break; > case RELO_SUBPROG_ADDR: > if (insn[0].src_reg != BPF_PSEUDO_FUNC) { > @@ -6151,6 +6162,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt, > } > load_attr.log_level = prog->log_level; > load_attr.prog_flags = prog->prog_flags; > + load_attr.fd_array = prog->obj->fd_array; > > if (prog->obj->gen_loader) { > bpf_gen__prog_load(prog->obj->gen_loader, &load_attr, > @@ -6763,9 +6775,46 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj, > } > > if (kern_btf != obj->btf_vmlinux) { > - pr_warn("extern (func ksym) '%s': function in kernel module is not supported\n", > - ext->name); > - return -ENOTSUP; > + int index = -1; > + > + if (!obj->fd_array) { > + obj->fd_array = calloc(8, sizeof(*obj->fd_array)); > + if (!obj->fd_array) > + return -ENOMEM; > + obj->fd_cap_cnt = 8; > + /* index = 0 is for vmlinux BTF, so skip it */ > + obj->nr_fds = 1; > + } this doesn't make sense, you use libbpf_ensure_mem() and shouldn't do anything like this, it's all taken care of already > + > + for (int i = 0; i < obj->nr_fds; i++) { > + if (obj->fd_array[i] == kern_btf_fd) { > + index = i; > + break; > + } > + } we can actually avoid all this. We already have a list of module BTFs in bpf_object (obj->btf_modules), where we remember their id, fd, etc. We can also remember their fd_arr_idx for quick lookup. Just teach find_ksym_btf_id() to optionally return struct module_btf * and use that to find/set idx. That seems cleaner and probably would be easier in the future as well. > + > + if (index == -1) { > + if (obj->nr_fds == obj->fd_cap_cnt) { don't check, libbpf_ensure_mem() handles that > + ret = libbpf_ensure_mem((void **)&obj->fd_array, > + &obj->fd_cap_cnt, sizeof(int), > + obj->fd_cap_cnt + 1); > + if (ret) > + return ret; > + } > + > + index = obj->nr_fds; > + obj->fd_array[obj->nr_fds++] = kern_btf_fd; > + } > + > + if (index > INT16_MAX) { > + /* insn->off is s16 */ > + pr_warn("extern (func ksym) '%s': module btf fd index too big\n", > + ext->name); can you log index value here as well? "module BTF FD index %d is too big\n"? > + return -E2BIG; > + } > + ext->ksym.offset = index; > + } else { > + ext->ksym.offset = 0; > } I think it will be cleaner if you move the entire offset determination logic after all the other checks are performed and ext is mostly populated. That will also make the logic shorter and simpler because if you find kern_btf_fd match, you can exit early (or probably rather goto to report the match and exit). Otherwise > > kern_func = btf__type_by_id(kern_btf, kfunc_id); this is actually extremely wasteful for module BTFs. Let's add internal (at least for now) helper that will search only for "own" BTF types in the BTF, skipping types in base BTF. Something like btf_type_by_id_own()? > @@ -6941,6 +6990,9 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr) > err = bpf_gen__finish(obj->gen_loader); > } > > + /* clean up fd_array */ > + zfree(&obj->fd_array); > + > /* clean up module BTFs */ > for (i = 0; i < obj->btf_module_cnt; i++) { > close(obj->btf_modules[i].fd); > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h > index ceb0c98979bc..44b8f381b035 100644 > --- a/tools/lib/bpf/libbpf_internal.h > +++ b/tools/lib/bpf/libbpf_internal.h > @@ -291,6 +291,7 @@ struct bpf_prog_load_params { > __u32 log_level; > char *log_buf; > size_t log_buf_sz; > + int *fd_array; > }; > > int libbpf__bpf_prog_load(const struct bpf_prog_load_params *load_attr); > -- > 2.33.0 >
On Wed, Sep 22, 2021 at 04:11:13AM IST, Andrii Nakryiko wrote: > On Mon, Sep 20, 2021 at 7:15 AM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > [...] > > + return -E2BIG; > > + } > > + ext->ksym.offset = index; > > > + } else { > > + ext->ksym.offset = 0; > > } > > I think it will be cleaner if you move the entire offset determination > logic after all the other checks are performed and ext is mostly > populated. That will also make the logic shorter and simpler because > if ayou find kern_btf_fd match, you can exit early (or probably rather Ack to everything else (including the other mail), but... > goto to report the match and exit). Otherwise > This sentence got eaten up. > > > > kern_func = btf__type_by_id(kern_btf, kfunc_id); > > this is actually extremely wasteful for module BTFs. Let's add > internal (at least for now) helper that will search only for "own" BTF > types in the BTF, skipping types in base BTF. Something like > btf_type_by_id_own()? > Just to make sure I am not misunderstanding: I don't see where this is wasteful. btf_type_by_id seems to not be searching anything, but just returns pointer in base BTF if kfunc_id < btf->start_id, otherwise in module BTF. What am I missing? I guess the 'kern_btf' name was the source of confusion? If so, I'll rename it. Thanks. -- Kartikeya
On Fri, Sep 24, 2021 at 4:54 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Wed, Sep 22, 2021 at 04:11:13AM IST, Andrii Nakryiko wrote: > > On Mon, Sep 20, 2021 at 7:15 AM Kumar Kartikeya Dwivedi > > <memxor@gmail.com> wrote: > > > [...] > > > + return -E2BIG; > > > + } > > > + ext->ksym.offset = index; > > > > > + } else { > > > + ext->ksym.offset = 0; > > > } > > > > I think it will be cleaner if you move the entire offset determination > > logic after all the other checks are performed and ext is mostly > > populated. That will also make the logic shorter and simpler because > > if ayou find kern_btf_fd match, you can exit early (or probably rather > > Ack to everything else (including the other mail), but... > > > goto to report the match and exit). Otherwise > > > > This sentence got eaten up. No idea what I was going to say here, sorry... Sometimes Gmail UI glitches with undo/redo, maybe that's what happened here. Doesn't matter, ignore the "Otherwise" part. > > > > > > > kern_func = btf__type_by_id(kern_btf, kfunc_id); > > > > this is actually extremely wasteful for module BTFs. Let's add > > internal (at least for now) helper that will search only for "own" BTF > > types in the BTF, skipping types in base BTF. Something like > > btf_type_by_id_own()? > > > > Just to make sure I am not misunderstanding: I don't see where this is wasteful. > btf_type_by_id seems to not be searching anything, but just returns pointer in > base BTF if kfunc_id < btf->start_id, otherwise in module BTF. > Hm, sorry... Right sentiment and thought, but wrong piece of code to quote it on. I had in mind the btf__find_by_name_kind() use in find_ksym_btf_id(). Once we start going over each module, we shouldn't be re-checking vmlinux BTF when doing btf__find_by_name_kind. It should only check the types that each module BTF adds on top of vmlinux BTF. That's what would be good to optimize, especially as more complicated BPF programs will start using more ksym vars and funcs. > What am I missing? I guess the 'kern_btf' name was the source of confusion? If > so, I'll rename it. > > Thanks. > > -- > Kartikeya
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c index 2401fad090c5..7d1741ceaa32 100644 --- a/tools/lib/bpf/bpf.c +++ b/tools/lib/bpf/bpf.c @@ -264,6 +264,7 @@ int libbpf__bpf_prog_load(const struct bpf_prog_load_params *load_attr) attr.line_info_rec_size = load_attr->line_info_rec_size; attr.line_info_cnt = load_attr->line_info_cnt; attr.line_info = ptr_to_u64(load_attr->line_info); + attr.fd_array = ptr_to_u64(load_attr->fd_array); if (load_attr->name) memcpy(attr.prog_name, load_attr->name, diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index da65a1666a5e..3049dfc6088e 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -420,6 +420,12 @@ struct extern_desc { /* local btf_id of the ksym extern's type. */ __u32 type_id; + /* offset to be patched in for insn->off, + * this is 0 for btf_vmlinux, and index + 1 + * for module BTF, where index is BTF index in + * obj->fd_array + */ + __s16 offset; } ksym; }; }; @@ -516,6 +522,10 @@ struct bpf_object { void *priv; bpf_object_clear_priv_t clear_priv; + int *fd_array; + size_t fd_cap_cnt; + int nr_fds; + char path[]; }; #define obj_elf_valid(o) ((o)->efile.elf) @@ -5357,6 +5367,7 @@ bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog) ext = &obj->externs[relo->sym_off]; insn[0].src_reg = BPF_PSEUDO_KFUNC_CALL; insn[0].imm = ext->ksym.kernel_btf_id; + insn[0].off = ext->ksym.offset; break; case RELO_SUBPROG_ADDR: if (insn[0].src_reg != BPF_PSEUDO_FUNC) { @@ -6151,6 +6162,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt, } load_attr.log_level = prog->log_level; load_attr.prog_flags = prog->prog_flags; + load_attr.fd_array = prog->obj->fd_array; if (prog->obj->gen_loader) { bpf_gen__prog_load(prog->obj->gen_loader, &load_attr, @@ -6763,9 +6775,46 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj, } if (kern_btf != obj->btf_vmlinux) { - pr_warn("extern (func ksym) '%s': function in kernel module is not supported\n", - ext->name); - return -ENOTSUP; + int index = -1; + + if (!obj->fd_array) { + obj->fd_array = calloc(8, sizeof(*obj->fd_array)); + if (!obj->fd_array) + return -ENOMEM; + obj->fd_cap_cnt = 8; + /* index = 0 is for vmlinux BTF, so skip it */ + obj->nr_fds = 1; + } + + for (int i = 0; i < obj->nr_fds; i++) { + if (obj->fd_array[i] == kern_btf_fd) { + index = i; + break; + } + } + + if (index == -1) { + if (obj->nr_fds == obj->fd_cap_cnt) { + ret = libbpf_ensure_mem((void **)&obj->fd_array, + &obj->fd_cap_cnt, sizeof(int), + obj->fd_cap_cnt + 1); + if (ret) + return ret; + } + + index = obj->nr_fds; + obj->fd_array[obj->nr_fds++] = kern_btf_fd; + } + + if (index > INT16_MAX) { + /* insn->off is s16 */ + pr_warn("extern (func ksym) '%s': module btf fd index too big\n", + ext->name); + return -E2BIG; + } + ext->ksym.offset = index; + } else { + ext->ksym.offset = 0; } kern_func = btf__type_by_id(kern_btf, kfunc_id); @@ -6941,6 +6990,9 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr) err = bpf_gen__finish(obj->gen_loader); } + /* clean up fd_array */ + zfree(&obj->fd_array); + /* clean up module BTFs */ for (i = 0; i < obj->btf_module_cnt; i++) { close(obj->btf_modules[i].fd); diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h index ceb0c98979bc..44b8f381b035 100644 --- a/tools/lib/bpf/libbpf_internal.h +++ b/tools/lib/bpf/libbpf_internal.h @@ -291,6 +291,7 @@ struct bpf_prog_load_params { __u32 log_level; char *log_buf; size_t log_buf_sz; + int *fd_array; }; int libbpf__bpf_prog_load(const struct bpf_prog_load_params *load_attr);
This patch adds libbpf support for kernel module function call support. The fd_array parameter is used during BPF program load is used to pass module BTFs referenced by the program. insn->off is set to index into this array, but starts from 1, because insn->off as 0 is reserved for btf_vmlinux. We try to use existing insn->off for a module, since the kernel limits the maximum distinct module BTFs for kfuncs to 256, and also because index must never exceed the maximum allowed value that can fit in insn->off (INT16_MAX). In the future, if kernel interprets signed offset as unsigned for kfunc calls, this limit can be increased to UINT16_MAX. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- tools/lib/bpf/bpf.c | 1 + tools/lib/bpf/libbpf.c | 58 +++++++++++++++++++++++++++++++-- tools/lib/bpf/libbpf_internal.h | 1 + 3 files changed, 57 insertions(+), 3 deletions(-)