On Wed, May 8, 2024 at 8:57 AM Cupertino Miranda
<cupertino.miranda@xxxxxxxxxx> wrote:
>
> This patch disables a few warnings to allow selftests to compile for
> GCC.
>
> -- progs/cpumask_failure.c --
>
> progs/bpf_misc.h:136:22: error: ‘cpumask’ is used uninitialized
> [-Werror=uninitialized]
> 136 | #define __sink(expr) asm volatile("" : "+g"(expr))
> | ^~~
> progs/cpumask_failure.c:68:9: note: in expansion of macro ‘__sink’
> 68 | __sink(cpumask);
>
> The macro __sink(cpumask) with the '+' contraint modifier forces the
> the compieler to expect a read and write from cpumask. GCC detects
> that cpumask is never initialized and reports an error.
this one seems like a legit unused variable that should just be removed.
>
> -- progs/dynptr_fail.c --
>
> progs/dynptr_fail.c:1444:9: error: ‘ptr1’ may be used uninitialized
> [-Werror=maybe-uninitialized]
> 1444 | bpf_dynptr_clone(&ptr1, &ptr2);
>
> Many of the tests in the file are related to the detection of
> uninitialized pointers by the verifier. GCC is able to detect possible
> uninititialized values, and reports this as an error.
>
We can do `struct bpf_dynptr ptr1 = {};` to satisfy compiler without
affecting what the test is actually testing.
Or at the very least, we should add those pragmas only around few
affected functions, not for the entire file.
I haven't looked at other cases, but let's take a step back a bit and
see if existing code makes sense and whether GCC warnings are real and
we should do something about them.
pw-bot: cr
> -- progs/test_tunnel_kern.c --
>
> progs/test_tunnel_kern.c:590:9: error: array subscript 1 is outside
> array bounds of ‘struct geneve_opt[1]’ [-Werror=array-bounds=]
> 590 | *(int *) &gopt.opt_data = bpf_htonl(0xdeadbeef);
> | ^~~~~~~~~~~~~~~~~~~~~~~
> progs/test_tunnel_kern.c:575:27: note: at offset 4 into object ‘gopt’ of
> size 4
> 575 | struct geneve_opt gopt;
>
> This tests accesses beyond the defined data for the struct geneve_opt
> which contains as last field "u8 opt_data[0]" which clearly does not get
> reserved space (in stack) in the function header. This pattern is
> repeated in ip6geneve_set_tunnel and geneve_set_tunnel functions.
> GCC is able to see this and emits a warning.
>
> -- progs/jeq_infer_not_null_fail.c --
>
> progs/jeq_infer_not_null_fail.c:21:40: error: array subscript ‘struct
> bpf_map[0]’ is partly outside array bounds of ‘struct <anonymous>[1]’
> [-Werror=array-bounds=]
> 21 | struct bpf_map *inner_map = map->inner_map_meta;
> | ^~
> progs/jeq_infer_not_null_fail.c:14:3: note: object ‘m_hash’ of size 32
> 14 | } m_hash SEC(".maps");
>
> This example defines m_hash in the context of the compilation unit and
> casts it to struct bpf_map which is much smaller than the size of struct
> bpf_map. It errors out in GCC when it attempts to access an element that
> would be defined in struct bpf_map outsize of the defined limits for
> m_hash.
>
> This change was tested in bpf-next master selftests without any
> regressions.
>
> Signed-off-by: Cupertino Miranda <cupertino.miranda@xxxxxxxxxx>
> Cc: jose.marchesi@xxxxxxxxxx
> Cc: david.faust@xxxxxxxxxx
> Cc: Yonghong Song <yonghong.song@xxxxxxxxx>
> Cc: Eduard Zingerman <eddyz87@xxxxxxxxx>
> ---
> tools/testing/selftests/bpf/progs/cpumask_failure.c | 4 ++++
> tools/testing/selftests/bpf/progs/dynptr_fail.c | 4 ++++
> tools/testing/selftests/bpf/progs/jeq_infer_not_null_fail.c | 4 ++++
> tools/testing/selftests/bpf/progs/test_tunnel_kern.c | 4 ++++
> 4 files changed, 16 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/cpumask_failure.c b/tools/testing/selftests/bpf/progs/cpumask_failure.c
> index a9bf6ea336cf..56a6adb6cbbb 100644
> --- a/tools/testing/selftests/bpf/progs/cpumask_failure.c
> +++ b/tools/testing/selftests/bpf/progs/cpumask_failure.c
> @@ -8,6 +8,10 @@
>
> #include "cpumask_common.h"
>
> +#ifndef __clang__
> +#pragma GCC diagnostic ignored "-Wuninitialized"
> +#endif
> +
> char _license[] SEC("license") = "GPL";
>
> /* Prototype for all of the program trace events below:
> diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> index 7ce7e827d5f0..9ceff0b5d143 100644
> --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
> +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> @@ -10,6 +10,10 @@
> #include "bpf_misc.h"
> #include "bpf_kfuncs.h"
>
> +#ifndef __clang__
> +#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
> +#endif
> +
> char _license[] SEC("license") = "GPL";
>
> struct test_info {
> diff --git a/tools/testing/selftests/bpf/progs/jeq_infer_not_null_fail.c b/tools/testing/selftests/bpf/progs/jeq_infer_not_null_fail.c
> index f46965053acb..4d619bea9c75 100644
> --- a/tools/testing/selftests/bpf/progs/jeq_infer_not_null_fail.c
> +++ b/tools/testing/selftests/bpf/progs/jeq_infer_not_null_fail.c
> @@ -4,6 +4,10 @@
> #include <bpf/bpf_helpers.h>
> #include "bpf_misc.h"
>
> +#ifndef __clang__
> +#pragma GCC diagnostic ignored "-Warray-bounds"
> +#endif
> +
> char _license[] SEC("license") = "GPL";
>
> struct {
> diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> index 3e436e6f7312..806c16809a4c 100644
> --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> @@ -13,6 +13,10 @@
> #include "bpf_kfuncs.h"
> #include "bpf_tracing_net.h"
>
> +#ifndef __clang__
> +#pragma GCC diagnostic ignored "-Warray-bounds"
> +#endif
> +
> #define log_err(__ret) bpf_printk("ERROR line:%d ret:%d\n", __LINE__, __ret)
>
> #define VXLAN_UDP_PORT 4789
> --
> 2.39.2
>
>
