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

[Yonghong Song <yhs@meta.com>]

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.