diff mbox series

[ovs-dev] datapath-windows: Update flow key in SET action

Message ID 1595849981-6776-1-git-send-email-jinjung@vmware.com
State Changes Requested
Headers show
Series [ovs-dev] datapath-windows: Update flow key in SET action | expand

Commit Message

Jinjun Gao July 27, 2020, 11:39 a.m. UTC 
The flow key is not updated when process OVS_ACTION_ATTR_SET action.
It will impact follow-up actions, such as, conntrack module cannot
find created conntrack entry if passing older flow key to it.

Reported-by: Rui Cao <rcao@vmware.com>
Signed-off-by: Jinjun Gao <jinjung@vmware.com>
---
 datapath-windows/ovsext/Actions.c | 41 +++++++++++++++++++++++----------------
 datapath-windows/ovsext/Actions.h |  3 +++
 2 files changed, 27 insertions(+), 17 deletions(-)

--
2.7.4

Comments

Alin Serdean July 28, 2020, 3:42 p.m. UTC | #1 
Thanks a lot for testing and fixing this as discussed offline.

Can you please split the assignment via the form:
“key->ipKey.nwDst = ipHdr->daddr = ipAttr->ipv4_dst”

It confuses the SDV tool from msft for some reason:
https://docs.microsoft.com/en-us/windows-hardware/drivers/develop/static-driver-verifier-known-issues

Thanks,
Alin.
Jinjun Gao July 29, 2020, 3:33 a.m. UTC | #2 
Thanks for review, Alin.

I have updated the patch and uploaded v2 patch. Please help review again.
Also, the PR (https://github.com/openvswitch/ovs/pull/326) has pasted in issue (https://github.com/openvswitch/ovs-issues/issues/190). Please help link them.

- Jinjun


From: Alin Serdean <aserdean@cloudbasesolutions.com>
Date: Tuesday, July 28, 2020 at 11:42 PM
To: Jinjun Gao <jinjung@vmware.com>, "dev@openvswitch.org" <dev@openvswitch.org>, Anand Kumar <kumaranand@vmware.com>
Cc: Rui Cao <rcao@vmware.com>, Jinjun Gao <jinjung@vmware.com>
Subject: RE: [PATCH] datapath-windows: Update flow key in SET action

Thanks a lot for testing and fixing this as discussed offline.

Can you please split the assignment via the form:
“key->ipKey.nwDst = ipHdr->daddr = ipAttr->ipv4_dst”

It confuses the SDV tool from msft for some reason:
https://docs.microsoft.com/en-us/windows-hardware/drivers/develop/static-driver-verifier-known-issues<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fwindows-hardware%2Fdrivers%2Fdevelop%2Fstatic-driver-verifier-known-issues&data=02%7C01%7Cjinjung%40vmware.com%7Ca74ea7845f494560b53f08d8330cd381%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637315477473836583&sdata=xbZ1YNsOVYOrJzG9wFlkoatGe0pX9%2BEA0SsOn%2FKG2g8%3D&reserved=0>

Thanks,
Alin.
diff mbox series

Patch

diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/Actions.c
index 699d2cd..0bc5444 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1284,6 +1284,7 @@  OvsActionMplsPush(OvsForwardingContext *ovsFwdCtx,
  */
 static __inline NDIS_STATUS
 OvsUpdateEthHeader(OvsForwardingContext *ovsFwdCtx,
+                   OvsFlowKey *key,
                    const struct ovs_key_ethernet *ethAttr)
 {
     PNET_BUFFER curNb;
@@ -1310,9 +1311,11 @@  OvsUpdateEthHeader(OvsForwardingContext *ovsFwdCtx,
     }
     ethHdr = (EthHdr *)(bufferStart + NET_BUFFER_CURRENT_MDL_OFFSET(curNb));

-    RtlCopyMemory(ethHdr->Destination, ethAttr->eth_dst,
-                   sizeof ethHdr->Destination);
-    RtlCopyMemory(ethHdr->Source, ethAttr->eth_src, sizeof ethHdr->Source);
+    RtlCopyMemory(ethHdr->Destination, ethAttr->eth_dst, ETH_ADDR_LENGTH);
+    RtlCopyMemory(ethHdr->Source, ethAttr->eth_src, ETH_ADDR_LENGTH);
+    /* Update l2 flow key */
+    RtlCopyMemory(key->l2.dlDst, ethAttr->eth_dst, ETH_ADDR_LENGTH);
+    RtlCopyMemory(key->l2.dlSrc, ethAttr->eth_src, ETH_ADDR_LENGTH);

     return NDIS_STATUS_SUCCESS;
 }
@@ -1401,6 +1404,7 @@  PUINT8 OvsGetHeaderBySize(OvsForwardingContext *ovsFwdCtx,
  */
 NDIS_STATUS
 OvsUpdateUdpPorts(OvsForwardingContext *ovsFwdCtx,
+                  OvsFlowKey *key,
                   const struct ovs_key_udp *udpAttr)
 {
     PUINT8 bufferStart;
@@ -1424,16 +1428,16 @@  OvsUpdateUdpPorts(OvsForwardingContext *ovsFwdCtx,
         if (udpHdr->source != udpAttr->udp_src) {
             udpHdr->check = ChecksumUpdate16(udpHdr->check, udpHdr->source,
                                              udpAttr->udp_src);
-            udpHdr->source = udpAttr->udp_src;
+            key->ipKey.l4.tpSrc = udpHdr->source = udpAttr->udp_src;
         }
         if (udpHdr->dest != udpAttr->udp_dst) {
             udpHdr->check = ChecksumUpdate16(udpHdr->check, udpHdr->dest,
                                              udpAttr->udp_dst);
-            udpHdr->dest = udpAttr->udp_dst;
+            key->ipKey.l4.tpDst = udpHdr->dest = udpAttr->udp_dst;
         }
     } else {
-        udpHdr->source = udpAttr->udp_src;
-        udpHdr->dest = udpAttr->udp_dst;
+        key->ipKey.l4.tpSrc = udpHdr->source = udpAttr->udp_src;
+        key->ipKey.l4.tpDst = udpHdr->dest = udpAttr->udp_dst;
     }

     return NDIS_STATUS_SUCCESS;
@@ -1448,6 +1452,7 @@  OvsUpdateUdpPorts(OvsForwardingContext *ovsFwdCtx,
  */
 NDIS_STATUS
 OvsUpdateTcpPorts(OvsForwardingContext *ovsFwdCtx,
+                  OvsFlowKey *key,
                   const struct ovs_key_tcp *tcpAttr)
 {
     PUINT8 bufferStart;
@@ -1471,12 +1476,12 @@  OvsUpdateTcpPorts(OvsForwardingContext *ovsFwdCtx,
     if (tcpHdr->source != tcpAttr->tcp_src) {
         tcpHdr->check = ChecksumUpdate16(tcpHdr->check, tcpHdr->source,
                                          tcpAttr->tcp_src);
-        tcpHdr->source = tcpAttr->tcp_src;
+        key->ipKey.l4.tpSrc = tcpHdr->source = tcpAttr->tcp_src;
     }
     if (tcpHdr->dest != tcpAttr->tcp_dst) {
         tcpHdr->check = ChecksumUpdate16(tcpHdr->check, tcpHdr->dest,
                                          tcpAttr->tcp_dst);
-        tcpHdr->dest = tcpAttr->tcp_dst;
+        key->ipKey.l4.tpDst = tcpHdr->dest = tcpAttr->tcp_dst;
     }

     return NDIS_STATUS_SUCCESS;
@@ -1604,6 +1609,7 @@  OvsUpdateAddressAndPort(OvsForwardingContext *ovsFwdCtx,
  */
 NDIS_STATUS
 OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
+                    OvsFlowKey *key,
                     const struct ovs_key_ipv4 *ipAttr)
 {
     PUINT8 bufferStart;
@@ -1656,7 +1662,7 @@  OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
             ipHdr->check = ChecksumUpdate32(ipHdr->check, ipHdr->saddr,
                                             ipAttr->ipv4_src);
         }
-        ipHdr->saddr = ipAttr->ipv4_src;
+        key->ipKey.nwSrc = ipHdr->saddr = ipAttr->ipv4_src;
     }
     if (ipHdr->daddr != ipAttr->ipv4_dst) {
         if (tcpHdr) {
@@ -1671,7 +1677,7 @@  OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
             ipHdr->check = ChecksumUpdate32(ipHdr->check, ipHdr->daddr,
                                             ipAttr->ipv4_dst);
         }
-        ipHdr->daddr = ipAttr->ipv4_dst;
+        key->ipKey.nwDst = ipHdr->daddr = ipAttr->ipv4_dst;
     }
     if (ipHdr->protocol != ipAttr->ipv4_proto) {
         UINT16 oldProto = (ipHdr->protocol << 16) & 0xff00;
@@ -1685,7 +1691,7 @@  OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
         if (ipHdr->check != 0) {
             ipHdr->check = ChecksumUpdate16(ipHdr->check, oldProto, newProto);
         }
-        ipHdr->protocol = ipAttr->ipv4_proto;
+        key->ipKey.nwProto = ipHdr->protocol = ipAttr->ipv4_proto;
     }
     if (ipHdr->ttl != ipAttr->ipv4_ttl) {
         UINT16 oldTtl = (ipHdr->ttl) & 0xff;
@@ -1693,7 +1699,7 @@  OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
         if (ipHdr->check != 0) {
             ipHdr->check = ChecksumUpdate16(ipHdr->check, oldTtl, newTtl);
         }
-        ipHdr->ttl = ipAttr->ipv4_ttl;
+        key->ipKey.nwTtl = ipHdr->ttl = ipAttr->ipv4_ttl;
     }

     return NDIS_STATUS_SUCCESS;
@@ -1716,12 +1722,12 @@  OvsExecuteSetAction(OvsForwardingContext *ovsFwdCtx,

     switch (type) {
     case OVS_KEY_ATTR_ETHERNET:
-        status = OvsUpdateEthHeader(ovsFwdCtx,
+        status = OvsUpdateEthHeader(ovsFwdCtx, key,
             NlAttrGetUnspec(a, sizeof(struct ovs_key_ethernet)));
         break;

     case OVS_KEY_ATTR_IPV4:
-        status = OvsUpdateIPv4Header(ovsFwdCtx,
+        status = OvsUpdateIPv4Header(ovsFwdCtx, key,
             NlAttrGetUnspec(a, sizeof(struct ovs_key_ipv4)));
         break;

@@ -1734,16 +1740,17 @@  OvsExecuteSetAction(OvsForwardingContext *ovsFwdCtx,
         status = SUCCEEDED(convertStatus) ? NDIS_STATUS_SUCCESS : NDIS_STATUS_FAILURE;
         ASSERT(status == NDIS_STATUS_SUCCESS);
         RtlCopyMemory(&ovsFwdCtx->tunKey, &tunKey, sizeof ovsFwdCtx->tunKey);
+        RtlCopyMemory(&key->tunKey, &tunKey, sizeof key->tunKey);
         break;
     }

     case OVS_KEY_ATTR_UDP:
-        status = OvsUpdateUdpPorts(ovsFwdCtx,
+        status = OvsUpdateUdpPorts(ovsFwdCtx, key,
             NlAttrGetUnspec(a, sizeof(struct ovs_key_udp)));
         break;

     case OVS_KEY_ATTR_TCP:
-        status = OvsUpdateTcpPorts(ovsFwdCtx,
+        status = OvsUpdateTcpPorts(ovsFwdCtx, key,
             NlAttrGetUnspec(a, sizeof(struct ovs_key_tcp)));
         break;

diff --git a/datapath-windows/ovsext/Actions.h b/datapath-windows/ovsext/Actions.h
index f324c6e..26469c2 100644
--- a/datapath-windows/ovsext/Actions.h
+++ b/datapath-windows/ovsext/Actions.h
@@ -117,14 +117,17 @@  PUINT8 OvsGetHeaderBySize(OvsForwardingContext *ovsFwdCtx,

 NDIS_STATUS
 OvsUpdateUdpPorts(OvsForwardingContext *ovsFwdCtx,
+                  OvsFlowKey *key,
                   const struct ovs_key_udp *udpAttr);

 NDIS_STATUS
 OvsUpdateTcpPorts(OvsForwardingContext *ovsFwdCtx,
+                  OvsFlowKey *key,
                   const struct ovs_key_tcp *tcpAttr);

 NDIS_STATUS
 OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
+                    OvsFlowKey *key,
                     const struct ovs_key_ipv4 *ipAttr);

 NDIS_STATUS