| Message ID | 20201119232244.2776720-1-andrii@kernel.org |
|---|---|
| Headers | show |
| Series | libbpf: add support for kernel module BTF CO-RE relocations | expand |
On Thu, Nov 19, 2020 at 03:22:42PM -0800, Andrii Nakryiko wrote: > Teach libbpf to search for candidate types for CO-RE relocations across kernel > modules BTFs, in addition to vmlinux BTF. If at least one candidate type is > found in vmlinux BTF, kernel module BTFs are not iterated. If vmlinux BTF has > no matching candidates, then find all kernel module BTFs and search for all > matching candidates across all of them. > > Kernel's support for module BTFs are inferred from the support for BTF name > pointer in BPF UAPI. > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- > tools/lib/bpf/libbpf.c | 185 ++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 172 insertions(+), 13 deletions(-) > [...] > +static int probe_module_btf(void) > +{ > + static const char strs[] = "\0int"; > + __u32 types[] = { > + /* int */ > + BTF_TYPE_INT_ENC(1, BTF_INT_SIGNED, 0, 32, 4), > + }; > + struct bpf_btf_info info; > + __u32 len = sizeof(info); > + char name[16]; > + int fd, err; > + > + fd = libbpf__load_raw_btf((char *)types, sizeof(types), strs, sizeof(strs)); > + if (fd < 0) > + return 0; /* BTF not supported at all */ > + > + len = sizeof(info); nit: reinit of len > + memset(&info, 0, sizeof(info)); use len in memset > + info.name = ptr_to_u64(name); > + info.name_len = sizeof(name); > + > + /* check that BPF_OBJ_GET_INFO_BY_FD supports specifying name pointer; > + * kernel's module BTF support coincides with support for > + * name/name_len fields in struct bpf_btf_info. > + */ > + err = bpf_obj_get_info_by_fd(fd, &info, &len); > + close(fd); > + return !err; > +} [...]
On Thu, Nov 19, 2020 at 4:55 PM Maciej Fijalkowski <maciej.fijalkowski@intel.com> wrote: > > On Thu, Nov 19, 2020 at 03:22:42PM -0800, Andrii Nakryiko wrote: > > Teach libbpf to search for candidate types for CO-RE relocations across kernel > > modules BTFs, in addition to vmlinux BTF. If at least one candidate type is > > found in vmlinux BTF, kernel module BTFs are not iterated. If vmlinux BTF has > > no matching candidates, then find all kernel module BTFs and search for all > > matching candidates across all of them. > > > > Kernel's support for module BTFs are inferred from the support for BTF name > > pointer in BPF UAPI. > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > --- > > tools/lib/bpf/libbpf.c | 185 ++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 172 insertions(+), 13 deletions(-) > > > > [...] > > > +static int probe_module_btf(void) > > +{ > > + static const char strs[] = "\0int"; > > + __u32 types[] = { > > + /* int */ > > + BTF_TYPE_INT_ENC(1, BTF_INT_SIGNED, 0, 32, 4), > > + }; > > + struct bpf_btf_info info; > > + __u32 len = sizeof(info); > > + char name[16]; > > + int fd, err; > > + > > + fd = libbpf__load_raw_btf((char *)types, sizeof(types), strs, sizeof(strs)); > > + if (fd < 0) > > + return 0; /* BTF not supported at all */ > > + > > + len = sizeof(info); > > nit: reinit of len > oops, right, I'll remove it > > + memset(&info, 0, sizeof(info)); > > use len in memset why? > > > + info.name = ptr_to_u64(name); > > + info.name_len = sizeof(name); > > + > > + /* check that BPF_OBJ_GET_INFO_BY_FD supports specifying name pointer; > > + * kernel's module BTF support coincides with support for > > + * name/name_len fields in struct bpf_btf_info. > > + */ > > + err = bpf_obj_get_info_by_fd(fd, &info, &len); > > + close(fd); > > + return !err; > > +} > > [...]
On Thu, Nov 19, 2020 at 05:24:43PM -0800, Andrii Nakryiko wrote: > On Thu, Nov 19, 2020 at 4:55 PM Maciej Fijalkowski > <maciej.fijalkowski@intel.com> wrote: > > > > On Thu, Nov 19, 2020 at 03:22:42PM -0800, Andrii Nakryiko wrote: > > > Teach libbpf to search for candidate types for CO-RE relocations across kernel > > > modules BTFs, in addition to vmlinux BTF. If at least one candidate type is > > > found in vmlinux BTF, kernel module BTFs are not iterated. If vmlinux BTF has > > > no matching candidates, then find all kernel module BTFs and search for all > > > matching candidates across all of them. > > > > > > Kernel's support for module BTFs are inferred from the support for BTF name > > > pointer in BPF UAPI. > > > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > > --- > > > tools/lib/bpf/libbpf.c | 185 ++++++++++++++++++++++++++++++++++++++--- > > > 1 file changed, 172 insertions(+), 13 deletions(-) > > > > > > > [...] > > > > > +static int probe_module_btf(void) > > > +{ > > > + static const char strs[] = "\0int"; > > > + __u32 types[] = { > > > + /* int */ > > > + BTF_TYPE_INT_ENC(1, BTF_INT_SIGNED, 0, 32, 4), > > > + }; > > > + struct bpf_btf_info info; > > > + __u32 len = sizeof(info); > > > + char name[16]; > > > + int fd, err; > > > + > > > + fd = libbpf__load_raw_btf((char *)types, sizeof(types), strs, sizeof(strs)); > > > + if (fd < 0) > > > + return 0; /* BTF not supported at all */ > > > + > > > + len = sizeof(info); > > > > nit: reinit of len > > > > oops, right, I'll remove it > > > > > + memset(&info, 0, sizeof(info)); > > > > use len in memset > > why? Hm, just to make use of local var? We might argue that current version is more readable, but then again I would question the len's existence. Do whatever you want, these were just nits :) > > > > > > + info.name = ptr_to_u64(name); > > > + info.name_len = sizeof(name); > > > + > > > + /* check that BPF_OBJ_GET_INFO_BY_FD supports specifying name pointer; > > > + * kernel's module BTF support coincides with support for > > > + * name/name_len fields in struct bpf_btf_info. > > > + */ > > > + err = bpf_obj_get_info_by_fd(fd, &info, &len); > > > + close(fd); > > > + return !err; > > > +} > > > > [...]
On Thu, Nov 19, 2020 at 6:14 PM Maciej Fijalkowski <maciej.fijalkowski@intel.com> wrote: > > On Thu, Nov 19, 2020 at 05:24:43PM -0800, Andrii Nakryiko wrote: > > On Thu, Nov 19, 2020 at 4:55 PM Maciej Fijalkowski > > <maciej.fijalkowski@intel.com> wrote: > > > > > > On Thu, Nov 19, 2020 at 03:22:42PM -0800, Andrii Nakryiko wrote: > > > > Teach libbpf to search for candidate types for CO-RE relocations across kernel > > > > modules BTFs, in addition to vmlinux BTF. If at least one candidate type is > > > > found in vmlinux BTF, kernel module BTFs are not iterated. If vmlinux BTF has > > > > no matching candidates, then find all kernel module BTFs and search for all > > > > matching candidates across all of them. > > > > > > > > Kernel's support for module BTFs are inferred from the support for BTF name > > > > pointer in BPF UAPI. > > > > > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > > > --- > > > > tools/lib/bpf/libbpf.c | 185 ++++++++++++++++++++++++++++++++++++++--- > > > > 1 file changed, 172 insertions(+), 13 deletions(-) > > > > > > > > > > [...] > > > > > > > +static int probe_module_btf(void) > > > > +{ > > > > + static const char strs[] = "\0int"; > > > > + __u32 types[] = { > > > > + /* int */ > > > > + BTF_TYPE_INT_ENC(1, BTF_INT_SIGNED, 0, 32, 4), > > > > + }; > > > > + struct bpf_btf_info info; > > > > + __u32 len = sizeof(info); > > > > + char name[16]; > > > > + int fd, err; > > > > + > > > > + fd = libbpf__load_raw_btf((char *)types, sizeof(types), strs, sizeof(strs)); > > > > + if (fd < 0) > > > > + return 0; /* BTF not supported at all */ > > > > + > > > > + len = sizeof(info); > > > > > > nit: reinit of len > > > > > > > oops, right, I'll remove it > > > > > > > > + memset(&info, 0, sizeof(info)); > > > > > > use len in memset > > > > why? > > Hm, just to make use of local var? We might argue that current version is I agree, I think sizeof(info) is more readable. But my point is that if you suggest something, please provide at least some argument for why you think it's better or why existing code is worse or wrong (if you think it is). > more readable, but then again I would question the len's existence. len is passed to the kernel by reference and the kernel is updating it with the actual length it has (which could be <, ==, or > than what the program specified). So it has to be in a variable. > > Do whatever you want, these were just nits :) > > > > > > > > > > + info.name = ptr_to_u64(name); > > > > + info.name_len = sizeof(name); > > > > + > > > > + /* check that BPF_OBJ_GET_INFO_BY_FD supports specifying name pointer; > > > > + * kernel's module BTF support coincides with support for > > > > + * name/name_len fields in struct bpf_btf_info. > > > > + */ > > > > + err = bpf_obj_get_info_by_fd(fd, &info, &len); here -------------------------------------------------^^^^ > > > > + close(fd); > > > > + return !err; > > > > +} > > > > > > [...]
On Thu, Nov 19, 2020 at 03:22:39PM -0800, Andrii Nakryiko wrote: > __module_address() needs to be called with preemption disabled or with > module_mutex taken. preempt_disable() is enough for read-only uses, which is > what this fix does. Acked-by: Martin KaFai Lau <kafai@fb.com>
On Thu, Nov 19, 2020 at 03:22:40PM -0800, Andrii Nakryiko wrote: [ ... ] > +int btf__get_from_id(__u32 id, struct btf **btf) > +{ > + struct btf *res; > + int btf_fd; > + > + *btf = NULL; > + btf_fd = bpf_btf_get_fd_by_id(id); > + if (btf_fd < 0) > + return 0; It should return an error. > + > + res = btf_get_from_fd(btf_fd, NULL); > + close(btf_fd); > + if (IS_ERR(res)) > + return PTR_ERR(res); > + > + *btf = res; > + return 0; > } >
On Fri, Nov 20, 2020 at 10:20 AM Martin KaFai Lau <kafai@fb.com> wrote: > > On Thu, Nov 19, 2020 at 03:22:40PM -0800, Andrii Nakryiko wrote: > [ ... ] > > > +int btf__get_from_id(__u32 id, struct btf **btf) > > +{ > > + struct btf *res; > > + int btf_fd; > > + > > + *btf = NULL; > > + btf_fd = bpf_btf_get_fd_by_id(id); > > + if (btf_fd < 0) > > + return 0; > It should return an error. > That would break the original behavior with (ret == 0 && btf == NULL), but I think it's more consistent, so I'll fix this and will add Fixes: tag. > > + > > + res = btf_get_from_fd(btf_fd, NULL); > > + close(btf_fd); > > + if (IS_ERR(res)) > > + return PTR_ERR(res); > > + > > + *btf = res; > > + return 0; > > } > >
On Thu, Nov 19, 2020 at 03:22:42PM -0800, Andrii Nakryiko wrote: [ ... ] > +static int load_module_btfs(struct bpf_object *obj) > +{ > + struct bpf_btf_info info; > + struct module_btf *mod_btf; > + struct btf *btf; > + char name[64]; > + __u32 id, len; > + int err, fd; > + > + if (obj->btf_modules_loaded) > + return 0; > + > + /* don't do this again, even if we find no module BTFs */ > + obj->btf_modules_loaded = true; > + > + /* kernel too old to support module BTFs */ > + if (!kernel_supports(FEAT_MODULE_BTF)) > + return 0; > + > + while (true) { > + err = bpf_btf_get_next_id(id, &id); > + if (err && errno == ENOENT) > + return 0; > + if (err) { > + err = -errno; > + pr_warn("failed to iterate BTF objects: %d\n", err); > + return err; > + } > + > + fd = bpf_btf_get_fd_by_id(id); > + if (fd < 0) { > + if (errno == ENOENT) > + continue; /* expected race: BTF was unloaded */ > + err = -errno; > + pr_warn("failed to get BTF object #%d FD: %d\n", id, err); > + return err; > + } > + > + len = sizeof(info); > + memset(&info, 0, sizeof(info)); > + info.name = ptr_to_u64(name); > + info.name_len = sizeof(name); > + > + err = bpf_obj_get_info_by_fd(fd, &info, &len); > + if (err) { > + err = -errno; > + pr_warn("failed to get BTF object #%d info: %d\n", id, err); close(fd); > + return err; > + } > + > + /* ignore non-module BTFs */ > + if (!info.kernel_btf || strcmp(name, "vmlinux") == 0) { > + close(fd); > + continue; > + } > + [ ... ] > @@ -8656,9 +8815,6 @@ static inline int __find_vmlinux_btf_id(struct btf *btf, const char *name, > else > err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC); > > - if (err <= 0) > - pr_warn("%s is not found in vmlinux BTF\n", name); > - > return err; > } > > @@ -8675,6 +8831,9 @@ int libbpf_find_vmlinux_btf_id(const char *name, > } > > err = __find_vmlinux_btf_id(btf, name, attach_type); > + if (err <= 0) > + pr_warn("%s is not found in vmlinux BTF\n", name); > + Please explain this move in the commit message. > btf__free(btf); > return err; > } > -- > 2.24.1 >
On Fri, Nov 20, 2020 at 3:06 PM Martin KaFai Lau <kafai@fb.com> wrote: > > On Thu, Nov 19, 2020 at 03:22:42PM -0800, Andrii Nakryiko wrote: > [ ... ] > > > +static int load_module_btfs(struct bpf_object *obj) > > +{ > > + struct bpf_btf_info info; > > + struct module_btf *mod_btf; > > + struct btf *btf; > > + char name[64]; > > + __u32 id, len; > > + int err, fd; > > + > > + if (obj->btf_modules_loaded) > > + return 0; > > + > > + /* don't do this again, even if we find no module BTFs */ > > + obj->btf_modules_loaded = true; > > + > > + /* kernel too old to support module BTFs */ > > + if (!kernel_supports(FEAT_MODULE_BTF)) > > + return 0; > > + > > + while (true) { > > + err = bpf_btf_get_next_id(id, &id); > > + if (err && errno == ENOENT) > > + return 0; > > + if (err) { > > + err = -errno; > > + pr_warn("failed to iterate BTF objects: %d\n", err); > > + return err; > > + } > > + > > + fd = bpf_btf_get_fd_by_id(id); > > + if (fd < 0) { > > + if (errno == ENOENT) > > + continue; /* expected race: BTF was unloaded */ > > + err = -errno; > > + pr_warn("failed to get BTF object #%d FD: %d\n", id, err); > > + return err; > > + } > > + > > + len = sizeof(info); > > + memset(&info, 0, sizeof(info)); > > + info.name = ptr_to_u64(name); > > + info.name_len = sizeof(name); > > + > > + err = bpf_obj_get_info_by_fd(fd, &info, &len); > > + if (err) { > > + err = -errno; > > + pr_warn("failed to get BTF object #%d info: %d\n", id, err); > > close(fd); > > > + return err; > > + } > > + > > + /* ignore non-module BTFs */ > > + if (!info.kernel_btf || strcmp(name, "vmlinux") == 0) { > > + close(fd); > > + continue; > > + } > > + > > [ ... ] > > > @@ -8656,9 +8815,6 @@ static inline int __find_vmlinux_btf_id(struct btf *btf, const char *name, > > else > > err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC); > > > > - if (err <= 0) > > - pr_warn("%s is not found in vmlinux BTF\n", name); > > - > > return err; > > } > > > > @@ -8675,6 +8831,9 @@ int libbpf_find_vmlinux_btf_id(const char *name, > > } > > > > err = __find_vmlinux_btf_id(btf, name, attach_type); > > + if (err <= 0) > > + pr_warn("%s is not found in vmlinux BTF\n", name); > > + > Please explain this move in the commit message. ok, I'll add something about that. The short answer is that __find_vmlinux_btf_id() is now expected to not find a type in vmlinux BTF, so emitting error would be wrong. So I moved it up a level where it's not expected. > > > btf__free(btf); > > return err; > > } > > -- > > 2.24.1 > >
On Thu, Nov 19, 2020 at 03:22:39PM -0800, Andrii Nakryiko wrote: > __module_address() needs to be called with preemption disabled or with > module_mutex taken. preempt_disable() is enough for read-only uses, which is > what this fix does. > > Fixes: a38d1107f937 ("bpf: support raw tracepoints in modules") > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- > kernel/trace/bpf_trace.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index d255bc9b2bfa..bb98a377050a 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -2060,7 +2060,11 @@ struct bpf_raw_event_map *bpf_get_raw_tracepoint(const char *name) > > void bpf_put_raw_tracepoint(struct bpf_raw_event_map *btp) > { > - struct module *mod = __module_address((unsigned long)btp); > + struct module *mod; > + > + preempt_disable(); > + mod = __module_address((unsigned long)btp); > + preempt_enable(); > > if (mod) > module_put(mod); I don't understand why 'mod' cannot become dangling pointer after preempt_enable(). Either it needs a comment explaining why it's ok or module_put() should be in preempt disabled section.
On Mon, Nov 23, 2020 at 9:49 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Nov 19, 2020 at 03:22:39PM -0800, Andrii Nakryiko wrote: > > __module_address() needs to be called with preemption disabled or with > > module_mutex taken. preempt_disable() is enough for read-only uses, which is > > what this fix does. > > > > Fixes: a38d1107f937 ("bpf: support raw tracepoints in modules") > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > --- > > kernel/trace/bpf_trace.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index d255bc9b2bfa..bb98a377050a 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -2060,7 +2060,11 @@ struct bpf_raw_event_map *bpf_get_raw_tracepoint(const char *name) > > > > void bpf_put_raw_tracepoint(struct bpf_raw_event_map *btp) > > { > > - struct module *mod = __module_address((unsigned long)btp); > > + struct module *mod; > > + > > + preempt_disable(); > > + mod = __module_address((unsigned long)btp); > > + preempt_enable(); > > > > if (mod) > > module_put(mod); > > I don't understand why 'mod' cannot become dangling pointer after preempt_enable(). > Either it needs a comment explaining why it's ok or module_put() should > be in preempt disabled section. Yeah, I think it can, assuming the kernel module can be unloaded despite non-zero refcnt (probably happens with force unload?). I'll drop the `if (mod)` part (module_put() checks that internally) and will move module_put(mod) inside the preempt disable/enable region.