On 5/9/23 7:59 AM, Andrii Nakryiko wrote:
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
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.
