[ovs-dev,v6,2/5] datapath-windows: Added Ipv4 fragments support in Conntrack

This patch adds support for tracking Ipv4 fragments in conntrack module.
Individual fragments are not tracked and are consumed by the
fragmentation/reassembly. Only the reassembled Ipv4 datagram is tracked and
treated as a single ct entry.

Signed-off-by: Anand Kumar <kumaranand@vmware.com>
---
v5->v6: No Change
v4->v5:
	- Removed MRU argument from function declarations as MRU is now retained
	in _OVS_BUFFER_CONTEXT.
v3->v4: No Change
v2->v3:
	- Updated log messages and fixed alignment.
v1->v2: No change
---
 datapath-windows/ovsext/Actions.c   | 13 +++++++++----
 datapath-windows/ovsext/Conntrack.c | 35 ++++++++++++++++++++++++++---------
 datapath-windows/ovsext/Conntrack.h |  6 +++++-
 3 files changed, 40 insertions(+), 14 deletions(-)

Acked-by: Sairam Venugopal <vsairam@vmware.com>





On 3/24/17, 1:51 PM, "ovs-dev-bounces@openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of kumaranand@vmware.com> wrote:

>This patch adds support for tracking Ipv4 fragments in conntrack module.
>Individual fragments are not tracked and are consumed by the
>fragmentation/reassembly. Only the reassembled Ipv4 datagram is tracked and
>treated as a single ct entry.
>
>Signed-off-by: Anand Kumar <kumaranand@vmware.com>
>---
>v5->v6: No Change
>v4->v5:
>	- Removed MRU argument from function declarations as MRU is now retained
>	in _OVS_BUFFER_CONTEXT.
>v3->v4: No Change
>v2->v3:
>	- Updated log messages and fixed alignment.
>v1->v2: No change
>---
> datapath-windows/ovsext/Actions.c   | 13 +++++++++----
> datapath-windows/ovsext/Conntrack.c | 35 ++++++++++++++++++++++++++---------
> datapath-windows/ovsext/Conntrack.h |  6 +++++-
> 3 files changed, 40 insertions(+), 14 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/Actions.c
>index 46f84bc..cbc7640 100644
>--- a/datapath-windows/ovsext/Actions.c
>+++ b/datapath-windows/ovsext/Actions.c
>@@ -2032,11 +2032,16 @@ OvsDoExecuteActions(POVS_SWITCH_CONTEXT switchContext,
>                 }
>             }
> 
>-            status = OvsExecuteConntrackAction(ovsFwdCtx.curNbl, layers,
>-                                               key, (const PNL_ATTR)a);
>+            status = OvsExecuteConntrackAction(switchContext, &ovsFwdCtx.curNbl,
>+                                               ovsFwdCtx.completionList,
>+                                               ovsFwdCtx.fwdDetail->SourcePortId,
>+                                               layers, key, (const PNL_ATTR)a);
>             if (status != NDIS_STATUS_SUCCESS) {
>-                OVS_LOG_ERROR("CT Action failed");
>-                dropReason = L"OVS-conntrack action failed";
>+                /* Pending NBLs are consumed by Defragmentation. */
>+                if (status != NDIS_STATUS_PENDING) {
>+                    OVS_LOG_ERROR("CT Action failed");
>+                    dropReason = L"OVS-conntrack action failed";
>+                }
>                 goto dropit;
>             }
>             break;
>diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
>index 35ab7a1..ea3c111 100644
>--- a/datapath-windows/ovsext/Conntrack.c
>+++ b/datapath-windows/ovsext/Conntrack.c
>@@ -15,6 +15,7 @@
>  */
> 
> #include "Conntrack.h"
>+#include "IpFragment.h"
> #include "Jhash.h"
> #include "PacketParser.h"
> #include "Event.h"
>@@ -317,13 +318,23 @@ OvsCtEntryExpired(POVS_CT_ENTRY entry)
> }
> 
> static __inline NDIS_STATUS
>-OvsDetectCtPacket(OvsFlowKey *key)
>+OvsDetectCtPacket(POVS_SWITCH_CONTEXT switchContext,
>+                  PNET_BUFFER_LIST *curNbl,
>+                  OvsCompletionList *completionList,
>+                  NDIS_SWITCH_PORT_ID sourcePort,
>+                  OvsFlowKey *key,
>+                  PNET_BUFFER_LIST *newNbl)
> {
>     /* Currently we support only Unfragmented TCP packets */
>     switch (ntohs(key->l2.dlType)) {
>     case ETH_TYPE_IPV4:
>         if (key->ipKey.nwFrag != OVS_FRAG_TYPE_NONE) {
>-            return NDIS_STATUS_NOT_SUPPORTED;
>+            return OvsProcessIpv4Fragment(switchContext,
>+                                          curNbl,
>+                                          completionList,
>+                                          sourcePort,
>+                                          key->tunKey.tunnelId,
>+                                          newNbl);
>         }
>         if (key->ipKey.nwProto == IPPROTO_TCP
>             || key->ipKey.nwProto == IPPROTO_UDP
>@@ -706,11 +717,16 @@ OvsCtExecute_(PNET_BUFFER_LIST curNbl,
> /*
>  *---------------------------------------------------------------------------
>  * OvsExecuteConntrackAction
>- *     Executes Conntrack actions XXX - Add more
>+ *     Executes Conntrack actions
>+ *     For the Ipv4 fragments, consume the orginal fragment NBL
>+ *     XXX - Add more
>  *---------------------------------------------------------------------------
>  */
> NDIS_STATUS
>-OvsExecuteConntrackAction(PNET_BUFFER_LIST curNbl,
>+OvsExecuteConntrackAction(POVS_SWITCH_CONTEXT switchContext,
>+                          PNET_BUFFER_LIST *curNbl,
>+                          OvsCompletionList *completionList,
>+                          NDIS_SWITCH_PORT_ID sourcePort,
>                           OVS_PACKET_HDR_INFO *layers,
>                           OvsFlowKey *key,
>                           const PNL_ATTR a)
>@@ -722,10 +738,11 @@ OvsExecuteConntrackAction(PNET_BUFFER_LIST curNbl,
>     MD_MARK *mark = NULL;
>     MD_LABELS *labels = NULL;
>     PCHAR helper = NULL;
>-
>+    PNET_BUFFER_LIST newNbl = NULL;
>     NDIS_STATUS status;
> 
>-    status = OvsDetectCtPacket(key);
>+    status = OvsDetectCtPacket(switchContext, curNbl, completionList,
>+                               sourcePort, key, &newNbl);
>     if (status != NDIS_STATUS_SUCCESS) {
>         return status;
>     }
>@@ -764,9 +781,9 @@ OvsExecuteConntrackAction(PNET_BUFFER_LIST curNbl,
>         /* Force implicitly means commit */
>         commit = TRUE;
>     }
>-
>-    status = OvsCtExecute_(curNbl, key, layers, commit, force,
>-                           zone, mark, labels, helper);
>+    /* If newNbl is not allocated, use the current Nbl*/
>+    status = OvsCtExecute_(newNbl != NULL ? newNbl : *curNbl, key, layers,
>+                           commit, force, zone, mark, labels, helper);
>     return status;
> }
> 
>diff --git a/datapath-windows/ovsext/Conntrack.h b/datapath-windows/ovsext/Conntrack.h
>index af99885..595cdeb 100644
>--- a/datapath-windows/ovsext/Conntrack.h
>+++ b/datapath-windows/ovsext/Conntrack.h
>@@ -20,6 +20,7 @@
> #include "precomp.h"
> #include "Flow.h"
> #include "Debug.h"
>+#include "PacketIO.h"
> #include <stddef.h>
> 
> #ifdef OVS_DBG_MOD
>@@ -155,7 +156,10 @@ OvsGetTcpPayloadLength(PNET_BUFFER_LIST nbl)
> VOID OvsCleanupConntrack(VOID);
> NTSTATUS OvsInitConntrack(POVS_SWITCH_CONTEXT context);
> 
>-NDIS_STATUS OvsExecuteConntrackAction(PNET_BUFFER_LIST curNbl,
>+NDIS_STATUS OvsExecuteConntrackAction(POVS_SWITCH_CONTEXT switchContext,
>+                                      PNET_BUFFER_LIST *curNbl,
>+                                      OvsCompletionList *completionList,
>+                                      NDIS_SWITCH_PORT_ID sourcePort,
>                                       OVS_PACKET_HDR_INFO *layers,
>                                       OvsFlowKey *key,
>                                       const PNL_ATTR a);
>-- 
>2.9.3.windows.1
>
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=gssfQjIFgstVo8S15QV2f4iuiTmhwYggElt_sTWWtR4&s=jizecVgYbuvgsxEy2n0CStFmH9kJnKY3VDDv6OaXWv4&e=

Thanks a lot for the patch.

From what I see you and Yin(https://patchwork.ozlabs.org/patch/742367/) both benefit by using `ovsFwdCtx` (OvsForwardingContext) in the Conntrack pieces.
IMO it make sense just to fire a single patch just to add OvsExecuteConntrackAction/OvsCtExecute_ so both of you can benefit from it later.
What do you think Sai?

Also if you pass the forwarding context I don't think you will need the following, passing it over too multiple functions: 
> +    PNET_BUFFER_LIST newNbl = NULL;
[Alin Serdean] and initialize the curNbl from ovsFwdCtx with the new allocated nbl in OvsIpv4Reassemble. I will explain more in patch 0 why reiniting the fwdcontext is needed.
>      NDIS_STATUS status;
> 
> -    status = OvsDetectCtPacket(key);
> +    status = OvsDetectCtPacket(switchContext, curNbl, completionList,
> +                               sourcePort, key, &newNbl);
>      if (status != NDIS_STATUS_SUCCESS) {