On Tue, May 9, 2023 at 6:33 AM Yonghong Song <yhs@xxxxxxxx> 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@xxxxxxxxxxxxxx>
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> Ack with a nit below.
>
> Acked-by: Yonghong Song <yhs@xxxxxx>
>
> > ---
> > 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.
