[bpf-next,v3] bpftool: Use sysfs vmlinux when dumping BTF by ID

Commit Message

Larysa Zaremba May 13, 2022, 12:17 p.m. UTC

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 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 <[e-mail]>
Reviewed-by: Alexander Lobakin <[e-mail]>
---
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/[e-mail]/
[1] https://lore.kernel.org/bpf/[e-mail]/
---
 tools/bpf/bpftool/btf.c | 65 +++++++++++++++++++++++++++++++++++------
 1 file changed, 56 insertions(+), 9 deletions(-)

Comments

[e-mail] May 13, 2022, 11:10 p.m. UTC | #1

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <[e-mail]>:

On Fri, 13 May 2022 14:17:43 +0200 you 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 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.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v3] bpftool: Use sysfs vmlinux when dumping BTF by ID
    https://git.kernel.org/bpf/bpf-next/c/418fbe82578e

You are awesome, thank you!

Yonghong Song May 16, 2022, 8:03 p.m. UTC | #2

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 <[e-mail]>
> Reviewed-by: Alexander Lobakin <[e-mail]>
> ---
>  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/[e-mail]/
> [1] https://lore.kernel.org/bpf/[e-mail]/
> ---
>   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) {

Patch

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));
+		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));
+		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));
+		return false;
+	}
+
+	return strncmp(btf_name, "vmlinux", sizeof(btf_name)) && btf_info.kernel_btf;
+}
+
 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);
+			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) {