Re: [ovs-dev] [PATCH v2] selftests/net: fix uninitialized variables
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". Willem de Bruijn pointed out that this is an actual bug, and > suggested the fix that I'm using here (thanks!). > > 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. > > Cc: Willem de Bruijn > Signed-off-by: John Hubbard Reviewed-by: Willem de Bruijn Thanks! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] selftests/net: fix uninitialized variables
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 > --- > 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 */ ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] net: openvswitch: datapath: fix data type in queue_gso_packets
On Sat, Nov 25, 2017 at 2:14 PM, Gustavo A. R. Silva <garsi...@embeddedor.com> wrote: > gso_type is being used in binary AND operations together with SKB_GSO_UDP. > The issue is that variable gso_type is of type unsigned short and > SKB_GSO_UDP expands to more than 16 bits: > > SKB_GSO_UDP = 1 << 16 > > this makes any binary AND operation between gso_type and SKB_GSO_UDP to > be always zero, hence making some code unreachable and likely causing > undesired behavior. > > Fix this by changing the data type of variable gso_type to unsigned int. > > Addresses-Coverity-ID: 1462223 > Fixes: 0c19f846d582 ("net: accept UFO datagrams from tuntap and packet") > Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com> Acked-by: Willem de Bruijn <will...@google.com> Good catch, thanks! The issue here is that I brought back SKB_GSO_UDP at the end of the list at 1 << 16 to avoid renaming of all that used to follow, while it used to be defined as 1 << 1. The skb_shinfo(skb)->gso_type field itself has been expanded as of v4.12 in commit 7f564528a480 ("skbuff: Extend gso_type to unsigned int."). A quick scan shows a few nic drivers that also still cast to unsigned short: bnxt, atl1c and qede. Since UFO hardware offload no longer exists, this is safe wrt this patch. And it is fine for older kernels as no driver supported the previous entry at 1 << 16, SKB_GSO_ESP. But it is fragile wrt follow-on offloads. Probably a net-next patch. The only other likely issue I spotted with the longer gso_type is in trace events. net_dev_start_xmit and net_dev_rx_verbose_template both export as u16. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev