| Message ID | 20220220134813.3411982-2-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Introduce typed pointer support in BPF maps | expand |
On Sun, Feb 20, 2022 at 07:17:59PM +0530, Kumar Kartikeya Dwivedi wrote: > In next few patches, we need a helper that searches all kernel BTFs > (vmlinux and module BTFs), and finds the type denoted by 'name' and > 'kind'. Turns out bpf_btf_find_by_name_kind already does the same thing, > but it instead returns a BTF ID and optionally fd (if module BTF). This > is used for relocating ksyms in BPF loader code (bpftool gen skel -L). > > We extract the core code out into a new helper > btf_find_by_name_kind_all, which returns the BTF ID and BTF pointer in > an out parameter. The reference for the returned BTF pointer is only > bumped if it is a module BTF, this needs to be kept in mind when using > this helper. > > Hence, the user must release the BTF reference iff btf_is_module is > true, otherwise transfer the ownership to e.g. an fd. > > In case of the helper, the fd is only allocated for module BTFs, so no > extra handling for btf_vmlinux case is required. > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > kernel/bpf/btf.c | 47 +++++++++++++++++++++++++++++++---------------- > 1 file changed, 31 insertions(+), 16 deletions(-) > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 2c4c5dbe2abe..3645d8c14a18 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -6545,16 +6545,10 @@ static struct btf *btf_get_module_btf(const struct module *module) > return btf; > } > > -BPF_CALL_4(bpf_btf_find_by_name_kind, char *, name, int, name_sz, u32, kind, int, flags) > +static s32 btf_find_by_name_kind_all(const char *name, u32 kind, struct btf **btfp) The name is getting too long. How about bpf_find_btf_id() ? > { > struct btf *btf; > - long ret; > - > - if (flags) > - return -EINVAL; > - > - if (name_sz <= 1 || name[name_sz - 1]) > - return -EINVAL; > + s32 ret; > > btf = bpf_get_btf_vmlinux(); > if (IS_ERR(btf)) > @@ -6580,19 +6574,40 @@ BPF_CALL_4(bpf_btf_find_by_name_kind, char *, name, int, name_sz, u32, kind, int > spin_unlock_bh(&btf_idr_lock); > ret = btf_find_by_name_kind(mod_btf, name, kind); > if (ret > 0) { > - int btf_obj_fd; > - > - btf_obj_fd = __btf_new_fd(mod_btf); > - if (btf_obj_fd < 0) { > - btf_put(mod_btf); > - return btf_obj_fd; > - } > - return ret | (((u64)btf_obj_fd) << 32); > + *btfp = mod_btf; > + return ret; > } > spin_lock_bh(&btf_idr_lock); > btf_put(mod_btf); > } > spin_unlock_bh(&btf_idr_lock); > + } else { > + *btfp = btf; > + } Since we're refactoring let's drop the indent. How about if (ret > 0) { *btfp = btf; return ret; } idr_for_each_entry(). and move the func right after btf_find_by_name_kind(), so that later patch doesn't need to do: static s32 bpf_find_btf_id(); Eventually this helper might become global with this name. Also may be do btf_get() for vmlinux_btf too? In case it reduces 'if (btf_is_module())' checks.
On Tue, Feb 22, 2022 at 10:58:11AM IST, Alexei Starovoitov wrote: > On Sun, Feb 20, 2022 at 07:17:59PM +0530, Kumar Kartikeya Dwivedi wrote: > > In next few patches, we need a helper that searches all kernel BTFs > > (vmlinux and module BTFs), and finds the type denoted by 'name' and > > 'kind'. Turns out bpf_btf_find_by_name_kind already does the same thing, > > but it instead returns a BTF ID and optionally fd (if module BTF). This > > is used for relocating ksyms in BPF loader code (bpftool gen skel -L). > > > > We extract the core code out into a new helper > > btf_find_by_name_kind_all, which returns the BTF ID and BTF pointer in > > an out parameter. The reference for the returned BTF pointer is only > > bumped if it is a module BTF, this needs to be kept in mind when using > > this helper. > > > > Hence, the user must release the BTF reference iff btf_is_module is > > true, otherwise transfer the ownership to e.g. an fd. > > > > In case of the helper, the fd is only allocated for module BTFs, so no > > extra handling for btf_vmlinux case is required. > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > kernel/bpf/btf.c | 47 +++++++++++++++++++++++++++++++---------------- > > 1 file changed, 31 insertions(+), 16 deletions(-) > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > index 2c4c5dbe2abe..3645d8c14a18 100644 > > --- a/kernel/bpf/btf.c > > +++ b/kernel/bpf/btf.c > > @@ -6545,16 +6545,10 @@ static struct btf *btf_get_module_btf(const struct module *module) > > return btf; > > } > > > > -BPF_CALL_4(bpf_btf_find_by_name_kind, char *, name, int, name_sz, u32, kind, int, flags) > > +static s32 btf_find_by_name_kind_all(const char *name, u32 kind, struct btf **btfp) > > The name is getting too long. > How about bpf_find_btf_id() ? > > > { > > struct btf *btf; > > - long ret; > > - > > - if (flags) > > - return -EINVAL; > > - > > - if (name_sz <= 1 || name[name_sz - 1]) > > - return -EINVAL; > > + s32 ret; > > > > btf = bpf_get_btf_vmlinux(); > > if (IS_ERR(btf)) > > @@ -6580,19 +6574,40 @@ BPF_CALL_4(bpf_btf_find_by_name_kind, char *, name, int, name_sz, u32, kind, int > > spin_unlock_bh(&btf_idr_lock); > > ret = btf_find_by_name_kind(mod_btf, name, kind); > > if (ret > 0) { > > - int btf_obj_fd; > > - > > - btf_obj_fd = __btf_new_fd(mod_btf); > > - if (btf_obj_fd < 0) { > > - btf_put(mod_btf); > > - return btf_obj_fd; > > - } > > - return ret | (((u64)btf_obj_fd) << 32); > > + *btfp = mod_btf; > > + return ret; > > } > > spin_lock_bh(&btf_idr_lock); > > btf_put(mod_btf); > > } > > spin_unlock_bh(&btf_idr_lock); > > + } else { > > + *btfp = btf; > > + } > > Since we're refactoring let's drop the indent. > How about > if (ret > 0) { > *btfp = btf; > return ret; > } > idr_for_each_entry(). > > and move the func right after btf_find_by_name_kind(), > so that later patch doesn't need to do: > static s32 bpf_find_btf_id(); > Eventually this helper might become global with this name. > Ok, will change. > Also may be do btf_get() for vmlinux_btf too? > In case it reduces 'if (btf_is_module())' checks. Right, should also change this for btf_get_module_btf then, to make things consistent. -- Kartikeya
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 2c4c5dbe2abe..3645d8c14a18 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -6545,16 +6545,10 @@ static struct btf *btf_get_module_btf(const struct module *module) return btf; } -BPF_CALL_4(bpf_btf_find_by_name_kind, char *, name, int, name_sz, u32, kind, int, flags) +static s32 btf_find_by_name_kind_all(const char *name, u32 kind, struct btf **btfp) { struct btf *btf; - long ret; - - if (flags) - return -EINVAL; - - if (name_sz <= 1 || name[name_sz - 1]) - return -EINVAL; + s32 ret; btf = bpf_get_btf_vmlinux(); if (IS_ERR(btf)) @@ -6580,19 +6574,40 @@ BPF_CALL_4(bpf_btf_find_by_name_kind, char *, name, int, name_sz, u32, kind, int spin_unlock_bh(&btf_idr_lock); ret = btf_find_by_name_kind(mod_btf, name, kind); if (ret > 0) { - int btf_obj_fd; - - btf_obj_fd = __btf_new_fd(mod_btf); - if (btf_obj_fd < 0) { - btf_put(mod_btf); - return btf_obj_fd; - } - return ret | (((u64)btf_obj_fd) << 32); + *btfp = mod_btf; + return ret; } spin_lock_bh(&btf_idr_lock); btf_put(mod_btf); } spin_unlock_bh(&btf_idr_lock); + } else { + *btfp = btf; + } + return ret; +} + +BPF_CALL_4(bpf_btf_find_by_name_kind, char *, name, int, name_sz, u32, kind, int, flags) +{ + struct btf *btf = NULL; + int btf_obj_fd = 0; + long ret; + + if (flags) + return -EINVAL; + + if (name_sz <= 1 || name[name_sz - 1]) + return -EINVAL; + + ret = btf_find_by_name_kind_all(name, kind, &btf); + if (ret > 0 && btf_is_module(btf)) { + /* reference for btf is only raised if module BTF */ + btf_obj_fd = __btf_new_fd(btf); + if (btf_obj_fd < 0) { + btf_put(btf); + return btf_obj_fd; + } + return ret | (((u64)btf_obj_fd) << 32); } return ret; }
In next few patches, we need a helper that searches all kernel BTFs (vmlinux and module BTFs), and finds the type denoted by 'name' and 'kind'. Turns out bpf_btf_find_by_name_kind already does the same thing, but it instead returns a BTF ID and optionally fd (if module BTF). This is used for relocating ksyms in BPF loader code (bpftool gen skel -L). We extract the core code out into a new helper btf_find_by_name_kind_all, which returns the BTF ID and BTF pointer in an out parameter. The reference for the returned BTF pointer is only bumped if it is a module BTF, this needs to be kept in mind when using this helper. Hence, the user must release the BTF reference iff btf_is_module is true, otherwise transfer the ownership to e.g. an fd. In case of the helper, the fd is only allocated for module BTFs, so no extra handling for btf_vmlinux case is required. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- kernel/bpf/btf.c | 47 +++++++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 16 deletions(-)