| Message ID | 20240221015652.260595-1-mkp@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] 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 | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On Tue, Feb 20, 2024 at 8:56 PM Mike Pattrick <mkp@redhat.com> 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 <mkp@redhat.com> One of the github CI runners failed this in test "bfd - bfd decay". I believe this is a false negative. -M
Mike Pattrick <mkp@redhat.com> writes: > 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> > --- Recheck-request: github-robot > 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 | 8 +++++--- > 6 files changed, 37 insertions(+), 12 deletions(-) > > 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) { Previously, this encap csum flag would be ignored for IPv6 tunnels, but now it will be honored for them. It should be noted in the NEWS file, since it is user impacting. > /* 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..300aefefe 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -3207,9 +3207,11 @@ > <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. Default is unset, set to > + <code>true</code> to enable or <code>false</code> to disable. > + If this value is unset, checksums are enabled by default for > + UDP IPv6 tunnels and disabled otherwise. Checksums present on > + incoming packets will be validated regardless of this setting. > </p> This section is a bit difficult to read. Maybe something like: Optional. Compute encapsulation header (either GRE or UDP) checksums on outgoing packets. When unset (or by default), checksum computing for outgoing packets will be enabled for UDP IPv6 tunnels, and disabled otherwise. When false, no checksum computing will be disabled 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. I'm not very good at technical writing, so take the above as a suggestion. > <p>
Mike Pattrick <mkp@redhat.com> writes: > On Tue, Feb 20, 2024 at 8:56 PM Mike Pattrick <mkp@redhat.com> 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 <mkp@redhat.com> > > One of the github CI runners failed this in test "bfd - bfd decay". I > believe this is a false negative. In the future, you can send a recheck request (I included one with my previous comment) to re-run and clear these "flaky" tests. > -M > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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..300aefefe 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -3207,9 +3207,11 @@ <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. Default is unset, set to + <code>true</code> to enable or <code>false</code> to disable. + If this value is unset, checksums are enabled by default for + UDP IPv6 tunnels and disabled otherwise. Checksums present on + incoming packets will be validated 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> --- 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 | 8 +++++--- 6 files changed, 37 insertions(+), 12 deletions(-)