[ovs-dev] 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(-)

Hi Sai,

I meant that  if an entry is returned by this function, it's always going
to be "explicitly created", rather than "an existing entry". If that is
that case, you can remove all lines that is related to entryCreated in this
function and determine whether entryCreated is true from the caller. I'm
bringing it up because you are refactoring this function for simplicity and
coherence anyway. But of course it's your decision whether to make it in a
separate patch.

Best regards,
Yin Lin

On Tue, Jun 20, 2017 at 3:29 PM, Sairam Venugopal <vsairam@vmware.com>
wrote:

> How will you do that in this code? We need to distinguish between an
> existing entry and entry explicitly created. If you want to send out an
> incremental patch, feel free to.
>
> Thanks,
> Sairam
>
> From: Yin Lin
> Date: Tuesday, June 20, 2017 at 3:26 PM
>
> To: Sairam Venugopal
> Subject: Re: [ovs-dev] [PATCH] datapath-windows: Fix potential memory
> leak while creating conntrack entry
>
> Hi Sai,
>
> In that case, can we get rid of the boolean and ask the caller to fire the
> event based on whether entry == NULL? For this particular function I don't
> see any reason to take an extra parameter. If one return value can be
> easily inferred from the other return value, there is no need to return two.
>
> Best regards,
> Yin Lin
>
> On Tue, Jun 20, 2017 at 2:38 PM, Sairam Venugopal <vsairam@vmware.com>
> wrote:
>
>> This variable is used to determine if a CT Entry was created without any
>> errors. We then rely on this variable to fire an event to state that a new
>> CT Entry was created.
>>
>> This event was previously inside the OvsCtAddEntry but had to be pulled
>> out to include Mark and Label fields.
>>
>>
>> From: Yin Lin
>> Date: Tuesday, June 20, 2017 at 2:34 PM
>> To: Sairam Venugopal
>> Subject: Re: [ovs-dev] [PATCH] datapath-windows: Fix potential memory
>> leak while creating conntrack entry
>>
>> Hi Sai,
>>
>> Thanks for the fix. One question though. I noticed that entryCreated is
>> TRUE if and only if entry == NULL at the end of this function. So why do we
>> need this variable?
>>
>> Best regards,
>> Yin Lin
>>
>> On Tue, Jun 20, 2017 at 1:36 PM, Sairam Venugopal <vsairam@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@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;
>>>              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;
>>> +            }
>>> +        } else {
>>> +            /* OvsAllocateMemoryWithTag returned NULL; treat as invalid
>>> */
>>> +            state = OVS_CS_F_INVALID;
>>> +        }
>>>      }
>>> +
>>> +
>>>      OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL);
>>>      return entry;
>>>  invalid:
>>> --
>>> 2.9.0.windows.1
>>>
>>> _______________________________________________
>>> dev mailing list
>>> dev@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwMFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=NI6_edAf0frY3hp9xUptfjlSmambHlipsfnL0LlGK7Q&s=xoRf13uyjLhpM6u4eOiyLM8gEJaA-NbPkNon_A7Yi6U&e=>
>>>
>>
>>
>