[PATCH net 0/3] ENETC GSO fixes

[PATCH net 0/3] ENETC GSO fixes

[Vladimir Oltean]

Sashiko derailed me from "[PATCH v5 net 0/4] Fix short frame
transmission in enetc":
https://lore.kernel.org/netdev/20261021145314.1435049-1-vladimir.oltean@nxp.com/

which also touches the driver GSO code path, so I'm making a detour
to fix 3 unrelated issues there (which I could all reproduce using
LLM-produced programs).

The main highlight of this series is the introduction of
.ndo_features_check() for enetc, which runs for every transmitted skb.
Each patch makes its implementation more and more complex.

These patches all assume that at .ndo_features_check() time,
skb_shinfo(skb)->gso_segs is set properly. There might be issues there,
see commit 67b0cca9ba88 ("net: validate xmit skbs in generic_xdp_tx()")
which fixes one case which led to gso_segs reaching the driver while
being zero. For some reason it was merged to net-next though.

Running a bidirectional eno0 <-> swp0 forwarding of raw IPv4 64B packets
(no GRO or GSO) on LS1028A-QDS rather than some micro-benchmark,
I am not seeing a noticeable performance degradation. It is in the
noise margin.
Before: 7440 Kpps ingress, 586 Kpps egress
After: 7440 Kpps ingress, 588 Kpps egress

Vladimir Oltean (3):
  net: enetc: let core segment GSO super-skb when TSO would not fit the
    TX ring
  net: enetc: reserve VLAN extension BDs for software TSO
  net: enetc: let core segment GSO super-skb when headers exceed
    TSO_HEADER_SIZE

 drivers/net/ethernet/freescale/enetc/enetc.c  | 52 +++++++++++++++++--
 drivers/net/ethernet/freescale/enetc/enetc.h  |  3 ++
 .../net/ethernet/freescale/enetc/enetc4_pf.c  |  1 +
 .../net/ethernet/freescale/enetc/enetc_pf.c   |  1 +
 .../net/ethernet/freescale/enetc/enetc_vf.c   |  2 +
 5 files changed, 54 insertions(+), 5 deletions(-)

-- 
2.53.0

[PATCH net 1/3] net: enetc: let core segment GSO super-skb when TSO would not fit the TX ring

[Vladimir Oltean]

For simplicity reasons, the enetc software GSO implementation waits for
the TX ring to free up sufficiently that all BDs that would be generated
by driver-level segmentation can be enqueued in one go. To pass the time,
it stops the queue and returns NETDEV_TX_BUSY for the core to requeue.

The problem is that without any limits on gso_segs, some GSO super-skbs
can never be sent, and the aforementioned behaviour results in the queue
stalling (there is also no ndo_tx_timeout implementation).

This was reported by Sashiko, but is a valid issue which can be easily
triggered from any source that can craft gso_segs in a way that
tso_count_descs() can exceed 2047 (the ring size minus the obligatory
free entry). Valid reproduction methods include TCP peers whose MSS is
close to tcp_min_snd_mss plus large (64KB) TSO super-skbs; UDP GSO using
setsockopt(UDP_SEGMENT); maybe others.

Add an ndo_features_check() that clears the GSO features for skbs that
take the software TSO path and whose descriptor estimate is larger than
the ring could ever provide. The core then segments these skbs itself
before calling ndo_start_xmit().

Capping the segment count with netif_set_tso_max_segs() would be
simpler, but its drawback is that it's static, so it would have to
assume the worst case for every skb (MAX_SKB_FRAGS, and later the
per-segment VLAN and padding BDs). This would reduce TSO sizes even for
skbs that fit.

Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc.c  | 31 ++++++++++++++++---
 drivers/net/ethernet/freescale/enetc/enetc.h  |  3 ++
 .../net/ethernet/freescale/enetc/enetc4_pf.c  |  1 +
 .../net/ethernet/freescale/enetc/enetc_pf.c   |  1 +
 .../net/ethernet/freescale/enetc/enetc_vf.c   |  2 ++
 5 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index c6571b5cc842..1606ac57016f 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -695,6 +695,14 @@ static int enetc_lso_get_hdr_len(const struct sk_buff *skb)
 	return hdr_len;
 }
 
+static bool enetc_skb_uses_lso(const struct enetc_ndev_priv *priv,
+			       const struct sk_buff *skb)
+{
+	/* LSO data unit lengths of up to 256KB are supported */
+	return priv->active_offloads & ENETC_F_LSO &&
+	       (skb->len - enetc_lso_get_hdr_len(skb)) <= ENETC_LSO_MAX_DATA_LEN;
+}
+
 static void enetc_lso_start(struct sk_buff *skb, struct enetc_lso_t *lso)
 {
 	lso->lso_seg_size = skb_shinfo(skb)->gso_size;
@@ -1018,10 +1026,7 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb,
 	tx_ring = priv->tx_ring[skb->queue_mapping];
 
 	if (skb_is_gso(skb)) {
-		/* LSO data unit lengths of up to 256KB are supported */
-		if (priv->active_offloads & ENETC_F_LSO &&
-		    (skb->len - enetc_lso_get_hdr_len(skb)) <=
-		    ENETC_LSO_MAX_DATA_LEN) {
+		if (enetc_skb_uses_lso(priv, skb)) {
 			if (enetc_bd_unused(tx_ring) < enetc_lso_count_descs(skb)) {
 				netif_stop_subqueue(ndev, tx_ring->index);
 				return NETDEV_TX_BUSY;
@@ -1098,6 +1103,24 @@ netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev)
 }
 EXPORT_SYMBOL_GPL(enetc_xmit);
 
+netdev_features_t enetc_features_check(struct sk_buff *skb,
+				       struct net_device *ndev,
+				       netdev_features_t features)
+{
+	struct enetc_ndev_priv *priv = netdev_priv(ndev);
+
+	features = vlan_features_check(skb, features);
+
+	if (!skb_is_gso(skb) || enetc_skb_uses_lso(priv, skb))
+		return features;
+
+	if (tso_count_descs(skb) >= priv->tx_bd_count)
+		features &= ~NETIF_F_GSO_MASK;
+
+	return features;
+}
+EXPORT_SYMBOL_GPL(enetc_features_check);
+
 static irqreturn_t enetc_msix(int irq, void *data)
 {
 	struct enetc_int_vector	*v = data;
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index d9e91832a9c1..1e3290d1a388 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -529,6 +529,9 @@ int enetc_close(struct net_device *ndev);
 void enetc_start(struct net_device *ndev);
 void enetc_stop(struct net_device *ndev);
 netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev);
+netdev_features_t enetc_features_check(struct sk_buff *skb,
+				       struct net_device *ndev,
+				       netdev_features_t features);
 struct net_device_stats *enetc_get_stats(struct net_device *ndev);
 void enetc_set_features(struct net_device *ndev, netdev_features_t features);
 int enetc_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd);
diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
index 71c971618388..bfe0eaf50975 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
@@ -586,6 +586,7 @@ static const struct net_device_ops enetc4_ndev_ops = {
 	.ndo_open		= enetc_open,
 	.ndo_stop		= enetc_close,
 	.ndo_start_xmit		= enetc_xmit,
+	.ndo_features_check	= enetc_features_check,
 	.ndo_get_stats		= enetc_get_stats,
 	.ndo_set_mac_address	= enetc_pf_set_mac_addr,
 	.ndo_set_rx_mode_async	= enetc4_pf_set_rx_mode,
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index d77a07cece28..86ca88196a4b 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
@@ -483,6 +483,7 @@ static const struct net_device_ops enetc_ndev_ops = {
 	.ndo_open		= enetc_open,
 	.ndo_stop		= enetc_close,
 	.ndo_start_xmit		= enetc_xmit,
+	.ndo_features_check	= enetc_features_check,
 	.ndo_get_stats		= enetc_get_stats,
 	.ndo_set_mac_address	= enetc_pf_set_mac_addr,
 	.ndo_set_rx_mode	= enetc_pf_set_rx_mode,
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_vf.c b/drivers/net/ethernet/freescale/enetc/enetc_vf.c
index f70b6d4290f2..223f974727bc 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_vf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_vf.c
@@ -440,6 +440,7 @@ static const struct net_device_ops enetc_ndev_ops = {
 	.ndo_open		= enetc_open,
 	.ndo_stop		= enetc_close,
 	.ndo_start_xmit		= enetc_xmit,
+	.ndo_features_check	= enetc_features_check,
 	.ndo_get_stats		= enetc_get_stats,
 	.ndo_set_mac_address	= enetc_vf_set_mac_addr,
 	.ndo_set_features	= enetc_vf_set_features,
@@ -453,6 +454,7 @@ static const struct net_device_ops enetc4_ndev_ops = {
 	.ndo_open		= enetc_open,
 	.ndo_stop		= enetc_close,
 	.ndo_start_xmit		= enetc_xmit,
+	.ndo_features_check	= enetc_features_check,
 	.ndo_get_stats		= enetc_get_stats,
 	.ndo_set_mac_address	= enetc_vf_set_mac_addr,
 	.ndo_set_features	= enetc_vf_set_features,
-- 
2.53.0

[PATCH net 2/3] net: enetc: reserve VLAN extension BDs for software TSO

[Vladimir Oltean]

enetc_start_xmit() uses tso_count_descs() to check whether the TX ring
has enough free BDs for a software TSO frame. That estimates one header
BD and one data BD per segment, plus one BD per frag. However, when the
skb carries a VLAN tag, enetc_map_tx_tso_hdr() also adds an extension
BD to every segment, which is not accounted for.

With a highly segmented VLAN TSO skb and a nearly full ring, the driver
can then write past next_to_clean and overwrite BDs still owned by the
hardware.

Add enetc_tso_count_descs() which accounts for the extension BD of each
segment when VLAN insertion is needed.

Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
This comes from the previous patch set:
https://lore.kernel.org/netdev/20261021145314.1435049-2-vladimir.oltean@nxp.com/

v5->v1: also patch enetc_features_check()
v4->v5: patch is new
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 1606ac57016f..7f089a84d9a6 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -674,6 +674,17 @@ static void enetc_tso_complete_csum(struct enetc_bdr *tx_ring, struct tso_t *tso
 	}
 }
 
+static int enetc_tso_count_descs(const struct sk_buff *skb)
+{
+	int count = tso_count_descs(skb);
+
+	/* Each segment also needs an extension BD for VLAN insertion */
+	if (skb_vlan_tag_present(skb))
+		count += skb_shinfo(skb)->gso_segs;
+
+	return count;
+}
+
 static int enetc_lso_count_descs(const struct sk_buff *skb)
 {
 	/* 4 BDs: 1 BD for LSO header + 1 BD for extended BD + 1 BD
@@ -1034,7 +1045,7 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb,
 
 			count = enetc_lso_hw_offload(tx_ring, skb);
 		} else {
-			if (enetc_bd_unused(tx_ring) < tso_count_descs(skb)) {
+			if (enetc_bd_unused(tx_ring) < enetc_tso_count_descs(skb)) {
 				netif_stop_subqueue(ndev, tx_ring->index);
 				return NETDEV_TX_BUSY;
 			}
@@ -1114,7 +1125,7 @@ netdev_features_t enetc_features_check(struct sk_buff *skb,
 	if (!skb_is_gso(skb) || enetc_skb_uses_lso(priv, skb))
 		return features;
 
-	if (tso_count_descs(skb) >= priv->tx_bd_count)
+	if (enetc_tso_count_descs(skb) >= priv->tx_bd_count)
 		features &= ~NETIF_F_GSO_MASK;
 
 	return features;
-- 
2.53.0

[PATCH net 3/3] net: enetc: let core segment GSO super-skb when headers exceed TSO_HEADER_SIZE

[Vladimir Oltean]

Like many other drivers, both the software TSO path
(enetc_map_tx_tso_buffs()) and the LSO path (enetc_lso_map_hdr()) copy
the L2+L3+L4 headers of the skb into a fixed-size TSO_HEADER_SIZE (256
byte) slot of the per-ring dma_alloc_coherent() header area, without
checking that they fit.

Sashiko casually points out during review of an unrelated patch:
https://lore.kernel.org/netdev/179294166520.3134.18097439321876673016@kernel.org/
that TSO_HEADER_SIZE does not constitute the maximum size that packet
headers can have for GSO super-skbs, but rather, an essentially
arbitrary number.

An example where headers can exceed TSO_HEADER_SIZE is with an IPv6
extension named Segment Routing Header, set through setsockopt(IPV6_RTHDR),
with a type 4 routing header. With 11 or more segments, the headers of a
TCP/IPv6 packet exceed 256 bytes:

  Ethernet                14
  IPv6                    40
  SRH, 12 segments        8 + 12 * 16 = 200
  TCP (with timestamps)   32
                          ---
                          286  (30 bytes too many)

No privileges are required to request this socket option (but it will
cause driver writes outside allocated memory). A reproducer sketch can
be found at:
https://gist.github.com/vladimiroltean/7cecf636b788b29f361a2fbdae646010

Bizarrely, enetc is the only affected driver, because the core
accidentally protects most other drivers against this bug in
gso_features_check(). There, we have this logic that saves the day:

	/* NETIF_F_IPV6_CSUM does not support IPv6 extension headers,
	 * so neither does TSO that depends on it.
	 */
	if (features & NETIF_F_IPV6_CSUM &&
	    skb_has_ipv6_extension_hdr(skb))
		features &= ~(NETIF_F_IPV6_CSUM | NETIF_F_TSO6 | NETIF_F_GSO_UDP_L4);

enetc is the outlier because it only advertises the more complete
NETIF_F_HW_CSUM, so for it, this fixup does not clear the NETIF_F_TSO6
and NETIF_F_GSO_UDP_L4 bits.

[ astute reviewers may ask: what if we disable NETIF_F_IPV6_CSUM at
  runtime, via ethtool -k, so as to bypass this fixup, don't we make
  all drivers vulnerable?
  No, because netdev_fix_features() will trigger the code path which
  also prints "Dropping TSO6 features since no CSUM feature.\n".
  So TSO super-skbs will now be segmented in the stack. ]

Surveying every driver that uses `TSO_HEADER_SIZE`:

 Driver  | TSO6 | Checksum feature    | Core guard fires? | Verdict
 ---------------|---------------------|-------------------|---------
 enetc   | yes  | `NETIF_F_HW_CSUM`   | no                | vulnerable
 bnxt    | yes  | `NETIF_F_IPV6_CSUM` | yes               | safe
 nicvf   | yes  | `NETIF_F_IPV6_CSUM` | yes               | safe
 otx2    | yes  | `NETIF_F_IPV6_CSUM` | yes               | safe
 fec     | no   | `NETIF_F_IPV6_CSUM` | n/a               | safe
 mvneta  | no   | `NETIF_F_IPV6_CSUM` | n/a               | safe
 mvpp2   | no   | `NETIF_F_IPV6_CSUM` | n/a               | safe
 mv643xx | no   | `NETIF_F_IP_CSUM`   | n/a               | safe
 dpaa2   | no   | `NETIF_F_IPV6_CSUM` | n/a               | safe (*)

(*) dpaa2 also uses a fixed-size `kmalloc(TSO_HEADER_SIZE)` per segment,
so the identical overflow would apply if it ever gained `NETIF_F_TSO6`.

The above survey is what influenced the decision to make a localized
patch in enetc_features_check() rather than create a huge disturbance in
the core, even though, IMO, TSO_HEADER_SIZE is a huge foot gun.

Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO")
Fixes: 69797ff888d3 ("net: enetc: add LSO support for i.MX95 ENETC PF")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 7f089a84d9a6..3c5478238e35 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1122,7 +1122,15 @@ netdev_features_t enetc_features_check(struct sk_buff *skb,
 
 	features = vlan_features_check(skb, features);
 
-	if (!skb_is_gso(skb) || enetc_skb_uses_lso(priv, skb))
+	if (!skb_is_gso(skb))
+		return features;
+
+	if (enetc_lso_get_hdr_len(skb) > TSO_HEADER_SIZE) {
+		features &= ~NETIF_F_GSO_MASK;
+		return features;
+	}
+
+	if (enetc_skb_uses_lso(priv, skb))
 		return features;
 
 	if (enetc_tso_count_descs(skb) >= priv->tx_bd_count)
-- 
2.53.0