| Message ID | 20170622185428.9640-1-kumaranand@vmware.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Anand, Can you split this patch up instead to handle Conntrack.c and Conntrack-Nat.c in a separate one? This way we can keep the testing isolated to the changes. Acking Conntrack.c change: Acked-by: Sairam Venugopal <vsairam@vmware.com> On 6/22/17, 11:54 AM, "ovs-dev-bounces@openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of kumaranand@vmware.com> wrote: >In conntrack lookup, ICMP type and code fields were not being used to >determine a matching entry. As a result, ICMP4_ECHO_REQUEST packet could >be tracked as ICMP4_ECHO_REPLY packet and vice versa, which is invalid. > >To fix this, add ICMP type and code fields for matching a conntrack entry. >Also updated OvsNatKeyAreSame() and OvsHashNatKey() to include these two >fields. > >Signed-off-by: Anand Kumar <kumaranand@vmware.com> >--- > datapath-windows/ovsext/Conntrack-nat.c | 32 ++++++++++---------------------- > datapath-windows/ovsext/Conntrack.c | 16 +++++++--------- > 2 files changed, 17 insertions(+), 31 deletions(-) > >diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath-windows/ovsext/Conntrack-nat.c >index ae6b92c..f3c282c 100644 >--- a/datapath-windows/ovsext/Conntrack-nat.c >+++ b/datapath-windows/ovsext/Conntrack-nat.c >@@ -13,17 +13,11 @@ PLIST_ENTRY ovsUnNatTable = NULL; > static __inline UINT32 > OvsHashNatKey(const OVS_CT_KEY *key) > { >- UINT32 hash = 0; >-#define HASH_ADD(field) \ >- hash = OvsJhashBytes(&key->field, sizeof(key->field), hash) >- >- HASH_ADD(src.addr.ipv4_aligned); >- HASH_ADD(dst.addr.ipv4_aligned); >- HASH_ADD(src.port); >- HASH_ADD(dst.port); >- HASH_ADD(zone); >-#undef HASH_ADD >- return hash; >+ UINT32 hash[3]; >+ hash[0] = OvsJhashBytes(&key->src, sizeof(key->src), 0); >+ hash[1] = OvsJhashBytes(&key->dst, sizeof(key->dst), 0); >+ hash[2] = OvsJhashBytes(&key->zone, sizeof(key->zone), 0); >+ return OvsJhashWords(hash, 3, OVS_HASH_BASIS); > } > > /* >@@ -35,17 +29,11 @@ OvsHashNatKey(const OVS_CT_KEY *key) > static __inline BOOLEAN > OvsNatKeyAreSame(const OVS_CT_KEY *key1, const OVS_CT_KEY *key2) > { >- // XXX: Compare IPv6 key as well >-#define FIELD_COMPARE(field) \ >- if (key1->field != key2->field) return FALSE >- >- FIELD_COMPARE(src.addr.ipv4_aligned); >- FIELD_COMPARE(dst.addr.ipv4_aligned); >- FIELD_COMPARE(src.port); >- FIELD_COMPARE(dst.port); >- FIELD_COMPARE(zone); >- return TRUE; >-#undef FIELD_COMPARE >+ return ((NdisEqualMemory(&key1->src, &key2->src, >+ sizeof(struct ct_endpoint))) && >+ (NdisEqualMemory(&key1->dst, &key2->dst, >+ sizeof(struct ct_endpoint))) && >+ (key1->zone == key2->zone)); > } > > /* >diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c >index 07a9583..e97d6ce 100644 >--- a/datapath-windows/ovsext/Conntrack.c >+++ b/datapath-windows/ovsext/Conntrack.c >@@ -383,15 +383,13 @@ OvsDetectCtPacket(OvsForwardingContext *fwdCtx, > BOOLEAN > OvsCtKeyAreSame(OVS_CT_KEY ctxKey, OVS_CT_KEY entryKey) > { >- return ((ctxKey.src.addr.ipv4 == entryKey.src.addr.ipv4) && >- (ctxKey.src.addr.ipv4_aligned == entryKey.src.addr.ipv4_aligned) && >- (ctxKey.src.port == entryKey.src.port) && >- (ctxKey.dst.addr.ipv4 == entryKey.dst.addr.ipv4) && >- (ctxKey.dst.addr.ipv4_aligned == entryKey.dst.addr.ipv4_aligned) && >- (ctxKey.dst.port == entryKey.dst.port) && >- (ctxKey.dl_type == entryKey.dl_type) && >- (ctxKey.nw_proto == entryKey.nw_proto) && >- (ctxKey.zone == entryKey.zone)); >+ return ((NdisEqualMemory(&ctxKey.src, &entryKey.src, >+ sizeof(struct ct_endpoint))) && >+ (NdisEqualMemory(&ctxKey.dst, &entryKey.dst, >+ sizeof(struct ct_endpoint))) && >+ (ctxKey.dl_type == entryKey.dl_type) && >+ (ctxKey.nw_proto == entryKey.nw_proto) && >+ (ctxKey.zone == entryKey.zone)); > } > > static __inline VOID >-- >2.9.3.windows.1 > >_______________________________________________ >dev mailing list >dev@openvswitch.org >https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=RcKP7MCbcn9PTpNRinYsO9Q0x4MrsrHutzBQ0hweH2M&s=7sFDQthiIL809LnOAfhczTX9Lk7iZDvnFhR77N4DfbE&e=
Thanks for the review.
I will split up the patch and send out Conntract-Nat.c changes in a separate patch.
Regards,
Anand Kumar
On 6/23/17, 1:50 PM, "Sairam Venugopal" <vsairam@vmware.com> wrote:
Hi Anand,
Can you split this patch up instead to handle Conntrack.c and Conntrack-Nat.c in a separate one? This way we can keep the testing isolated to the changes.
Acking Conntrack.c change:
Acked-by: Sairam Venugopal <vsairam@vmware.com>
On 6/22/17, 11:54 AM, "ovs-dev-bounces@openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of kumaranand@vmware.com> wrote:
>In conntrack lookup, ICMP type and code fields were not being used to
>determine a matching entry. As a result, ICMP4_ECHO_REQUEST packet could
>be tracked as ICMP4_ECHO_REPLY packet and vice versa, which is invalid.
>
>To fix this, add ICMP type and code fields for matching a conntrack entry.
>Also updated OvsNatKeyAreSame() and OvsHashNatKey() to include these two
>fields.
>
>Signed-off-by: Anand Kumar <kumaranand@vmware.com>
>---
> datapath-windows/ovsext/Conntrack-nat.c | 32 ++++++++++----------------------
> datapath-windows/ovsext/Conntrack.c | 16 +++++++---------
> 2 files changed, 17 insertions(+), 31 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath-windows/ovsext/Conntrack-nat.c
>index ae6b92c..f3c282c 100644
>--- a/datapath-windows/ovsext/Conntrack-nat.c
>+++ b/datapath-windows/ovsext/Conntrack-nat.c
>@@ -13,17 +13,11 @@ PLIST_ENTRY ovsUnNatTable = NULL;
> static __inline UINT32
> OvsHashNatKey(const OVS_CT_KEY *key)
> {
>- UINT32 hash = 0;
>-#define HASH_ADD(field) \
>- hash = OvsJhashBytes(&key->field, sizeof(key->field), hash)
>-
>- HASH_ADD(src.addr.ipv4_aligned);
>- HASH_ADD(dst.addr.ipv4_aligned);
>- HASH_ADD(src.port);
>- HASH_ADD(dst.port);
>- HASH_ADD(zone);
>-#undef HASH_ADD
>- return hash;
>+ UINT32 hash[3];
>+ hash[0] = OvsJhashBytes(&key->src, sizeof(key->src), 0);
>+ hash[1] = OvsJhashBytes(&key->dst, sizeof(key->dst), 0);
>+ hash[2] = OvsJhashBytes(&key->zone, sizeof(key->zone), 0);
>+ return OvsJhashWords(hash, 3, OVS_HASH_BASIS);
> }
>
> /*
>@@ -35,17 +29,11 @@ OvsHashNatKey(const OVS_CT_KEY *key)
> static __inline BOOLEAN
> OvsNatKeyAreSame(const OVS_CT_KEY *key1, const OVS_CT_KEY *key2)
> {
>- // XXX: Compare IPv6 key as well
>-#define FIELD_COMPARE(field) \
>- if (key1->field != key2->field) return FALSE
>-
>- FIELD_COMPARE(src.addr.ipv4_aligned);
>- FIELD_COMPARE(dst.addr.ipv4_aligned);
>- FIELD_COMPARE(src.port);
>- FIELD_COMPARE(dst.port);
>- FIELD_COMPARE(zone);
>- return TRUE;
>-#undef FIELD_COMPARE
>+ return ((NdisEqualMemory(&key1->src, &key2->src,
>+ sizeof(struct ct_endpoint))) &&
>+ (NdisEqualMemory(&key1->dst, &key2->dst,
>+ sizeof(struct ct_endpoint))) &&
>+ (key1->zone == key2->zone));
> }
>
> /*
>diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
>index 07a9583..e97d6ce 100644
>--- a/datapath-windows/ovsext/Conntrack.c
>+++ b/datapath-windows/ovsext/Conntrack.c
>@@ -383,15 +383,13 @@ OvsDetectCtPacket(OvsForwardingContext *fwdCtx,
> BOOLEAN
> OvsCtKeyAreSame(OVS_CT_KEY ctxKey, OVS_CT_KEY entryKey)
> {
>- return ((ctxKey.src.addr.ipv4 == entryKey.src.addr.ipv4) &&
>- (ctxKey.src.addr.ipv4_aligned == entryKey.src.addr.ipv4_aligned) &&
>- (ctxKey.src.port == entryKey.src.port) &&
>- (ctxKey.dst.addr.ipv4 == entryKey.dst.addr.ipv4) &&
>- (ctxKey.dst.addr.ipv4_aligned == entryKey.dst.addr.ipv4_aligned) &&
>- (ctxKey.dst.port == entryKey.dst.port) &&
>- (ctxKey.dl_type == entryKey.dl_type) &&
>- (ctxKey.nw_proto == entryKey.nw_proto) &&
>- (ctxKey.zone == entryKey.zone));
>+ return ((NdisEqualMemory(&ctxKey.src, &entryKey.src,
>+ sizeof(struct ct_endpoint))) &&
>+ (NdisEqualMemory(&ctxKey.dst, &entryKey.dst,
>+ sizeof(struct ct_endpoint))) &&
>+ (ctxKey.dl_type == entryKey.dl_type) &&
>+ (ctxKey.nw_proto == entryKey.nw_proto) &&
>+ (ctxKey.zone == entryKey.zone));
> }
>
> static __inline VOID
>--
>2.9.3.windows.1
>
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=RcKP7MCbcn9PTpNRinYsO9Q0x4MrsrHutzBQ0hweH2M&s=7sFDQthiIL809LnOAfhczTX9Lk7iZDvnFhR77N4DfbE&e=
diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath-windows/ovsext/Conntrack-nat.c index ae6b92c..f3c282c 100644 --- a/datapath-windows/ovsext/Conntrack-nat.c +++ b/datapath-windows/ovsext/Conntrack-nat.c @@ -13,17 +13,11 @@ PLIST_ENTRY ovsUnNatTable = NULL; static __inline UINT32 OvsHashNatKey(const OVS_CT_KEY *key) { - UINT32 hash = 0; -#define HASH_ADD(field) \ - hash = OvsJhashBytes(&key->field, sizeof(key->field), hash) - - HASH_ADD(src.addr.ipv4_aligned); - HASH_ADD(dst.addr.ipv4_aligned); - HASH_ADD(src.port); - HASH_ADD(dst.port); - HASH_ADD(zone); -#undef HASH_ADD - return hash; + UINT32 hash[3]; + hash[0] = OvsJhashBytes(&key->src, sizeof(key->src), 0); + hash[1] = OvsJhashBytes(&key->dst, sizeof(key->dst), 0); + hash[2] = OvsJhashBytes(&key->zone, sizeof(key->zone), 0); + return OvsJhashWords(hash, 3, OVS_HASH_BASIS); } /* @@ -35,17 +29,11 @@ OvsHashNatKey(const OVS_CT_KEY *key) static __inline BOOLEAN OvsNatKeyAreSame(const OVS_CT_KEY *key1, const OVS_CT_KEY *key2) { - // XXX: Compare IPv6 key as well -#define FIELD_COMPARE(field) \ - if (key1->field != key2->field) return FALSE - - FIELD_COMPARE(src.addr.ipv4_aligned); - FIELD_COMPARE(dst.addr.ipv4_aligned); - FIELD_COMPARE(src.port); - FIELD_COMPARE(dst.port); - FIELD_COMPARE(zone); - return TRUE; -#undef FIELD_COMPARE + return ((NdisEqualMemory(&key1->src, &key2->src, + sizeof(struct ct_endpoint))) && + (NdisEqualMemory(&key1->dst, &key2->dst, + sizeof(struct ct_endpoint))) && + (key1->zone == key2->zone)); } /* diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c index 07a9583..e97d6ce 100644 --- a/datapath-windows/ovsext/Conntrack.c +++ b/datapath-windows/ovsext/Conntrack.c @@ -383,15 +383,13 @@ OvsDetectCtPacket(OvsForwardingContext *fwdCtx, BOOLEAN OvsCtKeyAreSame(OVS_CT_KEY ctxKey, OVS_CT_KEY entryKey) { - return ((ctxKey.src.addr.ipv4 == entryKey.src.addr.ipv4) && - (ctxKey.src.addr.ipv4_aligned == entryKey.src.addr.ipv4_aligned) && - (ctxKey.src.port == entryKey.src.port) && - (ctxKey.dst.addr.ipv4 == entryKey.dst.addr.ipv4) && - (ctxKey.dst.addr.ipv4_aligned == entryKey.dst.addr.ipv4_aligned) && - (ctxKey.dst.port == entryKey.dst.port) && - (ctxKey.dl_type == entryKey.dl_type) && - (ctxKey.nw_proto == entryKey.nw_proto) && - (ctxKey.zone == entryKey.zone)); + return ((NdisEqualMemory(&ctxKey.src, &entryKey.src, + sizeof(struct ct_endpoint))) && + (NdisEqualMemory(&ctxKey.dst, &entryKey.dst, + sizeof(struct ct_endpoint))) && + (ctxKey.dl_type == entryKey.dl_type) && + (ctxKey.nw_proto == entryKey.nw_proto) && + (ctxKey.zone == entryKey.zone)); } static __inline VOID
In conntrack lookup, ICMP type and code fields were not being used to determine a matching entry. As a result, ICMP4_ECHO_REQUEST packet could be tracked as ICMP4_ECHO_REPLY packet and vice versa, which is invalid. To fix this, add ICMP type and code fields for matching a conntrack entry. Also updated OvsNatKeyAreSame() and OvsHashNatKey() to include these two fields. Signed-off-by: Anand Kumar <kumaranand@vmware.com> --- datapath-windows/ovsext/Conntrack-nat.c | 32 ++++++++++---------------------- datapath-windows/ovsext/Conntrack.c | 16 +++++++--------- 2 files changed, 17 insertions(+), 31 deletions(-)