Re: [PATCH net] net: stmmac: fix TX descriptor availability check for TSO traffic

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 10132 bytes --]

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0 (1)
> 
> - [High] stmmac_tso_xmit() now demands the exact descriptor count
>   returned by stmmac_tso_get_num_desc() (stmmac_main.c:4543-4544) but…
> 
> Pre-existing issues:
> - [High] proto_hdr_len in stmmac_tso_xmit() is declared as u8
>   (stmmac_main.c:4524) but is assigned the size_t result of…
> - [Low] The stmmac_tso_xmit() kernel-doc states "mss is fixed when enable
>   tso, so w/o programming the TDES3 ctx field." (stmmac_main.c:4512),…
> 
> --
> 
> net: stmmac: fix TX descriptor availability check for TSO traffic
> 
> This replaces the (skb->len - proto_hdr_len) / TSO_MAX_BUFF_SIZE + 1
> estimate in stmmac_tso_xmit() with stmmac_tso_get_num_desc(), which counts
> the header descriptor, the linear payload, every fragment and the MSS
> context descriptor. The new count is then used for the free descriptor
> check before the ring is programmed.
> 
> A few questions below.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 24656b35350b1..3566b33bc34e5 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -4454,6 +4454,26 @@ static bool stmmac_tso_valid_packet(struct sk_buff *skb)
> >  	       header_len + gso_size < 16383;
> >  }
> >  
> > +static int stmmac_tso_get_num_desc(struct stmmac_tx_queue *tx_q,
> > +				   struct sk_buff *skb, u32 pay_len)
> > +{
> > +	int i, ndesc = 1;
> > +
> > +	/* head payload */
> > +	ndesc += DIV_ROUND_UP(pay_len, TSO_MAX_BUFF_SIZE);
> > +	/* frag payload */
> > +	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
> > +		const skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
> > +
> > +		ndesc += DIV_ROUND_UP(skb_frag_size(frag),
> > +				      TSO_MAX_BUFF_SIZE);
> > +	}
> > +	/* MSS update requires a new descriptor */
> > +	ndesc += !!(skb_shinfo(skb)->gso_size != tx_q->mss);
> 
> [Severity: Low]
> This is a pre-existing issue and not introduced by this patch, but the new
> comment here now contradicts the stmmac_tso_xmit() kernel-doc that follows
> a few lines below, which still says:
> 
> 	 * mss is fixed when enable tso, so w/o programming the TDES3 ctx field.
> 
> stmmac_tso_xmit() does program a context descriptor whenever the skb MSS
> differs from the cached queue MSS:
> 
> 	if (mss != tx_q->mss) {
> 		...
> 		stmmac_set_mss(priv, mss_desc, mss);
> 		tx_q->mss = mss;
> 		tx_q->cur_tx = STMMAC_NEXT_ENTRY(tx_q->cur_tx, priv->dma_conf.dma_tx_size);
> 
> Would it make sense to drop or update that kernel-doc sentence while
> touching this area?

As pointed out by Sashiko, this issue is not introduced by this patch and it
can be fixed with a dedicated patch.

> 
> > +
> > +	return ndesc;
> > +}
> > +
> >  /**
> >   *  stmmac_tso_xmit - Tx entry point of the driver for oversized frames (TSO)
> >   *  @skb : the socket buffer
> > @@ -4497,10 +4517,10 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
> >  	struct stmmac_priv *priv = netdev_priv(dev);
> >  	unsigned int first_entry, entry, tx_packets;
> >  	struct stmmac_txq_stats *txq_stats;
> > +	int i, first_tx, nfrags, ndesc;
> >  	struct stmmac_tx_queue *tx_q;
> >  	bool set_ic, is_last_segment;
> >  	u32 pay_len, mss, queue;
> > -	int i, first_tx, nfrags;
> >  	u8 proto_hdr_len, hdr;
> >  	dma_addr_t des;
> >  
> > @@ -4513,14 +4533,15 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
> >  
> >  	/* Compute header lengths */
> >  	proto_hdr_len = stmmac_tso_header_size(skb);
> > +	pay_len = skb_headlen(skb) - proto_hdr_len; /* no frags */
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but the new pay_len computation
> inherits an existing truncation. proto_hdr_len is declared u8 while
> stmmac_tso_header_size() returns a size_t (skb_transport_offset() +
> sizeof(struct udphdr), or skb_tcp_all_headers()), and
> stmmac_tso_valid_packet() explicitly admits headers up to 1023 bytes into
> the hardware TSO path:
> 
> 	return header_len <= 1023 && gso_size >= 64 &&
> 	       header_len + gso_size < 16383;
> 
> For a header of 256..1023 bytes the store wraps modulo 256 (486 becomes
> 230, 256 becomes 0). The truncated value then defines the hardware
> contract, both for the payload start address:
> 
> 	stmmac_tso_allocator(priv, &entry, des + proto_hdr_len, pay_len,
> 			     (nfrags == 0), queue);
> 
> and for the descriptor lengths:
> 
> 	stmmac_prepare_tso_tx_desc(priv, first, 1, proto_hdr_len, 0, 1,
> 				   is_last_segment, hdr / 4,
> 				   skb->len - proto_hdr_len);
> 
> so the tail of the real header is handed to the engine as TCP/UDP payload
> while TDES3 declares more payload than the following descriptors describe
> (a header of exactly 256 also yields a zero length buffer1 with TSE set).
> A TCP over IPv6 socket carrying a few hundred bytes of sticky
> destination/hop-by-hop options makes skb_tcp_all_headers() exceed 255 while
> staying under 1023, so such an skb reaches stmmac_tso_xmit().
> 
> For the new pay_len the error biases upward, so the ring accounting stays
> safe, but should proto_hdr_len be widened to unsigned int (or size_t) since
> this patch now builds the descriptor count on it too?

As pointed out by Sashiko, this issue is not introduced by this patch and it
can be fixed with a dedicated patch.

> 
> >  	if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4)
> >  		hdr = sizeof(struct udphdr);
> >  	else
> >  		hdr = tcp_hdrlen(skb);
> >  
> > -	/* Desc availability based on threshold should be enough safe */
> > -	if (unlikely(stmmac_tx_avail(priv, queue) <
> > -		(((skb->len - proto_hdr_len) / TSO_MAX_BUFF_SIZE + 1)))) {
> > +	ndesc = stmmac_tso_get_num_desc(tx_q, skb, pay_len);
> > +	if (unlikely(stmmac_tx_avail(priv, queue) < ndesc)) {
> >  		if (!netif_tx_queue_stopped(netdev_get_tx_queue(dev, queue))) {
> >  			netif_tx_stop_queue(netdev_get_tx_queue(priv->dev,
> >  								queue));
> 
> [Severity: High]
> The new requirement is exact, but the TX flow control watermarks were left
> unchanged. Can this make the "Tx Ring full when queue awake" branch
> reachable for ordinary TSO traffic?
> 
> stmmac_tso_get_num_desc() returns at least nfrags + 2 for any TSO skb that
> has a linear payload, so with MAX_SKB_FRAGS = 17 plus an MSS change it asks
> for 20 descriptors, and up to roughly 23 when individual frags exceed
> TSO_MAX_BUFF_SIZE.
> 
> The tail of stmmac_tso_xmit() still stops the queue based on the old
> assumption:
> 
> 	if (unlikely(stmmac_tx_avail(priv, queue) <= (MAX_SKB_FRAGS + 1))) {
> 		netif_dbg(priv, hw, priv->dev, "%s: stop transmitted packets\n",
> 			  __func__);
> 		netif_tx_stop_queue(netdev_get_tx_queue(priv->dev, queue));
> 	}
> 
> so the queue is deliberately left awake with as few as 19 free slots, while
> the next TSO skb may need more. Does that leave a window where the stack
> submits an skb that does not fit, hitting the un-ratelimited netdev_err()
> in the ndo_start_xmit hot path together with netif_tx_stop_queue() and a
> NETDEV_TX_BUSY requeue?
> 
> The non-TSO path keeps the invariant the watermark was designed for,
> stmmac_xmit() needs nfrags + 1, which is always satisfied when the queue is
> awake:
> 
> 	if (unlikely(stmmac_tx_avail(priv, queue) < nfrags + 1)) {
> 
> The only place the queue is re-armed is stmmac_tx_clean():
> 
> 	if (unlikely(netif_tx_queue_stopped(netdev_get_tx_queue(priv->dev, queue))) &&
> 	    stmmac_tx_avail(priv, queue) > STMMAC_TX_THRESH(priv)) {
> 		...
> 		netif_tx_wake_queue(netdev_get_tx_queue(priv->dev, queue));
> 
> STMMAC_TX_THRESH() is dma_tx_size / 4, which is 16 on the 64 entry ring
> stmmac_set_ringparam() accepts (DMA_MIN_TX_SIZE in common.h). On such a
> ring the wake threshold is itself below the new requirement, so can the
> queue be woken while the skb still does not fit, producing a repeating
> stop/wake/TX_BUSY cycle with repeated error logging?
> 
> Should the stop and wake watermarks be raised to the worst case TSO
> descriptor count, or the skb linearized/pushed to software GSO, as part of
> this change?

The goal of this patch is to fix the memory corruption triggered by a TSO skb
that does not fit in the tx queue available memory space.
The case described above will be managed by the new condition added at the
beginning of stmmac_tso_xmit(). I think we can change the stop/wake tx queue
watermark with a dedicated patch that we do not need to backport (I guess this
is mostly an optimization). However, if you prefer to have it in a single patch,
I will add it in this one.

> 
> >  @@ -4532,8 +4553,6 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
> >  		return NETDEV_TX_BUSY;
> >  	}
> 
> One more on this branch: it sets XOFF without re-checking availability and
> without arming a wake source. If the ring drains concurrently
> (dirty_tx == cur_tx, no further TX completion and no NAPI run), does the
> XOFF state persist until dev_watchdog fires ndo_tx_timeout?

I guess this corner case has not been introduced by this patch since the same
approach is present in stmmac_xmit(). I think it should be addressed with a
dedicated patch. Moreover, is the corner case fixed by the ndo_tx_timeout
callback?

Regards,
Lorenzo

> 
> Since the patch is tagged for stable, would it be worth describing the new
> NETDEV_TX_BUSY and error logging behaviour in the commit message?
> 
> >  
> > -	pay_len = skb_headlen(skb) - proto_hdr_len; /* no frags */
> > -
> >  	mss = skb_shinfo(skb)->gso_size;
> >  
> >  	/* set new MSS value if needed */
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907-stmmac-fix-tso-nfrags-check-v1-1-328459906cdb%40oss.qualcomm.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]