Message-ID: <5404b784-2173-210d-6319-fa5f0156701e@isovalent.com>
Date:   Fri, 24 Apr 2020 18:08:16 +0100
From:   Quentin Monnet <quentin@...valent.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next 07/10] bpftool: expose attach_type-to-string
 array to non-cgroup code

2020-04-24 09:27 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@...il.com>
> On Fri, Apr 24, 2020 at 3:32 AM Quentin Monnet <quentin@...valent.com> wrote:
>>
>> 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...
>>
> 
> Well, NULL is default value, so having [__MAX_BPF_ATTACH_TYPE] = NULL
> just increases ARRAY_SIZE(attach_type_names) by one. Which is
> generally not needed, because we do proper < ARRAY_SIZE() checks
> everywhere... except for one place. show_bpf_prog in cgroup.c looks up
> name directly and can pass NULL into jsonw_string_field which will
> crash.
> 
> I can fix that by setting [__MAX_BPF_ATTACH_TYPE] to "unknown" or
> adding extra check in show_bpf_prog() code? Any preferences?

Maybe add the extra check, so we remove this [__MAX_BPF_ATTACH_TYPE]
indeed. It will be more consistent with the array with program names,
and as you say, all other places loop on ARRAY_SIZE() just fine.

Maybe we could print the integer value for the type if we don't know the
name? Not sure if this is good for JSON though.

> 
>>> -};
>>> -
>>>  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.
> 
> I don't think so. Here we'll iterate over all possible bpf_attach_type
> (as far as our copy of UAPI header is concerned, of course). If some
> of the values don't have entries in attach_type_name array, we'll get
> back NULL (same as with explicit [__MAX_BPF_ATTACH_TYPE] = NULL, btw),
> which will get handled properly in the loop. And caller will get back
> __MAX_BPF_ATTACH_TYPE as bpf_attach_type value. So unless I'm still
> missing something, it seems to be working exactly the same as before?
> 
>>
>> 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?
> 
> ARRAY_SIZE() == __MAX_BPF_ATTACH_TYPE, isn't it? Previously ARRAY_SIZE
> was (__MAX_BPF_ATTACH_TYPE + 1), but I don't think it's necessary?

ARRAY_SIZE() /should/ be equal to __MAX_BPF_ATTACH_TYPE, the concern is
only if new attach types get added to UAPI header and we forget to add
them to the array. In that case, the assumption is not longer valid and
we risk reading out of the array in parse_attach_type(). That was not
the case before, because we knew that the array was always big enough.
There was no risk to read beyond index __MAX_BPF_ATTACH_TYPE, there is
one now to read beyond index BPF_LSM_MAC when new types are added. Or am
I the one missing something?

> 
> The only difference is show_bpf_prog() which now is going to do out of
> array reads, while previously it would get NULL. But both cases are
> bad and needs fixing.
> 

Right, nice catch, this needs a fix.

Powered by blists - more mailing lists