| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <rphope3vevqgjbq3uh5l3wrtvlcls2dc3abi5caxfbemjgztuy@5qvyeoove3nr>
Date: Mon, 9 Dec 2024 15:56:26 -0700
From: Daniel Xu <dxu@...uu.xyz>
To: Quentin Monnet <qmo@...nel.org>
Cc: hawk@...nel.org, john.fastabend@...il.com, kuba@...nel.org,
ast@...nel.org, davem@...emloft.net, daniel@...earbox.net, andrii@...nel.org,
martin.lau@...ux.dev, eddyz87@...il.com, song@...nel.org, yonghong.song@...ux.dev,
kpsingh@...nel.org, sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org,
bpf@...r.kernel.org, linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
antony@...nome.org, toke@...nel.org
Subject: Re: [PATCH bpf-next v2] bpftool: btf: Support dumping a single type
from file
Hi Quentin,
On Mon, Dec 09, 2024 at 12:26:50PM GMT, Quentin Monnet wrote:
> On 07/12/2024 23:24, Daniel Xu wrote:
> > Some projects, for example xdp-tools [0], prefer to check in a minimized
> > vmlinux.h rather than the complete file which can get rather large.
> >
> > However, when you try to add a minimized version of a complex struct (eg
> > struct xfrm_state), things can get quite complex if you're trying to
> > manually untangle and deduplicate the dependencies.
> >
> > This commit teaches bpftool to do a minimized dump of a single type by
> > providing an optional root_id argument.
> >
> > Example usage:
> >
> > $ ./bpftool btf dump file ~/dev/linux/vmlinux | rg "STRUCT 'xfrm_state'"
> > [12643] STRUCT 'xfrm_state' size=912 vlen=58
> >
> > $ ./bpftool btf dump file ~/dev/linux/vmlinux root_id 12643 format c
> > #ifndef __VMLINUX_H__
> > #define __VMLINUX_H__
> >
> > [..]
> >
> > struct xfrm_type_offload;
> >
> > struct xfrm_sec_ctx;
> >
> > struct xfrm_state {
> > possible_net_t xs_net;
> > union {
> > struct hlist_node gclist;
> > struct hlist_node bydst;
> > };
> > union {
> > struct hlist_node dev_gclist;
> > struct hlist_node bysrc;
> > };
> > struct hlist_node byspi;
> > [..]
> >
> > [0]: https://github.com/xdp-project/xdp-tools/blob/master/headers/bpf/vmlinux.h
> >
> > Signed-off-by: Daniel Xu <dxu@...uu.xyz>
> > ---
> > Changes in v2:
> > * Add early error check for invalid BTF ID
> >
> > .../bpf/bpftool/Documentation/bpftool-btf.rst | 5 +++--
> > tools/bpf/bpftool/btf.c | 19 +++++++++++++++++++
> > 2 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-btf.rst b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> > index 3f6bca03ad2e..5abd0e99022f 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> > @@ -27,7 +27,7 @@ BTF COMMANDS
> > | **bpftool** **btf dump** *BTF_SRC* [**format** *FORMAT*]
> > | **bpftool** **btf help**
> > |
> > -| *BTF_SRC* := { **id** *BTF_ID* | **prog** *PROG* | **map** *MAP* [{**key** | **value** | **kv** | **all**}] | **file** *FILE* }
> > +| *BTF_SRC* := { **id** *BTF_ID* | **prog** *PROG* | **map** *MAP* [{**key** | **value** | **kv** | **all**}] | **file** *FILE* [**root_id** *ROOT_ID*] }
>
>
> Thanks for this!
No problem!
>
> root_id is not part of the BTF_SRC, I think it should be an option on
> the command line itself (3 lines above), after "format". And the change
> should also be repeated below in the description (the "format" option is
> missing, by the way, let's fix it too).
Sure, will do. I was originally thinking it would conflict with `map`
subcommand, as it also sets the root_type_ids, but we can just error out
if root_type_cnt > 0 and root_id is pased.
Will send v3 with below suggestions as well.
>
> Can you please also update the interactive help message at the end of
> btf.c?
>
> Can you please also update the bash completion file? I think it should
> look like this:
>
>
> ------
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index 0c541498c301..097d406ee21f 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -930,6 +930,9 @@ _bpftool()
> format)
> COMPREPLY=( $( compgen -W "c raw" -- "$cur" ) )
> ;;
> + root_id)
> + return 0;
> + ;;
> c)
> COMPREPLY=( $( compgen -W "unsorted" -- "$cur" ) )
> ;;
> @@ -937,13 +940,13 @@ _bpftool()
> # emit extra options
> case ${words[3]} in
> id|file)
> - _bpftool_once_attr 'format'
> + _bpftool_once_attr 'format root_id'
> ;;
> map|prog)
> if [[ ${words[3]} == "map" ]] && [[ $cword == 6 ]]; then
> COMPREPLY+=( $( compgen -W "key value kv all" -- "$cur" ) )
> fi
> - _bpftool_once_attr 'format'
> + _bpftool_once_attr 'format root_id'
> ;;
> *)
> ;;
> ------
>
>
> > | *FORMAT* := { **raw** | **c** [**unsorted**] }
> > | *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> > | *PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* | **name** *PROG_NAME* }
> > @@ -60,7 +60,8 @@ bpftool btf dump *BTF_SRC*
> >
> > When specifying *FILE*, an ELF file is expected, containing .BTF section
> > with well-defined BTF binary format data, typically produced by clang or
> > - pahole.
> > + pahole. You can choose to dump a single type and all its dependent types
> > + by providing an optional *ROOT_ID*.
> >
> > **format** option can be used to override default (raw) output format. Raw
> > (**raw**) or C-syntax (**c**) output formats are supported. With C-style
> > diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> > index d005e4fd6128..a75e17efaf5e 100644
> > --- a/tools/bpf/bpftool/btf.c
> > +++ b/tools/bpf/bpftool/btf.c
> > @@ -953,6 +953,8 @@ static int do_dump(int argc, char **argv)
> > NEXT_ARG();
> > } else if (is_prefix(src, "file")) {
> > const char sysfs_prefix[] = "/sys/kernel/btf/";
> > + __u32 root_id;
> > + char *end;
>
>
> I think we could move these declarations to a lower scope, under your
> "if (argc && is_prefix(...))".
>
>
> >
> > if (!base_btf &&
> > strncmp(*argv, sysfs_prefix, sizeof(sysfs_prefix) - 1) == 0 &&
> > @@ -967,6 +969,23 @@ static int do_dump(int argc, char **argv)
> > goto done;
> > }
> > NEXT_ARG();
> > +
> > + if (argc && is_prefix(*argv, "root_id")) {
> > + NEXT_ARG();
> > + root_id = strtoul(*argv, &end, 0);
> > + if (*end) {
> > + err = -1;
> > + p_err("can't parse %s as root ID", *argv);
> > + goto done;
> > + }
> > + if (root_id >= btf__type_cnt(btf)) {
> > + err = -EINVAL;
> > + p_err("invalid root ID: %u", root_id);
> > + goto done;
> > + }
> > + root_type_ids[root_type_cnt++] = root_id;
> > + NEXT_ARG();
> > + }
> > } else {
> > err = -1;
> > p_err("unrecognized BTF source specifier: '%s'", src);
>
>
> Same comment as above, it seems to be that the root_id controls the
> output for the command rather than the source, and I'd rather move this
> to the "while (argc)" loop where we process the "format" option rather
> than when parsing the source.
>
>
> pw-bot: cr
Powered by blists - more mailing lists