lkml.org 
[lkml]   [2022]   [May]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH bpf-next v3] bpftool: Use sysfs vmlinux when dumping BTF by ID
From


On 5/13/22 5:17 AM, Larysa Zaremba wrote:
> Currently, dumping almost all BTFs specified by id requires
> using the -B option to pass the base BTF. For kernel module
> BTFs the vmlinux BTF sysfs path should work.

This is not precise. It should be that
dumping all module BTFs specified by id requires using the -B
option. In current situation, to dump a btf associated with
a bpf program, -B option is not needed.

>
> This patch simplifies dumping by ID usage by loading
> vmlinux BTF from sysfs as base, if base BTF was not specified
> and the ID corresponds to a kernel module BTF.
>
> Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> Reviewed-by: Alexander Lobakin <alexandr.lobakin@intel.com>
> ---
> From v2[0]:
> - instead of using vmlinux as base only after the first unsuccessful
> attempt, set base to vmlinux before loading in applicable cases, precisely
> if no base was provided by user and id belongs to a kernel module BTF.
>
> From v1[1]:
> - base BTF is assumed to be vmlinux only for kernel BTFs.
>
> [0] https://lore.kernel.org/bpf/20220505130507.130670-1-larysa.zaremba@intel.com/
> [1] https://lore.kernel.org/bpf/20220428111442.111805-1-larysa.zaremba@intel.com/
> ---
> tools/bpf/bpftool/btf.c | 65 +++++++++++++++++++++++++++++++++++------
> 1 file changed, 56 insertions(+), 9 deletions(-)
>
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index a2c665beda87..0eb105c416fc 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -459,6 +459,54 @@ static int dump_btf_c(const struct btf *btf,
> return err;
> }
>
> +static const char sysfs_vmlinux[] = "/sys/kernel/btf/vmlinux";
> +
> +static struct btf *get_vmlinux_btf_from_sysfs(void)
> +{
> + struct btf *base;
> +
> + base = btf__parse(sysfs_vmlinux, NULL);
> + if (libbpf_get_error(base)) {
> + p_err("failed to parse vmlinux BTF at '%s': %ld\n",
> + sysfs_vmlinux, libbpf_get_error(base));

nit: libbpf_get_error() is called twice. Can be consolidated into
one call.

> + base = NULL;
> + }
> +
> + return base;
> +}
> +
> +#define BTF_NAME_BUFF_LEN 64
> +
> +static bool btf_is_kernel_module(__u32 btf_id)
> +{
> + struct bpf_btf_info btf_info = {};
> + char btf_name[BTF_NAME_BUFF_LEN];
> + int btf_fd;
> + __u32 len;
> + int err;
> +
> + btf_fd = bpf_btf_get_fd_by_id(btf_id);
> + if (btf_fd < 0) {
> + p_err("can't get BTF object by id (%u): %s",
> + btf_id, strerror(errno));

I am not sure whether we want p_err here or not.
The function is to test whether btf is a kernel_module.
If anything wrong happens here. The original logic
follows and an error will be printed out any way.

> + return false;
> + }
> +
> + len = sizeof(btf_info);
> + btf_info.name = ptr_to_u64(btf_name);
> + btf_info.name_len = sizeof(btf_name);
> + err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
> + close(btf_fd);
> +
> + if (err) {
> + p_err("can't get BTF (ID %u) object info: %s",
> + btf_id, strerror(errno));

The same as above, probably we don't need p_err here.

> + return false;
> + }
> +
> + return strncmp(btf_name, "vmlinux", sizeof(btf_name)) && btf_info.kernel_btf;

This should work considering current module names but the code itself
doesn't sound right. The characters after "vmlinux" could be arbitrary.
strncmp(btf_name, "vmlinux", strlen("vmlinux") + 1) is better.

> +}
> +
> static int do_dump(int argc, char **argv)
> {
> struct btf *btf = NULL, *base = NULL;
> @@ -536,18 +584,11 @@ static int do_dump(int argc, char **argv)
> NEXT_ARG();
> } else if (is_prefix(src, "file")) {
> const char sysfs_prefix[] = "/sys/kernel/btf/";
> - const char sysfs_vmlinux[] = "/sys/kernel/btf/vmlinux";
>
> if (!base_btf &&
> strncmp(*argv, sysfs_prefix, sizeof(sysfs_prefix) - 1) == 0 &&
> - strcmp(*argv, sysfs_vmlinux) != 0) {
> - base = btf__parse(sysfs_vmlinux, NULL);
> - if (libbpf_get_error(base)) {
> - p_err("failed to parse vmlinux BTF at '%s': %ld\n",
> - sysfs_vmlinux, libbpf_get_error(base));
> - base = NULL;
> - }
> - }
> + strcmp(*argv, sysfs_vmlinux))
> + base = get_vmlinux_btf_from_sysfs();
>
> btf = btf__parse_split(*argv, base ?: base_btf);
> err = libbpf_get_error(btf);
> @@ -591,6 +632,12 @@ static int do_dump(int argc, char **argv)
> }
>
> if (!btf) {
> + if (!base_btf && btf_is_kernel_module(btf_id)) {
> + p_info("Warning: valid base BTF was not specified with -B option, falling back on standard base BTF (%s)",
> + sysfs_vmlinux);

Having 'Warning' for p_info seems not appropriate. Similar to other
p_info, you can just remove 'Warning: ".

> + base_btf = get_vmlinux_btf_from_sysfs();
> + }
> +
> btf = btf__load_from_kernel_by_id_split(btf_id, base_btf);
> err = libbpf_get_error(btf);
> if (err) {

\
 
 \ /
  Last update: 2022-05-16 22:40    [W:0.918 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site