[ovs-dev] [PATCH] conntrack: Add a test for IPv4 UDP zero checksum

Tue Feb 16 22:54:48 UTC 2021

Hi Aaron,

Should we also consider the case where udp checksum is 0xffff?
I saw in netdev_tnl_calc_udp_csum, we set to 0xffff when a packet has
udp checksum = 0.

Thanks
William

On Wed, Feb 10, 2021 at 12:15 PM Flavio Leitner <fbl at sysclose.org> wrote:
>
> On Wed, Feb 10, 2021 at 02:45:06PM -0500, Aaron Conole wrote:
> > Flavio Leitner <fbl at sysclose.org> writes:
> >
> > > On Wed, Feb 10, 2021 at 11:49:33AM -0500, Aaron Conole wrote:
> > >> Recently, during some conntrack testing a bug was uncovered in a DPDK
> > >> PMD, which doesn't support an IPv4 packet with a zero checksum value.
> > >> In order to show that the connection tracking code in userspace
> > >> supports IPv4 UDP with a zero checksum, add a test case to enforce
> > >> this behavior.
> > >>
> > >> Reported-at: http://mails.dpdk.org/archives/dev/2021-January/198528.html
> > >> Reported-by: Paolo Valerio <pvalerio at redhat.com>
> > >> Signed-off-by: Aaron Conole <aconole at redhat.com>
> > >> ---
> > >>  tests/system-traffic.at | 43 +++++++++++++++++++++++++++++++++++++++++
> > >>  1 file changed, 43 insertions(+)
> > >>
> > >> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> > >> index fb5b9a36d2..4971ccc966 100644
> > >> --- a/tests/system-traffic.at
> > >> +++ b/tests/system-traffic.at
> > >> @@ -5927,6 +5927,48 @@ ovs-appctl dpif/dump-flows br0
> > >>  OVS_TRAFFIC_VSWITCHD_STOP
> > >>  AT_CLEANUP
> > >>
> > >> +
> > >> +AT_SETUP([conntrack - IPv4 UDP zero checksum])
> > >> +dnl This tracks sending zero checksum packets for udp over ipv4
> > >> +CHECK_CONNTRACK()
> > >> +OVS_TRAFFIC_VSWITCHD_START()
> > >> +OVS_CHECK_CT_CLEAR()
> > >> +
> > >> +ADD_NAMESPACES(at_ns0, at_ns1)
> > >> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01")
> > >> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02")
> > >> +dnl setup ct flows
> > >> +AT_DATA([flows.txt], [dnl
> > >> +table=0,priority=10  ip,udp,ct_state=-trk action=ct(zone=1,table=1)
> > >> +table=0,priority=0   action=drop
> > >> +table=1,priority=10  ct_state=-est+trk+new,ip,ct_zone=1,in_port=1 action=ct(commit,table=2)
> > >> +table=1,priority=10  ct_state=+est-new+trk,ct_zone=1,in_port=1 action=resubmit(,2)
> > >> +table=1,priority=0   action=drop
> > >> +table=2,priority=10  ct_state=+trk+new,in_port=1 action=2
> > >> +table=2,priority=10  ct_state=+trk+est action=2
> > >> +])
> > >> +
> > >> +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> > >> +
> > >> +# sending udp pkt
> > >> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 f0 00 00 01
> > >> 01 02 f0 00 00 01 01 01 08 00 45 00 00 28 00 01 00 00 40 11 64 c0 0a
> > >> 01 01 01 0a 01 01 02 04 d2 04 d2 00 14 00 00 aa aa aa aa aa aa aa aa
> > >> aa aa aa aa > /dev/null])
> > >
> > > That hex string translates to this packet which doesn't use cksum:
> > > 12:57:40.065353 IP (tos 0x0, ttl 64, id 1, offset 0, flags [none],
> > > proto UDP (17), length 40)
> > >     10.1.1.1.search-agent > 10.1.1.2.search-agent: [no cksum] UDP,
> > > length 12
> >
> > Yes, there should be no UDP checksum.  This is the point of this test.
>
> Sorry, I was confirming that the hex string does use a packet
> as this patch says. It's good.
>
>
> > >> +
> > >> +sleep 1
> > >
> > > Can we use OVS_WAIT_UNTIL() or OVS_WAIT_WHILE() ?
> >
> > Good idea.
> >
> > >> +
> > >> +dnl ensure CT picked up the packet
> > >> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1)], [0], [dnl
> > >> +udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>)
> > >> +])
> > >> +
> > >> +AT_CHECK([ovs-ofctl dump-flows br0 | grep table=2, | OFPROTO_CLEAR_DURATION_IDLE],
> > >> +         [0], [dnl
> > >> + cookie=0x0, duration=<cleared>, table=2, n_packets=1, n_bytes=54,
> > >> idle_age=<cleared>, priority=10,ct_state=+new+trk,in_port=1
> > >> actions=output:2
> > >> + cookie=0x0, duration=<cleared>, table=2, n_packets=0, n_bytes=0,
> > >> idle_age=<cleared>, priority=10,ct_state=+est+trk actions=output:2
> > >> +])
> > >> +
> > >> +OVS_TRAFFIC_VSWITCHD_STOP
> > >> +AT_CLEANUP
> > >> +
> > >>  AT_SETUP([conntrack - Multiple ICMP traverse])
> > >>  dnl This tracks sending ICMP packets via conntrack multiple times for the
> > >>  dnl same packet
> > >> @@ -5971,6 +6013,7 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep table=2, | OFPROTO_CLEAR_DURATION_IDLE
> > >>  OVS_TRAFFIC_VSWITCHD_STOP
> > >>  AT_CLEANUP
> > >>
> > >> +
> > >
> > > There mixed styles. Some use two lines, some use single line
> > > and some use two lines with a dnl ----- or ^L.
> > >
> > > I guess we should stick to single line, but I don't have a
> > > strong preference.
> >
> > Oops, this snuck in.  I will remove it, and we can do something there
> > with a different patch if someone thinks it is needed.
>
> Cool,
> thanks
> fbl
>
>
> >
> > > Otherwise the patch works for me.
> > >
> > > Thanks,
> > > fbl
> > >
> > >>  AT_BANNER([802.1ad])
> > >>
> > >>  AT_SETUP([802.1ad - vlan_limit])
> > >> --
> > >> 2.25.4
> > >>
> > >> _______________________________________________
> > >> dev mailing list
> > >> dev at openvswitch.org
> > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> --
> fbl
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev