diff mbox series

[ovs-dev,branch-3.2,v5] dp-packet: Correct IPv4 checksum calculation.

Message ID 20240826054503.134628-1-mkp@redhat.com
State Accepted, archived
Commit 659bbfe452142e9aae401c5c1ff8cc482222ad28
Headers show
Series [ovs-dev,branch-3.2,v5] dp-packet: Correct IPv4 checksum calculation. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Mike Pattrick Aug. 26, 2024, 5:45 a.m. UTC
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>
---
v5: Send packet twice so checksum isn't resolved in upcall
---
 lib/dp-packet.h      | 11 +++++++++--
 tests/dpif-netdev.at | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)

Comments

Ilya Maximets Aug. 27, 2024, 1:23 p.m. UTC | #1
On 8/26/24 07:45, 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>
> ---
> v5: Send packet twice so checksum isn't resolved in upcall
> ---
>  lib/dp-packet.h      | 11 +++++++++--
>  tests/dpif-netdev.at | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 2 deletions(-)

Thanks!  Applied to branch-3.2.

Best regards, Ilya Maximets.
diff mbox series

Patch

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..ac14e3376 100644
--- a/tests/dpif-netdev.at
+++ b/tests/dpif-netdev.at
@@ -807,6 +807,45 @@  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 neither this
+dnl packet nor the following one contain a correct checksum. OVS is
+dnl expected to replace this dummy checksum with a valid one if possible.
+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])
+dnl In this version of OVS, checksum offload packets have their checksum
+dnl resovled in upcalls. Send the packet twice so the checksum offload is
+dnl resolved after the packet is modified.
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 OPT_PKT])
+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/eeee/dd2e/" > expout])
+AT_CHECK([echo "MICROGRAM" >> expout])
+AT_CHECK([tail -n 2 p2.pcap.txt], [0], [expout])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP