[ovs-dev] [PATCH v5] datapath-windows: Add support for UDP and ICMP to Conntrack Module

Sairam Venugopal vsairam at vmware.com
Wed Jun 15 23:15:50 UTC 2016


Alin,

Thanks for the review. I have sent a v6. I realized that the 2nd comment
was previously addressed in v2, but not reflected in the v5 of the patch.

Thanks,
Sairam

On 6/13/16, 10:31 PM, "Alin Serdean" <aserdean at cloudbasesolutions.com>
wrote:

>Hi Sai,
>
>Thanks for incorporating the comments so far. It looks good but we need
>to treat corner cases like no valid resource allocations and NULL checks
>for parameter.
>
>
>
>A few small nits inlined.
>
>
>
>Thanks,
>
>Alin.
>
><-----------cut-------->
>
>>          ctx->key.src.port = flowKey->ipKey.l4.tpSrc;
>
>>          ctx->key.dst.port = flowKey->ipKey.l4.tpDst;
>
>> +        if (flowKey->ipKey.nwProto == IPPROTO_ICMP) {
>
>> +            ICMPHdr icmpStorage;
>
>> +            const ICMPHdr *icmp;
>
>> +            icmp = OvsGetIcmp(curNbl, l4Offset, &icmpStorage);
>
>> +            ASSERT(icmp);
>
>> +            ctx->key.src.port = ctx->key.dst.port =
>
>> + icmp->fields.echo.id;
>
>> +
>
>> +            /* Related bit is set when ICMP has an error */
>
>> +            /* XXX parse out the appropriate src and dst from inner
>>pkt */
>
>> +            switch (icmp->type) {
>
>> +               case ICMP4_DEST_UNREACH:
>
>> +               case ICMP4_TIME_EXCEEDED:
>
>> +               case ICMP4_PARAM_PROB:
>
>> +               case ICMP4_SOURCE_QUENCH:
>
>> +               case ICMP4_REDIRECT: {
>
>> +                   ctx->related = TRUE;
>
>> +               }
>
>[Alin Gabriel Serdean: ] break;
>
>> +               default:
>
>> +                   ctx->related = FALSE;
>
>[Alin Gabriel Serdean: ] break; Without breaks ctx->related will always
>be false;
>
>> +            }
>
>> +        }
>
>
>
><-----------cut-------->
>
>>              //Delete and update the Conntrack
>
>>              OvsCtEntryDelete(ctx->entry);
>
>>              ctx->entry = NULL;
>
>> -            entry = OvsCtEntryCreate(tcp, curNbl, ctx, key,
>
>> -                                     commit, currentTime);
>
>> +            entry = OvsCtEntryCreate(curNbl, key->ipKey.nwProto,
>>l4Offset,
>
>> +                                     ctx, key, commit, currentTime);
>
>[Alin Gabriel Serdean: ] entry can be null, if for example the tcp packet
>was invalid. We need to treat that case because the following code will
>run after.
>
>    /* Copy mark and label from entry into flowKey. If actions specify
>
>       different mark and label, update the flowKey. */
>
>    OvsCtUpdateFlowKey(key, state, zone, entry->mark, &entry->labels);
>
>
>
>>              break;
>
>>          }
>
>>      }
>
>> @@ -401,15 +493,12 @@ OvsCtExecute_(PNET_BUFFER_LIST curNbl,
>
>>      NDIS_STATUS status = NDIS_STATUS_SUCCESS;
>
>>      POVS_CT_ENTRY entry = NULL;
>
>>      OvsConntrackKeyLookupCtx ctx = { 0 };
>
>> -    TCPHdr tcpStorage;
>
>> -    UINT64 currentTime;
>
>>      LOCK_STATE_EX lockState;
>
>> -    const TCPHdr *tcp;
>
>> -    tcp = OvsGetTcp(curNbl, layers->l4Offset, &tcpStorage);
>
>> +    UINT64 currentTime;
>
>>      NdisGetCurrentSystemTime((LARGE_INTEGER *) &currentTime);
>
>> 
>
>>      /* Retrieve the Conntrack Key related fields from packet */
>
>> -    OvsCtSetupLookupCtx(key, zone, &ctx);
>
>> +    OvsCtSetupLookupCtx(key, zone, &ctx, curNbl, layers->l4Offset);
>
>> 
>
>>      NdisAcquireRWLockWrite(ovsConntrackLockObj, &lockState, 0);
>
>> 
>
>> @@ -418,11 +507,12 @@ OvsCtExecute_(PNET_BUFFER_LIST curNbl,
>
>> 
>
>>      if (!entry) {
>
>>          /* If no matching entry was found, create one and add New
>>state */
>
>> -        entry = OvsCtEntryCreate(tcp, curNbl, &ctx,
>
>> +        entry = OvsCtEntryCreate(curNbl, key->ipKey.nwProto,
>
>> +                                 layers->l4Offset, &ctx,
>
>>                                   key, commit, currentTime);
>
>>      } else {
>
>>          /* Process the entry and update CT flags */
>
>> -        entry = OvsProcessConntrackEntry(curNbl, tcp, &ctx, key,
>
>> +        entry = OvsProcessConntrackEntry(curNbl, layers->l4Offset,
>
>> + &ctx, key,
>
>>                                           zone, commit, currentTime);
>
>>      }
>
>> 
>
>> diff --git a/datapath-windows/ovsext/Conntrack.h b/datapath-
>
>> windows/ovsext/Conntrack.h
>
>> index a754544..883ac57 100644
>
>> --- a/datapath-windows/ovsext/Conntrack.h
>
>> +++ b/datapath-windows/ovsext/Conntrack.h
>
>> @@ -80,8 +80,11 @@ typedef struct OvsConntrackKeyLookupCtx {
>
>> 
>
>>  #define CT_HASH_TABLE_SIZE ((UINT32)1 << 10)  #define
>
>> CT_HASH_TABLE_MASK (CT_HASH_TABLE_SIZE - 1)
>
>> -#define CT_ENTRY_TIMEOUT (2 * 600000000)   // 2m
>
>> -#define CT_CLEANUP_INTERVAL (2 * 600000000) // 2m
>
>> +#define CT_INTERVAL_SEC 10000000LL //1s
>
>> +#define CT_ENTRY_TIMEOUT (2 * 60 * CT_INTERVAL_SEC)   // 2m
>
>> +#define CT_CLEANUP_INTERVAL (2 * 60 * CT_INTERVAL_SEC) // 2m
>
>> +
>
>> +
>
>>  /* Given POINTER, the address of the given MEMBER in a STRUCT object,
>
>> returns
>
>>     the STRUCT object. */
>
>>  #define CONTAINER_OF(POINTER, STRUCT, MEMBER)
>> \
>
>> @@ -99,9 +102,13 @@ BOOLEAN OvsConntrackValidateTcpPacket(const
>
>> TCPHdr *tcp);  OVS_CT_ENTRY * OvsConntrackCreateTcpEntry(const TCPHdr
>
>> *tcp,
>
>>                                            PNET_BUFFER_LIST nbl,
>
>>                                            UINT64 now);
>
>> +OVS_CT_ENTRY * OvsConntrackCreateOtherEntry(UINT64 now);
>
>>  enum CT_UPDATE_RES OvsConntrackUpdateTcpEntry(OVS_CT_ENTRY*
>
>> conn_,
>
>>                                                const TCPHdr *tcp,
>
>>                                                PNET_BUFFER_LIST nbl,
>
>>                                                BOOLEAN reply,
>
>>                                                UINT64 now); -#endif /*
>>__OVS_CONNTRACK_H_ */
>
>> \ No newline at end of file
>
>> +enum ct_update_res OvsConntrackUpdateOtherEntry(OVS_CT_ENTRY
>
>> *conn_,
>
>> +                                                BOOLEAN reply,
>
>> +                                                UINT64 now); #endif /*
>
>> +__OVS_CONNTRACK_H_ */
>
>> diff --git a/datapath-windows/ovsext/ovsext.vcxproj b/datapath-
>
>> windows/ovsext/ovsext.vcxproj
>
>> index 0356ddf..0ad4c58 100644
>
>> --- a/datapath-windows/ovsext/ovsext.vcxproj
>
>> +++ b/datapath-windows/ovsext/ovsext.vcxproj
>
>> @@ -176,6 +176,7 @@
>
>>    <ItemGroup>
>
>>      <ClCompile Include="Actions.c" />
>
>>      <ClCompile Include="BufferMgmt.c" />
>
>> +    <ClCompile Include="Conntrack-other.c" />
>
>>      <ClCompile Include="Conntrack-tcp.c" />
>
>>      <ClCompile Include="Conntrack.c" />
>
>>      <ClCompile Include="Debug.c" />
>
>> --
>
>> 2.5.0.windows.1
>
>> 
>
>> _______________________________________________
>
>> dev mailing list
>
>> dev at openvswitch.org
>
>> 
>>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailm
>>an_listinfo_dev&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=
>>Dcruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=xvCJOl4b1LctBJTX-A6F3pVVs73
>>xHZzLjSIA3s8XeD0&s=5QcN4e3x0ubrsCKIoGSFRIvJ6rV26f5Ope3ID635gDg&e=
>




More information about the dev mailing list