On 01/08/2023 18:09, Yonghong Song wrote:
>
>
> On 8/1/23 3:29 AM, Alan Maguire wrote:
>> commit bdeeed3498c7 ("libbpf: fix offsetof() and container_of() to
>> work with CO-RE")
>>
>> ...was backported to stable trees such as 5.15. The problem is that
>> with older
>> LLVM/clang (14/15) - which is often used for older kernels - we see
>> compilation
>> failures in BPF selftests now:
>>
>> In file included from progs/test_cls_redirect_subprogs.c:2:
>> progs/test_cls_redirect.c:90:2: error: static assertion expression is
>> not an integral constant expression
>> sizeof(flow_ports_t) !=
>> ^~~~~~~~~~~~~~~~~~~~~~~
>> progs/test_cls_redirect.c:91:3: note: cast that performs the
>> conversions of a reinterpret_cast is not allowed in a constant expression
>> offsetofend(struct bpf_sock_tuple, ipv4.dport) -
>> ^
>> progs/test_cls_redirect.c:32:3: note: expanded from macro 'offsetofend'
>> (offsetof(TYPE, MEMBER) + sizeof((((TYPE *)0)->MEMBER)))
>> ^
>> tools/testing/selftests/bpf/tools/include/bpf/bpf_helpers.h:86:33:
>> note: expanded from macro 'offsetof'
>> ^
>> In file included from progs/test_cls_redirect_subprogs.c:2:
>> progs/test_cls_redirect.c:95:2: error: static assertion expression is
>> not an integral constant expression
>> sizeof(flow_ports_t) !=
>> ^~~~~~~~~~~~~~~~~~~~~~~
>> progs/test_cls_redirect.c:96:3: note: cast that performs the
>> conversions of a reinterpret_cast is not allowed in a constant expression
>> offsetofend(struct bpf_sock_tuple, ipv6.dport) -
>> ^
>> progs/test_cls_redirect.c:32:3: note: expanded from macro 'offsetofend'
>> (offsetof(TYPE, MEMBER) + sizeof((((TYPE *)0)->MEMBER)))
>> ^
>> tools/testing/selftests/bpf/tools/include/bpf/bpf_helpers.h:86:33:
>> note: expanded from macro 'offsetof'
>> ^
>> 2 errors generated.
>> make: *** [Makefile:594:
>> tools/testing/selftests/bpf/test_cls_redirect_subprogs.bpf.o] Error 1
>>
>> The problem is the new offsetof() does not play nice with static asserts.
>> Given that the context is a static assert (and CO-RE relocation is not
>> needed at compile time), offsetof() usage can be replaced by
>> __builtin_offsetof(), and all is well. Define __builtin_offsetofend()
>> to be used in static asserts also, since offsetofend() is also defined in
>> bpf_util.h and is used in userspace progs, so redefining offsetofend()
>> in test_cls_redirect.h won't work.
>>
>> Fixes: bdeeed3498c7 ("libbpf: fix offsetof() and container_of() to
>> work with CO-RE")
>> Reported-by: Colm Harrington <colm.harrington@xxxxxxxxxx>
>> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
>> ---
>> tools/testing/selftests/bpf/progs/test_cls_redirect.c | 11 ++++-------
>> tools/testing/selftests/bpf/progs/test_cls_redirect.h | 3 +++
>> .../selftests/bpf/progs/test_cls_redirect_dynptr.c | 11 ++++-------
>> 3 files changed, 11 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/progs/test_cls_redirect.c
>> b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
>> index 66b304982245..e68e0544827c 100644
>> --- a/tools/testing/selftests/bpf/progs/test_cls_redirect.c
>> +++ b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
>> @@ -28,9 +28,6 @@
>> #define INLINING __always_inline
>> #endif
>> -#define offsetofend(TYPE, MEMBER) \
>> - (offsetof(TYPE, MEMBER) + sizeof((((TYPE *)0)->MEMBER)))
>> -
>> #define IP_OFFSET_MASK (0x1FFF)
>> #define IP_MF (0x2000)
>> @@ -88,13 +85,13 @@ typedef struct {
>> _Static_assert(
>> sizeof(flow_ports_t) !=
>> - offsetofend(struct bpf_sock_tuple, ipv4.dport) -
>> - offsetof(struct bpf_sock_tuple, ipv4.sport) - 1,
>> + __builtin_offsetofend(struct bpf_sock_tuple, ipv4.dport) -
>> + __builtin_offsetof(struct bpf_sock_tuple, ipv4.sport) - 1,
>> "flow_ports_t must match sport and dport in struct
>> bpf_sock_tuple");
>> _Static_assert(
>> sizeof(flow_ports_t) !=
>> - offsetofend(struct bpf_sock_tuple, ipv6.dport) -
>> - offsetof(struct bpf_sock_tuple, ipv6.sport) - 1,
>> + __builtin_offsetofend(struct bpf_sock_tuple, ipv6.dport) -
>> + __builtin_offsetof(struct bpf_sock_tuple, ipv6.sport) - 1,
>> "flow_ports_t must match sport and dport in struct
>> bpf_sock_tuple");
>> typedef int ret_t;
>> diff --git a/tools/testing/selftests/bpf/progs/test_cls_redirect.h
>> b/tools/testing/selftests/bpf/progs/test_cls_redirect.h
>> index 76eab0aacba0..1de0b727a3f6 100644
>> --- a/tools/testing/selftests/bpf/progs/test_cls_redirect.h
>> +++ b/tools/testing/selftests/bpf/progs/test_cls_redirect.h
>> @@ -12,6 +12,9 @@
>> #include <linux/ipv6.h>
>> #include <linux/udp.h>
>> +#define __builtin_offsetofend(TYPE, MEMBER) \
>> + (__builtin_offsetof(TYPE, MEMBER) + sizeof((((TYPE *)0)->MEMBER)))
>
> I think this can be simplified to undef and re-define offsetof like below:
>
> #ifdef offsetof
> #undef offsetof
> #define offsetof(type, member) __builtin_offsetof(type, member)
> #endif
>
> Then other changes in this patch become unnecessary.
>
> You can add comments for the above code to explain
> why you want to redefine 'offsetof'.
>
That's one way to solve it alright, but then other instances of
offsetof() in the BPF code (that are not part of static asserts) aren't
CO-RE-safe. Probably not a big concern for a test case that is usually
run against the same kernel, but it's perhaps worth retaining the
distinction between compile-time and run-time needs?
Alan
>
>> +
>> struct gre_base_hdr {
>> uint16_t flags;
>> uint16_t protocol;
>> diff --git
>> a/tools/testing/selftests/bpf/progs/test_cls_redirect_dynptr.c
>> b/tools/testing/selftests/bpf/progs/test_cls_redirect_dynptr.c
>> index f41c81212ee9..463b0513f871 100644
>> --- a/tools/testing/selftests/bpf/progs/test_cls_redirect_dynptr.c
>> +++ b/tools/testing/selftests/bpf/progs/test_cls_redirect_dynptr.c
>> @@ -23,9 +23,6 @@
>> #include "test_cls_redirect.h"
>> #include "bpf_kfuncs.h"
>> -#define offsetofend(TYPE, MEMBER) \
>> - (offsetof(TYPE, MEMBER) + sizeof((((TYPE *)0)->MEMBER)))
>> -
>> #define IP_OFFSET_MASK (0x1FFF)
>> #define IP_MF (0x2000)
>> @@ -83,13 +80,13 @@ typedef struct {
>> _Static_assert(
>> sizeof(flow_ports_t) !=
>> - offsetofend(struct bpf_sock_tuple, ipv4.dport) -
>> - offsetof(struct bpf_sock_tuple, ipv4.sport) - 1,
>> + __builtin_offsetofend(struct bpf_sock_tuple, ipv4.dport) -
>> + __builtin_offsetof(struct bpf_sock_tuple, ipv4.sport) - 1,
>> "flow_ports_t must match sport and dport in struct
>> bpf_sock_tuple");
>> _Static_assert(
>> sizeof(flow_ports_t) !=
>> - offsetofend(struct bpf_sock_tuple, ipv6.dport) -
>> - offsetof(struct bpf_sock_tuple, ipv6.sport) - 1,
>> + __builtin_offsetofend(struct bpf_sock_tuple, ipv6.dport) -
>> + __builtin_offsetof(struct bpf_sock_tuple, ipv6.sport) - 1,
>> "flow_ports_t must match sport and dport in struct
>> bpf_sock_tuple");
>> struct iphdr_info {
