Re: [PATCH bpf-next] bpftool: Support dumping BTF object by name

[Quentin Monnet <quentin@isovalent.com>]

On 28/08/2023 15:04, Hengqi Chen wrote:
> Like maps and progs, add support to dump BTF
> objects by name ([0]).

Hi, thanks for looking into this!

Can we also please support referencing by name for "bpftool btf list"?
This will require collecting a list of the different programs with a
matching name, like we do for "bpftool prog list name <foo>" (but dumps
should still fail if there are more than one program matching, for
consistency with "bpftool prog dump").

> 
>   [0] Closes: https://github.com/libbpf/bpftool/issues/56
> 
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---
>  tools/bpf/bpftool/btf.c | 92 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 91 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 91fcb75babe3..cb8d78ff4081 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -547,6 +547,83 @@ static bool btf_is_kernel_module(__u32 btf_id)
>  	return btf_info.kernel_btf && strncmp(btf_name, "vmlinux", sizeof(btf_name)) != 0;
>  }
> 
> +static int btf_id_by_name(char *name, __u32 *btf_id)
> +{
> +	bool found = false;
> +	__u32 id = 0;
> +	int fd, err;
> +
> +	while (true) {
> +		struct bpf_btf_info info = {};
> +		__u32 len = sizeof(info);

"info_len" instead of "len" would be more explicit, and this can
probably be a const.

> +		char btf_name[64];
> +
> +		err = bpf_btf_get_next_id(id, &id);
> +		if (err) {
> +			if (errno == ENOENT) {
> +				if (found)
> +					err = 0;
> +				else
> +					p_err("no BTF object match name %s", name);
> +				break;
> +			}
> +
> +			p_err("can't get next BTF object: %s%s",
> +			      strerror(errno),
> +			      errno == EINVAL ? " -- kernel too old?" : "");
> +			return -1;
> +		}
> +
> +		fd = bpf_btf_get_fd_by_id(id);
> +		if (fd < 0) {
> +			p_err("can't get BTF by id (%u): %s",
> +			      id, strerror(errno));
> +			return -1;
> +		}
> +
> +		err = bpf_btf_get_info_by_fd(fd, &info, &len);
> +		if (err) {
> +			p_err("can't get BTF info (%u): %s",
> +			      id, strerror(errno));
> +			goto err_close_fd;
> +		}
> +
> +		if (info.name_len) {
> +			memset(&info, 0, sizeof(info));
> +			info.name_len = sizeof(btf_name);
> +			info.name = ptr_to_u64(btf_name);
> +			len = sizeof(info);

sizeof(info) is the same as before, no need to reassign "len" (and we
can use "len" in the memset()).

> +
> +			err = bpf_btf_get_info_by_fd(fd, &info, &len);
> +			if (err) {
> +				p_err("can't get BTF info (%u): %s",
> +				      id, strerror(errno));
> +				goto err_close_fd;
> +			}
> +		}
> +
> +		close(fd);
> +
> +		if (strncmp(name, u64_to_ptr(info.name), BPF_OBJ_NAME_LEN))

There's no guarantee that "info.name" is set, here. Some BTF objects are
anonymous and won't have a name. I'm getting a segfault from this
strncmp() when trying the patch locally, because I've got anonymous BTF
objects loaded and I end up passing a null pointer to the function. Can
you please fix this?

A follow-up question is how to handle anonymous objects. Given that
we want to filter on names, we should probably allow empty name as well:
"bpftool btf list name ''" should list anonymous objects, what do you
think?

> +			continue;
> +
> +		if (found) {
> +			p_err("multiple BTF object match name %s", name);
> +			return -1;
> +		}
> +
> +		*btf_id = id;
> +		found = true;
> +	}
> +
> +	return err;
> +
> +err_close_fd:
> +	close(fd);
> +	return err;
> +}
> +
> +
>  static int do_dump(int argc, char **argv)
>  {
>  	struct btf *btf = NULL, *base = NULL;
> @@ -637,6 +714,19 @@ static int do_dump(int argc, char **argv)
>  			      *argv, strerror(errno));
>  			goto done;
>  		}
> +		NEXT_ARG();
> +	} else if (is_prefix(src, "name")) {
> +		char *name = *argv;
> +
> +		if (strlen(name) > BPF_OBJ_NAME_LEN - 1) {
> +			p_err("can't parse name");
> +			return -1;
> +		}
> +
> +		err = btf_id_by_name(name, &btf_id);
> +		if (err)
> +			return -1;
> +
>  		NEXT_ARG();
>  	} else {
>  		err = -1;
> @@ -1062,7 +1152,7 @@ static int do_help(int argc, char **argv)
>  		"       %1$s %2$s dump BTF_SRC [format FORMAT]\n"
>  		"       %1$s %2$s help\n"
>  		"\n"
> -		"       BTF_SRC := { id BTF_ID | prog PROG | map MAP [{key | value | kv | all}] | file FILE }\n"
> +		"       BTF_SRC := { id BTF_ID | name NAME | prog PROG | map MAP [{key | value | kv | all}] | file FILE }\n"

We will also need to update the command description in the relevant man
page (.../Documentation/bpftool-btf.rst), and the bash completion
(.../bash-completion/bpftool).

Here's what I'd suggest for the bash completion:

------
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 085bf18f3659..9aa0d2efe938 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -98,6 +98,12 @@ _bpftool_get_btf_ids()
         command sed -n 's/.*"id": \(.*\),$/\1/p' )" -- "$cur" ) )
 }
 
+_bpftool_get_btf_names()
+{
+    COMPREPLY+=( $( compgen -W "$( bpftool -jp btf 2>&1 | \
+        command sed -n 's/.*"name": "\(.*\)",\?$/\1/p' )" -- "$cur" ) )
+}
+
 _bpftool_get_link_ids()
 {
     COMPREPLY+=( $( compgen -W "$( bpftool -jp link 2>&1 | \
@@ -899,7 +905,7 @@ _bpftool()
                 dump)
                     case $prev in
                         $command)
-                            COMPREPLY+=( $( compgen -W "id map prog file" -- \
+                            COMPREPLY+=( $( compgen -W "id map name prog file" -- \
                                 "$cur" ) )
                             return 0
                             ;;
@@ -933,6 +939,9 @@ _bpftool()
                                 map)
                                     _bpftool_get_map_names
                                     ;;
+                                $command)
+                                    _bpftool_get_btf_names
+                                    ;;
                             esac
                             return 0
                             ;;
@@ -961,11 +970,14 @@ _bpftool()
                 show|list)
                     case $prev in
                         $command)
-                            COMPREPLY+=( $( compgen -W "id" -- "$cur" ) )
+                            COMPREPLY+=( $( compgen -W "id name" -- "$cur" ) )
                             ;;
                         id)
                             _bpftool_get_btf_ids
                             ;;
+                        name)
+                            _bpftool_get_btf_names
+                            ;;
                     esac
                     return 0
                     ;;
------

Please also make sure to Cc the other BPF maintainers for your next
version.

Thanks,
Quentin