[bpf-next,v3,09/12] bpftool: Use libbpf_bpf_attach_type_str

Commit Message

Daniel Müller May 19, 2022, 9:29 p.m. UTC

This change switches bpftool over to using the recently introduced
libbpf_bpf_attach_type_str function instead of maintaining its own
string representation for the bpf_attach_type enum.

Note that contrary to other enum types, the variant names that bpftool
maps bpf_attach_type to do not follow a simple to follow rule. With
bpf_prog_type, for example, the textual representation can easily be
inferred by stripping the BPF_PROG_TYPE_ prefix and lowercasing the
remaining string. bpf_attach_type violates this rule for various
variants.
We decided to fix up this deficiency with this change, meaning that
bpftool uses the same textual representations as libbpf. Supporting
test, completion scripts, and man pages have been adjusted accordingly.
However, we did add support for accepting (the now undocumented)
original attach type names when they are provided by users.

For the test (test_bpftool_synctypes.py), I have removed the enum
representation checks, because we no longer mirror the various enum
variant names in bpftool source code. For the man page, help text, and
completion script checks we are now using enum definitions from
uapi/linux/bpf.h as the source of truth directly.

Signed-off-by: Daniel Müller <[e-mail]>
---
 .../bpftool/Documentation/bpftool-cgroup.rst  |  16 +-
 .../bpftool/Documentation/bpftool-prog.rst    |   5 +-
 tools/bpf/bpftool/bash-completion/bpftool     |  18 +-
 tools/bpf/bpftool/cgroup.c                    |  49 ++++--
 tools/bpf/bpftool/common.c                    |  82 ++++-----
 tools/bpf/bpftool/link.c                      |  15 +-
 tools/bpf/bpftool/main.h                      |  17 ++
 tools/bpf/bpftool/prog.c                      |  26 ++-
 .../selftests/bpf/test_bpftool_synctypes.py   | 163 ++++++++----------
 9 files changed, 213 insertions(+), 178 deletions(-)

Comments

Quentin Monnet May 23, 2022, 11:48 a.m. UTC | #1

2022-05-19 21:29 UTC+0000 ~ Daniel Müller <[e-mail]>
> This change switches bpftool over to using the recently introduced
> libbpf_bpf_attach_type_str function instead of maintaining its own
> string representation for the bpf_attach_type enum.
> 
> Note that contrary to other enum types, the variant names that bpftool
> maps bpf_attach_type to do not follow a simple to follow rule. With
> bpf_prog_type, for example, the textual representation can easily be
> inferred by stripping the BPF_PROG_TYPE_ prefix and lowercasing the
> remaining string. bpf_attach_type violates this rule for various
> variants.
> We decided to fix up this deficiency with this change, meaning that
> bpftool uses the same textual representations as libbpf. Supporting
> test, completion scripts, and man pages have been adjusted accordingly.
> However, we did add support for accepting (the now undocumented)
> original attach type names when they are provided by users.
> 
> For the test (test_bpftool_synctypes.py), I have removed the enum
> representation checks, because we no longer mirror the various enum
> variant names in bpftool source code. For the man page, help text, and
> completion script checks we are now using enum definitions from
> uapi/linux/bpf.h as the source of truth directly.
> 
> Signed-off-by: Daniel Müller <[e-mail]>
> ---
>  .../bpftool/Documentation/bpftool-cgroup.rst  |  16 +-
>  .../bpftool/Documentation/bpftool-prog.rst    |   5 +-
>  tools/bpf/bpftool/bash-completion/bpftool     |  18 +-
>  tools/bpf/bpftool/cgroup.c                    |  49 ++++--
>  tools/bpf/bpftool/common.c                    |  82 ++++-----
>  tools/bpf/bpftool/link.c                      |  15 +-
>  tools/bpf/bpftool/main.h                      |  17 ++
>  tools/bpf/bpftool/prog.c                      |  26 ++-
>  .../selftests/bpf/test_bpftool_synctypes.py   | 163 ++++++++----------
>  9 files changed, 213 insertions(+), 178 deletions(-)

> diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
> index effe136..c111a5 100644
> --- a/tools/bpf/bpftool/cgroup.c
> +++ b/tools/bpf/bpftool/cgroup.c
> @@ -21,25 +21,39 @@
>  #define HELP_SPEC_ATTACH_FLAGS						\
>  	"ATTACH_FLAGS := { multi | override }"
>  
> -#define HELP_SPEC_ATTACH_TYPES						       \
> -	"       ATTACH_TYPE := { ingress | egress | sock_create |\n"	       \
> -	"                        sock_ops | device | bind4 | bind6 |\n"	       \
> -	"                        post_bind4 | post_bind6 | connect4 |\n"       \
> -	"                        connect6 | getpeername4 | getpeername6 |\n"   \
> -	"                        getsockname4 | getsockname6 | sendmsg4 |\n"   \
> -	"                        sendmsg6 | recvmsg4 | recvmsg6 |\n"           \
> -	"                        sysctl | getsockopt | setsockopt |\n"	       \
> -	"                        sock_release }"
> +#define HELP_SPEC_ATTACH_TYPES						\
> +	"       ATTACH_TYPE := { cgroup_inet_ingress | cgroup_inet_egress |\n" \
> +	"                        cgroup_inet_sock_create | cgroup_sock_ops |\n" \
> +	"                        cgroup_device | cgroup_inet4_bind |\n" \
> +	"                        cgroup_inet6_bind | cgroup_inet4_post_bind |\n" \
> +	"                        cgroup_inet6_post_bind | cgroup_inet4_connect |\n" \
> +	"                        cgroup_inet6_connect | cgroup_inet4_getpeername |\n" \
> +	"                        cgroup_inet6_getpeername | cgroup_inet4_getsockname |\n" \
> +	"                        cgroup_inet6_getsockname | cgroup_udp4_sendmsg |\n" \
> +	"                        cgroup_udp6_sendmsg | cgroup_udp4_recvmsg |\n" \
> +	"                        cgroup_udp6_recvmsg | cgroup_sysctl |\n" \
> +	"                        cgroup_getsockopt | cgroup_setsockopt |\n" \
> +	"                        cgroup_inet_sock_release }"
>  
>  static unsigned int query_flags;
>  
>  static enum bpf_attach_type parse_attach_type(const char *str)
>  {
> +	const char *attach_type_str;
>  	enum bpf_attach_type type;
>  
> -	for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) {
> -		if (attach_type_name[type] &&
> -		    is_prefix(str, attach_type_name[type]))
> +	for (type = 0; ; type++) {
> +		attach_type_str = libbpf_bpf_attach_type_str(type);
> +		if (!attach_type_str)
> +			break;
> +		if (is_prefix(str, attach_type_str))

With so many shared prefixes here, I'm wondering if it would make more
sense to compare the whole string instead? Otherwise it's hard to guess
which type “bpftool c a <cgroup> cgroup_ <prog>” will use. At the same
time we allow prefixing arguments everywhere else, so maybe not worth
changing it here. Or we could maybe error out if the string length is <=
strlen("cgroup_")? Let's see for a follow-up maybe.

> +			return type;
> +
> +		/* Also check traditionally used attach type strings. */
> +		attach_type_str = bpf_attach_type_input_str(type);
> +		if (!attach_type_str)
> +			continue;
> +		if (is_prefix(str, attach_type_str))
>  			return type;
>  	}
>  

> diff --git a/tools/testing/selftests/bpf/test_bpftool_synctypes.py b/tools/testing/selftests/bpf/test_bpftool_synctypes.py
> index c0e7acd..0ca3c1 100755
> --- a/tools/testing/selftests/bpf/test_bpftool_synctypes.py
> +++ b/tools/testing/selftests/bpf/test_bpftool_synctypes.py

> @@ -139,21 +139,20 @@ class FileExtractor(object):
>  
>      def get_types_from_array(self, array_name):
>          """
> -        Search for and parse an array associating names to BPF_* enum members,
> -        for example:
> +        Search for and parse an array white-listing BPF_* enum members, for

The coding style now recommends against the “white-listing”. Maybe
“[...] a list of allowed BPF_* enum members”?

> +        example:
>  
> -            const char * const prog_type_name[] = {
> -                    [BPF_PROG_TYPE_UNSPEC]                  = "unspec",
> -                    [BPF_PROG_TYPE_SOCKET_FILTER]           = "socket_filter",
> -                    [BPF_PROG_TYPE_KPROBE]                  = "kprobe",
> +            const bool prog_type_name[] = {
> +                    [BPF_PROG_TYPE_UNSPEC]                  = true,
> +                    [BPF_PROG_TYPE_SOCKET_FILTER]           = true,
> +                    [BPF_PROG_TYPE_KPROBE]                  = true,
>              };
>  
> -        Return a dictionary with the enum member names as keys and the
> -        associated names as values, for example:
> +        Return a set of the enum members, for example:
>  
> -            {'BPF_PROG_TYPE_UNSPEC': 'unspec',
> -             'BPF_PROG_TYPE_SOCKET_FILTER': 'socket_filter',
> -             'BPF_PROG_TYPE_KPROBE': 'kprobe'}
> +            {'BPF_PROG_TYPE_UNSPEC',
> +             'BPF_PROG_TYPE_SOCKET_FILTER',
> +             'BPF_PROG_TYPE_KPROBE'}
>  
>          @array_name: name of the array to parse
>          """

> @@ -525,34 +521,18 @@ def main():
>      bashcomp_map_types = bashcomp_info.get_map_types()
>  
>      verify(source_map_types, help_map_types,
> -            f'Comparing {MapFileExtractor.filename} (map_type_name) and {MapFileExtractor.filename} (do_help() TYPE):')
> +            f'Comparing {BpfHeaderExtractor.filename} (bpf_map_type) and {MapFileExtractor.filename} (do_help() TYPE):')
>      verify(source_map_types, man_map_types,
> -            f'Comparing {MapFileExtractor.filename} (map_type_name) and {ManMapExtractor.filename} (TYPE):')
> +            f'Comparing {BpfHeaderExtractor.filename} (bpf_map_type) and {ManMapExtractor.filename} (TYPE):')
>      verify(help_map_options, man_map_options,
>              f'Comparing {MapFileExtractor.filename} (do_help() OPTIONS) and {ManMapExtractor.filename} (OPTIONS):')
>      verify(source_map_types, bashcomp_map_types,
> -            f'Comparing {MapFileExtractor.filename} (map_type_name) and {BashcompExtractor.filename} (BPFTOOL_MAP_CREATE_TYPES):')
> +            f'Comparing {BpfHeaderExtractor.filename} (bpf_map_type) and {BashcompExtractor.filename} (BPFTOOL_MAP_CREATE_TYPES):')
>  
>      # Program types (enum)
>  
> -    ref = bpf_info.get_prog_types()
> -
>      prog_info = ProgFileExtractor()

Nit: Let's remove "# Program types (enum)" and move "prog_info = ..."
under "# Attach types"?

> -    prog_types = set(prog_info.get_prog_types().keys())
> -
> -    verify(ref, prog_types,
> -            f'Comparing BPF header (enum bpf_prog_type) and {ProgFileExtractor.filename} (prog_type_name):')
> -
> -    # Attach types (enum)
> -
> -    ref = bpf_info.get_attach_types()
> -    bpf_info.close()
> -
> -    common_info = CommonFileExtractor()
> -    attach_types = common_info.get_attach_types()
> -
> -    verify(ref, attach_types,
> -            f'Comparing BPF header (enum bpf_attach_type) and {CommonFileExtractor.filename} (attach_type_name):')
> +    prog_types = bpf_info.get_prog_types()

It looks like prog_types is unused? I suspect the intention was to
compare with the program types that bpftool supports in prog.c. Looking
at this script, it seems there is no such check currently, which is
likely an ommission on my side. We should add it eventually, but given
that this is beyond the scope of this PR, let's remove "prog_types" for now?

>  
>      # Attach types (names)
>

Andrii Nakryiko May 23, 2022, 6:05 p.m. UTC | #2

On Mon, May 23, 2022 at 4:48 AM Quentin Monnet <[e-mail]> wrote:
>
> 2022-05-19 21:29 UTC+0000 ~ Daniel Müller <[e-mail]>
> > This change switches bpftool over to using the recently introduced
> > libbpf_bpf_attach_type_str function instead of maintaining its own
> > string representation for the bpf_attach_type enum.
> >
> > Note that contrary to other enum types, the variant names that bpftool
> > maps bpf_attach_type to do not follow a simple to follow rule. With
> > bpf_prog_type, for example, the textual representation can easily be
> > inferred by stripping the BPF_PROG_TYPE_ prefix and lowercasing the
> > remaining string. bpf_attach_type violates this rule for various
> > variants.
> > We decided to fix up this deficiency with this change, meaning that
> > bpftool uses the same textual representations as libbpf. Supporting
> > test, completion scripts, and man pages have been adjusted accordingly.
> > However, we did add support for accepting (the now undocumented)
> > original attach type names when they are provided by users.
> >
> > For the test (test_bpftool_synctypes.py), I have removed the enum
> > representation checks, because we no longer mirror the various enum
> > variant names in bpftool source code. For the man page, help text, and
> > completion script checks we are now using enum definitions from
> > uapi/linux/bpf.h as the source of truth directly.
> >
> > Signed-off-by: Daniel Müller <[e-mail]>
> > ---
> >  .../bpftool/Documentation/bpftool-cgroup.rst  |  16 +-
> >  .../bpftool/Documentation/bpftool-prog.rst    |   5 +-
> >  tools/bpf/bpftool/bash-completion/bpftool     |  18 +-
> >  tools/bpf/bpftool/cgroup.c                    |  49 ++++--
> >  tools/bpf/bpftool/common.c                    |  82 ++++-----
> >  tools/bpf/bpftool/link.c                      |  15 +-
> >  tools/bpf/bpftool/main.h                      |  17 ++
> >  tools/bpf/bpftool/prog.c                      |  26 ++-
> >  .../selftests/bpf/test_bpftool_synctypes.py   | 163 ++++++++----------
> >  9 files changed, 213 insertions(+), 178 deletions(-)

[...]

> >  static enum bpf_attach_type parse_attach_type(const char *str)
> >  {
> > +     const char *attach_type_str;
> >       enum bpf_attach_type type;
> >
> > -     for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) {
> > -             if (attach_type_name[type] &&
> > -                 is_prefix(str, attach_type_name[type]))
> > +     for (type = 0; ; type++) {
> > +             attach_type_str = libbpf_bpf_attach_type_str(type);
> > +             if (!attach_type_str)
> > +                     break;
> > +             if (is_prefix(str, attach_type_str))
>
> With so many shared prefixes here, I'm wondering if it would make more
> sense to compare the whole string instead? Otherwise it's hard to guess
> which type “bpftool c a <cgroup> cgroup_ <prog>” will use. At the same
> time we allow prefixing arguments everywhere else, so maybe not worth
> changing it here. Or we could maybe error out if the string length is <=
> strlen("cgroup_")? Let's see for a follow-up maybe.
>

Let's make string match exact for new strings and keep is_prefix()
logic for legacy values? It's better to split this loop into two then,
one over new-style strings and then over legacy strings.

> > +                     return type;
> > +
> > +             /* Also check traditionally used attach type strings. */
> > +             attach_type_str = bpf_attach_type_input_str(type);
> > +             if (!attach_type_str)
> > +                     continue;
> > +             if (is_prefix(str, attach_type_str))
> >                       return type;
> >       }
> >
>

[...]

Daniel Müller May 23, 2022, 8:14 p.m. UTC | #3

On Mon, May 23, 2022 at 12:48:09PM +0100, Quentin Monnet wrote:
> 2022-05-19 21:29 UTC+0000 ~ Daniel Müller <[e-mail]>
> > This change switches bpftool over to using the recently introduced
> > libbpf_bpf_attach_type_str function instead of maintaining its own
> > string representation for the bpf_attach_type enum.
> > 
> > Note that contrary to other enum types, the variant names that bpftool
> > maps bpf_attach_type to do not follow a simple to follow rule. With
> > bpf_prog_type, for example, the textual representation can easily be
> > inferred by stripping the BPF_PROG_TYPE_ prefix and lowercasing the
> > remaining string. bpf_attach_type violates this rule for various
> > variants.
> > We decided to fix up this deficiency with this change, meaning that
> > bpftool uses the same textual representations as libbpf. Supporting
> > test, completion scripts, and man pages have been adjusted accordingly.
> > However, we did add support for accepting (the now undocumented)
> > original attach type names when they are provided by users.
> > 
> > For the test (test_bpftool_synctypes.py), I have removed the enum
> > representation checks, because we no longer mirror the various enum
> > variant names in bpftool source code. For the man page, help text, and
> > completion script checks we are now using enum definitions from
> > uapi/linux/bpf.h as the source of truth directly.
> > 
> > Signed-off-by: Daniel Müller <[e-mail]>
> > ---
> >  .../bpftool/Documentation/bpftool-cgroup.rst  |  16 +-
> >  .../bpftool/Documentation/bpftool-prog.rst    |   5 +-
> >  tools/bpf/bpftool/bash-completion/bpftool     |  18 +-
> >  tools/bpf/bpftool/cgroup.c                    |  49 ++++--
> >  tools/bpf/bpftool/common.c                    |  82 ++++-----
> >  tools/bpf/bpftool/link.c                      |  15 +-
> >  tools/bpf/bpftool/main.h                      |  17 ++
> >  tools/bpf/bpftool/prog.c                      |  26 ++-
> >  .../selftests/bpf/test_bpftool_synctypes.py   | 163 ++++++++----------
> >  9 files changed, 213 insertions(+), 178 deletions(-)
> 
> > diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
> > index effe136..c111a5 100644
> > --- a/tools/bpf/bpftool/cgroup.c
> > +++ b/tools/bpf/bpftool/cgroup.c
> > @@ -21,25 +21,39 @@
> >  #define HELP_SPEC_ATTACH_FLAGS						\
> >  	"ATTACH_FLAGS := { multi | override }"
> >  
> > -#define HELP_SPEC_ATTACH_TYPES						       \
> > -	"       ATTACH_TYPE := { ingress | egress | sock_create |\n"	       \
> > -	"                        sock_ops | device | bind4 | bind6 |\n"	       \
> > -	"                        post_bind4 | post_bind6 | connect4 |\n"       \
> > -	"                        connect6 | getpeername4 | getpeername6 |\n"   \
> > -	"                        getsockname4 | getsockname6 | sendmsg4 |\n"   \
> > -	"                        sendmsg6 | recvmsg4 | recvmsg6 |\n"           \
> > -	"                        sysctl | getsockopt | setsockopt |\n"	       \
> > -	"                        sock_release }"
> > +#define HELP_SPEC_ATTACH_TYPES						\
> > +	"       ATTACH_TYPE := { cgroup_inet_ingress | cgroup_inet_egress |\n" \
> > +	"                        cgroup_inet_sock_create | cgroup_sock_ops |\n" \
> > +	"                        cgroup_device | cgroup_inet4_bind |\n" \
> > +	"                        cgroup_inet6_bind | cgroup_inet4_post_bind |\n" \
> > +	"                        cgroup_inet6_post_bind | cgroup_inet4_connect |\n" \
> > +	"                        cgroup_inet6_connect | cgroup_inet4_getpeername |\n" \
> > +	"                        cgroup_inet6_getpeername | cgroup_inet4_getsockname |\n" \
> > +	"                        cgroup_inet6_getsockname | cgroup_udp4_sendmsg |\n" \
> > +	"                        cgroup_udp6_sendmsg | cgroup_udp4_recvmsg |\n" \
> > +	"                        cgroup_udp6_recvmsg | cgroup_sysctl |\n" \
> > +	"                        cgroup_getsockopt | cgroup_setsockopt |\n" \
> > +	"                        cgroup_inet_sock_release }"
> >  
> >  static unsigned int query_flags;
> >  
> >  static enum bpf_attach_type parse_attach_type(const char *str)
> >  {
> > +	const char *attach_type_str;
> >  	enum bpf_attach_type type;
> >  
> > -	for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) {
> > -		if (attach_type_name[type] &&
> > -		    is_prefix(str, attach_type_name[type]))
> > +	for (type = 0; ; type++) {
> > +		attach_type_str = libbpf_bpf_attach_type_str(type);
> > +		if (!attach_type_str)
> > +			break;
> > +		if (is_prefix(str, attach_type_str))
> 
> With so many shared prefixes here, I'm wondering if it would make more
> sense to compare the whole string instead? Otherwise it's hard to guess
> which type “bpftool c a <cgroup> cgroup_ <prog>” will use. At the same
> time we allow prefixing arguments everywhere else, so maybe not worth
> changing it here. Or we could maybe error out if the string length is <=
> strlen("cgroup_")? Let's see for a follow-up maybe.

It is true that it could get confusing, but I'd think it's mostly for write-once
cases where this functionality is used. And there I really see value in
supporting prefixes. I also agree that it's what we do elsewhere. What I think
we should consider fixing, though, is just doing short-circuited first-matches
check. In my opinion we should error out if there are multiple matches instead.
After all, what is first depends on numeric values and these are not really
obvious to the user, I think (and certainly nothing I would want to be bothered
with at this point).

As that is an existing problem, I'd suggest we leave the existing behavior until
we address that.

> > +			return type;
> > +
> > +		/* Also check traditionally used attach type strings. */
> > +		attach_type_str = bpf_attach_type_input_str(type);
> > +		if (!attach_type_str)
> > +			continue;
> > +		if (is_prefix(str, attach_type_str))
> >  			return type;
> >  	}
> >  
> 
> > diff --git a/tools/testing/selftests/bpf/test_bpftool_synctypes.py b/tools/testing/selftests/bpf/test_bpftool_synctypes.py
> > index c0e7acd..0ca3c1 100755
> > --- a/tools/testing/selftests/bpf/test_bpftool_synctypes.py
> > +++ b/tools/testing/selftests/bpf/test_bpftool_synctypes.py
> 
> > @@ -139,21 +139,20 @@ class FileExtractor(object):
> >  
> >      def get_types_from_array(self, array_name):
> >          """
> > -        Search for and parse an array associating names to BPF_* enum members,
> > -        for example:
> > +        Search for and parse an array white-listing BPF_* enum members, for
> 
> The coding style now recommends against the “white-listing”. Maybe
> “[...] a list of allowed BPF_* enum members”?

Ah, good catch, thanks. Will fix it.

[...]

> > @@ -525,34 +521,18 @@ def main():
> >      bashcomp_map_types = bashcomp_info.get_map_types()
> >  
> >      verify(source_map_types, help_map_types,
> > -            f'Comparing {MapFileExtractor.filename} (map_type_name) and {MapFileExtractor.filename} (do_help() TYPE):')
> > +            f'Comparing {BpfHeaderExtractor.filename} (bpf_map_type) and {MapFileExtractor.filename} (do_help() TYPE):')
> >      verify(source_map_types, man_map_types,
> > -            f'Comparing {MapFileExtractor.filename} (map_type_name) and {ManMapExtractor.filename} (TYPE):')
> > +            f'Comparing {BpfHeaderExtractor.filename} (bpf_map_type) and {ManMapExtractor.filename} (TYPE):')
> >      verify(help_map_options, man_map_options,
> >              f'Comparing {MapFileExtractor.filename} (do_help() OPTIONS) and {ManMapExtractor.filename} (OPTIONS):')
> >      verify(source_map_types, bashcomp_map_types,
> > -            f'Comparing {MapFileExtractor.filename} (map_type_name) and {BashcompExtractor.filename} (BPFTOOL_MAP_CREATE_TYPES):')
> > +            f'Comparing {BpfHeaderExtractor.filename} (bpf_map_type) and {BashcompExtractor.filename} (BPFTOOL_MAP_CREATE_TYPES):')
> >  
> >      # Program types (enum)
> >  
> > -    ref = bpf_info.get_prog_types()
> > -
> >      prog_info = ProgFileExtractor()
> 
> Nit: Let's remove "# Program types (enum)" and move "prog_info = ..."
> under "# Attach types"?

Sounds good. Done.

> > -    prog_types = set(prog_info.get_prog_types().keys())
> > -
> > -    verify(ref, prog_types,
> > -            f'Comparing BPF header (enum bpf_prog_type) and {ProgFileExtractor.filename} (prog_type_name):')
> > -
> > -    # Attach types (enum)
> > -
> > -    ref = bpf_info.get_attach_types()
> > -    bpf_info.close()
> > -
> > -    common_info = CommonFileExtractor()
> > -    attach_types = common_info.get_attach_types()
> > -
> > -    verify(ref, attach_types,
> > -            f'Comparing BPF header (enum bpf_attach_type) and {CommonFileExtractor.filename} (attach_type_name):')
> > +    prog_types = bpf_info.get_prog_types()
> 
> It looks like prog_types is unused? I suspect the intention was to
> compare with the program types that bpftool supports in prog.c. Looking
> at this script, it seems there is no such check currently, which is
> likely an ommission on my side. We should add it eventually, but given
> that this is beyond the scope of this PR, let's remove "prog_types" for now?

Yep, unsure how I missed it. Thanks for spotting.

Thanks for the review.

Daniel

Daniel Müller May 23, 2022, 8:17 p.m. UTC | #4

On Mon, May 23, 2022 at 11:05:08AM -0700, Andrii Nakryiko wrote:
> On Mon, May 23, 2022 at 4:48 AM Quentin Monnet <[e-mail]> wrote:
> >
> > 2022-05-19 21:29 UTC+0000 ~ Daniel Müller <[e-mail]>
> > > This change switches bpftool over to using the recently introduced
> > > libbpf_bpf_attach_type_str function instead of maintaining its own
> > > string representation for the bpf_attach_type enum.
> > >
> > > Note that contrary to other enum types, the variant names that bpftool
> > > maps bpf_attach_type to do not follow a simple to follow rule. With
> > > bpf_prog_type, for example, the textual representation can easily be
> > > inferred by stripping the BPF_PROG_TYPE_ prefix and lowercasing the
> > > remaining string. bpf_attach_type violates this rule for various
> > > variants.
> > > We decided to fix up this deficiency with this change, meaning that
> > > bpftool uses the same textual representations as libbpf. Supporting
> > > test, completion scripts, and man pages have been adjusted accordingly.
> > > However, we did add support for accepting (the now undocumented)
> > > original attach type names when they are provided by users.
> > >
> > > For the test (test_bpftool_synctypes.py), I have removed the enum
> > > representation checks, because we no longer mirror the various enum
> > > variant names in bpftool source code. For the man page, help text, and
> > > completion script checks we are now using enum definitions from
> > > uapi/linux/bpf.h as the source of truth directly.
> > >
> > > Signed-off-by: Daniel Müller <[e-mail]>
> > > ---
> > >  .../bpftool/Documentation/bpftool-cgroup.rst  |  16 +-
> > >  .../bpftool/Documentation/bpftool-prog.rst    |   5 +-
> > >  tools/bpf/bpftool/bash-completion/bpftool     |  18 +-
> > >  tools/bpf/bpftool/cgroup.c                    |  49 ++++--
> > >  tools/bpf/bpftool/common.c                    |  82 ++++-----
> > >  tools/bpf/bpftool/link.c                      |  15 +-
> > >  tools/bpf/bpftool/main.h                      |  17 ++
> > >  tools/bpf/bpftool/prog.c                      |  26 ++-
> > >  .../selftests/bpf/test_bpftool_synctypes.py   | 163 ++++++++----------
> > >  9 files changed, 213 insertions(+), 178 deletions(-)
> 
> [...]
> 
> > >  static enum bpf_attach_type parse_attach_type(const char *str)
> > >  {
> > > +     const char *attach_type_str;
> > >       enum bpf_attach_type type;
> > >
> > > -     for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) {
> > > -             if (attach_type_name[type] &&
> > > -                 is_prefix(str, attach_type_name[type]))
> > > +     for (type = 0; ; type++) {
> > > +             attach_type_str = libbpf_bpf_attach_type_str(type);
> > > +             if (!attach_type_str)
> > > +                     break;
> > > +             if (is_prefix(str, attach_type_str))
> >
> > With so many shared prefixes here, I'm wondering if it would make more
> > sense to compare the whole string instead? Otherwise it's hard to guess
> > which type “bpftool c a <cgroup> cgroup_ <prog>” will use. At the same
> > time we allow prefixing arguments everywhere else, so maybe not worth
> > changing it here. Or we could maybe error out if the string length is <=
> > strlen("cgroup_")? Let's see for a follow-up maybe.
> >
> 
> Let's make string match exact for new strings and keep is_prefix()
> logic for legacy values? It's better to split this loop into two then,
> one over new-style strings and then over legacy strings.

Okay, let's do that then.

Thanks,
Daniel

Patch

diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
index a17e9a..bd015e 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
@@ -31,11 +31,17 @@  CGROUP COMMANDS
 |	**bpftool** **cgroup help**
 |
 |	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
-|	*ATTACH_TYPE* := { **ingress** | **egress** | **sock_create** | **sock_ops** | **device** |
-|		**bind4** | **bind6** | **post_bind4** | **post_bind6** | **connect4** | **connect6** |
-|		**getpeername4** | **getpeername6** | **getsockname4** | **getsockname6** | **sendmsg4** |
-|		**sendmsg6** | **recvmsg4** | **recvmsg6** | **sysctl** | **getsockopt** | **setsockopt** |
-|		**sock_release** }
+|	*ATTACH_TYPE* := { **cgroup_inet_ingress** | **cgroup_inet_egress** |
+|		**cgroup_inet_sock_create** | **cgroup_sock_ops** |
+|		**cgroup_device** | **cgroup_inet4_bind** | **cgroup_inet6_bind** |
+|		**cgroup_inet4_post_bind** | **cgroup_inet6_post_bind** |
+|		**cgroup_inet4_connect** | **cgroup_inet6_connect** |
+|		**cgroup_inet4_getpeername** | **cgroup_inet6_getpeername** |
+|		**cgroup_inet4_getsockname** | **cgroup_inet6_getsockname** |
+|		**cgroup_udp4_sendmsg** | **cgroup_udp6_sendmsg** |
+|		**cgroup_udp4_recvmsg** | **cgroup_udp6_recvmsg** |
+|		**cgroup_sysctl** | **cgroup_getsockopt** | **cgroup_setsockopt** |
+|		**cgroup_inet_sock_release** }
 |	*ATTACH_FLAGS* := { **multi** | **override** }
 
 DESCRIPTION
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index a2e935..eb1b2a 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -53,8 +53,9 @@  PROG COMMANDS
 |		**cgroup/getsockopt** | **cgroup/setsockopt** | **cgroup/sock_release** |
 |		**struct_ops** | **fentry** | **fexit** | **freplace** | **sk_lookup**
 |	}
-|       *ATTACH_TYPE* := {
-|		**msg_verdict** | **skb_verdict** | **stream_verdict** | **stream_parser** | **flow_dissector**
+|	*ATTACH_TYPE* := {
+|		**sk_msg_verdict** | **sk_skb_verdict** | **sk_skb_stream_verdict** |
+|		**sk_skb_stream_parser** | **flow_dissector**
 |	}
 |	*METRICs* := {
 |		**cycles** | **instructions** | **l1d_loads** | **llc_misses** |
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 5df8d7..91f89a 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -407,8 +407,8 @@  _bpftool()
                             return 0
                             ;;
                         5)
-                            local BPFTOOL_PROG_ATTACH_TYPES='msg_verdict \
-                                skb_verdict stream_verdict stream_parser \
+                            local BPFTOOL_PROG_ATTACH_TYPES='sk_msg_verdict \
+                                sk_skb_verdict sk_skb_stream_verdict sk_skb_stream_parser \
                                 flow_dissector'
                             COMPREPLY=( $( compgen -W "$BPFTOOL_PROG_ATTACH_TYPES" -- "$cur" ) )
                             return 0
@@ -1039,12 +1039,14 @@  _bpftool()
                     return 0
                     ;;
                 attach|detach)
-                    local BPFTOOL_CGROUP_ATTACH_TYPES='ingress egress \
-                        sock_create sock_ops device \
-                        bind4 bind6 post_bind4 post_bind6 connect4 connect6 \
-                        getpeername4 getpeername6 getsockname4 getsockname6 \
-                        sendmsg4 sendmsg6 recvmsg4 recvmsg6 sysctl getsockopt \
-                        setsockopt sock_release'
+                    local BPFTOOL_CGROUP_ATTACH_TYPES='cgroup_inet_ingress cgroup_inet_egress \
+                        cgroup_inet_sock_create cgroup_sock_ops cgroup_device cgroup_inet4_bind \
+                        cgroup_inet6_bind cgroup_inet4_post_bind cgroup_inet6_post_bind \
+                        cgroup_inet4_connect cgroup_inet6_connect cgroup_inet4_getpeername \
+                        cgroup_inet6_getpeername cgroup_inet4_getsockname cgroup_inet6_getsockname \
+                        cgroup_udp4_sendmsg cgroup_udp6_sendmsg cgroup_udp4_recvmsg \
+                        cgroup_udp6_recvmsg cgroup_sysctl cgroup_getsockopt cgroup_setsockopt \
+                        cgroup_inet_sock_release'
                     local ATTACH_FLAGS='multi override'
                     local PROG_TYPE='id pinned tag name'
                     # Check for $prev = $command first
diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
index effe136..c111a5 100644
--- a/tools/bpf/bpftool/cgroup.c
+++ b/tools/bpf/bpftool/cgroup.c
@@ -21,25 +21,39 @@ 
 #define HELP_SPEC_ATTACH_FLAGS						\
 	"ATTACH_FLAGS := { multi | override }"
 
-#define HELP_SPEC_ATTACH_TYPES						       \
-	"       ATTACH_TYPE := { ingress | egress | sock_create |\n"	       \
-	"                        sock_ops | device | bind4 | bind6 |\n"	       \
-	"                        post_bind4 | post_bind6 | connect4 |\n"       \
-	"                        connect6 | getpeername4 | getpeername6 |\n"   \
-	"                        getsockname4 | getsockname6 | sendmsg4 |\n"   \
-	"                        sendmsg6 | recvmsg4 | recvmsg6 |\n"           \
-	"                        sysctl | getsockopt | setsockopt |\n"	       \
-	"                        sock_release }"
+#define HELP_SPEC_ATTACH_TYPES						\
+	"       ATTACH_TYPE := { cgroup_inet_ingress | cgroup_inet_egress |\n" \
+	"                        cgroup_inet_sock_create | cgroup_sock_ops |\n" \
+	"                        cgroup_device | cgroup_inet4_bind |\n" \
+	"                        cgroup_inet6_bind | cgroup_inet4_post_bind |\n" \
+	"                        cgroup_inet6_post_bind | cgroup_inet4_connect |\n" \
+	"                        cgroup_inet6_connect | cgroup_inet4_getpeername |\n" \
+	"                        cgroup_inet6_getpeername | cgroup_inet4_getsockname |\n" \
+	"                        cgroup_inet6_getsockname | cgroup_udp4_sendmsg |\n" \
+	"                        cgroup_udp6_sendmsg | cgroup_udp4_recvmsg |\n" \
+	"                        cgroup_udp6_recvmsg | cgroup_sysctl |\n" \
+	"                        cgroup_getsockopt | cgroup_setsockopt |\n" \
+	"                        cgroup_inet_sock_release }"
 
 static unsigned int query_flags;
 
 static enum bpf_attach_type parse_attach_type(const char *str)
 {
+	const char *attach_type_str;
 	enum bpf_attach_type type;
 
-	for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) {
-		if (attach_type_name[type] &&
-		    is_prefix(str, attach_type_name[type]))
+	for (type = 0; ; type++) {
+		attach_type_str = libbpf_bpf_attach_type_str(type);
+		if (!attach_type_str)
+			break;
+		if (is_prefix(str, attach_type_str))
+			return type;
+
+		/* Also check traditionally used attach type strings. */
+		attach_type_str = bpf_attach_type_input_str(type);
+		if (!attach_type_str)
+			continue;
+		if (is_prefix(str, attach_type_str))
 			return type;
 	}
 
@@ -52,6 +66,7 @@  static int show_bpf_prog(int id, enum bpf_attach_type attach_type,
 {
 	char prog_name[MAX_PROG_FULL_NAME];
 	struct bpf_prog_info info = {};
+	const char *attach_type_str;
 	__u32 info_len = sizeof(info);
 	int prog_fd;
 
@@ -64,13 +79,13 @@  static int show_bpf_prog(int id, enum bpf_attach_type attach_type,
 		return -1;
 	}
 
+	attach_type_str = libbpf_bpf_attach_type_str(attach_type);
 	get_prog_full_name(&info, prog_fd, prog_name, sizeof(prog_name));
 	if (json_output) {
 		jsonw_start_object(json_wtr);
 		jsonw_uint_field(json_wtr, "id", info.id);
-		if (attach_type < ARRAY_SIZE(attach_type_name))
-			jsonw_string_field(json_wtr, "attach_type",
-					   attach_type_name[attach_type]);
+		if (attach_type_str)
+			jsonw_string_field(json_wtr, "attach_type", attach_type_str);
 		else
 			jsonw_uint_field(json_wtr, "attach_type", attach_type);
 		jsonw_string_field(json_wtr, "attach_flags",
@@ -79,8 +94,8 @@  static int show_bpf_prog(int id, enum bpf_attach_type attach_type,
 		jsonw_end_object(json_wtr);
 	} else {
 		printf("%s%-8u ", level ? "    " : "", info.id);
-		if (attach_type < ARRAY_SIZE(attach_type_name))
-			printf("%-15s", attach_type_name[attach_type]);
+		if (attach_type_str)
+			printf("%-15s", attach_type_str);
 		else
 			printf("type %-10u", attach_type);
 		printf(" %-15s %-15s\n", attach_flags_str, prog_name);
diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
index c74014..b659eac 100644
--- a/tools/bpf/bpftool/common.c
+++ b/tools/bpf/bpftool/common.c
@@ -31,52 +31,6 @@ 
 #define BPF_FS_MAGIC		0xcafe4a11
 #endif
 
-const char * const attach_type_name[__MAX_BPF_ATTACH_TYPE] = {
-	[BPF_CGROUP_INET_INGRESS]	= "ingress",
-	[BPF_CGROUP_INET_EGRESS]	= "egress",
-	[BPF_CGROUP_INET_SOCK_CREATE]	= "sock_create",
-	[BPF_CGROUP_INET_SOCK_RELEASE]	= "sock_release",
-	[BPF_CGROUP_SOCK_OPS]		= "sock_ops",
-	[BPF_CGROUP_DEVICE]		= "device",
-	[BPF_CGROUP_INET4_BIND]		= "bind4",
-	[BPF_CGROUP_INET6_BIND]		= "bind6",
-	[BPF_CGROUP_INET4_CONNECT]	= "connect4",
-	[BPF_CGROUP_INET6_CONNECT]	= "connect6",
-	[BPF_CGROUP_INET4_POST_BIND]	= "post_bind4",
-	[BPF_CGROUP_INET6_POST_BIND]	= "post_bind6",
-	[BPF_CGROUP_INET4_GETPEERNAME]	= "getpeername4",
-	[BPF_CGROUP_INET6_GETPEERNAME]	= "getpeername6",
-	[BPF_CGROUP_INET4_GETSOCKNAME]	= "getsockname4",
-	[BPF_CGROUP_INET6_GETSOCKNAME]	= "getsockname6",
-	[BPF_CGROUP_UDP4_SENDMSG]	= "sendmsg4",
-	[BPF_CGROUP_UDP6_SENDMSG]	= "sendmsg6",
-	[BPF_CGROUP_SYSCTL]		= "sysctl",
-	[BPF_CGROUP_UDP4_RECVMSG]	= "recvmsg4",
-	[BPF_CGROUP_UDP6_RECVMSG]	= "recvmsg6",
-	[BPF_CGROUP_GETSOCKOPT]		= "getsockopt",
-	[BPF_CGROUP_SETSOCKOPT]		= "setsockopt",
-	[BPF_SK_SKB_STREAM_PARSER]	= "sk_skb_stream_parser",
-	[BPF_SK_SKB_STREAM_VERDICT]	= "sk_skb_stream_verdict",
-	[BPF_SK_SKB_VERDICT]		= "sk_skb_verdict",
-	[BPF_SK_MSG_VERDICT]		= "sk_msg_verdict",
-	[BPF_LIRC_MODE2]		= "lirc_mode2",
-	[BPF_FLOW_DISSECTOR]		= "flow_dissector",
-	[BPF_TRACE_RAW_TP]		= "raw_tp",
-	[BPF_TRACE_FENTRY]		= "fentry",
-	[BPF_TRACE_FEXIT]		= "fexit",
-	[BPF_MODIFY_RETURN]		= "mod_ret",
-	[BPF_LSM_MAC]			= "lsm_mac",
-	[BPF_SK_LOOKUP]			= "sk_lookup",
-	[BPF_TRACE_ITER]		= "trace_iter",
-	[BPF_XDP_DEVMAP]		= "xdp_devmap",
-	[BPF_XDP_CPUMAP]		= "xdp_cpumap",
-	[BPF_XDP]			= "xdp",
-	[BPF_SK_REUSEPORT_SELECT]	= "sk_skb_reuseport_select",
-	[BPF_SK_REUSEPORT_SELECT_OR_MIGRATE]	= "sk_skb_reuseport_select_or_migrate",
-	[BPF_PERF_EVENT]		= "perf_event",
-	[BPF_TRACE_KPROBE_MULTI]	= "trace_kprobe_multi",
-};
-
 void p_err(const char *fmt, ...)
 {
 	va_list ap;
@@ -1009,3 +963,39 @@  bool equal_fn_for_key_as_id(const void *k1, const void *k2, void *ctx)
 {
 	return k1 == k2;
 }
+
+const char *bpf_attach_type_input_str(enum bpf_attach_type t)
+{
+	switch (t) {
+	case BPF_CGROUP_INET_INGRESS:		return "ingress";
+	case BPF_CGROUP_INET_EGRESS:		return "egress";
+	case BPF_CGROUP_INET_SOCK_CREATE:	return "sock_create";
+	case BPF_CGROUP_INET_SOCK_RELEASE:	return "sock_release";
+	case BPF_CGROUP_SOCK_OPS:		return "sock_ops";
+	case BPF_CGROUP_DEVICE:			return "device";
+	case BPF_CGROUP_INET4_BIND:		return "bind4";
+	case BPF_CGROUP_INET6_BIND:		return "bind6";
+	case BPF_CGROUP_INET4_CONNECT:		return "connect4";
+	case BPF_CGROUP_INET6_CONNECT:		return "connect6";
+	case BPF_CGROUP_INET4_POST_BIND:	return "post_bind4";
+	case BPF_CGROUP_INET6_POST_BIND:	return "post_bind6";
+	case BPF_CGROUP_INET4_GETPEERNAME:	return "getpeername4";
+	case BPF_CGROUP_INET6_GETPEERNAME:	return "getpeername6";
+	case BPF_CGROUP_INET4_GETSOCKNAME:	return "getsockname4";
+	case BPF_CGROUP_INET6_GETSOCKNAME:	return "getsockname6";
+	case BPF_CGROUP_UDP4_SENDMSG:		return "sendmsg4";
+	case BPF_CGROUP_UDP6_SENDMSG:		return "sendmsg6";
+	case BPF_CGROUP_SYSCTL:			return "sysctl";
+	case BPF_CGROUP_UDP4_RECVMSG:		return "recvmsg4";
+	case BPF_CGROUP_UDP6_RECVMSG:		return "recvmsg6";
+	case BPF_CGROUP_GETSOCKOPT:		return "getsockopt";
+	case BPF_CGROUP_SETSOCKOPT:		return "setsockopt";
+	case BPF_TRACE_RAW_TP:			return "raw_tp";
+	case BPF_TRACE_FENTRY:			return "fentry";
+	case BPF_TRACE_FEXIT:			return "fexit";
+	case BPF_MODIFY_RETURN:			return "mod_ret";
+	case BPF_SK_REUSEPORT_SELECT:		return "sk_skb_reuseport_select";
+	case BPF_SK_REUSEPORT_SELECT_OR_MIGRATE:	return "sk_skb_reuseport_select_or_migrate";
+	default:	return NULL;
+	}
+}
diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
index e27108..66a254 100644
--- a/tools/bpf/bpftool/link.c
+++ b/tools/bpf/bpftool/link.c
@@ -78,9 +78,11 @@  show_link_header_json(struct bpf_link_info *info, json_writer_t *wtr)
 
 static void show_link_attach_type_json(__u32 attach_type, json_writer_t *wtr)
 {
-	if (attach_type < ARRAY_SIZE(attach_type_name))
-		jsonw_string_field(wtr, "attach_type",
-				   attach_type_name[attach_type]);
+	const char *attach_type_str;
+
+	attach_type_str = libbpf_bpf_attach_type_str(attach_type);
+	if (attach_type_str)
+		jsonw_string_field(wtr, "attach_type", attach_type_str);
 	else
 		jsonw_uint_field(wtr, "attach_type", attach_type);
 }
@@ -196,8 +198,11 @@  static void show_link_header_plain(struct bpf_link_info *info)
 
 static void show_link_attach_type_plain(__u32 attach_type)
 {
-	if (attach_type < ARRAY_SIZE(attach_type_name))
-		printf("attach_type %s  ", attach_type_name[attach_type]);
+	const char *attach_type_str;
+
+	attach_type_str = libbpf_bpf_attach_type_str(attach_type);
+	if (attach_type_str)
+		printf("attach_type %s  ", attach_type_str);
 	else
 		printf("attach_type %u  ", attach_type);
 }
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index e4fdaa0..a686b2c 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -243,6 +243,23 @@  int print_all_levels(__maybe_unused enum libbpf_print_level level,
 size_t hash_fn_for_key_as_id(const void *key, void *ctx);
 bool equal_fn_for_key_as_id(const void *k1, const void *k2, void *ctx);
 
+/**
+ * bpf_attach_type_input_str - convert the provided attach type value into a
+ * textual representation that we accept for input purposes.
+ *
+ * This function acts is similar in nature to libbpf_bpf_attach_type_str, but
+ * only recognizes some of attach type names that have been used by the program
+ * in the past and which do not follow the string inference scheme that libbpf
+ * uses.
+ * These textual representations should only be used for user input.
+ *
+ * @t: The attach type
+ * Returns a pointer to a static string identifying the attach type. NULL is
+ * returned for unknown bpf_attach_type values or those whose representation
+ * does not differ from that in libbpf itself.
+ */
+const char *bpf_attach_type_input_str(enum bpf_attach_type t);
+
 static inline void *u32_as_hash_field(__u32 x)
 {
 	return (void *)(uintptr_t)x;
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 39e1e71..a45fa3 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -41,12 +41,24 @@  enum dump_mode {
 	DUMP_XLATED,
 };
 
+static const bool attach_types[] = {
+	[BPF_SK_SKB_STREAM_PARSER] = true,
+	[BPF_SK_SKB_STREAM_VERDICT] = true,
+	[BPF_SK_SKB_VERDICT] = true,
+	[BPF_SK_MSG_VERDICT] = true,
+	[BPF_FLOW_DISSECTOR] = true,
+	[__MAX_BPF_ATTACH_TYPE] = false,
+};
+
+/*
+ * Textual representations traditionally used by the program and kept around for
+ * the sake of backwards compatibility.
+ */
 static const char * const attach_type_strings[] = {
 	[BPF_SK_SKB_STREAM_PARSER] = "stream_parser",
 	[BPF_SK_SKB_STREAM_VERDICT] = "stream_verdict",
 	[BPF_SK_SKB_VERDICT] = "skb_verdict",
 	[BPF_SK_MSG_VERDICT] = "msg_verdict",
-	[BPF_FLOW_DISSECTOR] = "flow_dissector",
 	[__MAX_BPF_ATTACH_TYPE] = NULL,
 };
 
@@ -57,6 +69,14 @@  static enum bpf_attach_type parse_attach_type(const char *str)
 	enum bpf_attach_type type;
 
 	for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) {
+		if (attach_types[type]) {
+			const char *attach_type_str;
+
+			attach_type_str = libbpf_bpf_attach_type_str(type);
+			if (is_prefix(str, attach_type_str))
+				return type;
+		}
+
 		if (attach_type_strings[type] &&
 		    is_prefix(str, attach_type_strings[type]))
 			return type;
@@ -2341,8 +2361,8 @@  static int do_help(int argc, char **argv)
 		"                 cgroup/sendmsg6 | cgroup/recvmsg4 | cgroup/recvmsg6 |\n"
 		"                 cgroup/getsockopt | cgroup/setsockopt | cgroup/sock_release |\n"
 		"                 struct_ops | fentry | fexit | freplace | sk_lookup }\n"
-		"       ATTACH_TYPE := { msg_verdict | skb_verdict | stream_verdict |\n"
-		"                        stream_parser | flow_dissector }\n"
+		"       ATTACH_TYPE := { sk_msg_verdict | sk_skb_verdict | sk_skb_stream_verdict |\n"
+		"                        sk_skb_stream_parser | flow_dissector }\n"
 		"       METRIC := { cycles | instructions | l1d_loads | llc_misses | itlb_misses | dtlb_misses }\n"
 		"       " HELP_SPEC_OPTIONS " |\n"
 		"                    {-f|--bpffs} | {-m|--mapcompat} | {-n|--nomount} |\n"
diff --git a/tools/testing/selftests/bpf/test_bpftool_synctypes.py b/tools/testing/selftests/bpf/test_bpftool_synctypes.py
index c0e7acd..0ca3c1 100755
--- a/tools/testing/selftests/bpf/test_bpftool_synctypes.py
+++ b/tools/testing/selftests/bpf/test_bpftool_synctypes.py
@@ -58,7 +58,7 @@  class BlockParser(object):
 
 class ArrayParser(BlockParser):
     """
-    A parser for extracting dicionaries of values from some BPF-related arrays.
+    A parser for extracting a set of values from some BPF-related arrays.
     @reader: a pointer to the open file to parse
     @array_name: name of the array to parse
     """
@@ -66,7 +66,7 @@  class ArrayParser(BlockParser):
 
     def __init__(self, reader, array_name):
         self.array_name = array_name
-        self.start_marker = re.compile(f'(static )?const char \* const {self.array_name}\[.*\] = {{\n')
+        self.start_marker = re.compile(f'(static )?const bool {self.array_name}\[.*\] = {{\n')
         super().__init__(reader)
 
     def search_block(self):
@@ -80,15 +80,15 @@  class ArrayParser(BlockParser):
         Parse a block and return data as a dictionary. Items to extract must be
         on separate lines in the file.
         """
-        pattern = re.compile('\[(BPF_\w*)\]\s*= "(.*)",?$')
-        entries = {}
+        pattern = re.compile('\[(BPF_\w*)\]\s*= (true|false),?$')
+        entries = set()
         while True:
             line = self.reader.readline()
             if line == '' or re.match(self.end_marker, line):
                 break
             capture = pattern.search(line)
             if capture:
-                entries[capture.group(1)] = capture.group(2)
+                entries |= {capture.group(1)}
         return entries
 
 class InlineListParser(BlockParser):
@@ -115,7 +115,7 @@  class InlineListParser(BlockParser):
 class FileExtractor(object):
     """
     A generic reader for extracting data from a given file. This class contains
-    several helper methods that wrap arround parser objects to extract values
+    several helper methods that wrap around parser objects to extract values
     from different structures.
     This class does not offer a way to set a filename, which is expected to be
     defined in children classes.
@@ -139,21 +139,20 @@  class FileExtractor(object):
 
     def get_types_from_array(self, array_name):
         """
-        Search for and parse an array associating names to BPF_* enum members,
-        for example:
+        Search for and parse an array white-listing BPF_* enum members, for
+        example:
 
-            const char * const prog_type_name[] = {
-                    [BPF_PROG_TYPE_UNSPEC]                  = "unspec",
-                    [BPF_PROG_TYPE_SOCKET_FILTER]           = "socket_filter",
-                    [BPF_PROG_TYPE_KPROBE]                  = "kprobe",
+            const bool prog_type_name[] = {
+                    [BPF_PROG_TYPE_UNSPEC]                  = true,
+                    [BPF_PROG_TYPE_SOCKET_FILTER]           = true,
+                    [BPF_PROG_TYPE_KPROBE]                  = true,
             };
 
-        Return a dictionary with the enum member names as keys and the
-        associated names as values, for example:
+        Return a set of the enum members, for example:
 
-            {'BPF_PROG_TYPE_UNSPEC': 'unspec',
-             'BPF_PROG_TYPE_SOCKET_FILTER': 'socket_filter',
-             'BPF_PROG_TYPE_KPROBE': 'kprobe'}
+            {'BPF_PROG_TYPE_UNSPEC',
+             'BPF_PROG_TYPE_SOCKET_FILTER',
+             'BPF_PROG_TYPE_KPROBE'}
 
         @array_name: name of the array to parse
         """
@@ -186,6 +185,27 @@  class FileExtractor(object):
         parser.search_block(start_marker)
         return parser.parse(pattern, end_marker)
 
+    def make_enum_map(self, names, enum_prefix):
+        """
+        Search for and parse an enum containing BPF_* members, just as get_enum
+        does. However, instead of just returning a set of the variant names,
+        also generate a textual representation from them by (assuming and)
+        removing a provided prefix and lowercasing the remainder. Then return a
+        dict mapping from name to textual representation.
+
+        @enum_values: a set of enum values; e.g., as retrieved by get_enum
+        @enum_prefix: the prefix to remove from each of the variants to infer
+        textual representation
+        """
+        mapping = {}
+        for name in names:
+            if not name.startswith(enum_prefix):
+                raise Exception(f"enum variant {name} does not start with {enum_prefix}")
+            text = name[len(enum_prefix):].lower()
+            mapping[name] = text
+
+        return mapping
+
     def __get_description_list(self, start_marker, pattern, end_marker):
         parser = InlineListParser(self.reader)
         parser.search_block(start_marker)
@@ -333,11 +353,9 @@  class ProgFileExtractor(SourceFileExtractor):
     """
     filename = os.path.join(BPFTOOL_DIR, 'prog.c')
 
-    def get_prog_types(self):
-        return self.get_types_from_array('prog_type_name')
-
     def get_attach_types(self):
-        return self.get_types_from_array('attach_type_strings')
+        types = self.get_types_from_array('attach_types')
+        return self.make_enum_map(types, 'BPF_')
 
     def get_prog_attach_help(self):
         return self.get_help_list('ATTACH_TYPE')
@@ -348,9 +366,6 @@  class MapFileExtractor(SourceFileExtractor):
     """
     filename = os.path.join(BPFTOOL_DIR, 'map.c')
 
-    def get_map_types(self):
-        return self.get_types_from_array('map_type_name')
-
     def get_map_help(self):
         return self.get_help_list('TYPE')
 
@@ -363,30 +378,6 @@  class CgroupFileExtractor(SourceFileExtractor):
     def get_prog_attach_help(self):
         return self.get_help_list('ATTACH_TYPE')
 
-class CommonFileExtractor(SourceFileExtractor):
-    """
-    An extractor for bpftool's common.c.
-    """
-    filename = os.path.join(BPFTOOL_DIR, 'common.c')
-
-    def __init__(self):
-        super().__init__()
-        self.attach_types = {}
-
-    def get_attach_types(self):
-        if not self.attach_types:
-            self.attach_types = self.get_types_from_array('attach_type_name')
-        return self.attach_types
-
-    def get_cgroup_attach_types(self):
-        if not self.attach_types:
-            self.get_attach_types()
-        cgroup_types = {}
-        for (key, value) in self.attach_types.items():
-            if key.find('BPF_CGROUP') != -1:
-                cgroup_types[key] = value
-        return cgroup_types
-
 class GenericSourceExtractor(SourceFileExtractor):
     """
     An extractor for generic source code files.
@@ -403,14 +394,28 @@  class BpfHeaderExtractor(FileExtractor):
     """
     filename = os.path.join(INCLUDE_DIR, 'uapi/linux/bpf.h')
 
+    def __init__(self):
+        super().__init__()
+        self.attach_types = {}
+
     def get_prog_types(self):
         return self.get_enum('bpf_prog_type')
 
-    def get_map_types(self):
-        return self.get_enum('bpf_map_type')
+    def get_map_type_map(self):
+        names = self.get_enum('bpf_map_type')
+        return self.make_enum_map(names, 'BPF_MAP_TYPE_')
 
-    def get_attach_types(self):
-        return self.get_enum('bpf_attach_type')
+    def get_attach_type_map(self):
+        if not self.attach_types:
+          names = self.get_enum('bpf_attach_type')
+          self.attach_types = self.make_enum_map(names, 'BPF_')
+        return self.attach_types
+
+    def get_cgroup_attach_type_map(self):
+        if not self.attach_types:
+            self.get_attach_type_map()
+        return {name: text for name, text in self.attach_types.items()
+            if name.startswith('BPF_CGROUP')}
 
 class ManPageExtractor(FileExtractor):
     """
@@ -495,21 +500,12 @@  def main():
     """)
     args = argParser.parse_args()
 
-    # Map types (enum)
-
     bpf_info = BpfHeaderExtractor()
-    ref = bpf_info.get_map_types()
-
-    map_info = MapFileExtractor()
-    source_map_items = map_info.get_map_types()
-    map_types_enum = set(source_map_items.keys())
-
-    verify(ref, map_types_enum,
-            f'Comparing BPF header (enum bpf_map_type) and {MapFileExtractor.filename} (map_type_name):')
 
     # Map types (names)
 
-    source_map_types = set(source_map_items.values())
+    map_info = MapFileExtractor()
+    source_map_types = set(bpf_info.get_map_type_map().values())
     source_map_types.discard('unspec')
 
     help_map_types = map_info.get_map_help()
@@ -525,34 +521,18 @@  def main():
     bashcomp_map_types = bashcomp_info.get_map_types()
 
     verify(source_map_types, help_map_types,
-            f'Comparing {MapFileExtractor.filename} (map_type_name) and {MapFileExtractor.filename} (do_help() TYPE):')
+            f'Comparing {BpfHeaderExtractor.filename} (bpf_map_type) and {MapFileExtractor.filename} (do_help() TYPE):')
     verify(source_map_types, man_map_types,
-            f'Comparing {MapFileExtractor.filename} (map_type_name) and {ManMapExtractor.filename} (TYPE):')
+            f'Comparing {BpfHeaderExtractor.filename} (bpf_map_type) and {ManMapExtractor.filename} (TYPE):')
     verify(help_map_options, man_map_options,
             f'Comparing {MapFileExtractor.filename} (do_help() OPTIONS) and {ManMapExtractor.filename} (OPTIONS):')
     verify(source_map_types, bashcomp_map_types,
-            f'Comparing {MapFileExtractor.filename} (map_type_name) and {BashcompExtractor.filename} (BPFTOOL_MAP_CREATE_TYPES):')
+            f'Comparing {BpfHeaderExtractor.filename} (bpf_map_type) and {BashcompExtractor.filename} (BPFTOOL_MAP_CREATE_TYPES):')
 
     # Program types (enum)
 
-    ref = bpf_info.get_prog_types()
-
     prog_info = ProgFileExtractor()
-    prog_types = set(prog_info.get_prog_types().keys())
-
-    verify(ref, prog_types,
-            f'Comparing BPF header (enum bpf_prog_type) and {ProgFileExtractor.filename} (prog_type_name):')
-
-    # Attach types (enum)
-
-    ref = bpf_info.get_attach_types()
-    bpf_info.close()
-
-    common_info = CommonFileExtractor()
-    attach_types = common_info.get_attach_types()
-
-    verify(ref, attach_types,
-            f'Comparing BPF header (enum bpf_attach_type) and {CommonFileExtractor.filename} (attach_type_name):')
+    prog_types = bpf_info.get_prog_types()
 
     # Attach types (names)
 
@@ -571,18 +551,17 @@  def main():
     bashcomp_prog_attach_types = bashcomp_info.get_prog_attach_types()
 
     verify(source_prog_attach_types, help_prog_attach_types,
-            f'Comparing {ProgFileExtractor.filename} (attach_type_strings) and {ProgFileExtractor.filename} (do_help() ATTACH_TYPE):')
+            f'Comparing {ProgFileExtractor.filename} (bpf_attach_type) and {ProgFileExtractor.filename} (do_help() ATTACH_TYPE):')
     verify(source_prog_attach_types, man_prog_attach_types,
-            f'Comparing {ProgFileExtractor.filename} (attach_type_strings) and {ManProgExtractor.filename} (ATTACH_TYPE):')
+            f'Comparing {ProgFileExtractor.filename} (bpf_attach_type) and {ManProgExtractor.filename} (ATTACH_TYPE):')
     verify(help_prog_options, man_prog_options,
             f'Comparing {ProgFileExtractor.filename} (do_help() OPTIONS) and {ManProgExtractor.filename} (OPTIONS):')
     verify(source_prog_attach_types, bashcomp_prog_attach_types,
-            f'Comparing {ProgFileExtractor.filename} (attach_type_strings) and {BashcompExtractor.filename} (BPFTOOL_PROG_ATTACH_TYPES):')
+            f'Comparing {ProgFileExtractor.filename} (bpf_attach_type) and {BashcompExtractor.filename} (BPFTOOL_PROG_ATTACH_TYPES):')
 
     # Cgroup attach types
-
-    source_cgroup_attach_types = set(common_info.get_cgroup_attach_types().values())
-    common_info.close()
+    source_cgroup_attach_types = set(bpf_info.get_cgroup_attach_type_map().values())
+    bpf_info.close()
 
     cgroup_info = CgroupFileExtractor()
     help_cgroup_attach_types = cgroup_info.get_prog_attach_help()
@@ -598,13 +577,13 @@  def main():
     bashcomp_info.close()
 
     verify(source_cgroup_attach_types, help_cgroup_attach_types,
-            f'Comparing {CommonFileExtractor.filename} (attach_type_strings) and {CgroupFileExtractor.filename} (do_help() ATTACH_TYPE):')
+            f'Comparing {BpfHeaderExtractor.filename} (bpf_attach_type) and {CgroupFileExtractor.filename} (do_help() ATTACH_TYPE):')
     verify(source_cgroup_attach_types, man_cgroup_attach_types,
-            f'Comparing {CommonFileExtractor.filename} (attach_type_strings) and {ManCgroupExtractor.filename} (ATTACH_TYPE):')
+            f'Comparing {BpfHeaderExtractor.filename} (bpf_attach_type) and {ManCgroupExtractor.filename} (ATTACH_TYPE):')
     verify(help_cgroup_options, man_cgroup_options,
             f'Comparing {CgroupFileExtractor.filename} (do_help() OPTIONS) and {ManCgroupExtractor.filename} (OPTIONS):')
     verify(source_cgroup_attach_types, bashcomp_cgroup_attach_types,
-            f'Comparing {CommonFileExtractor.filename} (attach_type_strings) and {BashcompExtractor.filename} (BPFTOOL_CGROUP_ATTACH_TYPES):')
+            f'Comparing {BpfHeaderExtractor.filename} (bpf_attach_type) and {BashcompExtractor.filename} (BPFTOOL_CGROUP_ATTACH_TYPES):')
 
     # Options for remaining commands