[bpf-next] selftests/bpf: Fix a few tests for GCC related warnings.

Commit Message

Cupertino Miranda May 8, 2024, 3:41 p.m. UTC

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.

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

-- 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 <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: Yonghong Song <[e-mail]>
Cc: Eduard Zingerman <[e-mail]>
---
 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(+)

Comments

Andrii Nakryiko May 8, 2024, 8:56 p.m. UTC | #1

On Wed, May 8, 2024 at 8:57 AM Cupertino Miranda
<[e-mail]> 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 <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: Yonghong Song <[e-mail]>
> Cc: Eduard Zingerman <[e-mail]>
> ---
>  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
>
>

Patch

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