[RFC] net: remove NETIF_F_GSO_FRAGLIST from NETIF_F_GSO_SOFTWARE

[RFC] net: remove NETIF_F_GSO_FRAGLIST from NETIF_F_GSO_SOFTWARE

[Felix Fietkau]

Several drivers set NETIF_F_GSO_SOFTWARE, but mangle fraglist GRO packets
in a way that they can't be properly segmented anymore.
In order to properly deal with this, remove fraglist GSO from
NETIF_F_GSO_SOFTWARE and switch to NETIF_F_GSO_SOFTWARE_ALL (which includes
fraglist GSO) in places where it's safe to add.

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 drivers/net/dummy.c                  | 2 +-
 drivers/net/loopback.c               | 2 +-
 drivers/net/macvlan.c                | 2 +-
 include/linux/netdev_features.h      | 5 +++--
 net/8021q/vlan.h                     | 2 +-
 net/8021q/vlan_dev.c                 | 4 ++--
 net/core/sock.c                      | 2 +-
 net/mac80211/ieee80211_i.h           | 2 +-
 net/openvswitch/vport-internal_dev.c | 2 +-
 9 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c
index d29b5d7af0d7..e2d0837c953c 100644
--- a/drivers/net/dummy.c
+++ b/drivers/net/dummy.c
@@ -110,7 +110,7 @@ static void dummy_setup(struct net_device *dev)
 	dev->flags &= ~IFF_MULTICAST;
 	dev->priv_flags |= IFF_LIVE_ADDR_CHANGE | IFF_NO_QUEUE;
 	dev->features	|= NETIF_F_SG | NETIF_F_FRAGLIST;
-	dev->features	|= NETIF_F_GSO_SOFTWARE;
+	dev->features	|= NETIF_F_GSO_SOFTWARE_ALL;
 	dev->features	|= NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_LLTX;
 	dev->features	|= NETIF_F_GSO_ENCAP_ALL;
 	dev->hw_features |= dev->features;
diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index 2b486e7c749c..50cd6eb93eb7 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -172,7 +172,7 @@ static void gen_lo_setup(struct net_device *dev,
 	dev->flags		= IFF_LOOPBACK;
 	dev->priv_flags		|= IFF_LIVE_ADDR_CHANGE | IFF_NO_QUEUE;
 	netif_keep_dst(dev);
-	dev->hw_features	= NETIF_F_GSO_SOFTWARE;
+	dev->hw_features	= NETIF_F_GSO_SOFTWARE_ALL;
 	dev->features		= NETIF_F_SG | NETIF_F_FRAGLIST
 		| NETIF_F_GSO_SOFTWARE
 		| NETIF_F_HW_CSUM
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 24298a33e0e9..cd0b50199354 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -897,7 +897,7 @@ static int macvlan_hwtstamp_set(struct net_device *dev,
 static struct lock_class_key macvlan_netdev_addr_lock_key;
 
 #define ALWAYS_ON_OFFLOADS \
-	(NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_GSO_SOFTWARE | \
+	(NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_GSO_SOFTWARE_ALL | \
 	 NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL)
 
 #define ALWAYS_ON_FEATURES (ALWAYS_ON_OFFLOADS | NETIF_F_LLTX)
diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 7c2d77d75a88..8b6d7f532065 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -219,13 +219,14 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)
 
 /* List of features with software fallbacks. */
 #define NETIF_F_GSO_SOFTWARE	(NETIF_F_ALL_TSO | NETIF_F_GSO_SCTP |	     \
-				 NETIF_F_GSO_UDP_L4 | NETIF_F_GSO_FRAGLIST)
+				 NETIF_F_GSO_UDP_L4)
+#define NETIF_F_GSO_SOFTWARE_ALL (NETIF_F_GSO_SOFTWARE | NETIF_F_GSO_FRAGLIST)
 
 /*
  * If one device supports one of these features, then enable them
  * for all in netdev_increment_features.
  */
-#define NETIF_F_ONE_FOR_ALL	(NETIF_F_GSO_SOFTWARE | NETIF_F_GSO_ROBUST | \
+#define NETIF_F_ONE_FOR_ALL	(NETIF_F_GSO_SOFTWARE_ALL | NETIF_F_GSO_ROBUST | \
 				 NETIF_F_SG | NETIF_F_HIGHDMA |		\
 				 NETIF_F_FRAGLIST | NETIF_F_VLAN_CHALLENGED)
 
diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h
index 5eaf38875554..4ce1e8cf6b2e 100644
--- a/net/8021q/vlan.h
+++ b/net/8021q/vlan.h
@@ -108,7 +108,7 @@ static inline netdev_features_t vlan_tnl_features(struct net_device *real_dev)
 	netdev_features_t ret;
 
 	ret = real_dev->hw_enc_features &
-	      (NETIF_F_CSUM_MASK | NETIF_F_GSO_SOFTWARE |
+	      (NETIF_F_CSUM_MASK | NETIF_F_GSO_SOFTWARE_ALL |
 	       NETIF_F_GSO_ENCAP_ALL);
 
 	if ((ret & NETIF_F_GSO_ENCAP_ALL) && (ret & NETIF_F_CSUM_MASK))
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 217be32426b5..194fb71e8463 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -561,7 +561,7 @@ static int vlan_dev_init(struct net_device *dev)
 		dev->state |= (1 << __LINK_STATE_NOCARRIER);
 
 	dev->hw_features = NETIF_F_HW_CSUM | NETIF_F_SG |
-			   NETIF_F_FRAGLIST | NETIF_F_GSO_SOFTWARE |
+			   NETIF_F_FRAGLIST | NETIF_F_GSO_SOFTWARE_ALL |
 			   NETIF_F_GSO_ENCAP_ALL |
 			   NETIF_F_HIGHDMA | NETIF_F_SCTP_CRC |
 			   NETIF_F_ALL_FCOE;
@@ -654,7 +654,7 @@ static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
 	if (lower_features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
 		lower_features |= NETIF_F_HW_CSUM;
 	features = netdev_intersect_features(features, lower_features);
-	features |= old_features & (NETIF_F_SOFT_FEATURES | NETIF_F_GSO_SOFTWARE);
+	features |= old_features & (NETIF_F_SOFT_FEATURES | NETIF_F_GSO_SOFTWARE_ALL);
 	features |= NETIF_F_LLTX;
 
 	return features;
diff --git a/net/core/sock.c b/net/core/sock.c
index 9abc4fe25953..1973e3092ed8 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2451,7 +2451,7 @@ void sk_setup_caps(struct sock *sk, struct dst_entry *dst)
 	if (sk_is_tcp(sk))
 		sk->sk_route_caps |= NETIF_F_GSO;
 	if (sk->sk_route_caps & NETIF_F_GSO)
-		sk->sk_route_caps |= NETIF_F_GSO_SOFTWARE;
+		sk->sk_route_caps |= NETIF_F_GSO_SOFTWARE_ALL;
 	if (unlikely(sk->sk_gso_disabled))
 		sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
 	if (sk_can_gso(sk)) {
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index a3485e4c6132..8de4f8cabf3a 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -2009,7 +2009,7 @@ void ieee80211_color_collision_detection_work(struct work_struct *work);
 /* interface handling */
 #define MAC80211_SUPPORTED_FEATURES_TX	(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | \
 					 NETIF_F_HW_CSUM | NETIF_F_SG | \
-					 NETIF_F_HIGHDMA | NETIF_F_GSO_SOFTWARE | \
+					 NETIF_F_HIGHDMA | NETIF_F_GSO_SOFTWARE_ALL | \
 					 NETIF_F_HW_TC)
 #define MAC80211_SUPPORTED_FEATURES_RX	(NETIF_F_RXCSUM)
 #define MAC80211_SUPPORTED_FEATURES	(MAC80211_SUPPORTED_FEATURES_TX | \
diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c
index 4b33133cbdff..6c4b231c3957 100644
--- a/net/openvswitch/vport-internal_dev.c
+++ b/net/openvswitch/vport-internal_dev.c
@@ -109,7 +109,7 @@ static void do_setup(struct net_device *netdev)
 
 	netdev->features = NETIF_F_LLTX | NETIF_F_SG | NETIF_F_FRAGLIST |
 			   NETIF_F_HIGHDMA | NETIF_F_HW_CSUM |
-			   NETIF_F_GSO_SOFTWARE | NETIF_F_GSO_ENCAP_ALL;
+			   NETIF_F_GSO_SOFTWARE_ALL | NETIF_F_GSO_ENCAP_ALL;
 
 	netdev->vlan_features = netdev->features;
 	netdev->hw_enc_features = netdev->features;
-- 
2.44.0

Re: [RFC] net: remove NETIF_F_GSO_FRAGLIST from NETIF_F_GSO_SOFTWARE

[Willem de Bruijn]

Felix Fietkau wrote:
> Several drivers set NETIF_F_GSO_SOFTWARE, but mangle fraglist GRO packets
> in a way that they can't be properly segmented anymore.

Can you share a bit more concrete detail: which driver, for instance, and
how does it mangle the packet?

I assume something with inserting or deleting tunnel headers. But the
fraglist skbs should be able to reproduce the modified header in
skb_segment_list?

> In order to properly deal with this, remove fraglist GSO from
> NETIF_F_GSO_SOFTWARE and switch to NETIF_F_GSO_SOFTWARE_ALL (which includes
> fraglist GSO) in places where it's safe to add.
> 
> Signed-off-by: Felix Fietkau <nbd@nbd.name>

Re: [RFC] net: remove NETIF_F_GSO_FRAGLIST from NETIF_F_GSO_SOFTWARE

[Felix Fietkau]

On 16.08.24 16:13, Willem de Bruijn wrote:
> Felix Fietkau wrote:
>> Several drivers set NETIF_F_GSO_SOFTWARE, but mangle fraglist GRO packets
>> in a way that they can't be properly segmented anymore.
> 
> Can you share a bit more concrete detail: which driver, for instance, and
> how does it mangle the packet?
> 
> I assume something with inserting or deleting tunnel headers. But the
> fraglist skbs should be able to reproduce the modified header in
> skb_segment_list?

I've received bug reports from a variety of people, often with very 
little context, usually crashing while segmenting fraglist skbs.

The ones that people were able to easily reproduce involved bridging 
with vxlan or virtio net devices. People using those two reported that 
my patch fixed their crashes.

I will try to gather more information about the root cause of the crashes.

- Felix

Re: [RFC] net: remove NETIF_F_GSO_FRAGLIST from NETIF_F_GSO_SOFTWARE

[Alexander Lobakin]

From: Felix Fietkau <nbd@nbd.name>
Date: Fri, 16 Aug 2024 09:59:15 +0200

Please use scripts/get_maintainer.pl next time. I remember it was me who
added fraglist to GSO_SOFTWARE, but I'm not Cced =\

> Several drivers set NETIF_F_GSO_SOFTWARE, but mangle fraglist GRO packets

Which ones precisely?

> in a way that they can't be properly segmented anymore.
> In order to properly deal with this, remove fraglist GSO from
> NETIF_F_GSO_SOFTWARE and switch to NETIF_F_GSO_SOFTWARE_ALL (which includes
> fraglist GSO) in places where it's safe to add.

Why isn't bridge changed? It's only a software layer, I don't think it
break fraglist skbs anyhow.

> 
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>  drivers/net/dummy.c                  | 2 +-
>  drivers/net/loopback.c               | 2 +-
>  drivers/net/macvlan.c                | 2 +-
>  include/linux/netdev_features.h      | 5 +++--
>  net/8021q/vlan.h                     | 2 +-
>  net/8021q/vlan_dev.c                 | 4 ++--
>  net/core/sock.c                      | 2 +-
>  net/mac80211/ieee80211_i.h           | 2 +-
>  net/openvswitch/vport-internal_dev.c | 2 +-
>  9 files changed, 12 insertions(+), 11 deletions(-)

Thanks,
Olek