| Message ID | 20240807-udp-gso-egress-from-tunnel-v3-1-8828d93c5b45@cloudflare.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Don't take HW USO path when packets can't be checksummed by device | expand |
Jakub Sitnicki wrote: > UDP segmentation offload inherently depends on checksum offload. It should > not be possible to disable checksum offload while leaving USO enabled. > Enforce this dependency in code. > > There is a single tx-udp-segmentation feature flag to indicate support for > both IPv4/6, hence the devices wishing to support USO must offer checksum > offload for both IP versions. > > Fixes: 83aa025f535f ("udp: add gso support to virtual devices") Was this not introduced by removing the CHECKSUM_PARTIAL check in udp_send_skb? > Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> > --- > net/core/dev.c | 27 ++++++++++++++++++--------- > 1 file changed, 18 insertions(+), 9 deletions(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 751d9b70e6ad..dfb12164b35d 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -9912,6 +9912,16 @@ static void netdev_sync_lower_features(struct net_device *upper, > } > } > > +#define IP_CSUM_MASK (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM) > + Perhaps NETIF_F_IP_CSUM_MASK in netdev_features.h right below #define NETIF_F_CSUM_MASK Then again, for a stable patch we want a small patch. Then I'd define as a constant netdev_features_t inside the function scope. Minor: still prefix with netdev_ even though it's a static function? > +static bool has_ip_or_hw_csum(netdev_features_t features) > +{ > + bool ip_csum = (features & IP_CSUM_MASK) == IP_CSUM_MASK; > + bool hw_csum = features & NETIF_F_HW_CSUM; > + > + return ip_csum || hw_csum; > +} > + > static netdev_features_t netdev_fix_features(struct net_device *dev, > netdev_features_t features) > { > @@ -9993,15 +10003,9 @@ static netdev_features_t netdev_fix_features(struct net_device *dev, > features &= ~NETIF_F_LRO; > } > > - if (features & NETIF_F_HW_TLS_TX) { > - bool ip_csum = (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) == > - (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM); > - bool hw_csum = features & NETIF_F_HW_CSUM; > - > - if (!ip_csum && !hw_csum) { > - netdev_dbg(dev, "Dropping TLS TX HW offload feature since no CSUM feature.\n"); > - features &= ~NETIF_F_HW_TLS_TX; > - } > + if ((features & NETIF_F_HW_TLS_TX) && !has_ip_or_hw_csum(features)) { > + netdev_dbg(dev, "Dropping TLS TX HW offload feature since no CSUM feature.\n"); > + features &= ~NETIF_F_HW_TLS_TX; > } > > if ((features & NETIF_F_HW_TLS_RX) && !(features & NETIF_F_RXCSUM)) { > @@ -10009,6 +10013,11 @@ static netdev_features_t netdev_fix_features(struct net_device *dev, > features &= ~NETIF_F_HW_TLS_RX; > } > > + if ((features & NETIF_F_GSO_UDP_L4) && !has_ip_or_hw_csum(features)) { > + netdev_dbg(dev, "Dropping USO feature since no CSUM feature.\n"); > + features &= ~NETIF_F_GSO_UDP_L4; > + } > + > return features; > } > > > -- > 2.40.1 >
On Wed, Aug 07, 2024 at 10:29 PM -04, Willem de Bruijn wrote: > Jakub Sitnicki wrote: >> UDP segmentation offload inherently depends on checksum offload. It should >> not be possible to disable checksum offload while leaving USO enabled. >> Enforce this dependency in code. >> >> There is a single tx-udp-segmentation feature flag to indicate support for >> both IPv4/6, hence the devices wishing to support USO must offer checksum >> offload for both IP versions. >> >> Fixes: 83aa025f535f ("udp: add gso support to virtual devices") > > Was this not introduced by removing the CHECKSUM_PARTIAL check in > udp_send_skb? Makes sense. It only became a problem after that change. Will update. > >> Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com> >> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> >> --- >> net/core/dev.c | 27 ++++++++++++++++++--------- >> 1 file changed, 18 insertions(+), 9 deletions(-) >> >> diff --git a/net/core/dev.c b/net/core/dev.c >> index 751d9b70e6ad..dfb12164b35d 100644 >> --- a/net/core/dev.c >> +++ b/net/core/dev.c >> @@ -9912,6 +9912,16 @@ static void netdev_sync_lower_features(struct net_device *upper, >> } >> } >> >> +#define IP_CSUM_MASK (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM) >> + > > Perhaps NETIF_F_IP_CSUM_MASK in netdev_features.h right below > > #define NETIF_F_CSUM_MASK > > Then again, for a stable patch we want a small patch. Then I'd define > as a constant netdev_features_t inside the function scope. > > Minor: still prefix with netdev_ even though it's a static function? Will apply all feedback. Thanks. [...]
diff --git a/net/core/dev.c b/net/core/dev.c index 751d9b70e6ad..dfb12164b35d 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -9912,6 +9912,16 @@ static void netdev_sync_lower_features(struct net_device *upper, } } +#define IP_CSUM_MASK (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM) + +static bool has_ip_or_hw_csum(netdev_features_t features) +{ + bool ip_csum = (features & IP_CSUM_MASK) == IP_CSUM_MASK; + bool hw_csum = features & NETIF_F_HW_CSUM; + + return ip_csum || hw_csum; +} + static netdev_features_t netdev_fix_features(struct net_device *dev, netdev_features_t features) { @@ -9993,15 +10003,9 @@ static netdev_features_t netdev_fix_features(struct net_device *dev, features &= ~NETIF_F_LRO; } - if (features & NETIF_F_HW_TLS_TX) { - bool ip_csum = (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) == - (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM); - bool hw_csum = features & NETIF_F_HW_CSUM; - - if (!ip_csum && !hw_csum) { - netdev_dbg(dev, "Dropping TLS TX HW offload feature since no CSUM feature.\n"); - features &= ~NETIF_F_HW_TLS_TX; - } + if ((features & NETIF_F_HW_TLS_TX) && !has_ip_or_hw_csum(features)) { + netdev_dbg(dev, "Dropping TLS TX HW offload feature since no CSUM feature.\n"); + features &= ~NETIF_F_HW_TLS_TX; } if ((features & NETIF_F_HW_TLS_RX) && !(features & NETIF_F_RXCSUM)) { @@ -10009,6 +10013,11 @@ static netdev_features_t netdev_fix_features(struct net_device *dev, features &= ~NETIF_F_HW_TLS_RX; } + if ((features & NETIF_F_GSO_UDP_L4) && !has_ip_or_hw_csum(features)) { + netdev_dbg(dev, "Dropping USO feature since no CSUM feature.\n"); + features &= ~NETIF_F_GSO_UDP_L4; + } + return features; }
UDP segmentation offload inherently depends on checksum offload. It should not be possible to disable checksum offload while leaving USO enabled. Enforce this dependency in code. There is a single tx-udp-segmentation feature flag to indicate support for both IPv4/6, hence the devices wishing to support USO must offer checksum offload for both IP versions. Fixes: 83aa025f535f ("udp: add gso support to virtual devices") Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> --- net/core/dev.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)