diff mbox series

[ovs-dev,v2,1/1] datapath-windows:Reset PseudoChecksum value only for TX direction offload case

Message ID 20211022090513.91799-1-pweisong@vmware.com
State Accepted
Delegated to: Alin Gabriel Serdean
Headers show
Series [ovs-dev,v2,1/1] datapath-windows:Reset PseudoChecksum value only for TX direction offload case | 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

Wilson Peng Oct. 22, 2021, 9:05 a.m. UTC 
While testing OVS-windows flows for the DNAT/SNAT action, the checksum in
TCP header is set incorrectly when TCP offload is enabled by default. As a
result,the packet will be dropped on the Windows VM when processing the packet
 from Linux VM which has included correct checksum at first.  On the Windows VM,
 it has gone through two NAT actions and OVS Windows kernel will reset the
checksum to PseudoChecksum and then it will lose the original correct checksum
value which is set outside.

Back to the Nat TCP/UDP checksum value reset logic,it should reset it TCP checksum
To be PseudoChecksum value only on Tx direction for TCP Offload case.  For the packet
From the outside,  OVS Windows Kernel does not need reset the TCP/UDP checksum as
It should be the job of the received network driver to get out a correct checksum
Value.

>>>sample flow on default configuration on both Windows VM and Linux VM
(src=192.168.252.1,dst=10.110.225.146)-->dnat/snat-> (src=169.254.169.253,
Dst=10.176.26.107) Without the fix the return back packet(src=10.176.26.107,
Dst=169.254.169.253) will have the correct TCP checksum. After the reverse NAT
Actions, it will be changed to be packet (src=10.110.225.146, Dst=192.168.252.1)
But with incorrect TCP checksum 0xa97a which is
The PseudoChecksum. Related packet is put on the reported issue below.

Reported-at:https://github.com/openvswitch/ovs-issues/issues/231
Signed-off-by: Wilson Peng <pweisong@vmware.com>
---
 datapath-windows/ovsext/Actions.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Alin-Gabriel Serdean Oct. 28, 2021, 3:39 a.m. UTC | #1 
> While testing OVS-windows flows for the DNAT/SNAT action, the checksum in
> TCP header is set incorrectly when TCP offload is enabled by default. As a
> result,the packet will be dropped on the Windows VM when processing the packet
>  from Linux VM which has included correct checksum at first.  On the Windows VM,
>  it has gone through two NAT actions and OVS Windows kernel will reset the
> checksum to PseudoChecksum and then it will lose the original correct checksum
> value which is set outside.
> 
> Back to the Nat TCP/UDP checksum value reset logic,it should reset it TCP checksum
> To be PseudoChecksum value only on Tx direction for TCP Offload case.  For the packet
> From the outside,  OVS Windows Kernel does not need reset the TCP/UDP checksum as
> It should be the job of the received network driver to get out a correct checksum
> Value.
> 
> >>>sample flow on default configuration on both Windows VM and Linux VM
> (src=192.168.252.1,dst=10.110.225.146)-->dnat/snat-> (src=169.254.169.253,
> Dst=10.176.26.107) Without the fix the return back packet(src=10.176.26.107,
> Dst=169.254.169.253) will have the correct TCP checksum. After the reverse NAT
> Actions, it will be changed to be packet (src=10.110.225.146, Dst=192.168.252.1)
> But with incorrect TCP checksum 0xa97a which is
> The PseudoChecksum. Related packet is put on the reported issue below.
> 
> Reported-at:https://github.com/openvswitch/ovs-issues/issues/231
> Signed-off-by: Wilson Peng <pweisong@vmware.com>
> ---

Applied on master and backported until branch-2.13.

Thank you for the patch!


Alin.
diff mbox series

Patch

diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/Actions.c
index 592cb7467..0c18c6254 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1527,6 +1527,7 @@  OvsUpdateAddressAndPort(OvsForwardingContext *ovsFwdCtx,
 
     csumInfo.Value = NET_BUFFER_LIST_INFO(ovsFwdCtx->curNbl,
                                           TcpIpChecksumNetBufferListInfo);
+
     /*
      * Adjust the IP header inline as dictated by the action, and also update
      * the IP and the TCP checksum for the data modified.
@@ -1535,6 +1536,7 @@  OvsUpdateAddressAndPort(OvsForwardingContext *ovsFwdCtx,
      * ChecksumUpdate32(). Ignoring this for now, since for the most common
      * case, we only update the TTL.
      */
+     /*Only tx direction the checksum value will be reset to be PseudoChecksum*/
 
     if (isSource) {
         addrField = &ipHdr->saddr;
@@ -1551,7 +1553,7 @@  OvsUpdateAddressAndPort(OvsForwardingContext *ovsFwdCtx,
                         ((BOOLEAN)csumInfo.Receive.UdpChecksumSucceeded ||
                          (BOOLEAN)csumInfo.Receive.UdpChecksumFailed);
         }
-        if (l4Offload) {
+        if (isTx && l4Offload) {
             *checkField = IPPseudoChecksum(&newAddr, &ipHdr->daddr,
                 tcpHdr ? IPPROTO_TCP : IPPROTO_UDP,
                 ntohs(ipHdr->tot_len) - ipHdr->ihl * 4);
@@ -1572,7 +1574,7 @@  OvsUpdateAddressAndPort(OvsForwardingContext *ovsFwdCtx,
                          (BOOLEAN)csumInfo.Receive.UdpChecksumFailed);
         }
 
-       if (l4Offload) {
+       if (isTx && l4Offload) {
             *checkField = IPPseudoChecksum(&ipHdr->saddr, &newAddr,
                 tcpHdr ? IPPROTO_TCP : IPPROTO_UDP,
                 ntohs(ipHdr->tot_len) - ipHdr->ihl * 4);
@@ -1581,7 +1583,7 @@  OvsUpdateAddressAndPort(OvsForwardingContext *ovsFwdCtx,
 
     if (*addrField != newAddr) {
         UINT32 oldAddr = *addrField;
-        if (checkField && *checkField != 0 && !l4Offload) {
+        if ((checkField && *checkField != 0) && (!l4Offload || !isTx)) {
             /* Recompute total checksum. */
             *checkField = ChecksumUpdate32(*checkField, oldAddr,
                                             newAddr);
@@ -1590,11 +1592,12 @@  OvsUpdateAddressAndPort(OvsForwardingContext *ovsFwdCtx,
             ipHdr->check = ChecksumUpdate32(ipHdr->check, oldAddr,
                                             newAddr);
         }
+
         *addrField = newAddr;
     }
 
     if (portField && *portField != newPort) {
-        if (checkField && !l4Offload) {
+        if ((checkField) && (!l4Offload || !isTx)) {
             /* Recompute total checksum. */
             *checkField = ChecksumUpdate16(*checkField, *portField,
                                            newPort);