Re: [PATCH net-next v3 1/2] net: stmmac: take ownership of saved RX state at poll entry

[netdev-bot+sashiko@kernel.org]

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

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

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] At the reviewed commit, stmmac_rx() resets skb/error/len at the
  top of the loop only when `!in_progress`, instead of whenever count >…
- [Low] The commit message leaves out one of the behaviour changes.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it remains at
  the end of the series and the patch reworks the same continuation-…

--

net: stmmac: take ownership of saved RX state at poll entry

This patch changes stmmac_rx() and stmmac_rx_zc() so they take the saved
RX state at poll entry and clear it right away. The state is saved again
only if the frame is still incomplete, which a new in_progress flag tracks.
The patch also frees any saved partial skb in
__free_dma_rx_desc_resources() and stmmac_reset_rx_queue().

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 0c381ae0d0ffa..b2d20628ed214 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -5652,6 +5657,8 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
>  		if (!buf->xdp)
>  			break;
>  
> +		in_progress = status & rx_not_ls;
> +
>  		if (priv->extend_desc)
>  			stmmac_rx_extended_status(priv, &priv->xstats,
>  						  rx_q->dma_erx + entry);

[Severity: High]
This isn't a bug introduced by this patch, but this patch reworks the
continuation state in stmmac_rx_zc(), so it seems worth raising here. Can
a frame that spans more than one descriptor be delivered with the wrong
length?

The non-LS branch drops the buffer without updating len or setting error:

	/* XSK pool expects RX frame 1:1 mapped to XSK buffer */
	if (likely(status & rx_not_ls)) {
		xsk_buff_free(buf->xdp);
		buf->xdp = NULL;
		dirty++;
		count++;
		goto read_again;
	}

The LS descriptor is then handled as a complete frame with len == 0:

	buf1_len = stmmac_rx_buf1_len(priv, p, status, len);
	len += buf1_len;
	...
	buf->xdp->data_end = buf->xdp->data + buf1_len;

stmmac_rx_buf1_len() returns min(dma_buf_sz, plen - len), so buf1_len
comes from the total frame length. The LS buffer only holds the tail of
the frame, though. Also, the hardware buffer size for an XSK queue is
xsk_pool_get_rx_frame_size() (see stmmac_set_queue_rx_buf_size()), not
dma_buf_sz.

stmmac_xdp_enable_pool() only requires frame_size >= ETH_FRAME_LEN +
VLAN_HLEN * 2, and its comment says "XDP ZC does not span multiple
frame". However, dwmac4 sets GMAC_CONFIG_JE in GMAC_CORE_INIT. So jumbo
frames from the wire are accepted and split across descriptors.

For example, with 4K chunks frame_size is 3840. A 5000-byte frame leaves
1160 real bytes in the LS buffer, but data_end = data + 1532. The XDP
program, AF_XDP or stmmac_construct_skb_zc() would then see a frame whose
L2 header comes from the sender's payload, followed by 372 stale umem
bytes. If frame_size is in [1522, 1536), data_end can also run a few
bytes past the end of the chunk.

The new in_progress tracking only carries error and len, and both are 0
here, so it doesn't change this.

Would it work to set error = 1 for a non-LS descriptor in ZC mode? The new
in_progress tracking would then carry the drop state across poll and
DMA-owned boundaries. The follow-up patch in the series doesn't touch
stmmac_rx_zc().

[ ... ]

> @@ -5788,25 +5796,28 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)

[ ... ]

> -		if (!count && rx_q->state_saved) {
> -			skb = rx_q->state.skb;
> -			error = rx_q->state.error;
> -			len = rx_q->state.len;
> -		} else {
> -			rx_q->state_saved = false;
> +		if (!in_progress) {
>  			skb = NULL;
>  			error = 0;
>  			len = 0;
>  		}
> -
>  read_again:
>  		if (count >= limit)
>  			break;
> @@ -5835,6 +5846,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  
>  		prefetch(np);
>  
> +		in_progress = status & rx_not_ls;
> +
>  		if (priv->extend_desc)
>  			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
>  		if (unlikely(status == discard_frame)) {

[Severity: High]
Can len carry over into a new frame when an XDP program returns XDP_TX
or XDP_REDIRECT on the first, non-LS descriptor of a multi-descriptor
frame?

in_progress is set from status & rx_not_ls before the XDP verdict, and
the TX/REDIRECT path doesn't clear it:

	} else if (xdp_res & (STMMAC_XDP_TX |
			      STMMAC_XDP_REDIRECT)) {
		xdp_status |= xdp_res;
		buf->page = NULL;
		skb = NULL;
		count++;
		continue;
	}

The next iteration skips the !in_progress reset. len is kept while skb
is NULL, so the continuation descriptor is treated as the start of a
frame.

If plen - len is 1 to 3 bytes, stmmac_rx_buf1_len() returns that value
and stmmac_rx_buf2_len() returns 0. Then this code:

	if (stmmac_xdp_is_enabled(priv) && !buf2_len) {
		buf1_len -= ETH_FCS_LEN;
		fcs_stripped = true;
	}

wraps buf1_len to about 0xfffffffd. xdp_prepare_buff() then sets
data_end about 4GiB past data. On XDP_PASS, this later code would hit
skb_over_panic():

	buf1_len = ctx.xdp.data_end - ctx.xdp.data;
	...
	skb_put(skb, buf1_len);

dwmac4 sets GMAC_CONFIG_JE, so a remote sender can choose the frame
length.

Before this patch, the XDP_CONSUMED plus rx_not_ls goto read_again path
already kept len. The mid-frame TX/REDIRECT route looks new, though.

The next patch in the series, "net: stmmac: rework stmmac_rx to support
XDP rx multi-buff", removes this path. After it, the XDP program only runs
after the LS descriptor, and the FCS is stripped by
stmmac_xdp_shrink_tail(), which bounds-checks the offset.

This patch carries Fixes: tags. Could it be backported to stable without
that rework?

[ ... ]

> @@ -8339,6 +8352,9 @@ static void stmmac_reset_rx_queue(struct stmmac_priv *priv, u32 queue)
>  {
>  	struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
>  
> +	dev_kfree_skb_any(rx_q->state.skb);
> +	rx_q->state.skb = NULL;
> +	rx_q->state_saved = false;
>  	rx_q->cur_rx = 0;
>  	rx_q->dirty_rx = 0;
>  }

[Severity: Low]
The commit message says:

    Release a saved partial skb when the RX ring is destroyed.

That covers the __free_dma_rx_desc_resources() hunk. The
stmmac_reset_rx_queue() hunk doesn't destroy the ring, though. It runs
through stmmac_reset_queues_param() from __stmmac_open(),
stmmac_xdp_open() and stmmac_resume(), and is also called directly from
stmmac_enable_rx_queue().

Before this patch, stmmac_reset_rx_queue() only cleared cur_rx and
dirty_rx. So after stmmac_resume(), the first poll could add descriptors
from the re-armed ring to the partial skb saved before suspend.

Could the commit message also mention the open, resume and queue-enable
change? The patch carries Fixes: tags and may go to stable.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004-stmmac-rx-mb-v3-0-50fa171af9ec%40oss.qualcomm.com