[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] net: enetc: VLAN tag push after enetc_features_check()
  overflows TSO header
- [Medium] net: enetc: u8 hdr_len truncation in enetc_lso_start() for
  256-byte headers
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] net: enetc: lockup in enetc_map_tx_tso_hdr() via undersized TSO
  descriptor [1]
--

commit b5eb34f7b5b23a1224c351b91a143ecbd34866d9
Author: Vladimir Oltean <vladimir.oltean@nxp.com>

net: enetc: let core segment GSO super-skb when headers exceed TSO_HEADER_SIZE

This commit clears NETIF_F_GSO_MASK in enetc_features_check() when the GSO
packet headers exceed TSO_HEADER_SIZE so the networking core segments the
packet instead of overflowing the per-ring TSO header slot.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 7f089a84d9a65..3c5478238e356 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;
> +	}

[Severity: High]
Can a non-offloaded VLAN tag push the header past TSO_HEADER_SIZE after
this check?

In validate_xmit_skb(), netif_skb_features() runs before
validate_xmit_vlan().  When a GSO skb carries an out-of-band VLAN tag that
is not offloaded in hardware (such as an 802.1ad S-TAG, or an 802.1Q C-TAG
when NETIF_F_HW_VLAN_CTAG_TX is disabled), a 253..256 byte header passes
enetc_features_check() and then grows by VLAN_HLEN (4 bytes) when
validate_xmit_vlan() calls __vlan_hwaccel_push_inside().  In
enetc_map_tx_tso_buffs(), tso_build_hdr() then copies 257..260 bytes into
a 256-byte slot in tx_ring->tso_headers, overflowing into the next slot or
past the end of the coherent DMA buffer.

[Severity: Medium]
Can a 256-byte header still break the hardware LSO path because
lso->hdr_len is a u8?

Because this check uses > TSO_HEADER_SIZE (256), a GSO packet whose headers
are 256 bytes passes through to enetc_lso_start():

drivers/net/ethernet/freescale/enetc/enetc.h:
struct enetc_lso_t {
	...
	u8	hdr_len; /* LSO header length */

drivers/net/ethernet/freescale/enetc/enetc.c:enetc_lso_start() {
	...
	lso->hdr_len = enetc_lso_get_hdr_len(skb);
	lso->total_len = skb->len - lso->hdr_len;
}

Assigning 256 to lso->hdr_len truncates it to 0, so enetc_lso_map_hdr()
copies 0 header bytes and programs hdr_len = 0 in the LSO descriptor while
enetc_lso_map_data() maps the headers as payload.

[Severity: High]
This problem wasn't introduced by this patch, but in [1] can a GSO
packet with hdr_len < 16 lock up the controller in enetc_map_tx_tso_hdr()?

While this check bounds the maximum header length, a crafted GSO packet
(for example via AF_PACKET) with hdr_len < 16 still reaches
enetc_map_tx_tso_hdr(), which writes hdr_len directly into
txbd_tmp.buf_len.  An undersized first TSO descriptor below the hardware's
16-byte minimum stalls the ENETC transmitter.

[1] https://sashiko.dev/#/bug/linux-835a7475-1686-40c3-bf3a-572547fbf43f

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111233149.479596-1-vladimir.oltean@nxp.com?part=3