Re: BPF selftests and strict aliasing

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

On 1/28/24 4:25 AM, Jose E. Marchesi wrote:
>> On 1/27/24 11:59 AM, Jose E. Marchesi wrote:
>>> Hello.
>>> The following BPF selftests perform type-punning:
>>>
>>>     progs/bind4_prog.c
>>>     136 |         user_ip4 |= ((volatile __u16 *)&ctx->user_ip4)[0] << 0;
>>>
>>>     progs/bind6_prog.c
>>>     149 |                 user_ip6 |= ((volatile __u16 *)&ctx->user_ip6[i])[0] << 0;
>>>
>>>     progs/dynptr_fail.c
>>>     549 |         val = *(int *)&ptr;
>>>
>>>     progs/linked_list_fail.c
>>>     318 |         return *(int *)&f->head;
>>>     329 |         *(int *)&f->head = 0;
>>>     338 |         f = bpf_obj_new(typeof(*f));
>>>     341 |         return *(int *)&f->node2;
>>>     349 |         f = bpf_obj_new(typeof(*f));
>>>     352 |         *(int *)&f->node2 = 0;
>>>
>>>     progs/map_kptr_fail.c
>>>      34 |         *(u32 *)&v->unref_ptr = 0;
>>>
>>>     progs/syscall.c
>>>     172 |         attr->map_id = ((struct bpf_map *)&outer_array_map)->id;
>>>
>>>     progs/test_pkt_md_access.c
>>>      13 |                 TYPE tmp = *(volatile TYPE *)&skb->FIELD;               \
>>>
>>>     progs/test_sk_lookup.c
>>>      31 |         (((__u16 *)&(value))[LSE_INDEX((index), sizeof(value) / 2)])
>>>     427 |         val_u32 = *(__u32 *)&ctx->remote_port;
>>>
>>>     progs/timer_crash.c
>>>      38 |         *(void **)&value = (void *)0xdeadcaf3;
>>>
>>> This results in GCC warnings with -Wall but violating strict aliasing
>>> may also result in the compiler incorrectly optimizing something.
>>>
>>> There are some alternatives to deal with this:
>>>
>>> a) To rewrite the tests to conform to strict aliasing rules.
>>>
>>> b) To build these tests using -fno-strict-aliasing to make sure the
>>>      compiler will not rely on strict aliasing while optimizing.
>>>
>>> c) To add pragmas to these test files to avoid the warning:
>>>      _Pragma("GCC diagnostic ignored \"-Wstrict-aliasing\"")
>>>
>>> I think b) is probably the best way to go, because it will avoid the
>>> warnings, will void potential problems with optimizations triggered by
>>> strict aliasing, and will not require to rewrite the tests.
>> I tried with latest clang with -fstrict-aliasing:
>>
>> [~/work/bpf-next/tools/testing/selftests/bpf (master)]$ cat run.sh
>> clang -g -Wall -Werror -D__TARGET_ARCH_x86 -mlittle-endian
>> -I/home/yhs/work/bpf-next/tools/testing/selftests/bpf/tools/include \
>>    -I/home/yhs/work/bpf-next/tools/testing/selftests/bpf -I/home/yhs/work/bpf-next/tools/include/uapi
>>    -I/home/yhs/work/bpf-next/tools/testing/selftests/usr/include
>>    -idirafter /home/yhs/work/llvm-project/llvm/build.19/install/lib/clang/19/include
>>    -idirafter /usr/local/include -idirafter /usr/include   -Wno-compare-distinct-pointer-types
>>    -DENABLE_ATOMICS_TESTS -O2 -fstrict-aliasing --target=bpf -c progs/bind4_prog.c -mcpu=v3
>>    -o /home/yhs/work/bpf-next/tools/testing/selftests/bpf/bind4_prog.bpf.o
>> [~/work/bpf-next/tools/testing/selftests/bpf (master)]$ ./run.sh
>> [~/work/bpf-next/tools/testing/selftests/bpf (master)]$
>>
>> I does not have compilation failure. I am wondering why -fstrict-aliasing won't have warning in clang side
>> but have warning in gcc side.
>> Your suggestion 'b' seems okay or we could even add -fno-strict-aliasing into common compilation flags,
>> but I would like to understand more about -fstrict-aliasing difference between gcc and clang.
> It may be that GCC is just better than clang detecting and reporting
> strict aliasing rules violations.  Or it may be that clang doesn't
> assume strict aliasing when optimizing with the specified level.
>
> In any case:
>
>    progs/bind4_progs.c
>      type punning from __u32 to __u16.  These are not compatible types.
>
>    progs/bind6
>      type punning from __u32 to __u16.  These are not compatible types.
>
>    progs/dynptr_fail.c
>      type punning from struct bpf_dynptr to int.  These are not
>      compatible types.

I tried below example with the above prog/dynptr_fail.c case with gcc 11.4
for native x86 target and didn't trigger the warning. Maybe this requires
latest gcc? Or test C file is not sufficient enough to trigger the warning?

[~/tmp1]$ cat t.c
struct t {
   char a;
   short b;
   int c;
};
void init(struct t *);
long foo() {
   struct t dummy;
   init(&dummy);
   return *(int *)&dummy;
}
[~/tmp1]$ gcc -Wall -Werror -O2 -g -Wno-compare-distinct-pointer-types -c t.c
[~/tmp1]$ gcc --version
gcc (GCC) 11.4.1 20230605 (Red Hat 11.4.1-2)
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

>
>    progs/linked_list_fail.c
>      type punning from struct bpf_list_head to int.  These are not
>      compatible types.
>
>    progs/map_kptr_fail.c
>      type punning from struct prog_test_ref_kfunc to __u32.  These are
>      not compatible types.
>
>    ...
>
> And so on.
>
>>> Provided [1] gets applied, I can prepare a patch that adds the following
>>> to selftests/bpf/Makefile:
>>>
>>>     progs/bin4_prog.c-CFLAGS := -fno-strict-aliasing
>>>     progs/bind6_prog.c-CFLAGS := -fno-strict-aliasing
>>>     progs/dynptr_fail.cw-CFLAGS := -fno-strict-aliasing
>>>     progs/linked_list_fail.c-CFLAGS := -fno-strict-aliasing
>>>     progs/map_kptr_fail.c-CFLAGS := -fno-strict-aliasing
>>>     progs/syscall.c-CFLAGS := -fno-strict-aliasing
>>>     progs/test_pkt_md_access.c-CFLAGS := -fno-strict-aliasing
>>>     progs/test_sk_lookup.c-CFLAGS := -fno-strict-aliasing
>>>     progs/timer_crash.c-CFLAGS := -fno-strict-aliasing
>>>
>>> [1] https://lore.kernel.org/bpf/20240127100702.21549-1-jose.marchesi@oracle.com/T/#u
>>>