Re: [PATCH bpf-next] libbpf: fix offsetof() and container_of() to work with CO-RE

[Yonghong Song <yhs@meta.com>]

On 5/9/23 7:59 AM, Andrii Nakryiko wrote:
> On Tue, May 9, 2023 at 6:33 AM Yonghong Song <yhs@meta.com> wrote:
>>
>>
>>
>> On 5/8/23 11:55 PM, Andrii Nakryiko wrote:
>>> It seems like __builtin_offset() doesn't preserve CO-RE field
>>> relocations properly. So if offsetof() macro is defined through
>>> __builtin_offset(), CO-RE-enabled BPF code using container_of() will be
>>> subtly and silently broken.
>>
>> This is true. See 63fe3fd393dc("libbpf: Do not use __builtin_offsetof
>> for offsetof"). At some point, we used __builtin_offset() and found
>> CO-RE relocation won't work, so the above commit switched back to
>> use ((unsigned long)&((TYPE *)0)->MEMBER).
>>
>>>
>>> To avoid this problem, redefine offsetof() and container_of() in the
>>> form that works with CO-RE relocations more reliably.
>>
>> I am okay with the change to forcefully define offsetof() and
>> container_of() since this is critical for correct CO-RE
>> relocations.
>>
>>>
>>> Fixes: 5fbc220862fc ("tools/libpf: Add offsetof/container_of macro in bpf_helpers.h")
>>> Reported-by: Lennart Poettering <lennart@poettering.net>
>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>>
>> Ack with a nit below.
>>
>> Acked-by: Yonghong Song <yhs@fb.com>
>>
>>> ---
>>>    tools/lib/bpf/bpf_helpers.h | 15 ++++++++++-----
>>>    1 file changed, 10 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
>>> index 929a3baca8ef..bbab9ad9dc5a 100644
>>> --- a/tools/lib/bpf/bpf_helpers.h
>>> +++ b/tools/lib/bpf/bpf_helpers.h
>>> @@ -77,16 +77,21 @@
>>>    /*
>>>     * Helper macros to manipulate data structures
>>>     */
>>> -#ifndef offsetof
>>> -#define offsetof(TYPE, MEMBER)       ((unsigned long)&((TYPE *)0)->MEMBER)
>>> -#endif
>>> -#ifndef container_of
>>> +
>>> +/* offsetof() definition that uses __builtin_offset() might not preserve field
>>> + * offset CO-RE relocation properly, so force-redefine offsetof() using
>>> + * old-school approach which works with CO-RE correctly
>>> + */
>>> +#undef offsetof
>>
>> I am not sure whether the above 'undef' is good or bad. In my opinion,
>> I would just remove the above 'undef'. If user defines
>> offset as __builtin_offset, the compiler will issue a warning
>> and user should remove that macro or undef them.
> 
> If we don't #undef, we are almost guaranteed to get a compilation
> warning. See [0], where I repro this problem based on Lennart's
> original code.
> 
>    [0] https://github.com/anakryiko/libbpf-bootstrap/commit/2bad3e7f48e4e4eea1a083620f21eba59aa75b1a
>

Okay, I see. They didn't use vmlinux.h...

>>
>> Otherwise, user may get impression that their __builtin_offset
>> is working but actually it is not. the same for container_of.
> 
> Can we just fix __builtin_offset() to generate/preserve field offset
> CO-RE relocation?

I checked with llvm17 source code. The __builtin_offset() is handled
in two places:
   - clang/lib/Sema/SemaExpr.cpp: func BuildBuiltinOffsetOf
   - clang/lib/CodeGen/CGExprScalar.cpp: func VisitOffsetOfExpr

As expected, the above two functions process __builtin_offset()
without any arch specific handling.

I guess we could try to add CO-RE relocation to them but not
sure whether upstream likes it or not.

But the same issue exists in earlier clang versions.
So I am okay with the current patch with
"#undef offsetof" and "#undef container_of" although
clang headers does not have macro for "container_of".

> 
>>
>>> +#define offsetof(type, member)       ((unsigned long)&((type *)0)->member)
>>> +
>>> +/* redefined container_of() to ensure we use the above offsetof() macro */
>>> +#undef container_of
>>>    #define container_of(ptr, type, member)                             \
>>>        ({                                                      \
>>>                void *__mptr = (void *)(ptr);                   \
>>>                ((type *)(__mptr - offsetof(type, member)));    \
>>>        })
>>> -#endif
>>>
>>>    /*
>>>     * Compiler (optimization) barrier.