Date:   Mon, 16 May 2022 13:03:48 -0700
From:   Yonghong Song <yhs@...com>
To:     Larysa Zaremba <larysa.zaremba@...el.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org,
        linux-kernel@...r.kernel.org, Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Quentin Monnet <quentin@...valent.com>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Alexander Lobakin <alexandr.lobakin@...el.com>
Subject: Re: [PATCH bpf-next v3] bpftool: Use sysfs vmlinux when dumping BTF
 by ID



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@...el.com>
> Reviewed-by: Alexander Lobakin <alexandr.lobakin@...el.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) {

Powered by blists - more mailing lists