On Fri, Aug 16, 2024 at 10:04 AM Ilya Maximets <i.maxim...@ovn.org> wrote:
>
> On 8/15/24 22:03, Mike Pattrick wrote:
> > During the transition towards checksum offloading, the function to
> > handle software fallback of IPv4 checksums didn't account for the
> > options field.
> >
> > Fixes: 5d11c47d3ebe ("userspace: Enable IP checksum offloading by default.")
> > Reported-by: Jun Wang <junwan...@cestc.cn>
> > Signed-off-by: Mike Pattrick <m...@redhat.com>
> > ---
> > NB: In 3.2 the early checksum offloading patches caused packets from
> > the netdev-dummy/receive appctl command to resolve checksums prior to
> > odp-execute. This fixed in 3.3, but that can't easily be backported
> > without also backporting unrelated code.
> >
> > To enable this patch to be backported, I gave one of the packets a
> > correct checksum and changed the comment so it was correct but also the
> > same number of lines as in the previous patch.
> >
>
> Hi, Mike. Thanks for looking into this, but I'm still a bit confused
> about what is going on. The packets in the test do not trigger the
> dp_packet_ip_set_header_csum() call, i.e. these tests do not actually
> test the code change in this patch. However, the previous ${bad_frame}
> does trigger the function and the checksum is re-calculated for it.
>
> What's the difference between these two? Are IP options affecting the
> logic somehow?
This is unrelated to the content of the packet, and is instead related
to how dummy/receive packets are handled in offloading scenarios. This
was fixed in: 084c8087292c ("userspace: Support VXLAN and GENEVE
TSO.")
>
> Best regards, Ilya Maximets.
>
> > ---
> > lib/dp-packet.h | 11 +++++++++--
> > tests/dpif-netdev.at | 35 +++++++++++++++++++++++++++++++++++
> > 2 files changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> > index 70ddf8aa4..25a0d31a1 100644
> > --- a/lib/dp-packet.h
> > +++ b/lib/dp-packet.h
> > @@ -1142,10 +1142,17 @@ static inline void
> > dp_packet_ip_set_header_csum(struct dp_packet *p)
> > {
> > struct ip_header *ip = dp_packet_l3(p);
> > + size_t l3_size = dp_packet_l3_size(p);
> > + size_t ip_len;
> >
> > ovs_assert(ip);
> > - ip->ip_csum = 0;
> > - ip->ip_csum = csum(ip, sizeof *ip);
> > +
> > + ip_len = IP_IHL(ip->ip_ihl_ver) * 4;
> > +
> > + if (OVS_LIKELY(ip_len >= IP_HEADER_LEN && ip_len < l3_size)) {
> > + ip->ip_csum = 0;
> > + ip->ip_csum = csum(ip, ip_len);
> > + }
> > }
> >
> > /* Returns 'true' if the packet 'p' has good integrity and the
> > diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> > index 061e13af8..93226521a 100644
> > --- a/tests/dpif-netdev.at
> > +++ b/tests/dpif-netdev.at
> > @@ -807,6 +807,41 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p1
> > ${bad_frame}])
> > AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
> > AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0], [${good_expected}
> > ])
> > +
> > +dnl Test with IP optional fields in a valid packet. Note that this packet
> > +dnl contains a correct checksum, but the following packet doesn't. This is
> > +dnl because the dummy interface resets checksum offload on packet recipt.
> > +m4_define([OPT_PKT], m4_join([],
> > +dnl eth(dst=aa:aa:aa:aa:aa:aa,src=bb:bb:bb:bb:bb:bb,type=0x0800)
> > +[aaaaaaaaaaaabbbbbbbbbbbb0800],
> > +dnl ipv4(dst=10.0.0.2,src=10.0.0.1,proto=1,len=60,tot_len=68,csum=0x94d6)
> > +[4f000044abab0000400194d60a0000010a000002],
> > +dnl IPv4 Opt: type 7 (Record Route) len 39 + type 0 (EOL).
> > +[07270c010203040a000003000000000000000000],
> > +[0000000000000000000000000000000000000000],
> > +dnl icmp(type=8,code=0), csum 0x3e2f incorrect, should be 0x412f.
> > +[08003e2fb6d00000]))
> > +
> > +dnl IP header indicates optional fields but doesn't contain any.
> > +m4_define([MICROGRAM], m4_join([],
> > +dnl eth(dst=aa:aa:aa:aa:aa:aa,src=bb:bb:bb:bb:bb:bb,type=0x0800)
> > +[aaaaaaaaaaaabbbbbbbbbbbb0800],
> > +dnl ipv4(dst=10.0.0.2,src=10.0.0.1,proto=1,len=60,tot_len=68,csum=0xeeee)
> > +[4f000044abab00004001eeee0a0000010a000002]))
> > +
> > +AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=true])
> > +AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=true])
> > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 OPT_PKT])
> > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 MICROGRAM])
> > +AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
> > +
> > +dnl Build the expected modified packets. The first packet has a valid IPv4
> > +dnl checksum and modified destination IP address. The second packet isn't
> > +dnl expected to change.
> > +AT_CHECK([echo "OPT_PKT" | sed -e "s/0a000002/c0a80101/" -e "s/94d6/dd2e/"
> > > expout])
> > +AT_CHECK([echo "MICROGRAM" >> expout])
> > +AT_CHECK([tail -n 2 p2.pcap.txt], [0], [expout])
> > +
> > OVS_VSWITCHD_STOP
> > AT_CLEANUP
> >
> > --
> > 2.43.5
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
