diff mbox

[ovs-dev] datapath-windows: Do not modify port field for ICMP during SNAT/DNAT

Message ID 20170811035902.4512-1-kumaranand@vmware.com
State Superseded
Headers show

Commit Message

Anand Kumar Aug. 11, 2017, 3:59 a.m. UTC 
During SNAT/DNAT, we should not be updating the port field of ct_endpoint
struct, as ICMP packets do not have port information. Since port and
icmp_id are overlapped in ct_endpoint struct, icmp_id gets changed.
As a result, NAT look up fails to find a matching entry.

This patch addresses this issue by not modifying icmp_id field during
SNAT/DNAT only for ICMP traffic

The current NAT module doesn't take the ICMP type/id/code into account
during the lookups. Fix this to make it similar with the other conntrack
module.

Signed-off-by: Anand Kumar <kumaranand@vmware.com>
---
 datapath-windows/ovsext/Conntrack-nat.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

Comments

Shashank Ram Aug. 11, 2017, 4:39 a.m. UTC | #1
Sairam Venugopal Aug. 11, 2017, 9:36 p.m. UTC | #2 
Acked-by: Sairam Venugopal <vsairam@vmware.com>




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

>During SNAT/DNAT, we should not be updating the port field of ct_endpoint
>struct, as ICMP packets do not have port information. Since port and
>icmp_id are overlapped in ct_endpoint struct, icmp_id gets changed.
>As a result, NAT look up fails to find a matching entry.
>
>This patch addresses this issue by not modifying icmp_id field during
>SNAT/DNAT only for ICMP traffic
>
>The current NAT module doesn't take the ICMP type/id/code into account
>during the lookups. Fix this to make it similar with the other conntrack
>module.
>
>Signed-off-by: Anand Kumar <kumaranand@vmware.com>
>---
> datapath-windows/ovsext/Conntrack-nat.c | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath-windows/ovsext/Conntrack-nat.c
>index ae6b92c..eb6f9db 100644
>--- a/datapath-windows/ovsext/Conntrack-nat.c
>+++ b/datapath-windows/ovsext/Conntrack-nat.c
>@@ -22,6 +22,12 @@ OvsHashNatKey(const OVS_CT_KEY *key)
>     HASH_ADD(src.port);
>     HASH_ADD(dst.port);
>     HASH_ADD(zone);
>+    /* icmp_id and port overlap in the union */
>+    HASH_ADD(src.icmp_type);
>+    HASH_ADD(dst.icmp_type);
>+    HASH_ADD(src.icmp_code);
>+    HASH_ADD(dst.icmp_code);
>+
> #undef HASH_ADD
>     return hash;
> }
>@@ -44,6 +50,12 @@ OvsNatKeyAreSame(const OVS_CT_KEY *key1, const OVS_CT_KEY *key2)
>     FIELD_COMPARE(src.port);
>     FIELD_COMPARE(dst.port);
>     FIELD_COMPARE(zone);
>+    FIELD_COMPARE(src.icmp_id);
>+    FIELD_COMPARE(dst.icmp_id);
>+    FIELD_COMPARE(src.icmp_type);
>+    FIELD_COMPARE(dst.icmp_type);
>+    FIELD_COMPARE(src.icmp_code);
>+    FIELD_COMPARE(dst.icmp_code);
>     return TRUE;
> #undef FIELD_COMPARE
> }
>@@ -253,6 +265,7 @@ OvsNatAddEntry(OVS_NAT_ENTRY* entry)
>  *     Update an Conntrack entry with NAT information. Translated address and
>  *     port will be generated and write back to the conntrack entry as a
>  *     result.
>+ *     Note: For ICMP, only address is translated.
>  *----------------------------------------------------------------------------
>  */
> BOOLEAN
>@@ -271,7 +284,7 @@ OvsNatTranslateCtEntry(OVS_CT_ENTRY *entry)
>     BOOLEAN allPortsTried;
>     BOOLEAN originalPortsTried;
>     struct ct_addr firstAddr;
>-    
>+
>     uint32_t hash = OvsNatHashRange(entry, 0);
> 
>     if ((entry->natInfo.natAction & NAT_ACTION_SRC) &&
>@@ -310,10 +323,14 @@ OvsNatTranslateCtEntry(OVS_CT_ENTRY *entry)
>     for (;;) {
>         if (entry->natInfo.natAction & NAT_ACTION_SRC) {
>             entry->rev_key.dst.addr = ctAddr;
>-            entry->rev_key.dst.port = htons(port);
>+            if (entry->rev_key.nw_proto != IPPROTO_ICMP) {
>+                entry->rev_key.dst.port = htons(port);
>+            }
>         } else {
>             entry->rev_key.src.addr = ctAddr;
>-            entry->rev_key.src.port = htons(port);
>+            if (entry->rev_key.nw_proto != IPPROTO_ICMP) {
>+                entry->rev_key.src.port = htons(port);
>+            }
>         }
> 
>         OVS_NAT_ENTRY *natEntry = OvsNatLookup(&entry->rev_key, TRUE);
>-- 
>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=DsmJOWCzvx6z5N9EJRhI-qvjbQUILl-ehTn-JLw4RZQ&s=NPGcReKGVlLHh2rvCJh0r8h-l38sSxpm8V-uPaVe9sY&e=
Alin Serdean Aug. 15, 2017, 3:14 a.m. UTC | #3 
We should revisit how we do hashes and compares over the 'OVS_CT_KEY' at some point.

As you pointed "/* icmp_id and port overlap in the union */"
You can drop the lines:
>      HASH_ADD(src.port);
>      HASH_ADD(dst.port);
And
>      FIELD_COMPARE(src.port);
>      FIELD_COMPARE(dst.port);
the outcome should be the same.

Everything else looks good.
Thanks,
Alin.

> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> bounces@openvswitch.org] On Behalf Of Anand Kumar
> Sent: Friday, August 11, 2017 6:59 AM
> To: dev@openvswitch.org
> Subject: [ovs-dev] [PATCH] datapath-windows: Do not modify port field for
> ICMP during SNAT/DNAT
> 
> During SNAT/DNAT, we should not be updating the port field of ct_endpoint
> struct, as ICMP packets do not have port information. Since port and icmp_id
> are overlapped in ct_endpoint struct, icmp_id gets changed.
> As a result, NAT look up fails to find a matching entry.
> 
> This patch addresses this issue by not modifying icmp_id field during
> SNAT/DNAT only for ICMP traffic
> 
> The current NAT module doesn't take the ICMP type/id/code into account
> during the lookups. Fix this to make it similar with the other conntrack
> module.
> 
> Signed-off-by: Anand Kumar <kumaranand@vmware.com>
> ---
>  datapath-windows/ovsext/Conntrack-nat.c | 23 ++++++++++++++++++++--
> -
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath-
> windows/ovsext/Conntrack-nat.c
> index ae6b92c..eb6f9db 100644
> --- a/datapath-windows/ovsext/Conntrack-nat.c
> +++ b/datapath-windows/ovsext/Conntrack-nat.c
> @@ -22,6 +22,12 @@ OvsHashNatKey(const OVS_CT_KEY *key)
>      HASH_ADD(src.port);
>      HASH_ADD(dst.port);
>      HASH_ADD(zone);
> +    /* icmp_id and port overlap in the union */
> +    HASH_ADD(src.icmp_type);
> +    HASH_ADD(dst.icmp_type);
> +    HASH_ADD(src.icmp_code);
> +    HASH_ADD(dst.icmp_code);
> +
>  #undef HASH_ADD
>      return hash;
>  }
> @@ -44,6 +50,12 @@ OvsNatKeyAreSame(const OVS_CT_KEY *key1, const
> OVS_CT_KEY *key2)
>      FIELD_COMPARE(src.port);
>      FIELD_COMPARE(dst.port);
>      FIELD_COMPARE(zone);
> +    FIELD_COMPARE(src.icmp_id);
> +    FIELD_COMPARE(dst.icmp_id);
> +    FIELD_COMPARE(src.icmp_type);
> +    FIELD_COMPARE(dst.icmp_type);
> +    FIELD_COMPARE(src.icmp_code);
> +    FIELD_COMPARE(dst.icmp_code);
>      return TRUE;
>  #undef FIELD_COMPARE
>  }
> @@ -253,6 +265,7 @@ OvsNatAddEntry(OVS_NAT_ENTRY* entry)
>   *     Update an Conntrack entry with NAT information. Translated address
> and
>   *     port will be generated and write back to the conntrack entry as a
>   *     result.
> + *     Note: For ICMP, only address is translated.
>   *----------------------------------------------------------------------------
>   */
>  BOOLEAN
> @@ -271,7 +284,7 @@ OvsNatTranslateCtEntry(OVS_CT_ENTRY *entry)
>      BOOLEAN allPortsTried;
>      BOOLEAN originalPortsTried;
>      struct ct_addr firstAddr;
> -
> +
>      uint32_t hash = OvsNatHashRange(entry, 0);
> 
>      if ((entry->natInfo.natAction & NAT_ACTION_SRC) && @@ -310,10
> +323,14 @@ OvsNatTranslateCtEntry(OVS_CT_ENTRY *entry)
>      for (;;) {
>          if (entry->natInfo.natAction & NAT_ACTION_SRC) {
>              entry->rev_key.dst.addr = ctAddr;
> -            entry->rev_key.dst.port = htons(port);
> +            if (entry->rev_key.nw_proto != IPPROTO_ICMP) {
> +                entry->rev_key.dst.port = htons(port);
> +            }
>          } else {
>              entry->rev_key.src.addr = ctAddr;
> -            entry->rev_key.src.port = htons(port);
> +            if (entry->rev_key.nw_proto != IPPROTO_ICMP) {
> +                entry->rev_key.src.port = htons(port);
> +            }
>          }
> 
>          OVS_NAT_ENTRY *natEntry = OvsNatLookup(&entry->rev_key, TRUE);
> --
> 2.9.3.windows.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Anand Kumar Aug. 15, 2017, 10:19 p.m. UTC | #4 
Hi Alin,

Thanks for reviewing the patch.  Please find my responses inline.
Will send out a v2 patch addressing the review comments.

Thanks,
Anand Kumar

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

    We should revisit how we do hashes and compares over the 'OVS_CT_KEY' at some point.
    
    As you pointed "/* icmp_id and port overlap in the union */"
    You can drop the lines:
    >      HASH_ADD(src.port);
    >      HASH_ADD(dst.port);
   [Anand Kumar] : We would still need it here since ICMP id is not included for computing hash.
    And
    >      FIELD_COMPARE(src.port);
    >      FIELD_COMPARE(dst.port);
    the outcome should be the same.
  [Anand Kumar] : Not required, will remove icmp_id field instead of port.
           FIELD_COMPARE(src.icmp_id);
           FIELD_COMPARE(dst.icmp_id);

    
    Everything else looks good.
    Thanks,
    Alin.
    
    > -----Original Message-----
    > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
    > bounces@openvswitch.org] On Behalf Of Anand Kumar
    > Sent: Friday, August 11, 2017 6:59 AM
    > To: dev@openvswitch.org
    > Subject: [ovs-dev] [PATCH] datapath-windows: Do not modify port field for
    > ICMP during SNAT/DNAT
    > 
    > During SNAT/DNAT, we should not be updating the port field of ct_endpoint
    > struct, as ICMP packets do not have port information. Since port and icmp_id
    > are overlapped in ct_endpoint struct, icmp_id gets changed.
    > As a result, NAT look up fails to find a matching entry.
    > 
    > This patch addresses this issue by not modifying icmp_id field during
    > SNAT/DNAT only for ICMP traffic
    > 
    > The current NAT module doesn't take the ICMP type/id/code into account
    > during the lookups. Fix this to make it similar with the other conntrack
    > module.
    > 
    > Signed-off-by: Anand Kumar <kumaranand@vmware.com>
    > ---
    >  datapath-windows/ovsext/Conntrack-nat.c | 23 ++++++++++++++++++++--
    > -
    >  1 file changed, 20 insertions(+), 3 deletions(-)
    > 
    > diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath-
    > windows/ovsext/Conntrack-nat.c
    > index ae6b92c..eb6f9db 100644
    > --- a/datapath-windows/ovsext/Conntrack-nat.c
    > +++ b/datapath-windows/ovsext/Conntrack-nat.c
    > @@ -22,6 +22,12 @@ OvsHashNatKey(const OVS_CT_KEY *key)
    >      HASH_ADD(src.port);
    >      HASH_ADD(dst.port);
    >      HASH_ADD(zone);
    > +    /* icmp_id and port overlap in the union */
    > +    HASH_ADD(src.icmp_type);
    > +    HASH_ADD(dst.icmp_type);
    > +    HASH_ADD(src.icmp_code);
    > +    HASH_ADD(dst.icmp_code);
    > +
    >  #undef HASH_ADD
    >      return hash;
    >  }
    > @@ -44,6 +50,12 @@ OvsNatKeyAreSame(const OVS_CT_KEY *key1, const
    > OVS_CT_KEY *key2)
    >      FIELD_COMPARE(src.port);
    >      FIELD_COMPARE(dst.port);
    >      FIELD_COMPARE(zone);
    > +    FIELD_COMPARE(src.icmp_id);
    > +    FIELD_COMPARE(dst.icmp_id);
    > +    FIELD_COMPARE(src.icmp_type);
    > +    FIELD_COMPARE(dst.icmp_type);
    > +    FIELD_COMPARE(src.icmp_code);
    > +    FIELD_COMPARE(dst.icmp_code);
    >      return TRUE;
    >  #undef FIELD_COMPARE
    >  }
    > @@ -253,6 +265,7 @@ OvsNatAddEntry(OVS_NAT_ENTRY* entry)
    >   *     Update an Conntrack entry with NAT information. Translated address
    > and
    >   *     port will be generated and write back to the conntrack entry as a
    >   *     result.
    > + *     Note: For ICMP, only address is translated.
    >   *----------------------------------------------------------------------------
    >   */
    >  BOOLEAN
    > @@ -271,7 +284,7 @@ OvsNatTranslateCtEntry(OVS_CT_ENTRY *entry)
    >      BOOLEAN allPortsTried;
    >      BOOLEAN originalPortsTried;
    >      struct ct_addr firstAddr;
    > -
    > +
    >      uint32_t hash = OvsNatHashRange(entry, 0);
    > 
    >      if ((entry->natInfo.natAction & NAT_ACTION_SRC) && @@ -310,10
    > +323,14 @@ OvsNatTranslateCtEntry(OVS_CT_ENTRY *entry)
    >      for (;;) {
    >          if (entry->natInfo.natAction & NAT_ACTION_SRC) {
    >              entry->rev_key.dst.addr = ctAddr;
    > -            entry->rev_key.dst.port = htons(port);
    > +            if (entry->rev_key.nw_proto != IPPROTO_ICMP) {
    > +                entry->rev_key.dst.port = htons(port);
    > +            }
    >          } else {
    >              entry->rev_key.src.addr = ctAddr;
    > -            entry->rev_key.src.port = htons(port);
    > +            if (entry->rev_key.nw_proto != IPPROTO_ICMP) {
    > +                entry->rev_key.src.port = htons(port);
    > +            }
    >          }
    > 
    >          OVS_NAT_ENTRY *natEntry = OvsNatLookup(&entry->rev_key, TRUE);
    > --
    > 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=DwIFAg&c=uilaK90D4TOVoH58JNXRgQ&r=Q5z9tBe-nAOpE7LIHSPV8uy5-437agMXvkeHHMkR8Us&m=wfdj2OXBLKbF3522VGnqdqJe_orJVBbMTQpVIT3ccV8&s=tlFFROWZGldsFcJ65VVeac49_xLylav_QLgUwG4uBRg&e=
diff mbox

Patch

diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath-windows/ovsext/Conntrack-nat.c
index ae6b92c..eb6f9db 100644
--- a/datapath-windows/ovsext/Conntrack-nat.c
+++ b/datapath-windows/ovsext/Conntrack-nat.c
@@ -22,6 +22,12 @@  OvsHashNatKey(const OVS_CT_KEY *key)
     HASH_ADD(src.port);
     HASH_ADD(dst.port);
     HASH_ADD(zone);
+    /* icmp_id and port overlap in the union */
+    HASH_ADD(src.icmp_type);
+    HASH_ADD(dst.icmp_type);
+    HASH_ADD(src.icmp_code);
+    HASH_ADD(dst.icmp_code);
+
 #undef HASH_ADD
     return hash;
 }
@@ -44,6 +50,12 @@  OvsNatKeyAreSame(const OVS_CT_KEY *key1, const OVS_CT_KEY *key2)
     FIELD_COMPARE(src.port);
     FIELD_COMPARE(dst.port);
     FIELD_COMPARE(zone);
+    FIELD_COMPARE(src.icmp_id);
+    FIELD_COMPARE(dst.icmp_id);
+    FIELD_COMPARE(src.icmp_type);
+    FIELD_COMPARE(dst.icmp_type);
+    FIELD_COMPARE(src.icmp_code);
+    FIELD_COMPARE(dst.icmp_code);
     return TRUE;
 #undef FIELD_COMPARE
 }
@@ -253,6 +265,7 @@  OvsNatAddEntry(OVS_NAT_ENTRY* entry)
  *     Update an Conntrack entry with NAT information. Translated address and
  *     port will be generated and write back to the conntrack entry as a
  *     result.
+ *     Note: For ICMP, only address is translated.
  *----------------------------------------------------------------------------
  */
 BOOLEAN
@@ -271,7 +284,7 @@  OvsNatTranslateCtEntry(OVS_CT_ENTRY *entry)
     BOOLEAN allPortsTried;
     BOOLEAN originalPortsTried;
     struct ct_addr firstAddr;
-    
+
     uint32_t hash = OvsNatHashRange(entry, 0);
 
     if ((entry->natInfo.natAction & NAT_ACTION_SRC) &&
@@ -310,10 +323,14 @@  OvsNatTranslateCtEntry(OVS_CT_ENTRY *entry)
     for (;;) {
         if (entry->natInfo.natAction & NAT_ACTION_SRC) {
             entry->rev_key.dst.addr = ctAddr;
-            entry->rev_key.dst.port = htons(port);
+            if (entry->rev_key.nw_proto != IPPROTO_ICMP) {
+                entry->rev_key.dst.port = htons(port);
+            }
         } else {
             entry->rev_key.src.addr = ctAddr;
-            entry->rev_key.src.port = htons(port);
+            if (entry->rev_key.nw_proto != IPPROTO_ICMP) {
+                entry->rev_key.src.port = htons(port);
+            }
         }
 
         OVS_NAT_ENTRY *natEntry = OvsNatLookup(&entry->rev_key, TRUE);