netdev - Re: [PATCH bpf-next 07/10] bpftool: expose attach_type-to-string array to non-cgroup code

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <34110254-6384-153f-af39-d5f9f3a50acb@isovalent.com>
Date:   Fri, 24 Apr 2020 11:32:07 +0100
From:   Quentin Monnet <quentin@...valent.com>
To:     Andrii Nakryiko <andriin@...com>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, ast@...com, daniel@...earbox.net
Cc:     andrii.nakryiko@...il.com, kernel-team@...com
Subject: Re: [PATCH bpf-next 07/10] bpftool: expose attach_type-to-string
 array to non-cgroup code

2020-04-23 22:35 UTC-0700 ~ Andrii Nakryiko <andriin@...com>
> Move attach_type_strings into main.h for access in non-cgroup code.
> bpf_attach_type is used for non-cgroup attach types quite widely now. So also
> complete missing string translations for non-cgroup attach types.
> 
> Signed-off-by: Andrii Nakryiko <andriin@...com>
> ---
>  tools/bpf/bpftool/cgroup.c | 28 +++-------------------------
>  tools/bpf/bpftool/main.h   | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
> index 62c6a1d7cd18..d1fd9c9f2690 100644
> --- a/tools/bpf/bpftool/cgroup.c
> +++ b/tools/bpf/bpftool/cgroup.c
> @@ -31,35 +31,13 @@
>  
>  static unsigned int query_flags;
>  
> -static const char * const attach_type_strings[] = {
> -	[BPF_CGROUP_INET_INGRESS] = "ingress",
> -	[BPF_CGROUP_INET_EGRESS] = "egress",
> -	[BPF_CGROUP_INET_SOCK_CREATE] = "sock_create",
> -	[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_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",
> -	[__MAX_BPF_ATTACH_TYPE] = NULL,

So you removed the "[__MAX_BPF_ATTACH_TYPE] = NULL" from the new array,
if I understand correctly this is because all attach type enum members
are now in the new attach_type_name[] so we're safe by looping until we
reach __MAX_BPF_ATTACH_TYPE. Sounds good in theory but...

> -};
> -
>  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_type_strings[type] &&
> -		    is_prefix(str, attach_type_strings[type]))
> +		if (attach_type_name[type] &&
> +		    is_prefix(str, attach_type_name[type]))
>  			return type;
>  	}

... I'm concerned the "attach_type_name[type]" here could segfault if we
add a new attach type to the kernel, but don't report it immediately to
bpftool's array.

Is there any drawback with keeping the "[__MAX_BPF_ATTACH_TYPE] = NULL"?
Or change here to loop on ARRAY_SIZE(), as you do in your own patch for
link?