Re: [PATCH bpf] selftests/bpf: fix static assert compilation issue for test_cls_*.c

[Yonghong Song <yonghong.song@linux.dev>]

On 8/1/23 10:57 AM, Alan Maguire wrote:
> 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@oracle.com>
>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>>> ---
>>>    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

There are no CORE relocation here. vmlinux.h is not involved
and no explicit CORE relocation requests in the related C code.
Also I checked all offsetof usage in related C files
(test_cls_redirect.c, test_cls_redirect_dynptr.c) and the
offsetof only operates on uapi headers so CORE relocation
for them are not needed.

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