On Wed, Aug 14, 2024 at 2:39 PM Ilya Maximets <[email protected]> wrote: > > On 7/26/24 18:23, 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 <[email protected]> > > Signed-off-by: Mike Pattrick <[email protected]> > > --- > > v2: > > - Added tests > > - Added additional checks > > --- > > lib/dp-packet.h | 31 ++++++++++++++++++++++++++++--- > > tests/dpif-netdev.at | 24 ++++++++++++++++++++++++ > > 2 files changed, 52 insertions(+), 3 deletions(-) > > > > diff --git a/lib/dp-packet.h b/lib/dp-packet.h > > index a75b1c5cd..e816b9f20 100644 > > --- a/lib/dp-packet.h > > +++ b/lib/dp-packet.h > > @@ -529,6 +529,16 @@ dp_packet_inner_l3(const struct dp_packet *b) > > : NULL; > > } > > > > +static inline size_t > > +dp_packet_inner_l3_size(const struct dp_packet *b) > > +{ > > + return OVS_LIKELY(b->inner_l3_ofs != UINT16_MAX) > > + ? (const char *) dp_packet_tail(b) > > + - (const char *) dp_packet_inner_l3(b) > > + - dp_packet_l2_pad_size(b) > > + : 0; > > +} > > + > > static inline void * > > dp_packet_inner_l4(const struct dp_packet *b) > > { > > @@ -1390,11 +1400,26 @@ dp_packet_hwol_l3_ipv4(const struct dp_packet *b) > > static inline void > > dp_packet_ip_set_header_csum(struct dp_packet *p, bool inner) > > { > > - struct ip_header *ip = (inner) ? dp_packet_inner_l3(p) : > > dp_packet_l3(p); > > + struct ip_header *ip; > > + size_t l3_size; > > + size_t ip_len; > > + > > + if (inner) { > > + ip = dp_packet_inner_l3(p); > > + l3_size = dp_packet_inner_l3_size(p); > > + } else { > > + ip = dp_packet_l3(p); > > + l3_size = dp_packet_l3_size(p); > > + } > > > > 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); > > + } > > } > > > > Thanks, Mike! The ode above seems correct to me. See a couple > comments for the test formatting below. > > > /* Returns 'true' if the packet 'p' has good integrity and the > > diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at > > index bdc24cc30..d31f13931 100644 > > --- a/tests/dpif-netdev.at > > +++ b/tests/dpif-netdev.at > > @@ -807,6 +807,30 @@ 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. > > This packet has an incorrect ICMP checksum. Not sure if that was the > intention, but it might be good to keep it incorrect just to check that > OVS doesn't fix it. But we should, probably, call that out as the > packet is not fully correct. > > > +optpkt=dnl > > +["aaaaaaaaaaaabbbbbbbbbbbb0800"]dnl > > +["4f000044abab00004001eeee0a0000010a000002"]dnl > > +["07270c010203040a00000300000000"]dnl > > +["00000000000000000000000000000000"]dnl > > +["00000000000000000008003e2fb6d00000"] > > + > > +dnl IP header is indicates optional fields but doesn't contain any > > 'is' should not be there and there should be a period at the end. > > > +microgram="aaaaaaaaaaaabbbbbbbbbbbb08004f000044abab00004001eeee0a0000010a000002" > > Since we can't generate these packets with packet-compose, we should > describe better what's inside. And since we're splitting lines, > we may as well use m4_define/m4_join, so the actual values end up in > the testsuite log. > > Maybe something like this: > > 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=0xeeee) > [4f000044abab00004001eeee0a0000010a000002], > 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 ${optpkt}]) > > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${microgram}]) > > +AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1]) > > +dnl The first packet has a checksum and the second doesn't > > Maybe clarify that we're talking about IPv4 checksum. > And also a period at the end. > > > +AT_CHECK_UNQUOTED([tail -n 2 p2.pcap.txt], [0], [dnl > > +[aaaaaaaaaaaabbbbbbbbbbbb08004f000044abab00004001dd2e0a000001c0a80101]dnl > > +[07270c010203040a0000030000000000000000000000000000000000000000000000]dnl > > +[00000000000008003e2fb6d00000] > > +[aaaaaaaaaaaabbbbbbbbbbbb08004f000044abab00004001eeee0a0000010a000002] > > +]) > > If we define packets as above then we could re-use them here like this: > > AT_CHECK([echo "OPT_PKT" | sed -e "s/0a000002/c0a80101/" -e "s/eeee/dd2e/" > > expout]) > AT_CHECK([echo "MICROGRAM" >> expout]) > AT_CHECK([tail -n 2 p2.pcap.txt], [0], [expout]) > > This also highlights the fact that the destination IP wasn't changed in > the malformed packet. > > What do you think?
Sounds good. I was able to use your provided m4_define and expected output construction unmodified. Cheers, M > > Best regards, Ilya Maximets. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
