diff mbox series

[ovs-dev,v2,1/1] datapath-windows: Pickup Ct tuple as CT lookup key in function OvsCtSetupLookupCtx

Message ID MA0PR01MB6218CEF8C63769DEC0C51516885A9@MA0PR01MB6218.INDPRD01.PROD.OUTLOOK.COM
State Accepted
Headers show
Series [ovs-dev,v2,1/1] datapath-windows: Pickup Ct tuple as CT lookup key in function OvsCtSetupLookupCtx | 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 Jan. 20, 2022, 12:32 a.m. UTC 
From: Wilson Peng <pweisong@vmware.com>

CT marks which are loaded in non-first commit will be lost in ovs-windows.In linux OVS,
the CT mark setting with same flow could be set successfully.

Currenlty Ovs-windows will create one new CT with the flowKey(Extracted from the packet itself)
If the packet is already done DNAT action after the 1st round flow processing. So the ct-mark
Set on previous Conntrack will be lost.In the fix, it will make use of CT tuple src/dst address
stored in the flowKey if the value is not zero and zone in the flowKey is same as the input zone.

In the fix, it is also to adjust function OvsProcessDeferredActions to make it clear.

 //DNAT flow
cookie=0x1040000000000, duration=950.326s, table=EndpointDNAT, n_packets=0, n_bytes=0, priority=200,tcp,reg3=0xc0a8fa2b,reg4=0x20050/0x7ffff
actions=ct(commit,table=AntreaPolicyEgressRule,zone=65520,nat(dst=192.168.250.43:80),exec(load:0x1->NXM_NX_CT_MARK[2])
// Append ct_mark flow
cookie=0x1000000000000, duration=11980.701s, table=SNATConntrackCommit, n_packets=6, n_bytes=396, priority=200,ct_state=+new+trk,ip,reg0=0x2
00/0x200,reg4=0/0xc00000 actions=load:0x3->NXM_NX_REG4[22..23],ct(commit,table=SNATConntrackCommit,zone=65520,exec(load:0x1->NXM_NX_CT_MARK[4
],load:0x1->NXM_NX_CT_MARK[5]))
// SNAT flow
cookie=0x1000000000000, duration=11980.701s, table=SNATConntrackCommit, n_packets=6, n_bytes=396, priority=200,ct_state=+new+trk,ip,reg0=0x6
00/0x600,reg4=0xc00000/0xc00000 actions=ct(commit,table=L2Forwarding,zone=65521,nat(src=192.168.250.1),exec(load:0x1->NXM_NX_CT_MARK[2]))

Reported-at:https://github.com/openvswitch/ovs-issues/issues/237
Signed-off-by: Wilson Peng <pweisong@vmware.com>
---
 datapath-windows/ovsext/Actions.c   |  2 +-
 datapath-windows/ovsext/Conntrack.c | 30 +++++++++++++++++++++++++++++
 datapath-windows/ovsext/Recirc.c    |  9 ++-------
 datapath-windows/ovsext/Recirc.h    |  3 +--
 4 files changed, 34 insertions(+), 10 deletions(-)

Comments

Alin-Gabriel Serdean Jan. 20, 2022, 1:28 a.m. UTC | #1 
Thank you for incorporating the comments!

Will apply it on master and backport it until branch-2.13.

Acked-by: Alin-Gabriel Serdean <aserdean@ovn.org>

Alin.

-----Original Message-----
From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of Wilson Peng
Sent: Thursday, January 20, 2022 2:32 AM
To: dev@openvswitch.org
Subject: [ovs-dev] [PATCH v2 1/1] datapath-windows: Pickup Ct tuple as CT
lookup key in function OvsCtSetupLookupCtx

From: Wilson Peng <pweisong@vmware.com>

CT marks which are loaded in non-first commit will be lost in ovs-windows.In
linux OVS, the CT mark setting with same flow could be set successfully.

Currenlty Ovs-windows will create one new CT with the flowKey(Extracted from
the packet itself) If the packet is already done DNAT action after the 1st
round flow processing. So the ct-mark Set on previous Conntrack will be
lost.In the fix, it will make use of CT tuple src/dst address stored in the
flowKey if the value is not zero and zone in the flowKey is same as the
input zone.

In the fix, it is also to adjust function OvsProcessDeferredActions to make
it clear.

 //DNAT flow
cookie=0x1040000000000, duration=950.326s, table=EndpointDNAT, n_packets=0,
n_bytes=0, priority=200,tcp,reg3=0xc0a8fa2b,reg4=0x20050/0x7ffff
actions=ct(commit,table=AntreaPolicyEgressRule,zone=65520,nat(dst=192.168.25
0.43:80),exec(load:0x1->NXM_NX_CT_MARK[2])
// Append ct_mark flow
cookie=0x1000000000000, duration=11980.701s, table=SNATConntrackCommit,
n_packets=6, n_bytes=396, priority=200,ct_state=+new+trk,ip,reg0=0x2
00/0x200,reg4=0/0xc00000
actions=load:0x3->NXM_NX_REG4[22..23],ct(commit,table=SNATConntrackCommit,zo
ne=65520,exec(load:0x1->NXM_NX_CT_MARK[4
],load:0x1->NXM_NX_CT_MARK[5]))
// SNAT flow
cookie=0x1000000000000, duration=11980.701s, table=SNATConntrackCommit,
n_packets=6, n_bytes=396, priority=200,ct_state=+new+trk,ip,reg0=0x6
00/0x600,reg4=0xc00000/0xc00000
actions=ct(commit,table=L2Forwarding,zone=65521,nat(src=192.168.250.1),exec(
load:0x1->NXM_NX_CT_MARK[2]))

Reported-at:https://github.com/openvswitch/ovs-issues/issues/237
Signed-off-by: Wilson Peng <pweisong@vmware.com>
---
 datapath-windows/ovsext/Actions.c   |  2 +-
 datapath-windows/ovsext/Conntrack.c | 30 +++++++++++++++++++++++++++++
 datapath-windows/ovsext/Recirc.c    |  9 ++-------
 datapath-windows/ovsext/Recirc.h    |  3 +--
 4 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/datapath-windows/ovsext/Actions.c
b/datapath-windows/ovsext/Actions.c
index 0c18c6254..70ac0a0e5 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -2366,7 +2366,7 @@ OvsActionsExecute(POVS_SWITCH_CONTEXT switchContext,
 
     if (status == STATUS_SUCCESS) {
         status = OvsProcessDeferredActions(switchContext, completionList,
-                                           portNo, sendFlags, NULL);
+                                           portNo, sendFlags);
     }
 
     return status;
diff --git a/datapath-windows/ovsext/Conntrack.c
b/datapath-windows/ovsext/Conntrack.c
index fd6f3bae0..7f1d2fb41 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -626,6 +626,31 @@ OvsReverseIcmpType(UINT8 type)
     }
 }
 
+static __inline void
+OvsPickupCtTupleAsLookupKey(POVS_CT_KEY ctKey, UINT16 zone, OvsFlowKey 
+*flowKey) {
+    UINT32 ipAddrSrc = 0, ipAddrDst = 0;
+
+    if (!flowKey || !ctKey) return;
+
+    if (flowKey->l2.dlType == htons(ETH_TYPE_IPV4)) {
+        ipAddrSrc = flowKey->ct.tuple_ipv4.ipv4_src;
+        ipAddrDst = flowKey->ct.tuple_ipv4.ipv4_dst;
+
+        if ((ipAddrSrc > 0 && ipAddrDst > 0) &&
+            (zone == flowKey->ct.zone)) {
+            /* if the ct tuple_ipv4 in flowKey is not null and ct.zone is
same with
+             * zone parameter pickup the tuple_ipv4 value as the lookup key
+             */
+            ctKey->src.addr.ipv4 = flowKey->ct.tuple_ipv4.ipv4_src;
+            ctKey->dst.addr.ipv4 = flowKey->ct.tuple_ipv4.ipv4_dst;
+            ctKey->nw_proto = flowKey->ct.tuple_ipv4.ipv4_proto;
+            ctKey->src.port = flowKey->ct.tuple_ipv4.src_port;
+            ctKey->dst.port = flowKey->ct.tuple_ipv4.dst_port;
+        }
+   }
+}
+
 static __inline NDIS_STATUS
 OvsCtSetupLookupCtx(OvsFlowKey *flowKey,
                     UINT16 zone,
@@ -646,6 +671,7 @@ OvsCtSetupLookupCtx(OvsFlowKey *flowKey,
 
         ctx->key.src.port = flowKey->ipKey.l4.tpSrc;
         ctx->key.dst.port = flowKey->ipKey.l4.tpDst;
+
         if (flowKey->ipKey.nwProto == IPPROTO_ICMP) {
             ICMPHdr icmpStorage;
             const ICMPHdr *icmp;
@@ -700,6 +726,10 @@ OvsCtSetupLookupCtx(OvsFlowKey *flowKey,
         /* Translate address first for reverse NAT */
         ctx->key = natEntry->ctEntry->key;
         OvsCtKeyReverse(&ctx->key);
+    } else {
+        if (flowKey->l2.dlType == htons(ETH_TYPE_IPV4)) {
+            OvsPickupCtTupleAsLookupKey(&(ctx->key), zone, flowKey);
+        }
     }
 
     ctx->hash = OvsCtHashKey(&ctx->key); diff --git
a/datapath-windows/ovsext/Recirc.c b/datapath-windows/ovsext/Recirc.c
index a32b75352..7a688c874 100644
--- a/datapath-windows/ovsext/Recirc.c
+++ b/datapath-windows/ovsext/Recirc.c
@@ -310,8 +310,7 @@ NDIS_STATUS
 OvsProcessDeferredActions(POVS_SWITCH_CONTEXT switchContext,
                           OvsCompletionList *completionList,
                           UINT32 portNo,
-                          ULONG sendFlags,
-                          OVS_PACKET_HDR_INFO *layers)
+                          ULONG sendFlags)
 {
     NDIS_STATUS status = NDIS_STATUS_SUCCESS;
     POVS_DEFERRED_ACTION_QUEUE queue = OvsDeferredActionsQueueGet(); @@
-320,11 +319,7 @@ OvsProcessDeferredActions(POVS_SWITCH_CONTEXT
switchContext,
 
     /* Process all deferred actions. */
     while ((deferredAction = OvsDeferredActionsQueuePop(queue)) != NULL) {
-        if (layers) {
-            layersDeferred = layers;
-         } else {
-            layersDeferred = &(deferredAction->layers);
-         }
+        layersDeferred = &(deferredAction->layers);
 
         if (deferredAction->actions) {
             status = OvsDoExecuteActions(switchContext,
diff --git a/datapath-windows/ovsext/Recirc.h
b/datapath-windows/ovsext/Recirc.h
index 74130a460..b2d02a65c 100644
--- a/datapath-windows/ovsext/Recirc.h
+++ b/datapath-windows/ovsext/Recirc.h
@@ -41,8 +41,7 @@ NDIS_STATUS
 OvsProcessDeferredActions(POVS_SWITCH_CONTEXT switchContext,
                           OvsCompletionList *completionList,
                           UINT32 portNo,
-                          ULONG sendFlags,
-                          OVS_PACKET_HDR_INFO *layers);
+                          ULONG sendFlags);
 
 /*
  *
--------------------------------------------------------------------------
--
2.24.3 (Apple Git-128)
diff mbox series

Patch

diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/Actions.c
index 0c18c6254..70ac0a0e5 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -2366,7 +2366,7 @@  OvsActionsExecute(POVS_SWITCH_CONTEXT switchContext,
 
     if (status == STATUS_SUCCESS) {
         status = OvsProcessDeferredActions(switchContext, completionList,
-                                           portNo, sendFlags, NULL);
+                                           portNo, sendFlags);
     }
 
     return status;
diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
index fd6f3bae0..7f1d2fb41 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -626,6 +626,31 @@  OvsReverseIcmpType(UINT8 type)
     }
 }
 
+static __inline void
+OvsPickupCtTupleAsLookupKey(POVS_CT_KEY ctKey, UINT16 zone, OvsFlowKey *flowKey)
+{
+    UINT32 ipAddrSrc = 0, ipAddrDst = 0;
+
+    if (!flowKey || !ctKey) return;
+
+    if (flowKey->l2.dlType == htons(ETH_TYPE_IPV4)) {
+        ipAddrSrc = flowKey->ct.tuple_ipv4.ipv4_src;
+        ipAddrDst = flowKey->ct.tuple_ipv4.ipv4_dst;
+
+        if ((ipAddrSrc > 0 && ipAddrDst > 0) &&
+            (zone == flowKey->ct.zone)) {
+            /* if the ct tuple_ipv4 in flowKey is not null and ct.zone is same with
+             * zone parameter pickup the tuple_ipv4 value as the lookup key
+             */
+            ctKey->src.addr.ipv4 = flowKey->ct.tuple_ipv4.ipv4_src;
+            ctKey->dst.addr.ipv4 = flowKey->ct.tuple_ipv4.ipv4_dst;
+            ctKey->nw_proto = flowKey->ct.tuple_ipv4.ipv4_proto;
+            ctKey->src.port = flowKey->ct.tuple_ipv4.src_port;
+            ctKey->dst.port = flowKey->ct.tuple_ipv4.dst_port;
+        }
+   }
+}
+
 static __inline NDIS_STATUS
 OvsCtSetupLookupCtx(OvsFlowKey *flowKey,
                     UINT16 zone,
@@ -646,6 +671,7 @@  OvsCtSetupLookupCtx(OvsFlowKey *flowKey,
 
         ctx->key.src.port = flowKey->ipKey.l4.tpSrc;
         ctx->key.dst.port = flowKey->ipKey.l4.tpDst;
+
         if (flowKey->ipKey.nwProto == IPPROTO_ICMP) {
             ICMPHdr icmpStorage;
             const ICMPHdr *icmp;
@@ -700,6 +726,10 @@  OvsCtSetupLookupCtx(OvsFlowKey *flowKey,
         /* Translate address first for reverse NAT */
         ctx->key = natEntry->ctEntry->key;
         OvsCtKeyReverse(&ctx->key);
+    } else {
+        if (flowKey->l2.dlType == htons(ETH_TYPE_IPV4)) {
+            OvsPickupCtTupleAsLookupKey(&(ctx->key), zone, flowKey);
+        }
     }
 
     ctx->hash = OvsCtHashKey(&ctx->key);
diff --git a/datapath-windows/ovsext/Recirc.c b/datapath-windows/ovsext/Recirc.c
index a32b75352..7a688c874 100644
--- a/datapath-windows/ovsext/Recirc.c
+++ b/datapath-windows/ovsext/Recirc.c
@@ -310,8 +310,7 @@  NDIS_STATUS
 OvsProcessDeferredActions(POVS_SWITCH_CONTEXT switchContext,
                           OvsCompletionList *completionList,
                           UINT32 portNo,
-                          ULONG sendFlags,
-                          OVS_PACKET_HDR_INFO *layers)
+                          ULONG sendFlags)
 {
     NDIS_STATUS status = NDIS_STATUS_SUCCESS;
     POVS_DEFERRED_ACTION_QUEUE queue = OvsDeferredActionsQueueGet();
@@ -320,11 +319,7 @@  OvsProcessDeferredActions(POVS_SWITCH_CONTEXT switchContext,
 
     /* Process all deferred actions. */
     while ((deferredAction = OvsDeferredActionsQueuePop(queue)) != NULL) {
-        if (layers) {
-            layersDeferred = layers;
-         } else {
-            layersDeferred = &(deferredAction->layers);
-         }
+        layersDeferred = &(deferredAction->layers);
 
         if (deferredAction->actions) {
             status = OvsDoExecuteActions(switchContext,
diff --git a/datapath-windows/ovsext/Recirc.h b/datapath-windows/ovsext/Recirc.h
index 74130a460..b2d02a65c 100644
--- a/datapath-windows/ovsext/Recirc.h
+++ b/datapath-windows/ovsext/Recirc.h
@@ -41,8 +41,7 @@  NDIS_STATUS
 OvsProcessDeferredActions(POVS_SWITCH_CONTEXT switchContext,
                           OvsCompletionList *completionList,
                           UINT32 portNo,
-                          ULONG sendFlags,
-                          OVS_PACKET_HDR_INFO *layers);
+                          ULONG sendFlags);
 
 /*
  * --------------------------------------------------------------------------