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

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

[Andrii Nakryiko]

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.

To avoid this problem, redefine offsetof() and container_of() in the
form that works with CO-RE relocations more reliably.

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>
---
 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
+#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.
-- 
2.34.1

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

[Yonghong Song]

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.

Otherwise, user may get impression that their __builtin_offset
is working but actually it is not. the same 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.

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

[Andrii Nakryiko]

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

>
> 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?

>
> > +#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.

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

[Yonghong Song]

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.

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Mon, 8 May 2023 23:55:02 -0700 you 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.
> 
> To avoid this problem, redefine offsetof() and container_of() in the
> form that works with CO-RE relocations more reliably.
> 
> [...]

Here is the summary with links:
  - [bpf-next] libbpf: fix offsetof() and container_of() to work with CO-RE
    https://git.kernel.org/bpf/bpf-next/c/bdeeed3498c7

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html