I'm also in the process of reviewing this. I'll send out my comments soon. Sent from Outlook Mobile<https://aka.ms/qtex0l>
On Mon, May 9, 2016 at 1:09 AM -0700, "Paul Boca" <pb...@cloudbasesolutions.com<mailto:pb...@cloudbasesolutions.com>> wrote: Acked-by: Paul-Daniel Boca <pb...@cloudbasesolutions.com> > -----Original Message----- > From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Sairam > Venugopal > Sent: Saturday, May 7, 2016 12:42 AM > To: dev@openvswitch.org > Subject: [ovs-dev] [PATCH v2] 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 | 180 ++++++++++++++++++++++---- > ---- > datapath-windows/ovsext/Conntrack.h | 4 + > datapath-windows/ovsext/ovsext.vcxproj | 1 + > 5 files changed, 219 insertions(+), 45 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_licenses_LICENSE-2D2.0&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=GyNqD6-Oq2gEyu4BbH7_e7Eco9bYR0MKhwKAWFhgPFY&s=TUpiDHT0BIYGjlsSbZkuzBaGsL330egZGoUcvBp04Q8&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..e193e9f 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,14 +433,18 @@ 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); > break; > } > } > /* 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); > + if (entry) { > + OvsCtUpdateFlowKey(key, state, zone, entry->mark, &entry->labels); > + } else { > + OvsCtUpdateFlowKey(key, state, zone, 0, NULL); > + } > return entry; > } > > @@ -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 *) ¤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 +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..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_mailman_listinfo_dev&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=GyNqD6-Oq2gEyu4BbH7_e7Eco9bYR0MKhwKAWFhgPFY&s=b69n4N5iu2lPqbE5ozCgPJrb4DDkEaf0O9LoJDQ-6Hc&e= _______________________________________________ dev mailing list dev@openvswitch.org https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailman_listinfo_dev&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=GyNqD6-Oq2gEyu4BbH7_e7Eco9bYR0MKhwKAWFhgPFY&s=b69n4N5iu2lPqbE5ozCgPJrb4DDkEaf0O9LoJDQ-6Hc&e= _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
