[ovs-dev] datapath-windows: Update ICMP-Type and Code comparison in CT lookup

Submitted by Anand Kumar on Aug. 11, 2017, 8:41 p.m.

Details

Message ID 20170811204154.4372-1-kumaranand@vmware.com
State New
Headers show

Commit Message

Anand Kumar Aug. 11, 2017, 8:41 p.m.
- Update the CT comparison function to compare individual fields instead of
NdisEqualMemory.
- Add in some padding for the ct_endpoint's union.
- Update the Orig Tuple to use ICMP Type and Code instead of Port for ICMP

Co-authored-by: Sairam Venugopal <vsairam@vmware.com>
Signed-off-by: Anand Kumar <kumaranand@vmware.com>
---
 datapath-windows/ovsext/Conntrack.c | 27 +++++++++++++++++++++------
 datapath-windows/ovsext/Conntrack.h |  5 ++++-
 2 files changed, 25 insertions(+), 7 deletions(-)

Comments

Sairam Venugopal Aug. 11, 2017, 9:36 p.m.
Acked-by: Sairam Venugopal <vsairam@vmware.com>





On 8/11/17, 1:41 PM, "ovs-dev-bounces@openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of kumaranand@vmware.com> wrote:

> - Update the CT comparison function to compare individual fields instead of
>NdisEqualMemory.
>- Add in some padding for the ct_endpoint's union.
>- Update the Orig Tuple to use ICMP Type and Code instead of Port for ICMP
>
>Co-authored-by: Sairam Venugopal <vsairam@vmware.com>
>Signed-off-by: Anand Kumar <kumaranand@vmware.com>
>---
> datapath-windows/ovsext/Conntrack.c | 27 +++++++++++++++++++++------
> datapath-windows/ovsext/Conntrack.h |  5 ++++-
> 2 files changed, 25 insertions(+), 7 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
>index 917ebee..81c2167 100644
>--- a/datapath-windows/ovsext/Conntrack.c
>+++ b/datapath-windows/ovsext/Conntrack.c
>@@ -373,10 +373,18 @@ OvsDetectCtPacket(OvsForwardingContext *fwdCtx,
> BOOLEAN
> OvsCtKeyAreSame(OVS_CT_KEY ctxKey, OVS_CT_KEY entryKey)
> {
>-    return ((NdisEqualMemory(&ctxKey.src, &entryKey.src,
>-                             sizeof(struct ct_endpoint))) &&
>-            (NdisEqualMemory(&ctxKey.dst, &entryKey.dst,
>-                             sizeof(struct ct_endpoint))) &&
>+    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.src.icmp_id == entryKey.src.icmp_id) &&
>+            (ctxKey.src.icmp_type == entryKey.src.icmp_type) &&
>+            (ctxKey.src.icmp_code == entryKey.src.icmp_code) &&
>+            (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.dst.icmp_id == entryKey.dst.icmp_id) &&
>+            (ctxKey.dst.icmp_type == entryKey.dst.icmp_type) &&
>+            (ctxKey.dst.icmp_code == entryKey.dst.icmp_code) &&
>             (ctxKey.dl_type == entryKey.dl_type) &&
>             (ctxKey.nw_proto == entryKey.nw_proto) &&
>             (ctxKey.zone == entryKey.zone));
>@@ -782,9 +790,16 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx,
> 
>         key->ct.tuple_ipv4.ipv4_src = ctKey->src.addr.ipv4_aligned;
>         key->ct.tuple_ipv4.ipv4_dst = ctKey->dst.addr.ipv4_aligned;
>-        key->ct.tuple_ipv4.src_port = ctKey->src.port;
>-        key->ct.tuple_ipv4.dst_port = ctKey->dst.port;
>         key->ct.tuple_ipv4.ipv4_proto = ctKey->nw_proto;
>+
>+        /* Orig tuple Port is overloaded to take in ICMP-Type & Code */
>+        /* This mimics the behavior in lib/conntrack.c*/
>+        key->ct.tuple_ipv4.src_port = ctKey->nw_proto != IPPROTO_ICMP ?
>+                                      ctKey->src.port :
>+                                      htons(ctKey->src.icmp_type);
>+        key->ct.tuple_ipv4.dst_port = ctKey->nw_proto != IPPROTO_ICMP ?
>+                                      ctKey->dst.port :
>+                                      htons(ctKey->src.icmp_code);
>     }
> 
>     if (entryCreated && entry) {
>diff --git a/datapath-windows/ovsext/Conntrack.h b/datapath-windows/ovsext/Conntrack.h
>index 04ca299..4904c7e 100644
>--- a/datapath-windows/ovsext/Conntrack.h
>+++ b/datapath-windows/ovsext/Conntrack.h
>@@ -41,7 +41,10 @@ struct ct_addr {
> struct ct_endpoint {
>     struct ct_addr addr;
>     union {
>-        ovs_be16 port;
>+        struct {
>+            ovs_be16 port;
>+            uint16 pad_port;
>+        };
>         struct {
>             ovs_be16 icmp_id;
>             uint8_t icmp_type;
>-- 
>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=jv5eh3bLA-gmtkDuLhimROtMSTOYl817LE81oKgfKo8&s=zIJGnXCflvEaG9FWrbvkouqo9uSfZ-KoRq7H_GzPiMo&e=
Alin Serdean Aug. 15, 2017, 3:04 a.m.
Hi Sai and Anand,

Thanks a lot for the patch. I have a few questions regarding the approach. Please see inline.

> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> bounces@openvswitch.org] On Behalf Of Anand Kumar
> Sent: Friday, August 11, 2017 11:42 PM
> To: dev@openvswitch.org
> Subject: [ovs-dev] [PATCH] datapath-windows: Update ICMP-Type and Code
> comparison in CT lookup
> 
>  - Update the CT comparison function to compare individual fields instead of
> NdisEqualMemory.
[Alin Serdean] I don't like this change, especially mixing both members of union, i.e:
> +            (ctxKey.dst.port == entryKey.dst.port) &&
> +            (ctxKey.dst.icmp_id == entryKey.dst.icmp_id) &&
Why are you trying to change via member by member comparison?
> - Add in some padding for the ct_endpoint's union.
[Alin Serdean] Why is this needed? Another question do we still need the 'pad' member inside ct_endpoint (https://github.com/openvswitch/ovs/blob/master/datapath-windows/ovsext/Conntrack.h#L51) ?
> - Update the Orig Tuple to use ICMP Type and Code instead of Port for ICMP
[Alin Serdean] Agreed
> 
> Co-authored-by: Sairam Venugopal <vsairam@vmware.com>
> Signed-off-by: Anand Kumar <kumaranand@vmware.com>
Anand Kumar Aug. 15, 2017, 10:21 p.m.
Hi Alin,

Thanks for the review. Please find my responses inline.
Will address the review comments and send out v2 version of the patch.

Thanks,
Anand Kumar

On 8/14/17, 8:04 PM, "Alin Serdean" <aserdean@cloudbasesolutions.com> wrote:

    Hi Sai and Anand,
    
    Thanks a lot for the patch. I have a few questions regarding the approach. Please see inline.
    
    > -----Original Message-----
    > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
    > bounces@openvswitch.org] On Behalf Of Anand Kumar
    > Sent: Friday, August 11, 2017 11:42 PM
    > To: dev@openvswitch.org
    > Subject: [ovs-dev] [PATCH] datapath-windows: Update ICMP-Type and Code
    > comparison in CT lookup
    > 
    >  - Update the CT comparison function to compare individual fields instead of
    > NdisEqualMemory.
    [Alin Serdean] I don't like this change, especially mixing both members of union, i.e:
    > +            (ctxKey.dst.port == entryKey.dst.port) &&
    > +            (ctxKey.dst.icmp_id == entryKey.dst.icmp_id) &&
    Why are you trying to change via member by member comparison?
   [Anand Kumar]: Done. Previously, we ran into issues while verifying NAT with ICMP, which now is fixed with my other patch.
    > - Add in some padding for the ct_endpoint's union.
    [Alin Serdean] Why is this needed? 
    [Anand Kumar] This is needed because size of the union is 32 bits. 
   Another question do we still need the 'pad' member inside ct_endpoint (https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitch_ovs_blob_master_datapath-2Dwindows_ovsext_Conntrack.h-23L51&d=DwIFAg&c=uilaK90D4TOVoH58JNXRgQ&r=Q5z9tBe-nAOpE7LIHSPV8uy5-437agMXvkeHHMkR8Us&m=1Rj8RlVaAovz2iLrWHvT6N469qFN036rzjLkBJweZDw&s=hky8AVI9m8GjZOD8r21WYvEHBPcDcVVZiwMKqk07IwU&e= ) ?
    [Anand Kumar] Yes, the padding at the end of structure is no longer needed. I will address in next version.
    > - Update the Orig Tuple to use ICMP Type and Code instead of Port for ICMP
    [Alin Serdean] Agreed
    > 
    > Co-authored-by: Sairam Venugopal <vsairam@vmware.com>
    > Signed-off-by: Anand Kumar <kumaranand@vmware.com>

Patch hide | download patch | download mbox

diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
index 917ebee..81c2167 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -373,10 +373,18 @@  OvsDetectCtPacket(OvsForwardingContext *fwdCtx,
 BOOLEAN
 OvsCtKeyAreSame(OVS_CT_KEY ctxKey, OVS_CT_KEY entryKey)
 {
-    return ((NdisEqualMemory(&ctxKey.src, &entryKey.src,
-                             sizeof(struct ct_endpoint))) &&
-            (NdisEqualMemory(&ctxKey.dst, &entryKey.dst,
-                             sizeof(struct ct_endpoint))) &&
+    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.src.icmp_id == entryKey.src.icmp_id) &&
+            (ctxKey.src.icmp_type == entryKey.src.icmp_type) &&
+            (ctxKey.src.icmp_code == entryKey.src.icmp_code) &&
+            (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.dst.icmp_id == entryKey.dst.icmp_id) &&
+            (ctxKey.dst.icmp_type == entryKey.dst.icmp_type) &&
+            (ctxKey.dst.icmp_code == entryKey.dst.icmp_code) &&
             (ctxKey.dl_type == entryKey.dl_type) &&
             (ctxKey.nw_proto == entryKey.nw_proto) &&
             (ctxKey.zone == entryKey.zone));
@@ -782,9 +790,16 @@  OvsCtExecute_(OvsForwardingContext *fwdCtx,
 
         key->ct.tuple_ipv4.ipv4_src = ctKey->src.addr.ipv4_aligned;
         key->ct.tuple_ipv4.ipv4_dst = ctKey->dst.addr.ipv4_aligned;
-        key->ct.tuple_ipv4.src_port = ctKey->src.port;
-        key->ct.tuple_ipv4.dst_port = ctKey->dst.port;
         key->ct.tuple_ipv4.ipv4_proto = ctKey->nw_proto;
+
+        /* Orig tuple Port is overloaded to take in ICMP-Type & Code */
+        /* This mimics the behavior in lib/conntrack.c*/
+        key->ct.tuple_ipv4.src_port = ctKey->nw_proto != IPPROTO_ICMP ?
+                                      ctKey->src.port :
+                                      htons(ctKey->src.icmp_type);
+        key->ct.tuple_ipv4.dst_port = ctKey->nw_proto != IPPROTO_ICMP ?
+                                      ctKey->dst.port :
+                                      htons(ctKey->src.icmp_code);
     }
 
     if (entryCreated && entry) {
diff --git a/datapath-windows/ovsext/Conntrack.h b/datapath-windows/ovsext/Conntrack.h
index 04ca299..4904c7e 100644
--- a/datapath-windows/ovsext/Conntrack.h
+++ b/datapath-windows/ovsext/Conntrack.h
@@ -41,7 +41,10 @@  struct ct_addr {
 struct ct_endpoint {
     struct ct_addr addr;
     union {
-        ovs_be16 port;
+        struct {
+            ovs_be16 port;
+            uint16 pad_port;
+        };
         struct {
             ovs_be16 icmp_id;
             uint8_t icmp_type;