On 2/21/24 19:06, 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.
Hi, Mike. That's an interesting feature. Thanks!
Though the patch subject states 'userspace: ' the code seem to affect
tunnels in general, since it modifies tunnel flags for all tunnels.
More on that below.
>
> Signed-off-by: Mike Pattrick <m...@redhat.com>
> ---
> v2: Changed documentation, and added a NEWS item
> v3: NEWS file merge conflict
> ---
> NEWS | 3 +++
> lib/netdev-native-tnl.c | 2 +-
> lib/netdev-vport.c | 13 +++++++++++--
> lib/netdev.h | 9 ++++++++-
> ofproto/tunnel.c | 11 +++++++++--
> tests/tunnel.at | 6 +++---
> vswitchd/vswitch.xml | 11 ++++++++---
> 7 files changed, 43 insertions(+), 12 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index c9e4064e6..3a75d3850 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -4,6 +4,9 @@ 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 tunnels will now honour the csum option. Configuring the
All IPv6 UDP tunnels?
Also, double spaces between sentences.
> + interface with "options:csum=false" now has the same effect in OVS
s/in OVS//
> + as the udp6zerocsumtx option has with kernel UDP tunnels.
s/kernel/Linux kenrel/
>
>
> 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) {
Please add a tunnel-push-pop test that verifies that cehcksums
are skipped.
> /* Write a value in now to mark that we should compute the checksum
> * later. 0xffff is handy because it is transparent to the
> * calculation. */
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 60caa02fb..f9a778988 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,11 @@ set_tunnel_config(struct netdev *dev_, const struct smap
> *args, char **errp)
> }
> }
>
> + /* The default csum state for GRE is special. */
Please explain what does it mean in the comment. And in the enum comment.
> + if (tnl_cfg.csum == NETDEV_TNL_CSUM_DEFAULT && strstr(type, "gre")) {
The tnl_cfg is initialized with memset, but the enum is not guaranteed
to start with a zero value.
> + tnl_cfg.csum = NETDEV_TNL_CSUM_DEFAULT_GRE;
> + }
> +
> enum tunnel_layers layers = tunnel_supported_layers(type, &tnl_cfg);
> const char *full_type = (strcmp(type, "vxlan") ? type
> : (tnl_cfg.exts & (1 << OVS_VXLAN_EXT_GPE)
> @@ -1026,8 +1033,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..a79531e6d 100644
> --- a/lib/netdev.h
> +++ b/lib/netdev.h
> @@ -111,6 +111,13 @@ enum netdev_srv6_flowlabel {
> SRV6_FLOWLABEL_COMPUTE,
> };
>
> +enum netdev_tnl_csum {
> + NETDEV_TNL_CSUM_DEFAULT,
> + NETDEV_TNL_CSUM_ENABLED,
> + NETDEV_TNL_CSUM_DISABLED,
> + NETDEV_TNL_CSUM_DEFAULT_GRE,
> +};
Some comments for the values would be nice to have.
> +
> /* Configuration specific to tunnels. */
> struct netdev_tunnel_config {
> ovs_be64 in_key;
> @@ -139,7 +146,7 @@ struct netdev_tunnel_config {
> uint8_t tos;
> bool tos_inherit;
>
> - bool csum;
> + enum netdev_tnl_csum csum;
> bool dont_fragment;
> enum netdev_pt_mode pt_mode;
>
> diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
> index 80ddee78a..6f462874e 100644
> --- a/ofproto/tunnel.c
> +++ b/ofproto/tunnel.c
> @@ -465,9 +465,14 @@ tnl_port_send(const struct ofport_dpif *ofport, struct
> flow *flow,
>
> flow->tunnel.flags &= ~(FLOW_TNL_F_MASK & ~FLOW_TNL_PUB_F_MASK);
> flow->tunnel.flags |= (cfg->dont_fragment ? FLOW_TNL_F_DONT_FRAGMENT : 0)
> - | (cfg->csum ? FLOW_TNL_F_CSUM : 0)
> | (cfg->out_key_present ? FLOW_TNL_F_KEY : 0);
>
> + if (cfg->csum == NETDEV_TNL_CSUM_ENABLED) {
> + flow->tunnel.flags |= FLOW_TNL_F_CSUM;
> + } else if (cfg->csum == NETDEV_TNL_CSUM_DEFAULT && !flow->tunnel.ip_dst)
> {
> + flow->tunnel.flags |= FLOW_TNL_F_CSUM;
This enables csum flag for tunnels that do not support it, see the
'has_csum' check in set_tunnel_config(). That's the reason this
change is causing csum flag to appear in SRv6 tests, while it is not
supposed to support it. SRv6 is not even a UDP or GRE tunne, so the
flag will not have any effect, but may create some confusion.
> + }
> +
> if (cfg->set_egress_pkt_mark) {
> flow->pkt_mark = cfg->egress_pkt_mark;
> wc->masks.pkt_mark = UINT32_MAX;
> @@ -706,8 +711,10 @@ tnl_port_format(const struct tnl_port *tnl_port, struct
> ds *ds)
> ds_put_cstr(ds, ", df=false");
> }
>
> - if (cfg->csum) {
> + if (cfg->csum == NETDEV_TNL_CSUM_ENABLED) {
> ds_put_cstr(ds, ", csum=true");
> + } else if (cfg->csum == NETDEV_TNL_CSUM_DISABLED) {
> + ds_put_cstr(ds, ", csum=false");
> }
>
> ds_put_cstr(ds, ")\n");
> diff --git a/tests/tunnel.at b/tests/tunnel.at
> index 282651ac7..e68be8b04 100644
> --- a/tests/tunnel.at
> +++ b/tests/tunnel.at
> @@ -1037,7 +1037,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>
> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'tunnel(tun_id=0,src=1.1.1.1,dst=1.1.1.2,ttl=64),in_port(4789)'], [0],
> [stdout])
> AT_CHECK([tail -1 stdout], [0],
> - [Datapath actions:
> set(tunnel(ipv6_dst=2001:cafe::1,ttl=64,tp_dst=4789,flags(df))),4789
> + [Datapath actions:
> set(tunnel(ipv6_dst=2001:cafe::1,ttl=64,tp_dst=4789,flags(df|csum))),4789
> ])
>
> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'tunnel(tun_id=0x0,ipv6_src=2001:cafe::1,ipv6_dst=2001:cafe::2,ttl=64),in_port(4789)'],
> [0], [stdout])
> @@ -1312,13 +1312,13 @@ port 6: p2 (srv6: ::->flow, key=0, legacy_l3, dp
> port=6, ttl=64)
> dnl Encap: ipv4 inner packet
> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=4,ttl=128,frag=no),tcp(src=8,dst=9)'],
> [0], [stdout])
> AT_CHECK([tail -1 stdout], [0],
> - [Datapath actions: set(tunnel(ipv6_dst=fc00::2,ttl=64,flags(df))),pop_eth,6
> + [Datapath actions:
> set(tunnel(ipv6_dst=fc00::2,ttl=64,flags(df|csum))),pop_eth,6
> ])
>
> dnl Encap: ipv6 inner packet
> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x86dd),ipv6(src=2001:cafe::92,dst=2001:cafe::88,label=0,proto=47,tclass=0x0,hlimit=64)'],
> [0], [stdout])
> AT_CHECK([tail -1 stdout], [0],
> - [Datapath actions: set(tunnel(ipv6_dst=fc00::2,ttl=64,flags(df))),pop_eth,6
> + [Datapath actions:
> set(tunnel(ipv6_dst=fc00::2,ttl=64,flags(df|csum))),pop_eth,6
SRv6 is an IPv6-IPv46 tunnel. 'csum' flag doesn't make any sense for it.
> ])
>
> OVS_VSWITCHD_STOP
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 612ba41e3..f802ea32d 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -3207,9 +3207,14 @@
> <column name="options" key="csum" type='{"type": "boolean"}'>
> <p>
> Optional. Compute encapsulation header (either GRE or UDP)
> - checksums on outgoing packets. Default is disabled, set to
> - <code>true</code> to enable. Checksums present on incoming
> - packets will be validated regardless of this setting.
> + checksums on outgoing packets. When unset (the default value),
> + checksum computing for outgoing packets is enabled for UDP IPv6
> + tunnels, and disabled otherwise.
What is the defualt behavior for GRE ?
> When set to false, no checksums
Enclose 'true' and 'false' with <code> tags. Also below.
> + will be computed for outgoing tunnel encapsulation packets.
s/packets/headers/ ?
> When
> + true, checksums will be computed for all outgoing tunnel
> + encapsulation packets.
*headers
> Checksums present on incoming packets will
> + be validated regardless of this setting. Incoming packets without
> + a checksum will also be accepted regardless of this setting.
And, please, double spaces between sentences.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
