[ovs-dev] [PATCH v2] datapath-windows: Fix potential memory leak while creating conntrack entry

Nithin Raju nithin at vmware.com
Wed Jun 21 19:33:29 UTC 2017


Acked-by: Nithin Raju <nithin at vmware.com<mailto:nithin at vmware.com>>

Thanks,
-- Nithin
R&D Manager, NSBU

On Jun 21, 2017, at 10:08 AM, Sairam Venugopal <vsairam at vmware.com<mailto:vsairam at vmware.com>> wrote:

OvsCtAddEntry returns TRUE or FALSE depending on whether
OvsNatTranslateCtEntry was successful or not. In the case of an
unsuccesful NAT translation, this will fail to insert the newly created
entry to the Conntrack Table. This entry needs to be freed and the states
should be accordingly in the flowKey instead of returning out.

Consolidated the parentEntry lookup and assignment portion across
different protocols and some minor refactoring to make the code more
readable.

Tests Done: Enabled driver verifier and tested the following:
- TCP & ICMP traffic through Conntrack Module.
- Flushed Conntrack Entries while traffic was flowing.
- Uninstalled and re-installed the driver when traffic was in progress.

Signed-off-by: Sairam Venugopal <vsairam at vmware.com<mailto:vsairam at vmware.com>>
---
datapath-windows/ovsext/Conntrack.c | 123 +++++++++++++++++-------------------
1 file changed, 57 insertions(+), 66 deletions(-)

diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
index 68ed395..b010484 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -214,87 +214,78 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
                 BOOLEAN *entryCreated)
{
    POVS_CT_ENTRY entry = NULL;
-    *entryCreated = FALSE;
    UINT32 state = 0;
+    POVS_CT_ENTRY parentEntry;
    PNET_BUFFER_LIST curNbl = fwdCtx->curNbl;
-    switch (ipProto)
-    {
-        case IPPROTO_TCP:
-        {
-            TCPHdr tcpStorage;
-            const TCPHdr *tcp;
-            tcp = OvsGetTcp(curNbl, l4Offset, &tcpStorage);
-            if (!OvsConntrackValidateTcpPacket(tcp)) {
-                goto invalid;
-            }
-
-            state |= OVS_CS_F_NEW;
-            POVS_CT_ENTRY parentEntry;
-            parentEntry = OvsCtRelatedLookup(ctx->key, currentTime);
-            if (parentEntry != NULL) {
-                state |= OVS_CS_F_RELATED;
-            }

-            if (commit) {
-                entry = OvsConntrackCreateTcpEntry(tcp, curNbl, currentTime);
-                if (!entry) {
-                    return NULL;
-                }
+    *entryCreated = FALSE;
+    state |= OVS_CS_F_NEW;

-                /* Set parent entry for related FTP connections */
-                entry->parent = parentEntry;
+    parentEntry = OvsCtRelatedLookup(ctx->key, currentTime);
+    if (parentEntry != NULL) {
+        state |= OVS_CS_F_RELATED;
+    }

-                *entryCreated = TRUE;
-            }
+    switch (ipProto) {
+    case IPPROTO_TCP:
+    {
+        TCPHdr tcpStorage;
+        const TCPHdr *tcp;
+        tcp = OvsGetTcp(curNbl, l4Offset, &tcpStorage);
+        if (!OvsConntrackValidateTcpPacket(tcp)) {
+            state = OVS_CS_F_INVALID;
            break;
        }
-        case IPPROTO_ICMP:
-        {
-            ICMPHdr storage;
-            const ICMPHdr *icmp;
-            icmp = OvsGetIcmp(curNbl, l4Offset, &storage);
-            if (!OvsConntrackValidateIcmpPacket(icmp)) {
-                goto invalid;
-            }

-            state |= OVS_CS_F_NEW;
-            if (commit) {
-                entry = OvsConntrackCreateIcmpEntry(currentTime);
-                if (entry) {
-                    /* XXX Add support for ICMP-Related */
-                    entry->parent = NULL;
-                }
-                *entryCreated = TRUE;
-            }
+        if (commit) {
+            entry = OvsConntrackCreateTcpEntry(tcp, curNbl, currentTime);
+        }
+        break;
+    }
+    case IPPROTO_ICMP:
+    {
+        ICMPHdr storage;
+        const ICMPHdr *icmp;
+        icmp = OvsGetIcmp(curNbl, l4Offset, &storage);
+        if (!OvsConntrackValidateIcmpPacket(icmp)) {
+            state = OVS_CS_F_INVALID;
            break;
        }
-        case IPPROTO_UDP:
-        {
-            state |= OVS_CS_F_NEW;
-            if (commit) {
-                entry = OvsConntrackCreateOtherEntry(currentTime);
-                if (entry) {
-                    /* Default UDP related to NULL until TFTP is supported */
-                    entry->parent = NULL;
-                }
+
+        if (commit) {
+            entry = OvsConntrackCreateIcmpEntry(currentTime);
+        }
+        break;
+    }
+    case IPPROTO_UDP:
+    {
+        if (commit) {
+            entry = OvsConntrackCreateOtherEntry(currentTime);
+        }
+        break;
+    }
+    default:
+        state = OVS_CS_F_INVALID;
+        break;
+    }
+
+    if (state != OVS_CS_F_INVALID && commit) {
+        if (entry) {
+            entry->parent = parentEntry;
+            if (OvsCtAddEntry(entry, ctx, natInfo, currentTime)) {
                *entryCreated = TRUE;
+            } else {
+                /* Unable to add entry to the list */
+                OvsFreeMemoryWithTag(entry, OVS_CT_POOL_TAG);
+                state = OVS_CS_F_INVALID;
+                entry = NULL;
            }
-            break;
+        } else {
+            /* OvsAllocateMemoryWithTag returned NULL; treat as invalid */
+            state = OVS_CS_F_INVALID;
        }
-        default:
-            goto invalid;
    }

-    if (commit && !entry) {
-        return NULL;
-    }
-    if (entry && !OvsCtAddEntry(entry, ctx, natInfo, currentTime)) {
-        return NULL;
-    }
-    OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL);
-    return entry;
-invalid:
-    state |= OVS_CS_F_INVALID;
    OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL);
    return entry;
}
--
2.9.0.windows.1

_______________________________________________
dev mailing list
dev at openvswitch.org<mailto:dev at openvswitch.org>
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=-xl6DPE_Y3uQD-mpZD7osBo2iL4s3jwdmSjTlGgjlsQ&m=bPI3XllGUxvk73TkWlr1sUxrJuqHKRec2SawRR9dth4&s=mC4cH3LNfjpkOzuKjJzK1G-ceVBW47hDmzk8WAUaPFg&e=



More information about the dev mailing list