[PATCH 1/2] selftests/net: suppress clang's "variable-sized type not at the end" warning

[PATCH 1/2] selftests/net: suppress clang's "variable-sized type not at the end" warning

[John Hubbard]

When building with clang, via:

    make LLVM=1 -C tools/testing/selftest

...clang warns that "a variable sized type not at the end of a struct or
class is a GNU extension".

These cases are not easily changed, because they involve structs that
are part of the API. Fortunately, however, the tests seem to be doing
just fine (specifically, neither affected test runs any differently with
gcc vs. clang builds, on my test system) regardless of the warning. So,
all the warning is doing is preventing a clean build of selftests/net.

Fix this by suppressing this particular clang warning for the
selftests/net suite.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/net/Makefile | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 7b6918d5f4af..956481174783 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -6,6 +6,10 @@ CFLAGS += -I../../../../usr/include/ $(KHDR_INCLUDES)
 # Additional include paths needed by kselftest.h
 CFLAGS += -I../
 
+ifneq ($(LLVM),)
+    CFLAGS += -Wno-gnu-variable-sized-type-not-at-end
+endif
+
 TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh \
 	      rtnetlink.sh xfrm_policy.sh test_blackhole_dev.sh
 TEST_PROGS += fib_tests.sh fib-onlink-tests.sh pmtu.sh udpgso.sh ip_defrag.sh

base-commit: f462ae0edd3703edd6f22fe41d336369c38b884b
prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
-- 
2.45.0

[PATCH 2/2] selftests/net: fix uninitialized variables

[John Hubbard]

When building with clang, via:

    make LLVM=1 -C tools/testing/selftest

...clang warns about three variables that are not initialized in all
cases:

1) The opt_ipproto_off variable is used uninitialized if "testname" is
not "ip". This seems like an actual bug.

2) The addr_len is used uninitialized, but only in the assert case,
   which bails out, so this is harmless.

3) The family variable in add_listener() is only used uninitialized in
   the error case (neither IPv4 nor IPv6 is specified), so it's also
   harmless.

Fix by initializing each variable.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/net/gro.c                 | 3 ++-
 tools/testing/selftests/net/ip_local_port_range.c | 2 +-
 tools/testing/selftests/net/mptcp/pm_nl_ctl.c     | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/gro.c b/tools/testing/selftests/net/gro.c
index 353e1e867fbb..0eb61edaad83 100644
--- a/tools/testing/selftests/net/gro.c
+++ b/tools/testing/selftests/net/gro.c
@@ -110,7 +110,8 @@ static void setup_sock_filter(int fd)
 	const int dport_off = tcp_offset + offsetof(struct tcphdr, dest);
 	const int ethproto_off = offsetof(struct ethhdr, h_proto);
 	int optlen = 0;
-	int ipproto_off, opt_ipproto_off;
+	int ipproto_off;
+	int opt_ipproto_off = 0;
 	int next_off;
 
 	if (proto == PF_INET)
diff --git a/tools/testing/selftests/net/ip_local_port_range.c b/tools/testing/selftests/net/ip_local_port_range.c
index 193b82745fd8..29451d2244b7 100644
--- a/tools/testing/selftests/net/ip_local_port_range.c
+++ b/tools/testing/selftests/net/ip_local_port_range.c
@@ -359,7 +359,7 @@ TEST_F(ip_local_port_range, late_bind)
 		struct sockaddr_in v4;
 		struct sockaddr_in6 v6;
 	} addr;
-	socklen_t addr_len;
+	socklen_t addr_len = 0;
 	const int one = 1;
 	int fd, err;
 	__u32 range;
diff --git a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
index 7426a2cbd4a0..7ad5a59adff2 100644
--- a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
+++ b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
@@ -1276,7 +1276,7 @@ int add_listener(int argc, char *argv[])
 	struct sockaddr_storage addr;
 	struct sockaddr_in6 *a6;
 	struct sockaddr_in *a4;
-	u_int16_t family;
+	u_int16_t family = AF_UNSPEC;
 	int enable = 1;
 	int sock;
 	int err;
-- 
2.45.0

Re: [PATCH 2/2] selftests/net: fix uninitialized variables

[Matthieu Baerts]

Hi John,

On 06/05/2024 00:26, John Hubbard wrote:
> When building with clang, via:
> 
>     make LLVM=1 -C tools/testing/selftest
> 
> ...clang warns about three variables that are not initialized in all
> cases:
> 
> 1) The opt_ipproto_off variable is used uninitialized if "testname" is
> not "ip". This seems like an actual bug.
> 
> 2) The addr_len is used uninitialized, but only in the assert case,
>    which bails out, so this is harmless.
> 
> 3) The family variable in add_listener() is only used uninitialized in
>    the error case (neither IPv4 nor IPv6 is specified), so it's also
>    harmless.
> 
> Fix by initializing each variable.
> 
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  tools/testing/selftests/net/gro.c                 | 3 ++-
>  tools/testing/selftests/net/ip_local_port_range.c | 2 +-
>  tools/testing/selftests/net/mptcp/pm_nl_ctl.c     | 2 +-

Thank you for fixing these warnings!

The modification in the MPTCP selftest directory looks good to me:

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH 2/2] selftests/net: fix uninitialized variables

[Willem de Bruijn]

John Hubbard wrote:
> When building with clang, via:
> 
>     make LLVM=1 -C tools/testing/selftest
> 
> ...clang warns about three variables that are not initialized in all
> cases:
> 
> 1) The opt_ipproto_off variable is used uninitialized if "testname" is
> not "ip". This seems like an actual bug.
> 
> 2) The addr_len is used uninitialized, but only in the assert case,
>    which bails out, so this is harmless.
> 
> 3) The family variable in add_listener() is only used uninitialized in
>    the error case (neither IPv4 nor IPv6 is specified), so it's also
>    harmless.
> 
> Fix by initializing each variable.
> 
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  tools/testing/selftests/net/gro.c                 | 3 ++-
>  tools/testing/selftests/net/ip_local_port_range.c | 2 +-
>  tools/testing/selftests/net/mptcp/pm_nl_ctl.c     | 2 +-
>  3 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/gro.c b/tools/testing/selftests/net/gro.c
> index 353e1e867fbb..0eb61edaad83 100644
> --- a/tools/testing/selftests/net/gro.c
> +++ b/tools/testing/selftests/net/gro.c
> @@ -110,7 +110,8 @@ static void setup_sock_filter(int fd)
>  	const int dport_off = tcp_offset + offsetof(struct tcphdr, dest);
>  	const int ethproto_off = offsetof(struct ethhdr, h_proto);
>  	int optlen = 0;
> -	int ipproto_off, opt_ipproto_off;
> +	int ipproto_off;
> +	int opt_ipproto_off = 0;

This is only intended to be used in the case where the IP proto is not TCP:

                        BPF_STMT(BPF_LD  + BPF_B   + BPF_ABS, ipproto_off),
+                       BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_TCP, 2, 0),
+                       BPF_STMT(BPF_LD  + BPF_B   + BPF_ABS, opt_ipproto_off),
                        BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_TCP, 0, 5),

In that case the test tries again at a different offset that accounts
for optional IPv6 extension headers.

This is indeed buggy, in that it might accidentally accept packets
that should be dropped.

Initializing to 0 compares against against the first byte of the
Ethernet header. Which is an external argument to the test. So
safest is to initialize opt_ipproto_off to ipproto_off and just
repeat the previous check. Perhaps:

@@ -118,6 +118,7 @@ static void setup_sock_filter(int fd)
        else
                next_off = offsetof(struct ipv6hdr, nexthdr);
        ipproto_off = ETH_HLEN + next_off;
+       opt_ipproto_off = ipproto_off;  /* overridden later if may have exthdrs */

Re: [PATCH 2/2] selftests/net: fix uninitialized variables

[John Hubbard]

On 5/6/24 11:00 AM, Willem de Bruijn wrote:
> John Hubbard wrote:
...
>> diff --git a/tools/testing/selftests/net/gro.c b/tools/testing/selftests/net/gro.c
>> index 353e1e867fbb..0eb61edaad83 100644
>> --- a/tools/testing/selftests/net/gro.c
>> +++ b/tools/testing/selftests/net/gro.c
>> @@ -110,7 +110,8 @@ static void setup_sock_filter(int fd)
>>   	const int dport_off = tcp_offset + offsetof(struct tcphdr, dest);
>>   	const int ethproto_off = offsetof(struct ethhdr, h_proto);
>>   	int optlen = 0;
>> -	int ipproto_off, opt_ipproto_off;
>> +	int ipproto_off;
>> +	int opt_ipproto_off = 0;
> 
> This is only intended to be used in the case where the IP proto is not TCP:
> 
>                          BPF_STMT(BPF_LD  + BPF_B   + BPF_ABS, ipproto_off),
> +                       BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_TCP, 2, 0),
> +                       BPF_STMT(BPF_LD  + BPF_B   + BPF_ABS, opt_ipproto_off),
>                          BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_TCP, 0, 5),
> 
> In that case the test tries again at a different offset that accounts
> for optional IPv6 extension headers.
> 
> This is indeed buggy, in that it might accidentally accept packets
> that should be dropped.
> 
> Initializing to 0 compares against against the first byte of the
> Ethernet header. Which is an external argument to the test. So
> safest is to initialize opt_ipproto_off to ipproto_off and just
> repeat the previous check. Perhaps:
> 
> @@ -118,6 +118,7 @@ static void setup_sock_filter(int fd)
>          else
>                  next_off = offsetof(struct ipv6hdr, nexthdr);
>          ipproto_off = ETH_HLEN + next_off;
> +       opt_ipproto_off = ipproto_off;  /* overridden later if may have exthdrs */

OK, thanks for pointing out the right fix, I'll send a v2 that does that.

thanks,
-- 
John Hubbard
NVIDIA