Re: [ovs-dev] [PATCH v5] tunnel: Allow UDP zero checksum with IPv6 tunnels.

2024-06-21 Thread Ilya Maximets 
On 3/27/24 16:55, Mike Pattrick wrote:
> This patch adopts the proposed RFC 6935 by allowing null UDP checksums
> even if the tunnel protocol is IPv6. This is already supported by Linux
> through the udp6zerocsumtx tunnel option. It is disabled by default and
> IPv6 tunnels are flagged as requiring a checksum, but this patch enables
> the user to set csum=false on IPv6 tunnels.
> 
> Signed-off-by: Mike Pattrick 
> ---
> v2: Changed documentation, and added a NEWS item
> v3: NEWS file merge conflict
> v4: Better comments, new test
> v5: Addressed identified nit's
> ---
>  NEWS  |  4 
>  lib/netdev-native-tnl.c   |  2 +-
>  lib/netdev-vport.c| 17 +++--
>  lib/netdev.h  | 18 +-
>  ofproto/tunnel.c  | 10 --
>  tests/tunnel-push-pop-ipv6.at |  9 +
>  tests/tunnel-push-pop.at  |  7 +++
>  tests/tunnel.at   |  2 +-
>  vswitchd/vswitch.xml  | 12 +---
>  9 files changed, 71 insertions(+), 10 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index c9e4064e6..6c8c4a2dc 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -4,6 +4,10 @@ Post-v3.3.0
>   * Conntrack now supports 'random' flag for selecting ports in a range
> while natting and 'persistent' flag for selection of the IP address
> from a range.
> + * IPv6 UDP tunnel encapsulation including Geneve and VXLAN will now
> +   honour the csum option.  Configuring the interface with
> +   "options:csum=false" now has the same effect as the udp6zerocsumtx
> +   option has with Linux kernel UDP tunnels.
>  
>  
>  v3.3.0 - 16 Feb 2024
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index dee9ab344..e8258bc4e 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -424,7 +424,7 @@ udp_build_header(const struct netdev_tunnel_config 
> *tnl_cfg,
>  udp = netdev_tnl_ip_build_header(data, params, IPPROTO_UDP, 0);
>  udp->udp_dst = tnl_cfg->dst_port;
>  
> -if (params->is_ipv6 || params->flow->tunnel.flags & FLOW_TNL_F_CSUM) {
> +if (params->flow->tunnel.flags & FLOW_TNL_F_CSUM) {
>  /* Write a value in now to mark that we should compute the checksum
>   * later. 0x is handy because it is transparent to the
>   * calculation. */
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 60caa02fb..234a4ebe1 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -702,7 +702,9 @@ set_tunnel_config(struct netdev *dev_, const struct smap 
> *args, char **errp)
>  tnl_cfg.dst_port = htons(atoi(node->value));
>  } else if (!strcmp(node->key, "csum") && has_csum) {
>  if (!strcmp(node->value, "true")) {
> -tnl_cfg.csum = true;
> +tnl_cfg.csum = NETDEV_TNL_CSUM_ENABLED;
> +} else if (!strcmp(node->value, "false")) {
> +tnl_cfg.csum = NETDEV_TNL_CSUM_DISABLED;
>  }
>  } else if (!strcmp(node->key, "seq") && has_seq) {
>  if (!strcmp(node->value, "true")) {
> @@ -850,6 +852,15 @@ set_tunnel_config(struct netdev *dev_, const struct smap 
> *args, char **errp)
>  }
>  }
>  
> +/* The default csum state for GRE is special as it does have an optional
> + * checksum but the default configuration isn't correlated with IP 
> version
> + * like UDP tunnels are.  Likewise, tunnels with no checksum at all must 
> be
> + * in this state. */
> +if (tnl_cfg.csum == NETDEV_TNL_CSUM_DEFAULT &&
> +(!has_csum || strstr(type, "gre"))) {
> +tnl_cfg.csum = NETDEV_TNL_DEFAULT_NO_CSUM;
> +}
> +
>  enum tunnel_layers layers = tunnel_supported_layers(type, _cfg);
>  const char *full_type = (strcmp(type, "vxlan") ? type
>   : (tnl_cfg.exts & (1 << OVS_VXLAN_EXT_GPE)
> @@ -1026,8 +1037,10 @@ get_tunnel_config(const struct netdev *dev, struct 
> smap *args)
>  }
>  }
>  
> -if (tnl_cfg->csum) {
> +if (tnl_cfg->csum == NETDEV_TNL_CSUM_ENABLED) {
>  smap_add(args, "csum", "true");
> +} else if (tnl_cfg->csum == NETDEV_TNL_CSUM_DISABLED) {
> +smap_add(args, "csum", "false");
>  }
>  
>  if (tnl_cfg->set_seq) {
> diff --git a/lib/netdev.h b/lib/netdev.h
> index 67a8486bd..5d253157c 100644
> --- a/lib/netdev.h
> +++ b/lib/netdev.h
> @@ -111,6 +111,22 @@ enum netdev_srv6_flowlabel {
>  SRV6_FLOWLABEL_COMPUTE,
>  };
>  
> +enum netdev_tnl_csum {
> +/* Default value for UDP tunnels if no configurations is present.  
> Enforce
> + * checksum calculation in IPv6 tunnels, disable in IPv4 tunnels. */
> +NETDEV_TNL_CSUM_DEFAULT = 0,
> +
> +/* Checksum explicitly to be calculated. */
> +NETDEV_TNL_CSUM_ENABLED,
> +
> +/* Checksum calculation explicitly disabled. */
> +NETDEV_TNL_CSUM_DISABLED,
> +
> +/* A value for when there is no checksum or the default value is no
> +

Re: [ovs-dev] [PATCH v5] tunnel: Allow UDP zero checksum with IPv6 tunnels.

2024-03-27 Thread Simon Horman 
On Wed, Mar 27, 2024 at 11:55:14AM -0400, Mike Pattrick wrote:
> This patch adopts the proposed RFC 6935 by allowing null UDP checksums
> even if the tunnel protocol is IPv6. This is already supported by Linux
> through the udp6zerocsumtx tunnel option. It is disabled by default and
> IPv6 tunnels are flagged as requiring a checksum, but this patch enables
> the user to set csum=false on IPv6 tunnels.
> 
> Signed-off-by: Mike Pattrick 
> ---
> v2: Changed documentation, and added a NEWS item
> v3: NEWS file merge conflict
> v4: Better comments, new test
> v5: Addressed identified nit's

Acked-by: Simon Horman 

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

[ovs-dev] [PATCH v5] tunnel: Allow UDP zero checksum with IPv6 tunnels.

2024-03-27 Thread Mike Pattrick 
This patch adopts the proposed RFC 6935 by allowing null UDP checksums
even if the tunnel protocol is IPv6. This is already supported by Linux
through the udp6zerocsumtx tunnel option. It is disabled by default and
IPv6 tunnels are flagged as requiring a checksum, but this patch enables
the user to set csum=false on IPv6 tunnels.

Signed-off-by: Mike Pattrick 
---
v2: Changed documentation, and added a NEWS item
v3: NEWS file merge conflict
v4: Better comments, new test
v5: Addressed identified nit's
---
 NEWS  |  4 
 lib/netdev-native-tnl.c   |  2 +-
 lib/netdev-vport.c| 17 +++--
 lib/netdev.h  | 18 +-
 ofproto/tunnel.c  | 10 --
 tests/tunnel-push-pop-ipv6.at |  9 +
 tests/tunnel-push-pop.at  |  7 +++
 tests/tunnel.at   |  2 +-
 vswitchd/vswitch.xml  | 12 +---
 9 files changed, 71 insertions(+), 10 deletions(-)

diff --git a/NEWS b/NEWS
index c9e4064e6..6c8c4a2dc 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,10 @@ Post-v3.3.0
  * Conntrack now supports 'random' flag for selecting ports in a range
while natting and 'persistent' flag for selection of the IP address
from a range.
+ * IPv6 UDP tunnel encapsulation including Geneve and VXLAN will now
+   honour the csum option.  Configuring the interface with
+   "options:csum=false" now has the same effect as the udp6zerocsumtx
+   option has with Linux kernel UDP tunnels.
 
 
 v3.3.0 - 16 Feb 2024
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
index dee9ab344..e8258bc4e 100644
--- a/lib/netdev-native-tnl.c
+++ b/lib/netdev-native-tnl.c
@@ -424,7 +424,7 @@ udp_build_header(const struct netdev_tunnel_config *tnl_cfg,
 udp = netdev_tnl_ip_build_header(data, params, IPPROTO_UDP, 0);
 udp->udp_dst = tnl_cfg->dst_port;
 
-if (params->is_ipv6 || params->flow->tunnel.flags & FLOW_TNL_F_CSUM) {
+if (params->flow->tunnel.flags & FLOW_TNL_F_CSUM) {
 /* Write a value in now to mark that we should compute the checksum
  * later. 0x is handy because it is transparent to the
  * calculation. */
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index 60caa02fb..234a4ebe1 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -702,7 +702,9 @@ set_tunnel_config(struct netdev *dev_, const struct smap 
*args, char **errp)
 tnl_cfg.dst_port = htons(atoi(node->value));
 } else if (!strcmp(node->key, "csum") && has_csum) {
 if (!strcmp(node->value, "true")) {
-tnl_cfg.csum = true;
+tnl_cfg.csum = NETDEV_TNL_CSUM_ENABLED;
+} else if (!strcmp(node->value, "false")) {
+tnl_cfg.csum = NETDEV_TNL_CSUM_DISABLED;
 }
 } else if (!strcmp(node->key, "seq") && has_seq) {
 if (!strcmp(node->value, "true")) {
@@ -850,6 +852,15 @@ set_tunnel_config(struct netdev *dev_, const struct smap 
*args, char **errp)
 }
 }
 
+/* The default csum state for GRE is special as it does have an optional
+ * checksum but the default configuration isn't correlated with IP version
+ * like UDP tunnels are.  Likewise, tunnels with no checksum at all must be
+ * in this state. */
+if (tnl_cfg.csum == NETDEV_TNL_CSUM_DEFAULT &&
+(!has_csum || strstr(type, "gre"))) {
+tnl_cfg.csum = NETDEV_TNL_DEFAULT_NO_CSUM;
+}
+
 enum tunnel_layers layers = tunnel_supported_layers(type, _cfg);
 const char *full_type = (strcmp(type, "vxlan") ? type
  : (tnl_cfg.exts & (1 << OVS_VXLAN_EXT_GPE)
@@ -1026,8 +1037,10 @@ get_tunnel_config(const struct netdev *dev, struct smap 
*args)
 }
 }
 
-if (tnl_cfg->csum) {
+if (tnl_cfg->csum == NETDEV_TNL_CSUM_ENABLED) {
 smap_add(args, "csum", "true");
+} else if (tnl_cfg->csum == NETDEV_TNL_CSUM_DISABLED) {
+smap_add(args, "csum", "false");
 }
 
 if (tnl_cfg->set_seq) {
diff --git a/lib/netdev.h b/lib/netdev.h
index 67a8486bd..5d253157c 100644
--- a/lib/netdev.h
+++ b/lib/netdev.h
@@ -111,6 +111,22 @@ enum netdev_srv6_flowlabel {
 SRV6_FLOWLABEL_COMPUTE,
 };
 
+enum netdev_tnl_csum {
+/* Default value for UDP tunnels if no configurations is present.  Enforce
+ * checksum calculation in IPv6 tunnels, disable in IPv4 tunnels. */
+NETDEV_TNL_CSUM_DEFAULT = 0,
+
+/* Checksum explicitly to be calculated. */
+NETDEV_TNL_CSUM_ENABLED,
+
+/* Checksum calculation explicitly disabled. */
+NETDEV_TNL_CSUM_DISABLED,
+
+/* A value for when there is no checksum or the default value is no
+ * checksum reguardless of IP version. */
+NETDEV_TNL_DEFAULT_NO_CSUM,
+};
+
 /* Configuration specific to tunnels. */
 struct netdev_tunnel_config {
 ovs_be64 in_key;
@@ -139,7 +155,7 @@ struct netdev_tunnel_config {
 uint8_t tos;
 bool tos_inherit;
 
-bool csum;