| Message ID | 20240726162334.619887-1-mkp@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v2] dp-packet: Correct IPv4 checksum calculation. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
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 <junwang01@cestc.cn> > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > 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? Best regards, Ilya Maximets.
On Wed, Aug 14, 2024 at 2:39 PM Ilya Maximets <i.maximets@ovn.org> 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 <junwang01@cestc.cn> > > Signed-off-by: Mike Pattrick <mkp@redhat.com> > > --- > > 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. >
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); + } } /* 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. +optpkt=dnl +["aaaaaaaaaaaabbbbbbbbbbbb0800"]dnl +["4f000044abab00004001eeee0a0000010a000002"]dnl +["07270c010203040a00000300000000"]dnl +["00000000000000000000000000000000"]dnl +["00000000000000000008003e2fb6d00000"] + +dnl IP header is indicates optional fields but doesn't contain any +microgram="aaaaaaaaaaaabbbbbbbbbbbb08004f000044abab00004001eeee0a0000010a000002" + +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 +AT_CHECK_UNQUOTED([tail -n 2 p2.pcap.txt], [0], [dnl +[aaaaaaaaaaaabbbbbbbbbbbb08004f000044abab00004001dd2e0a000001c0a80101]dnl +[07270c010203040a0000030000000000000000000000000000000000000000000000]dnl +[00000000000008003e2fb6d00000] +[aaaaaaaaaaaabbbbbbbbbbbb08004f000044abab00004001eeee0a0000010a000002] +]) OVS_VSWITCHD_STOP AT_CLEANUP
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 <junwang01@cestc.cn> Signed-off-by: Mike Pattrick <mkp@redhat.com> --- v2: - Added tests - Added additional checks --- lib/dp-packet.h | 31 ++++++++++++++++++++++++++++--- tests/dpif-netdev.at | 24 ++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 3 deletions(-)