Hi Paul, Thanks for reviewing the patch. You are right, I will update it to check for NULL and re-send it.
Thanks, Sairam On 5/6/16, 3:00 AM, "Paul Boca" <pb...@cloudbasesolutions.com> wrote: >Hi Sairam! > > > >I added a comment below. Besides this, looks good to me. > >Please correct me if I'm wrong. > > > >Thanks, > >Paul > > > >> -----Original Message----- > >> From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Sairam > >> Venugopal > >> Sent: Thursday, April 28, 2016 2:23 AM > >> To: dev@openvswitch.org > >> Subject: [ovs-dev] [PATCH] datapath-windows: Add support for UDP and >>ICMP > >> to Conntrack Module > >> > >> Enable support for UDP and ICMP in the connection tracking module on > >> Hyper-V. > >> > >> Signed-off-by: Sairam Venugopal <vsai...@vmware.com> > >> --- > >> datapath-windows/automake.mk | 1 + > >> datapath-windows/ovsext/Conntrack-other.c | 78 ++++++++++++++ > >> datapath-windows/ovsext/Conntrack.c | 174 >>++++++++++++++++++++++---- > >> ---- > >> datapath-windows/ovsext/Conntrack.h | 4 + > >> datapath-windows/ovsext/ovsext.vcxproj | 1 + > >> 5 files changed, 214 insertions(+), 44 deletions(-) > >> create mode 100644 datapath-windows/ovsext/Conntrack-other.c > >> > >> diff --git a/datapath-windows/automake.mk b/datapath- > >> windows/automake.mk > >> index c9af806..668cf2c 100644 > >> --- a/datapath-windows/automake.mk > >> +++ b/datapath-windows/automake.mk > >> @@ -13,6 +13,7 @@ EXTRA_DIST += \ > >> datapath-windows/ovsext/Atomic.h \ > >> datapath-windows/ovsext/BufferMgmt.c \ > >> datapath-windows/ovsext/BufferMgmt.h \ > >> + datapath-windows/ovsext/Conntrack-other.c \ > >> datapath-windows/ovsext/Conntrack-tcp.c \ > >> datapath-windows/ovsext/Conntrack.c \ > >> datapath-windows/ovsext/Conntrack.h \ > >> diff --git a/datapath-windows/ovsext/Conntrack-other.c b/datapath- > >> windows/ovsext/Conntrack-other.c > >> new file mode 100644 > >> index 0000000..bcd38d5 > >> --- /dev/null > >> +++ b/datapath-windows/ovsext/Conntrack-other.c > >> @@ -0,0 +1,78 @@ > >> +/* > >> + * Copyright (c) 2015, 2016 VMware, Inc. > >> + * > >> + * Licensed under the Apache License, Version 2.0 (the "License"); > >> + * you may not use this file except in compliance with the License. > >> + * You may obtain a copy of the License at: > >> + * > >> + * >>https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_licens >>es_LICENSE-2D2.0&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r >>=Dcruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=H9iFWy0mgbr-yfEWK5q7tVgH1V >>p5aeJJTe2FuyGMRIM&s=_n_tpqEXJrn1R2I9bXGMvgspbmfpklqr2chMB-GARPI&e= > >> + * > >> + * Unless required by applicable law or agreed to in writing, software > >> + * distributed under the License is distributed on an "AS IS" BASIS, > >> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > >> implied. > >> + * See the License for the specific language governing permissions and > >> + * limitations under the License. > >> + */ > >> + > >> +#include "Conntrack.h" > >> +#include <stddef.h> > >> + > >> +enum other_state { > >> + OTHERS_FIRST, > >> + OTHERS_MULTIPLE, > >> + OTHERS_BIDIR, > >> +}; > >> + > >> +struct conn_other { > >> + struct OVS_CT_ENTRY up; > >> + enum other_state state; > >> +}; > >> + > >> +static const long long other_timeouts[] = { > >> + [OTHERS_FIRST] = 60 * 10000000, > >> + [OTHERS_MULTIPLE] = 60 * 10000000, > >> + [OTHERS_BIDIR] = 30 * 10000000, > >> +}; > >> + > >> +static __inline struct conn_other* > >> +OvsCastConntrackEntryToOtherEntry(OVS_CT_ENTRY *conn) > >> +{ > >> + return CONTAINER_OF(conn, struct conn_other, up); > >> +} > >> + > >> +static __inline VOID > >> +OvsConntrackUpdateExpiration(struct conn_other *conn, long long now) > >> +{ > >> + conn->up.expiration = now + other_timeouts[conn->state]; > >> +} > >> + > >> +enum ct_update_res > >> +OvsConntrackUpdateOtherEntry(OVS_CT_ENTRY *conn_, > >> + BOOLEAN reply, > >> + UINT64 now) > >> +{ > >> + struct conn_other *conn = OvsCastConntrackEntryToOtherEntry(conn_); > >> + > >> + if (reply && conn->state != OTHERS_BIDIR) { > >> + conn->state = OTHERS_BIDIR; > >> + } else if (conn->state == OTHERS_FIRST) { > >> + conn->state = OTHERS_MULTIPLE; > >> + } > >> + > >> + OvsConntrackUpdateExpiration(conn, now); > >> + > >> + return CT_UPDATE_VALID; > >> +} > >> + > >> +OVS_CT_ENTRY * > >> +OvsConntrackCreateOtherEntry(UINT64 now) > >> +{ > >> + struct conn_other *conn; > >> + conn = OvsAllocateMemoryWithTag(sizeof(struct conn_other), > >> + OVS_CT_POOL_TAG); > >> + /* XXX Handle memory allocation error*/ > >> + conn->up = (OVS_CT_ENTRY) {0}; > >> + conn->state = OTHERS_FIRST; > >> + OvsConntrackUpdateExpiration(conn, now); > >> + return &conn->up; > >> +} > >> \ No newline at end of file > >> diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath- > >> windows/ovsext/Conntrack.c > >> index 544fd51..50204cb 100644 > >> --- a/datapath-windows/ovsext/Conntrack.c > >> +++ b/datapath-windows/ovsext/Conntrack.c > >> @@ -146,9 +146,20 @@ OvsCtUpdateFlowKey(struct OvsFlowKey *key, > >> } > >> } > >> > >> +static __inline VOID > >> +OvsCtAddEntry(POVS_CT_ENTRY entry, OvsConntrackKeyLookupCtx *ctx) > >> +{ > >> + NdisMoveMemory(&entry->key, &ctx->key, sizeof (OVS_CT_KEY)); > >> + NdisMoveMemory(&entry->rev_key, &ctx->key, sizeof (OVS_CT_KEY)); > >> + OvsCtKeyReverse(&entry->rev_key); > >> + InsertHeadList(&ovsConntrackTable[ctx->hash & CT_HASH_TABLE_MASK], > >> + &entry->link); > >> +} > >> + > >> static __inline POVS_CT_ENTRY > >> -OvsCtEntryCreate(const TCPHdr *tcp, > >> - PNET_BUFFER_LIST curNbl, > >> +OvsCtEntryCreate(PNET_BUFFER_LIST curNbl, > >> + UINT8 ipProto, > >> + UINT32 l4Offset, > >> OvsConntrackKeyLookupCtx *ctx, > >> OvsFlowKey *key, > >> BOOLEAN commit, > >> @@ -156,26 +167,71 @@ OvsCtEntryCreate(const TCPHdr *tcp, > >> { > >> POVS_CT_ENTRY entry = NULL; > >> UINT32 state = 0; > >> - if (!OvsConntrackValidateTcpPacket(tcp)) { > >> - state |= OVS_CS_F_INVALID; > >> - OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL); > >> - return entry; > >> - } > >> + 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; > >> + if (commit) { > >> + entry = OvsConntrackCreateTcpEntry(tcp, curNbl, >>currentTime); > >> + OvsCtAddEntry(entry, ctx); > >> + } > >> + > >> + OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL); > >> + return entry; > >> + } > >> + case IPPROTO_ICMP: > >> + case IPPROTO_UDP: > >> + state |= OVS_CS_F_NEW; > >> + if (commit) { > >> + entry = OvsConntrackCreateOtherEntry(currentTime); > >> + OvsCtAddEntry(entry, ctx); > >> + } > >> > >> - state |= OVS_CS_F_NEW; > >> - if (commit) { > >> - entry = OvsConntrackCreateTcpEntry(tcp, curNbl, currentTime); > >> - NdisMoveMemory(&entry->key, &ctx->key, sizeof (OVS_CT_KEY)); > >> - NdisMoveMemory(&entry->rev_key, &ctx->key, sizeof >>(OVS_CT_KEY)); > >> - OvsCtKeyReverse(&entry->rev_key); > >> - InsertHeadList(&ovsConntrackTable[ctx->hash & >>CT_HASH_TABLE_MASK], > >> - &entry->link); > >> + OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL); > >> + return entry; > >> + default: > >> + goto invalid; > >> } > >> > >> +invalid: > >> + state |= OVS_CS_F_INVALID; > >> OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL); > >> return entry; > >> } > >> > >> +static enum CT_UPDATE_RES > >> +OvsCtUpdateEntry(OVS_CT_ENTRY* entry, > >> + PNET_BUFFER_LIST nbl, > >> + UINT8 ipProto, > >> + UINT32 l4Offset, > >> + BOOLEAN reply, > >> + UINT64 now) > >> +{ > >> + switch (ipProto) > >> + { > >> + case IPPROTO_TCP: > >> + { > >> + TCPHdr tcpStorage; > >> + const TCPHdr *tcp; > >> + tcp = OvsGetTcp(nbl, l4Offset, &tcpStorage); > >> + return OvsConntrackUpdateTcpEntry(entry, tcp, nbl, reply, >>now); > >> + } > >> + case IPPROTO_ICMP: > >> + case IPPROTO_UDP: > >> + return OvsConntrackUpdateOtherEntry(entry, reply, now); > >> + default: > >> + return CT_UPDATE_INVALID; > >> + } > >> +} > >> + > >> static __inline VOID > >> OvsCtEntryDelete(POVS_CT_ENTRY entry) > >> { > >> @@ -204,10 +260,12 @@ OvsDetectCtPacket(OvsFlowKey *key) > >> if (key->ipKey.nwFrag != OVS_FRAG_TYPE_NONE) { > >> return NDIS_STATUS_NOT_SUPPORTED; > >> } > >> - if (key->ipKey.nwProto != IPPROTO_TCP) { > >> - return NDIS_STATUS_NOT_SUPPORTED; > >> + if (key->ipKey.nwProto == IPPROTO_TCP > >> + || key->ipKey.nwProto == IPPROTO_UDP > >> + || key->ipKey.nwProto == IPPROTO_ICMP) { > >> + return NDIS_STATUS_SUCCESS; > >> } > >> - return NDIS_STATUS_SUCCESS; > >> + return NDIS_STATUS_NOT_SUPPORTED; > >> case ETH_TYPE_IPV6: > >> return NDIS_STATUS_NOT_SUPPORTED; > >> } > >> @@ -265,16 +323,31 @@ OvsCtLookup(OvsConntrackKeyLookupCtx *ctx) > >> return found; > >> } > >> > >> -static __inline VOID > >> -OvsCtSetupLookupCtx(OvsFlowKey *flowKey, > >> - UINT16 zone, > >> - OvsConntrackKeyLookupCtx *ctx) > >> +static __inline UINT32 > >> +OvsExtractLookupCtxHash(OvsConntrackKeyLookupCtx *ctx) > >> { > >> UINT32 hsrc, hdst,hash; > >> + hsrc = OvsJhashBytes((UINT32*) &ctx->key.src, >>sizeof(ctx->key.src), 0); > >> + hdst = OvsJhashBytes((UINT32*) &ctx->key.dst, >>sizeof(ctx->key.dst), 0); > >> + hash = hsrc ^ hdst; /* TO identify reverse traffic */ > >> + return OvsJhashBytes((uint32_t *) &ctx->key.dst + 1, > >> + ((uint32_t *) (&ctx->key + 1) - > >> + (uint32_t *) (&ctx->key.dst + 1)), > >> + hash); > >> +} > >> > >> +static __inline NDIS_STATUS > >> +OvsCtSetupLookupCtx(OvsFlowKey *flowKey, > >> + UINT16 zone, > >> + OvsConntrackKeyLookupCtx *ctx, > >> + PNET_BUFFER_LIST curNbl, > >> + UINT32 l4Offset) > >> +{ > >> ctx->key.zone = zone; > >> ctx->key.dl_type = flowKey->l2.dlType; > >> + ctx->related = FALSE; > >> > >> + /* Extract L3 and L4*/ > >> if (flowKey->l2.dlType == htons(ETH_TYPE_IPV4)) { > >> ctx->key.src.addr.ipv4 = flowKey->ipKey.nwSrc; > >> ctx->key.dst.addr.ipv4 = flowKey->ipKey.nwDst; > >> @@ -282,6 +355,26 @@ OvsCtSetupLookupCtx(OvsFlowKey *flowKey, > >> > >> 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); > >> + 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; > >> + } > >> + default: > >> + ctx->related = FALSE; > >> + } > >> + } > >> } else if (flowKey->l2.dlType == htons(ETH_TYPE_IPV6)) { > >> ctx->key.src.addr.ipv6 = flowKey->ipv6Key.ipv6Src; > >> ctx->key.dst.addr.ipv6 = flowKey->ipv6Key.ipv6Dst; > >> @@ -289,18 +382,13 @@ OvsCtSetupLookupCtx(OvsFlowKey *flowKey, > >> > >> ctx->key.src.port = flowKey->ipv6Key.l4.tpSrc; > >> ctx->key.dst.port = flowKey->ipv6Key.l4.tpDst; > >> + /* XXX Handle ICMPv6 errors*/ > >> + } else { > >> + return NDIS_STATUS_INVALID_PACKET; > >> } > >> > >> - /* Related bit is set for ICMP and FTP (Not supported)*/ > >> - ctx->related = FALSE; > >> - > >> - hsrc = OvsJhashBytes((UINT32*) &ctx->key.src, >>sizeof(ctx->key.src), 0); > >> - hdst = OvsJhashBytes((UINT32*) &ctx->key.dst, >>sizeof(ctx->key.dst), 0); > >> - hash = hsrc ^ hdst; /* TO identify reverse traffic */ > >> - ctx->hash = OvsJhashBytes((uint32_t *) &ctx->key.dst + 1, > >> - ((uint32_t *) (&ctx->key + 1) - > >> - (uint32_t *) (&ctx->key.dst + 1)), > >> - hash); > >> + ctx->hash = OvsExtractLookupCtxHash(ctx); > >> + return NDIS_STATUS_SUCCESS; > >> } > >> > >> /* > >> @@ -311,7 +399,7 @@ OvsCtSetupLookupCtx(OvsFlowKey *flowKey, > >> */ > >> static __inline POVS_CT_ENTRY > >> OvsProcessConntrackEntry(PNET_BUFFER_LIST curNbl, > >> - const TCPHdr *tcp, > >> + UINT32 l4Offset, > >> OvsConntrackKeyLookupCtx *ctx, > >> OvsFlowKey *key, > >> UINT16 zone, > >> @@ -329,8 +417,8 @@ OvsProcessConntrackEntry(PNET_BUFFER_LIST curNbl, > >> } > >> } else { > >> CT_UPDATE_RES result; > >> - result = OvsConntrackUpdateTcpEntry(entry, tcp, curNbl, > >> - ctx->reply, currentTime); > >> + result = OvsCtUpdateEntry(entry, curNbl, key->ipKey.nwProto, > >> + l4Offset, ctx->reply, currentTime); > >> switch (result) { > >> case CT_UPDATE_VALID: > >> state |= OVS_CS_F_ESTABLISHED; > >> @@ -345,8 +433,8 @@ OvsProcessConntrackEntry(PNET_BUFFER_LIST curNbl, > >> //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); > > > >PB: If you don't have the commit attribute set then OvsCtEntryCreate will >return NULL and on > >OvsCtUpdateFlowKey function call, 'entry' variable gets dereferenced to >obtain mark > >and labels, but a NULL pointer dereference will happen. > > > >> break; > >> } > >> } > >> @@ -401,15 +489,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 *) ¤tTime); > >> > >> /* 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 +503,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..37dd0a6 100644 > >> --- a/datapath-windows/ovsext/Conntrack.h > >> +++ b/datapath-windows/ovsext/Conntrack.h > >> @@ -99,9 +99,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); > >> +enum ct_update_res OvsConntrackUpdateOtherEntry(OVS_CT_ENTRY *conn_, > >> + BOOLEAN reply, > >> + UINT64 now); > >> #endif /* __OVS_CONNTRACK_H_ */ > >> \ No newline at end of file > >> 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@openvswitch.org > >> >>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailm >>an_listinfo_dev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r= >>Dcruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=H9iFWy0mgbr-yfEWK5q7tVgH1Vp >>5aeJJTe2FuyGMRIM&s=ZOlrzacgKFMQFiPnn_IBWGsXwgHi3M4kWNJqfTJdvpU&e= > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
