| Message ID | 20240505222639.70317-2-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] selftests/net: suppress clang's "variable-sized type not at the end" warning | expand |
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
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 */
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,
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;
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(-)