Skip to content

Commit

Permalink
datapath-windows: add layers when adding the deferred actions
Browse files Browse the repository at this point in the history
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:openvswitch/ovs-issues#229
Signed-off-by: Wilson Peng <pweisong@vmware.com>
Signed-off-by: Alin-Gabriel Serdean <aserdean@ovn.org>
  • Loading branch information
pweisong authored and aserdean committed Oct 19, 2021
1 parent 3f71885 commit b7d9c49
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 6 deletions.
10 changes: 6 additions & 4 deletions datapath-windows/ovsext/Actions.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
18 changes: 16 additions & 2 deletions datapath-windows/ovsext/Recirc.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -309,25 +316,32 @@ 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,
deferredAction->nbl,
portNo,
sendFlags,
&deferredAction->key, NULL,
layers, deferredAction->actions,
layersDeferred, deferredAction->actions,
NlAttrGetSize(deferredAction->actions));
} else {
status = OvsDoRecirc(switchContext,
completionList,
deferredAction->nbl,
&deferredAction->key,
portNo,
layers);
layersDeferred);
}
}

Expand Down
3 changes: 3 additions & 0 deletions datapath-windows/ovsext/Recirc.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;

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

/*
Expand Down

0 comments on commit b7d9c49

Please sign in to comment.