diff mbox series

[ovs-dev] datapath-windows: Add CTA_HELP and CTA_TUPLE_MASTER

Message ID DM6PR05MB5434BAACD75001A114A8575BD4940@DM6PR05MB5434.namprd05.prod.outlook.com
State Changes Requested
Headers show
Series [ovs-dev] datapath-windows: Add CTA_HELP and CTA_TUPLE_MASTER | expand

Commit Message

Jinjun Gao June 23, 2020, 8:49 a.m. UTC 
Add helper and master if existing to a conntrack entry:
1, For CTA_HELP, only support FTP/TFTP;
2, For CTA_TUPLE_MASTER, only support FTP.

Signed-off-by: Jinjun Gao <jinjung@vmware.com>
---
 datapath-windows/ovsext/Conntrack-related.c |  1 -
 datapath-windows/ovsext/Conntrack.c         | 40 ++++++++++++++++++++++++++---
 datapath-windows/ovsext/Conntrack.h         |  1 +
 3 files changed, 37 insertions(+), 5 deletions(-)

--
1.8.5.6

Comments

Alin Serdean June 30, 2020, 9:34 a.m. UTC | #1 
Thanks for sending the patch.

Running the command:
make datapath_windows_analyze
resulted in the following:
ovsext\Conntrack.c(873): warning C28167: The function 'OvsCtExecute_' changes the IRQL and does not
restore the IRQL before it exits. It should be annotated to reflect the change or the IRQL should
be restored. IRQL was last set to 2 at line 955.
[datapath-windows\ovsext\ovsext.vcxproj]

There is a lock acquired but it is not released on the failure path.

You can fold in following incremental:

$ git diff datapath-windows/ovsext/Conntrack.c
diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
index 414b3f4b2..717d04f49 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -974,6 +974,7 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx,
                                                       OVS_CT_POOL_TAG);
         if (!entry->helper_name) {
             OVS_LOG_ERROR("Error while allocating memory");
+            OVS_RELEASE_SPIN_LOCK(&(entry->lock), irql);
             return NDIS_STATUS_RESOURCES;
         }

The rest looks good, but I have the following question:

     /* FTP ACTIVE - Server initiates the connection */
     if ((incomingKey.src.addr.ipv4 == entryKey.src.addr.ipv4) &&
-        (incomingKey.src.port == entryKey.src.port) &&
Why is the line above removed?


--
Alin.
Jinjun Gao June 30, 2020, 10:12 a.m. UTC | #2 
Thanks for review, Alin.

Please see the comments in inline [Jinjun].

--
Jinjun


From: Alin Serdean <aserdean@cloudbasesolutions.com>
Date: Tuesday, June 30, 2020 at 5:34 PM
To: Jinjun Gao <jinjung@vmware.com>, "dev@openvswitch.org" <dev@openvswitch.org>
Cc: Anand Kumar <kumaranand@vmware.com>
Subject: RE: [ovs-dev] datapath-windows: Add CTA_HELP and CTA_TUPLE_MASTER

Thanks for sending the patch.

Running the command:
make datapath_windows_analyze
resulted in the following:
ovsext\Conntrack.c(873): warning C28167: The function 'OvsCtExecute_' changes the IRQL and does not
restore the IRQL before it exits. It should be annotated to reflect the change or the IRQL should
be restored. IRQL was last set to 2 at line 955.
[datapath-windows\ovsext\ovsext.vcxproj]

There is a lock acquired but it is not released on the failure path.

You can fold in following incremental:

[Jinjun]: Will add it, thanks.

$ git diff datapath-windows/ovsext/Conntrack.c
diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
index 414b3f4b2..717d04f49 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -974,6 +974,7 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx,
                                                       OVS_CT_POOL_TAG);
         if (!entry->helper_name) {
             OVS_LOG_ERROR("Error while allocating memory");
+            OVS_RELEASE_SPIN_LOCK(&(entry->lock), irql);
             return NDIS_STATUS_RESOURCES;
         }

The rest looks good, but I have the following question:

     /* FTP ACTIVE - Server initiates the connection */
     if ((incomingKey.src.addr.ipv4 == entryKey.src.addr.ipv4) &&
-        (incomingKey.src.port == entryKey.src.port) &&
Why is the line above removed?
 [Jinjun]: I used pyftpdlib as ftp server to test this patch. Pyftpdlib doesn’t use 20 as data port and it chooses a random port as data port in active mode. It causes the incomingKey cannot match any entry in related table. In this case, the data flow cannot find control flow and add it as tuple master. After deleting above line, it works! If we don’t consider to support random data port in active mode, we need not to remove above line. It should be better to support random data port case in active mode. How do you think about it? If you agree, I can add some comments here to clear it.

--
Alin.
diff mbox series

Patch

diff --git a/datapath-windows/ovsext/Conntrack-related.c b/datapath-windows/ovsext/Conntrack-related.c
index 950be98..3bdd52f 100644
--- a/datapath-windows/ovsext/Conntrack-related.c
+++ b/datapath-windows/ovsext/Conntrack-related.c
@@ -48,7 +48,6 @@  OvsCtRelatedKeyAreSame(OVS_CT_KEY incomingKey, OVS_CT_KEY entryKey)

     /* FTP ACTIVE - Server initiates the connection */
     if ((incomingKey.src.addr.ipv4 == entryKey.src.addr.ipv4) &&
-        (incomingKey.src.port == entryKey.src.port) &&
         (incomingKey.dst.addr.ipv4 == entryKey.dst.addr.ipv4) &&
         (incomingKey.dst.port == entryKey.dst.port) &&
         (incomingKey.dl_type == entryKey.dl_type) &&
diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
index ba56116..864095f 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -480,6 +480,9 @@  OvsCtEntryDelete(POVS_CT_ENTRY entry, BOOLEAN forceDelete)
         RemoveEntryList(&entry->link);
         OVS_RELEASE_SPIN_LOCK(&(entry->lock), irql);
         NdisFreeSpinLock(&(entry->lock));
+        if (entry->helper_name) {
+            OvsFreeMemoryWithTag(entry->helper_name, OVS_CT_POOL_TAG);
+        }
         OvsFreeMemoryWithTag(entry, OVS_CT_POOL_TAG);
         NdisInterlockedDecrement((PLONG)&ctTotalEntries);
         return;
@@ -956,8 +959,6 @@  OvsCtExecute_(OvsForwardingContext *fwdCtx,

     if (OvsDetectFtpPacket(key)) {
         /* FTP parser will always be loaded */
-        UNREFERENCED_PARAMETER(helper);
-
         status = OvsCtHandleFtp(curNbl, key, layers, currentTime, entry,
                                 (ntohs(key->ipKey.l4.tpDst) == IPPORT_FTP));
         if (status != NDIS_STATUS_SUCCESS) {
@@ -965,6 +966,17 @@  OvsCtExecute_(OvsForwardingContext *fwdCtx,
         }
     }

+    if (!entry->helper_name && helper) {
+        entry->helper_name = OvsAllocateMemoryWithTag(strlen(helper) + 1,
+                                                      OVS_CT_POOL_TAG);
+        if (!entry->helper_name) {
+            OVS_LOG_ERROR("Error while allocating memory");
+            return NDIS_STATUS_RESOURCES;
+        }
+
+        memcpy(entry->helper_name, helper, strlen(helper) + 1);
+    }
+
     /* Add original tuple information to flow Key */
     if (entry->key.dl_type == ntohs(ETH_TYPE_IPV4)) {
         if (entry->parent != NULL) {
@@ -1039,8 +1051,8 @@  OvsExecuteConntrackAction(OvsForwardingContext *fwdCtx,
                 if (helper == NULL) {
                     return NDIS_STATUS_INVALID_PARAMETER;
                 }
-                if (strcmp("ftp", helper) != 0) {
-                    /* Only support FTP */
+                if (strcmp("ftp", helper) != 0 && strcmp("tftp", helper) != 0) {
+                    /* Only support FTP/TFTP */
                     return NDIS_STATUS_NOT_SUPPORTED;
                 }
                 break;
@@ -1680,6 +1692,26 @@  OvsCreateNlMsgFromCtEntry(POVS_CT_ENTRY entry,
         }
     }

+    if (entry->helper_name) {
+        UINT32 offset;
+        offset = NlMsgStartNested(&nlBuf, CTA_HELP);
+        if (!offset) {
+            return NDIS_STATUS_FAILURE;
+        }
+        if (!NlMsgPutTailString(&nlBuf, CTA_HELP_NAME, entry->helper_name)) {
+            return STATUS_INVALID_BUFFER_SIZE;
+        }
+        NlMsgEndNested(&nlBuf, offset);
+    }
+
+    if (entry->parent) {
+        status = MapCtKeyTupleToNl(&nlBuf, CTA_TUPLE_MASTER,
+                                   &((POVS_CT_ENTRY)entry->parent)->key);
+        if (status != NDIS_STATUS_SUCCESS) {
+           return STATUS_UNSUCCESSFUL;
+        }
+    }
+
     /* CTA_STATUS is required but not implemented. Default to 0 */
     if (!NlMsgPutTailU32(&nlBuf, CTA_STATUS, 0)) {
         return STATUS_INVALID_BUFFER_SIZE;
diff --git a/datapath-windows/ovsext/Conntrack.h b/datapath-windows/ovsext/Conntrack.h
index bc6580d..23b0058 100644
--- a/datapath-windows/ovsext/Conntrack.h
+++ b/datapath-windows/ovsext/Conntrack.h
@@ -108,6 +108,7 @@  typedef struct OVS_CT_ENTRY {
     struct ovs_key_ct_labels labels;
     NAT_ACTION_INFO natInfo;
     PVOID       parent; /* Points to main connection */
+    PCHAR       helper_name;
 } OVS_CT_ENTRY, *POVS_CT_ENTRY;

 typedef struct OVS_CT_REL_ENTRY {