netdev - Re: [PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration

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 for Android: free password hash cracker in your pocket

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

Message-ID: <e0b21110-3a82-3477-a8e1-1f3e83cf18b3@fb.com>
Date:   Mon, 21 Oct 2019 18:19:23 +0000
From:   Yonghong Song <yhs@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Toke Høiland-Jørgensen <toke@...hat.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] libbpf: make LIBBPF_OPTS macro strictly a
 variable declaration



On 10/21/19 10:38 AM, Andrii Nakryiko wrote:
> On Mon, Oct 21, 2019 at 10:18 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>>
>> Andrii Nakryiko <andriin@...com> writes:
>>
>>> LIBBPF_OPTS is implemented as a mix of field declaration and memset
>>> + assignment. This makes it neither variable declaration nor purely
>>> statements, which is a problem, because you can't mix it with either
>>> other variable declarations nor other function statements, because C90
>>> compiler mode emits warning on mixing all that together.
>>>
>>> This patch changes LIBBPF_OPTS into a strictly declaration of variable
>>> and solves this problem, as can be seen in case of bpftool, which
>>> previously would emit compiler warning, if done this way (LIBBPF_OPTS as
>>> part of function variables declaration block).
>>>
>>> Signed-off-by: Andrii Nakryiko <andriin@...com>
>>> ---
> 
> [...]
> 
>>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>>> index 0fdf086beba7..bf105e9e866f 100644
>>> --- a/tools/lib/bpf/libbpf.h
>>> +++ b/tools/lib/bpf/libbpf.h
>>> @@ -77,12 +77,13 @@ struct bpf_object_open_attr {
>>>    * bytes, but that's the best way I've found and it seems to work in practice.
>>>    */
>>>   #define LIBBPF_OPTS(TYPE, NAME, ...)                                     \
>>> -     struct TYPE NAME;                                                   \
>>> -     memset(&NAME, 0, sizeof(struct TYPE));                              \
>>> -     NAME = (struct TYPE) {                                              \
>>> -             .sz = sizeof(struct TYPE),                                  \
>>> -             __VA_ARGS__                                                 \
>>> -     }
>>> +     struct TYPE NAME = ({                                               \
>>> +             memset(&NAME, 0, sizeof(struct TYPE));                      \
>>> +             (struct TYPE) {                                             \
>>> +                     .sz = sizeof(struct TYPE),                          \

This ({ statements; ...; value; }) is used by bcc rewriter as well.

>>
>> Wait, you can stick arbitrary code inside a variable initialisation
>> block like this? How does that work? Is everything before the (struct
>> type) just ignored (and is that a cast)?
> 
> Well, you definitely can still arbitrary code into a ({ }) expression
> block, that's not that surprising.
> The surprising bit that I discovered just recently was that stuff like
> this compiles and works correctly, try it:
> 
>          void *x = &x;
>          printf("%lx == %lx\n", x, &x);

'void *x' just takes the address of the 'x' in the current scope.
It may looks like a use before define. but it actually works.

LGTM.
Acked-by: Yonghong Song <yhs@...com>

> 
> So I'm using the fact that variable address is available inside
> variable initialization block.
> 
> Beyond that, it's just a fancy, but standard (struct bla){ ...
> initializer list ...} syntax (it's not a struct initializer syntax,
> mind you, it's a struct assignment from struct literal). Fancy for
> sure, but it works and solves problems I mentioned in commit
> description.
> 
>>
>> -Toke
>>