| Message ID | 20240221180604.302865-1-mkp@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v3] userspace: Allow UDP zero checksum with IPv6 tunnels. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
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 <mkp@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.
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 + interface with "options:csum=false" now has the same effect in OVS + as the udp6zerocsumtx option has with 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. 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. */ + if (tnl_cfg.csum == NETDEV_TNL_CSUM_DEFAULT && strstr(type, "gre")) { + 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, +}; + /* 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; + } + 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 ]) 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. When set to false, no checksums + will be computed for outgoing tunnel encapsulation packets. When + true, checksums will be computed for all outgoing tunnel + encapsulation packets. 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. </p> <p>
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 <mkp@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(-)