Re: [PATCH v5 net 1/4] net: enetc: reserve VLAN extension BDs for software TSO

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (2) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix stops the ring overrun, but VLAN-tagged TSO skbs that
  can never fit now stall the TX queue for good.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the BUSY path
  the patch changes can lose a wakeup.
- [High] This problem wasn't introduced by this patch, but
  enetc_map_tx_tso_buffs() copies the whole L2+L3+L4 header into a fixed…

--

net: enetc: reserve VLAN extension BDs for software TSO

This commit adds enetc_tso_count_descs(). It extends tso_count_descs()
with one extension BD per segment when the skb carries a VLAN tag.
enetc_start_xmit() now uses it for the free BD check on the software TSO
path.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 80f0082f6c633..ab4bfefda6c8f 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -688,6 +688,17 @@ static int enetc_lso_count_descs(const struct sk_buff *skb)
>  	return skb_shinfo(skb)->nr_frags + 4;
>  }
>  
> +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;

[Severity: High]

Can this estimate exceed what the TX ring can ever provide? If so, the
queue would stop permanently instead of overrunning.

For a VLAN skb this returns 3 * gso_segs + nr_frags. The TX ring is
fixed at ENETC_TX_RING_DEFAULT_SIZE (2048) and there is no
set_ringparam. So enetc_bd_unused() returns at most 2047, even on an
empty ring.

The driver does not limit the segment count anywhere. There is no
netif_set_tso_max_segs(), no gso_max_segs and no ndo_features_check.

Take a 64KB TSO skb with an MSS of about 96 or less (gso_segs of roughly
683 or more). enetc_start_xmit() takes this path on every attempt:

	if (enetc_bd_unused(tx_ring) < enetc_tso_count_descs(skb)) {
		netif_stop_subqueue(ndev, tx_ring->index);
		return NETDEV_TX_BUSY;
	}

The qdisc requeues the same skb. The only runtime wake is in
enetc_clean_tx_ring():

	if (unlikely(tx_frm_cnt && netif_carrier_ok(ndev) &&
		     __netif_subqueue_stopped(ndev, tx_ring->index) &&
		     ...
		netif_wake_subqueue(ndev, tx_ring->index);

Once the ring drains, tx_frm_cnt stays 0. There is also no
ndo_tx_timeout, so nothing restarts the queue.

This can be triggered by a TCP sender on a VLAN on top of enetc using
the software TSO path. A remote peer can force a small MSS through its
SYN (down to tcp_min_snd_mss), and a local user can use TCP_MAXSEG.

Before this patch, most of these skbs overran the ring. Now they hang
the queue instead.

The same stall already existed for non-VLAN skbs above about 1016
segments. This change moves VLAN skbs with about 683 to 1015 segments
into it.

The estimate is a worst case, so some VLAN skbs that really fit are also
rejected forever. For example, gso_segs=680 with 17 segment-aligned
frags needs 2040 BDs, but the estimate is 2057.

The padding patch in this series ("net: enetc: do not rely on mEMAC
hardware frame padding") adds another gso_segs to the estimate. That
lowers the threshold to about 508 segments with VLAN and about 677
without.

Would it make sense to cap the segment count to what the ring can hold?
For example:

	netif_set_tso_max_segs(ndev, (bd_count - 1 - MAX_SKB_FRAGS) /
			       per_seg_bds);

Another option is an ndo_features_check that clears GSO when the count
exceeds bd_count - 1, so the core falls back to software GSO.

> +
> +	return count;
> +}
> +

[ ... ]

> @@ -1032,7 +1043,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;
>  			}

[Severity: High]

This is a pre-existing issue, not one introduced by this patch. Can
this stop path lose a wakeup?

enetc_bd_unused() reads tx_ring->next_to_clean without a lock. The
queue is then stopped with no smp_mb() and no re-check of free space.

On another CPU, enetc_clean_tx_ring() updates next_to_clean. It wakes
the queue only if it already sees the queue stopped and tx_frm_cnt is
non-zero:

CPU0 (enetc_start_xmit)              CPU1 (enetc_clean_tx_ring)
enetc_bd_unused() reads stale
next_to_clean, not enough room
                                     cleans the last in-flight frames
                                     tx_ring->next_to_clean = i;
                                     __netif_subqueue_stopped() is false,
                                     no wake
netif_stop_subqueue()
return NETDEV_TX_BUSY

At that point the ring has plenty of room, but no BDs are in flight. No
completion will call netif_wake_subqueue().

The only other wake is netif_tx_start_all_queues() on open, and there
is no ndo_tx_timeout.

The same pattern is present at the LSO, non-GSO and post-xmit stop
sites. This patch raises the BD requirement for VLAN TSO skbs, so this
path now runs more often, including when only a few short frames are in
flight.

Would netif_txq_maybe_stop() / netif_txq_completed_wake() fit here?
Alternatively, an smp_mb() plus a re-check after stopping could be
paired with an smp_mb() in the cleaner before it tests the stopped bit.

[Severity: High]

This isn't a bug introduced by this patch, but it is in the same
software TSO path. Can enetc_map_tx_tso_buffs() write past its
tx_ring->tso_headers slot when the headers are longer than
TSO_HEADER_SIZE?

	hdr = tx_ring->tso_headers + i * TSO_HEADER_SIZE;
	tso_build_hdr(skb, hdr, &tso, data_len, total_len == 0);

tso_build_hdr() copies hdr_len bytes, where hdr_len comes from
tso_start() and covers the full L2+L3+L4 header. Nothing compares it
against TSO_HEADER_SIZE (256).

enetc_lso_map_hdr() does the same:

	hdr = tx_ring->tso_headers + *i * TSO_HEADER_SIZE;
	memcpy(hdr, skb->data, lso->hdr_len);

enetc_alloc_tx_resource() allocates exactly bd_count * TSO_HEADER_SIZE
bytes for this buffer. An oversized header therefore spills into the
next slot. For the last ring index, it writes past the end of the
dma_alloc_coherent() allocation.

One possible trigger is an IPv6 TCP socket with a sticky IPV6_RTHDR
type-4 SRH, which doesn't need CAP_NET_RAW. With 11 or more segments,
the header is 14 + 40 + 184 + 32 = 270 bytes.

The IPv6 setsockopt and net/core/tso.c side hasn't been fully traced in
this tree, so the trigger may still need confirming.

Should skbs with hdr_len > TSO_HEADER_SIZE fall back to software
segmentation or be dropped? Clearing GSO for them in ndo_features_check
would be another option.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021145314.1435049-1-vladimir.oltean%40nxp.com