[ovs-dev] [PATCH v1 1/1] datapath-windows: add layers when adding the deferred actions

Wilson Peng pweisong at vmware.com
Wed Oct 13 11:06:47 UTC 2021


Currently the layers info propogated to ProcessDeferredActions may be
incorrect. Because of this, any subsequent usage of layers might result
in undesired behavior. Accordingly in this patch it will add the related
 layers in the deferred action to make sure the layers consistent with
the related NBL.

In the specified case 229, we have encountered one issue when doing
the decap Geneve Packet and doing the twice NAT(via two flow tables)
and found the HTTP packet will be changed the TCP sequence.

After debugging, we found the issue is caused by the not-updated
layers value isTcp and isUdp for Geneve decapping case.

The related function call chains are listed below,

OvsExecuteDpIoctl—>OvsActionsExecute—>OvsDoExecuteActions->OvsTunnelPortRx
——>OvsDoExecuteActions——〉nat ct action and recircle action
->OvsActionsExecute->defered_actions processing for nat and recircle action

For the Geneve packet decaping, it will firstly set the layers for Udp packet.
Then it will go on doing OVS flow extract to get the inner packet layers and
Processing the first nat action and first recircle action. After that datapath
Will do defered_actions processing on OvsActionsExecute. And it does inherit
The incorrect geneve packet layers value( isTCP 0 and isUdp 1).So in the second
Nat action processing it will get the wrong TCP Headers in OvsUpdateAddressAndPort
And it will update  related TCP check field value but in this case it will change
The packet Tcp seq value.

Reported-at:https://github.com/openvswitch/ovs-issues/issues/229
Signed-off-by: Wilson Peng <pweisong at vmware.com>
---
 datapath-windows/ovsext/Actions.c | 10 ++++++----
 datapath-windows/ovsext/Recirc.c  | 18 ++++++++++++++++--
 datapath-windows/ovsext/Recirc.h  |  3 +++
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/Actions.c
index 90ecb59f0..592cb7467 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1803,9 +1803,11 @@ OvsExecuteRecirc(OvsForwardingContext *ovsFwdCtx,
     }
 
     if (newNbl) {
-        deferredAction = OvsAddDeferredActions(newNbl, key, NULL);
+        deferredAction = OvsAddDeferredActions(newNbl, key, &(ovsFwdCtx->layers),
+                                               NULL);
     } else {
-        deferredAction = OvsAddDeferredActions(ovsFwdCtx->curNbl, key, NULL);
+        deferredAction = OvsAddDeferredActions(ovsFwdCtx->curNbl, key,
+                                              &(ovsFwdCtx->layers), NULL);
     }
 
     if (deferredAction) {
@@ -1975,7 +1977,7 @@ OvsExecuteSampleAction(OvsForwardingContext *ovsFwdCtx,
         return STATUS_SUCCESS;
     }
 
-    if (!OvsAddDeferredActions(newNbl, key, a)) {
+    if (!OvsAddDeferredActions(newNbl, key, &(ovsFwdCtx->layers), a)) {
         OVS_LOG_INFO(
             "Deferred actions limit reached, dropping sample action.");
         OvsCompleteNBL(ovsFwdCtx->switchContext, newNbl, TRUE);
@@ -2361,7 +2363,7 @@ OvsActionsExecute(POVS_SWITCH_CONTEXT switchContext,
 
     if (status == STATUS_SUCCESS) {
         status = OvsProcessDeferredActions(switchContext, completionList,
-                                           portNo, sendFlags, layers);
+                                           portNo, sendFlags, NULL);
     }
 
     return status;
diff --git a/datapath-windows/ovsext/Recirc.c b/datapath-windows/ovsext/Recirc.c
index 2febf060d..a32b75352 100644
--- a/datapath-windows/ovsext/Recirc.c
+++ b/datapath-windows/ovsext/Recirc.c
@@ -277,16 +277,23 @@ OvsDeferredActionsQueuePush(POVS_DEFERRED_ACTION_QUEUE queue)
 POVS_DEFERRED_ACTION
 OvsAddDeferredActions(PNET_BUFFER_LIST nbl,
                       OvsFlowKey *key,
+                      POVS_PACKET_HDR_INFO layers,
                       const PNL_ATTR actions)
 {
     POVS_DEFERRED_ACTION_QUEUE queue = OvsDeferredActionsQueueGet();
     POVS_DEFERRED_ACTION deferredAction = NULL;
+    OVS_PACKET_HDR_INFO layersInit = { 0 };
 
     deferredAction = OvsDeferredActionsQueuePush(queue);
     if (deferredAction) {
         deferredAction->nbl = nbl;
         deferredAction->actions = actions;
         deferredAction->key = *key;
+        if (layers) {
+            deferredAction->layers = *layers;
+        } else {
+            deferredAction->layers = layersInit;
+        }
     }
 
     return deferredAction;
@@ -309,9 +316,16 @@ OvsProcessDeferredActions(POVS_SWITCH_CONTEXT switchContext,
     NDIS_STATUS status = NDIS_STATUS_SUCCESS;
     POVS_DEFERRED_ACTION_QUEUE queue = OvsDeferredActionsQueueGet();
     POVS_DEFERRED_ACTION deferredAction = NULL;
+    POVS_PACKET_HDR_INFO layersDeferred = NULL;
 
     /* Process all deferred actions. */
     while ((deferredAction = OvsDeferredActionsQueuePop(queue)) != NULL) {
+        if (layers) {
+            layersDeferred = layers;
+         } else {
+            layersDeferred = &(deferredAction->layers);
+         }
+
         if (deferredAction->actions) {
             status = OvsDoExecuteActions(switchContext,
                                          completionList,
@@ -319,7 +333,7 @@ OvsProcessDeferredActions(POVS_SWITCH_CONTEXT switchContext,
                                          portNo,
                                          sendFlags,
                                          &deferredAction->key, NULL,
-                                         layers, deferredAction->actions,
+                                         layersDeferred, deferredAction->actions,
                                          NlAttrGetSize(deferredAction->actions));
         } else {
             status = OvsDoRecirc(switchContext,
@@ -327,7 +341,7 @@ OvsProcessDeferredActions(POVS_SWITCH_CONTEXT switchContext,
                                  deferredAction->nbl,
                                  &deferredAction->key,
                                  portNo,
-                                 layers);
+                                 layersDeferred);
         }
     }
 
diff --git a/datapath-windows/ovsext/Recirc.h b/datapath-windows/ovsext/Recirc.h
index 2b314ce27..74130a460 100644
--- a/datapath-windows/ovsext/Recirc.h
+++ b/datapath-windows/ovsext/Recirc.h
@@ -18,6 +18,7 @@
 #define __RECIRC_H_ 1
 
 #include "Actions.h"
+#include "NetProto.h"
 
 #define DEFERRED_ACTION_QUEUE_SIZE          10
 #define DEFERRED_ACTION_EXEC_LEVEL           4
@@ -26,6 +27,7 @@ typedef struct _OVS_DEFERRED_ACTION {
     PNET_BUFFER_LIST    nbl;
     PNL_ATTR            actions;
     OvsFlowKey          key;
+    OVS_PACKET_HDR_INFO layers;
 } OVS_DEFERRED_ACTION, *POVS_DEFERRED_ACTION;
 
 /*
@@ -52,6 +54,7 @@ OvsProcessDeferredActions(POVS_SWITCH_CONTEXT switchContext,
 POVS_DEFERRED_ACTION
 OvsAddDeferredActions(PNET_BUFFER_LIST packet,
                       OvsFlowKey *key,
+                      POVS_PACKET_HDR_INFO layers,
                       const PNL_ATTR actions);
 
 /*
-- 
2.17.1



More information about the dev mailing list