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

[Anand Kumar]

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" <aserd...@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-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Anand Kumar
> Sent: Friday, August 11, 2017 6:59 AM
    > To: d...@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 <kumaran...@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) {
> +  

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

[Alin Serdean]

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-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Anand Kumar
> Sent: Friday, August 11, 2017 6:59 AM
> To: d...@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 <kumaran...@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(>rev_key, TRUE);
> --
> 2.9.3.windows.1
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Sairam Venugopal]

Acked-by: Sairam Venugopal 




On 8/10/17, 8:59 PM, "ovs-dev-boun...@openvswitch.org on behalf of Anand Kumar" 
 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 
>---
> 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(>rev_key, TRUE);
>-- 
>2.9.3.windows.1
>
>___
>dev mailing list
>d...@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo=DsmJOWCzvx6z5N9EJRhI-qvjbQUILl-ehTn-JLw4RZQ=NPGcReKGVlLHh2rvCJh0r8h-l38sSxpm8V-uPaVe9sY=
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Shashank Ram]

From: ovs-dev-boun...@openvswitch.org <ovs-dev-boun...@openvswitch.org> on 
behalf of Anand Kumar <kumaran...@vmware.com>
Sent: Thursday, August 10, 2017 8:59 PM
To: d...@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 <kumaran...@vmware.com>
___

Acked-by: Shashank Ram <r...@vmware.com>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Anand Kumar]

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 
---
 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(>rev_key, TRUE);
-- 
2.9.3.windows.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev