| Message ID | 20201207052057.397223-1-saeed@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf] tools/bpftool: Add/Fix support for modules btf dump | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to bpf |
| netdev/tree_selection | success | Clearly marked for bpf |
On Sun, Dec 6, 2020 at 9:21 PM <saeed@kernel.org> wrote: > > From: Saeed Mahameed <saeedm@nvidia.com> > > While playing with BTF for modules, i noticed that executing the command: > $ bpftool btf dump id <module's btf id> > > Fails due to lack of information in the BTF data. > > Maybe I am missing a step but actually adding the support for this is > very simple. yes, bpftool takes -B <path> argument for specifying base BTF. So if you added -B /sys/kernel/btf/vmlinux, it should have worked. I've added auto-detection logic for the case of `btf dump file /sys/kernel/btf/<module>` (see [0]), and we can also add it for when ID corresponds to a module BTF. But I think it's simplest to re-use the logic and just open /sys/kernel/btf/vmlinux, instead of adding narrowly-focused libbpf API for that. > > To completely parse modules BTF data, we need the vmlinux BTF as their > "base btf", which can be easily found by iterating through the btf ids and > looking for btf.name == "vmlinux". > > I am not sure why this hasn't been added by the original patchset because I never though of dumping module BTF by id, given there is nicely named /sys/kernel/btf/<module> :) > "Integrate kernel module BTF support", as adding the support for > this is very trivial. Unless i am missing something, CCing Andrii.. > > Signed-off-by: Saeed Mahameed <saeedm@nvidia.com> > CC: Andrii Nakryiko <andrii@kernel.org> > --- > tools/lib/bpf/btf.c | 57 ++++++++++++++++++++++++++++++++++++++++ > tools/lib/bpf/btf.h | 2 ++ > tools/lib/bpf/libbpf.map | 1 + > 3 files changed, 60 insertions(+) > [...]
On Mon, 2020-12-07 at 19:14 -0800, Andrii Nakryiko wrote: > On Sun, Dec 6, 2020 at 9:21 PM <saeed@kernel.org> wrote: > > From: Saeed Mahameed <saeedm@nvidia.com> > > > > While playing with BTF for modules, i noticed that executing the > > command: > > $ bpftool btf dump id <module's btf id> > > > > Fails due to lack of information in the BTF data. > > > > Maybe I am missing a step but actually adding the support for this > > is > > very simple. > > yes, bpftool takes -B <path> argument for specifying base BTF. So if > you added -B /sys/kernel/btf/vmlinux, it should have worked. I've > added auto-detection logic for the case of `btf dump file > /sys/kernel/btf/<module>` (see [0]), and we can also add it for when > ID corresponds to a module BTF. But I think it's simplest to re-use > the logic and just open /sys/kernel/btf/vmlinux, instead of adding > narrowly-focused libbpf API for that. > When dumping with a file it works even without the -B since you lookup the vmlinux file, but the issue is not dumping from a file source, we need it by id.. > > To completely parse modules BTF data, we need the vmlinux BTF as > > their > > "base btf", which can be easily found by iterating through the btf > > ids and > > looking for btf.name == "vmlinux". > > > > I am not sure why this hasn't been added by the original patchset > > because I never though of dumping module BTF by id, given there is > nicely named /sys/kernel/btf/<module> :) > What if i didn't compile my kernel with SYSFS ? a user experience is a user experience, there is no reason to not support dump a module btf by id or to have different behavior for different BTF sources. I can revise this patch to support -B option and lookup vmlinux file if not provided for module btf dump by ids. but we still need to pass base_btf to btf__get_from_id() in order to support that, as was done for btf__parse_split() ... :/ Are you sure you don't like the current patch/libbpf API ? it is pretty straight forward and correct. > > "Integrate kernel module BTF support", as adding the support for > > this is very trivial. Unless i am missing something, CCing Andrii.. > > > > Signed-off-by: Saeed Mahameed <saeedm@nvidia.com> > > CC: Andrii Nakryiko <andrii@kernel.org> > > --- > > tools/lib/bpf/btf.c | 57 > > ++++++++++++++++++++++++++++++++++++++++ > > tools/lib/bpf/btf.h | 2 ++ > > tools/lib/bpf/libbpf.map | 1 + > > 3 files changed, 60 insertions(+) > > > > [...]
On Mon, Dec 7, 2020 at 10:26 PM Saeed Mahameed <saeed@kernel.org> wrote: > > On Mon, 2020-12-07 at 19:14 -0800, Andrii Nakryiko wrote: > > On Sun, Dec 6, 2020 at 9:21 PM <saeed@kernel.org> wrote: > > > From: Saeed Mahameed <saeedm@nvidia.com> > > > > > > While playing with BTF for modules, i noticed that executing the > > > command: > > > $ bpftool btf dump id <module's btf id> > > > > > > Fails due to lack of information in the BTF data. > > > > > > Maybe I am missing a step but actually adding the support for this > > > is > > > very simple. > > > > yes, bpftool takes -B <path> argument for specifying base BTF. So if > > you added -B /sys/kernel/btf/vmlinux, it should have worked. I've > > added auto-detection logic for the case of `btf dump file > > /sys/kernel/btf/<module>` (see [0]), and we can also add it for when > > ID corresponds to a module BTF. But I think it's simplest to re-use > > the logic and just open /sys/kernel/btf/vmlinux, instead of adding > > narrowly-focused libbpf API for that. > > > > When dumping with a file it works even without the -B since you lookup > the vmlinux file, but the issue is not dumping from a file source, we > need it by id.. > > > > To completely parse modules BTF data, we need the vmlinux BTF as > > > their > > > "base btf", which can be easily found by iterating through the btf > > > ids and > > > looking for btf.name == "vmlinux". > > > > > > I am not sure why this hasn't been added by the original patchset > > > > because I never though of dumping module BTF by id, given there is > > nicely named /sys/kernel/btf/<module> :) > > > > What if i didn't compile my kernel with SYSFS ? a user experience is a > user experience, there is no reason to not support dump a module btf by > id or to have different behavior for different BTF sources. Hm... I didn't claim otherwise and didn't oppose the feature, why the lecture about user experience? Not having sysfs is a valid point. In such cases, if BTF dumping is from ID and we see that it's a module BTF, finding vmlinux BTF from ID makes sense. > > I can revise this patch to support -B option and lookup vmlinux file if > not provided for module btf dump by ids. yep > > but we still need to pass base_btf to btf__get_from_id() in order to > support that, as was done for btf__parse_split() ... :/ btf__get_from_id_split() might be needed, yes. > > Are you sure you don't like the current patch/libbpf API ? it is pretty > straight forward and correct. I definitely don't like adding btf_get_kernel_id() API to libbpf. There is nothing special about it to warrant adding it as a public API. Everything we discussed can be done by bpftool. > > > > "Integrate kernel module BTF support", as adding the support for > > > this is very trivial. Unless i am missing something, CCing Andrii.. > > > > > > Signed-off-by: Saeed Mahameed <saeedm@nvidia.com> > > > CC: Andrii Nakryiko <andrii@kernel.org> > > > --- > > > tools/lib/bpf/btf.c | 57 > > > ++++++++++++++++++++++++++++++++++++++++ > > > tools/lib/bpf/btf.h | 2 ++ > > > tools/lib/bpf/libbpf.map | 1 + > > > 3 files changed, 60 insertions(+) > > > > > > > [...] >
On Mon, 2020-12-07 at 22:38 -0800, Andrii Nakryiko wrote: > On Mon, Dec 7, 2020 at 10:26 PM Saeed Mahameed <saeed@kernel.org> > wrote: > > On Mon, 2020-12-07 at 19:14 -0800, Andrii Nakryiko wrote: > > > On Sun, Dec 6, 2020 at 9:21 PM <saeed@kernel.org> wrote: > > > > From: Saeed Mahameed <saeedm@nvidia.com> > > > > [...] > > > > > > > > I am not sure why this hasn't been added by the original > > > > patchset > > > > > > because I never though of dumping module BTF by id, given there > > > is > > > nicely named /sys/kernel/btf/<module> :) > > > > > > > What if i didn't compile my kernel with SYSFS ? a user experience > > is a > > user experience, there is no reason to not support dump a module > > btf by > > id or to have different behavior for different BTF sources. > > Hm... I didn't claim otherwise and didn't oppose the feature, why the > lecture about user experience? > Sorry wasn't a lecture, just wanted to emphasize the motivation. > Not having sysfs is a valid point. In such cases, if BTF dumping is > from ID and we see that it's a module BTF, finding vmlinux BTF from > ID > makes sense. > > > I can revise this patch to support -B option and lookup vmlinux > > file if > > not provided for module btf dump by ids. > > yep > > > but we still need to pass base_btf to btf__get_from_id() in order > > to > > support that, as was done for btf__parse_split() ... :/ > > btf__get_from_id_split() might be needed, yes. > > > Are you sure you don't like the current patch/libbpf API ? it is > > pretty > > straight forward and correct. > > I definitely don't like adding btf_get_kernel_id() API to libbpf. > There is nothing special about it to warrant adding it as a public > API. Everything we discussed can be done by bpftool. > What about the case where sysfs isn't available ? we still need to find vmlinux's btf id..
On Mon, Dec 7, 2020 at 10:45 PM Saeed Mahameed <saeed@kernel.org> wrote: > > On Mon, 2020-12-07 at 22:38 -0800, Andrii Nakryiko wrote: > > On Mon, Dec 7, 2020 at 10:26 PM Saeed Mahameed <saeed@kernel.org> > > wrote: > > > On Mon, 2020-12-07 at 19:14 -0800, Andrii Nakryiko wrote: > > > > On Sun, Dec 6, 2020 at 9:21 PM <saeed@kernel.org> wrote: > > > > > From: Saeed Mahameed <saeedm@nvidia.com> > > > > > > [...] > > > > > > > > > > I am not sure why this hasn't been added by the original > > > > > patchset > > > > > > > > because I never though of dumping module BTF by id, given there > > > > is > > > > nicely named /sys/kernel/btf/<module> :) > > > > > > > > > > What if i didn't compile my kernel with SYSFS ? a user experience > > > is a > > > user experience, there is no reason to not support dump a module > > > btf by > > > id or to have different behavior for different BTF sources. > > > > Hm... I didn't claim otherwise and didn't oppose the feature, why the > > lecture about user experience? > > > > Sorry wasn't a lecture, just wanted to emphasize the motivation. > > > Not having sysfs is a valid point. In such cases, if BTF dumping is > > from ID and we see that it's a module BTF, finding vmlinux BTF from > > ID > > makes sense. > > > > > I can revise this patch to support -B option and lookup vmlinux > > > file if > > > not provided for module btf dump by ids. > > > > yep > > > > > but we still need to pass base_btf to btf__get_from_id() in order > > > to > > > support that, as was done for btf__parse_split() ... :/ > > > > btf__get_from_id_split() might be needed, yes. > > > > > Are you sure you don't like the current patch/libbpf API ? it is > > > pretty > > > straight forward and correct. > > > > I definitely don't like adding btf_get_kernel_id() API to libbpf. > > There is nothing special about it to warrant adding it as a public > > API. Everything we discussed can be done by bpftool. > > > > What about the case where sysfs isn't available ? > we still need to find vmlinux's btf id.. Right, but bpftool is perfectly capable of doing that without adding APIs to libbpf. That's why I wrote above: > > Not having sysfs is a valid point. In such cases, if BTF dumping is > > from ID and we see that it's a module BTF, finding vmlinux BTF from > > ID > > makes sense. > > >
On Mon, 2020-12-07 at 22:48 -0800, Andrii Nakryiko wrote: > On Mon, Dec 7, 2020 at 10:45 PM Saeed Mahameed <saeed@kernel.org> > wrote: > > On Mon, 2020-12-07 at 22:38 -0800, Andrii Nakryiko wrote: > > > On Mon, Dec 7, 2020 at 10:26 PM Saeed Mahameed <saeed@kernel.org> > > > wrote: > > > > On Mon, 2020-12-07 at 19:14 -0800, Andrii Nakryiko wrote: > > > > > On Sun, Dec 6, 2020 at 9:21 PM <saeed@kernel.org> wrote: > > > > > > From: Saeed Mahameed <saeedm@nvidia.com> > > > > > > > > [...] > > > > > > I am not sure why this hasn't been added by the original > > > > > > patchset > > > > > > > > > > because I never though of dumping module BTF by id, given > > > > > there > > > > > is > > > > > nicely named /sys/kernel/btf/<module> :) > > > > > > > > > > > > > What if i didn't compile my kernel with SYSFS ? a user > > > > experience > > > > is a > > > > user experience, there is no reason to not support dump a > > > > module > > > > btf by > > > > id or to have different behavior for different BTF sources. > > > > > > Hm... I didn't claim otherwise and didn't oppose the feature, why > > > the > > > lecture about user experience? > > > > > > > Sorry wasn't a lecture, just wanted to emphasize the motivation. > > > > > Not having sysfs is a valid point. In such cases, if BTF dumping > > > is > > > from ID and we see that it's a module BTF, finding vmlinux BTF > > > from > > > ID > > > makes sense. > > > > > > > I can revise this patch to support -B option and lookup vmlinux > > > > file if > > > > not provided for module btf dump by ids. > > > > > > yep > > > > > > > but we still need to pass base_btf to btf__get_from_id() in > > > > order > > > > to > > > > support that, as was done for btf__parse_split() ... :/ > > > > > > btf__get_from_id_split() might be needed, yes. > > > > > > > Are you sure you don't like the current patch/libbpf API ? it > > > > is > > > > pretty > > > > straight forward and correct. > > > > > > I definitely don't like adding btf_get_kernel_id() API to libbpf. > > > There is nothing special about it to warrant adding it as a > > > public > > > API. Everything we discussed can be done by bpftool. > > > > > > > What about the case where sysfs isn't available ? > > we still need to find vmlinux's btf id.. > > Right, but bpftool is perfectly capable of doing that without adding > APIs to libbpf. That's why I wrote above: > > > > Not having sysfs is a valid point. In such cases, if BTF > dumping is > > > from ID and we see that it's a module BTF, finding vmlinux BTF > from > > > ID > > > makes sense. Oh now i see, you want to scan for it in bpftool.. make sense.
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c index 3c3f2bc6c652..5900cccf82e2 100644 --- a/tools/lib/bpf/btf.c +++ b/tools/lib/bpf/btf.c @@ -1370,6 +1370,14 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf) goto exit_free; } + /* force base_btf for kernel modules */ + if (btf_info.kernel_btf && !base_btf) { + int id = btf_get_kernel_id(); + + /* Double check our btf is not the kernel BTF itself */ + if (id != btf_info.id) + btf__get_from_id(id, &base_btf); + } btf = btf_new(ptr, btf_info.btf_size, base_btf); exit_free: @@ -4623,3 +4631,52 @@ struct btf *libbpf_find_kernel_btf(void) pr_warn("failed to find valid kernel BTF\n"); return ERR_PTR(-ESRCH); } + +#define foreach_btf_id(id, err) \ + for (err = bpf_btf_get_next_id((id), (&id)); !err; ) + +/* + * Scan all ids for a kernel btf with name == "vmlinux" + */ +int btf_get_kernel_id(void) +{ + struct bpf_btf_info info; + __u32 len = sizeof(info); + char name[64]; + __u32 id = 0; + int err, fd; + + foreach_btf_id(id, 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; + } + + 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); + return err; + } + + if (info.kernel_btf && strcmp(name, "vmlinux") == 0) + return id; + + } + + if (err && errno != ENOENT) { + err = -errno; + pr_warn("failed to iterate BTF objects: %d\n", err); + return err; + } + + return -ENOENT; +} \ No newline at end of file diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h index 1237bcd1dd17..44075b086d1c 100644 --- a/tools/lib/bpf/btf.h +++ b/tools/lib/bpf/btf.h @@ -8,6 +8,7 @@ #include <stdbool.h> #include <linux/btf.h> #include <linux/types.h> +#include <uapi/linux/bpf.h> #include "libbpf_common.h" @@ -90,6 +91,7 @@ LIBBPF_API __u32 btf_ext__func_info_rec_size(const struct btf_ext *btf_ext); LIBBPF_API __u32 btf_ext__line_info_rec_size(const struct btf_ext *btf_ext); LIBBPF_API struct btf *libbpf_find_kernel_btf(void); +LIBBPF_API int btf_get_kernel_id(void); LIBBPF_API int btf__find_str(struct btf *btf, const char *s); LIBBPF_API int btf__add_str(struct btf *btf, const char *s); diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index 7c4126542e2b..727daeb57f35 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -348,4 +348,5 @@ LIBBPF_0.3.0 { btf__new_split; xsk_setup_xdp_prog; xsk_socket__update_xskmap; + btf_get_kernel_id } LIBBPF_0.2.0;