Thanks for the patch, please find comments inline. ________________________________________ From: [email protected] <[email protected]> on behalf of Sairam Venugopal <[email protected]> Sent: Tuesday, June 20, 2017 1:36 PM To: [email protected] Subject: [ovs-dev] [PATCH] datapath-windows: Fix potential memory leak while creating conntrack entry
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 <[email protected]> --- datapath-windows/ovsext/Conntrack.c | 59 +++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 32 deletions(-) diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c index 68ed395..bf9c4f4 100644 --- a/datapath-windows/ovsext/Conntrack.c +++ b/datapath-windows/ovsext/Conntrack.c @@ -214,9 +214,18 @@ 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; + + *entryCreated = FALSE; + state |= OVS_CS_F_NEW; + + parentEntry = OvsCtRelatedLookup(ctx->key, currentTime); + if (parentEntry != NULL) { + state |= OVS_CS_F_RELATED; + } + switch (ipProto) { case IPPROTO_TCP: @@ -228,23 +237,8 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx, 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; - } - - /* Set parent entry for related FTP connections */ - entry->parent = parentEntry; - - *entryCreated = TRUE; } break; } @@ -257,14 +251,8 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx, 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; } break; } @@ -273,11 +261,6 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx, state |= OVS_CS_F_NEW; >>> This is not needed since its initialized at the start? if (commit) { entry = OvsConntrackCreateOtherEntry(currentTime); - if (entry) { - /* Default UDP related to NULL until TFTP is supported */ - entry->parent = NULL; - } - *entryCreated = TRUE; } break; } @@ -285,12 +268,24 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx, goto invalid; } - if (commit && !entry) { - return NULL; - } - if (entry && !OvsCtAddEntry(entry, ctx, natInfo, currentTime)) { - return NULL; + if (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; >>> jump to "invalid" instead? + } + } else { + /* OvsAllocateMemoryWithTag returned NULL; treat as invalid */ + state = OVS_CS_F_INVALID; >>> jump to "invalid" instead? + } } + + OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL); return entry; invalid: -- _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
