[ovs-dev] [PATCH] datapath-windows: Include ICMP type and code fields to find a matching ct entry

[Anand Kumar]

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 at 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 at vmware.com>
    
    
    
    
    On 6/22/17, 11:54 AM, "ovs-dev-bounces at openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces at openvswitch.org on behalf of kumaranand at 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 at 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 at 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=