diff mbox series

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

Message ID 20211013110647.11127-1-pweisong@vmware.com
State Accepted
Delegated to: Alin Gabriel Serdean
Headers show
Series [ovs-dev,v1,1/1] datapath-windows: add layers when adding the deferred actions | 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. 13, 2021, 11:06 a.m. UTC
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@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(-)

Comments

Alin-Gabriel Serdean Oct. 19, 2021, 7:58 p.m. UTC | #1
> 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@vmware.com>
> ---
>

Thank you for tracking down the issue and fixing it!

I applied the patch and backported until branch-2.13.

If you want me to apply it further back please let me know.
diff mbox series

Patch

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);
 
 /*