Message ID | BY2PR0501MB2119C9805993E655EA94FAD2A2C50@BY2PR0501MB2119.namprd05.prod.outlook.com |
---|---|
State | Not Applicable |
Headers | show |
I have addressed this in V2. The invalid label is not necessary. On 6/20/17, 2:46 PM, "Shashank Ram" <rams@vmware.com> wrote: >Thanks for the patch, please find comments inline. >________________________________________ >From: ovs-dev-bounces@openvswitch.org <ovs-dev-bounces@openvswitch.org> on behalf of Sairam Venugopal <vsairam@vmware.com> >Sent: Tuesday, June 20, 2017 1:36 PM >To: dev@openvswitch.org >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 <vsairam@vmware.com> >--- > 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: >--
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;