Re: [PATCH net 2/6] net: stmmac: drop split frames on AF_XDP zero-copy RX

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (2) · Medium: 1 (2) · 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).

- [Medium] The commit message does not describe one behaviour change the
  patch makes, and nothing explains the second Fixes tag.

Pre-existing issues:
- [High] On GMAC4/XGMAC, stmmac_rx() in native (non-ZC) XDP mode still
  handles split RX frames wrongly at this commit.
- [High] On AF_XDP zero-copy XDP_PASS, stmmac_dispatch_skb_zc() ignores
  the csum_none bit from the descriptor status.
- [Medium] In stmmac_rx(), if napi_build_skb() fails on the first
  descriptor of a multi-descriptor frame, the code does `rx_dropped++;…
- [Medium] priv->xstats.rx_dropped and priv->xstats.rx_errors are device-
  wide `unsigned long` counters.

--

net: stmmac: drop split frames on AF_XDP zero-copy RX

When stmmac_rx_zc() drops a frame that spans more than one RX descriptor,
it now sets error, so the remaining descriptors are freed and the tail is
not handled as a new frame. The dwmac100/dwmac1000 descriptor callbacks now
return rx_not_ls together with discard_frame for non-last descriptors. The
discard_frame checks in stmmac_rx_zc() and stmmac_rx() change from == to &.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index fba21599a5e15..3fa2c15222d88 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5710,18 +5710,21 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
>  		if (priv->extend_desc)
>  			stmmac_rx_extended_status(priv, &priv->xstats,
>  						  rx_q->dma_erx + entry);
> -		if (unlikely(status == discard_frame)) {
> -			xsk_buff_free(buf->xdp);
> -			buf->xdp = NULL;
> -			dirty++;
> +
> +		if (unlikely(!error && (status & discard_frame))) {
>  			error = 1;
>  			if (!priv->hwts_rx_en)
>  				rx_errors++;
>  		}

[Severity: High]
This is a pre-existing issue, but with this test, a dwmac4 frame that only
has csum_none set still reaches the XDP program. That covers an L4 payload
checksum error without RDES3 ERROR_SUMMARY. On XDP_PASS the frame goes to
stmmac_dispatch_skb_zc(), and that function never receives status:

stmmac_dispatch_skb_zc() {
    ...
	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb))
		skb_checksum_none_assert(skb);
	else
		skb->ip_summed = CHECKSUM_UNNECESSARY;
    ...
}

dwmac4_dma_rx_chan_op_mode() always sets MTL_OP_MODE_DIS_TCP_EF, so the
hardware does not drop these frames. dwmac4_wrback_get_rx_status() reports
them with no discard_frame:

	if (rdes1 & RDES1_IP_PAYLOAD_ERROR) {
		x->ip_payload_err++;
		ret |= csum_none;
	}

Can a packet with a bad TCP/UDP checksum get marked CHECKSUM_UNNECESSARY
on the AF_XDP zero-copy XDP_PASS path and be delivered to sockets?

stmmac_rx() checks (status & csum_none) before it sets
CHECKSUM_UNNECESSARY. Should stmmac_dispatch_skb_zc() do the same check?
The gap is still there at the end of the series.

[ ... ]

> @@ -5730,8 +5733,9 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
>  		if (likely(status & rx_not_ls)) {
>  			xsk_buff_free(buf->xdp);
>  			buf->xdp = NULL;
> +			rx_dropped++;
> +			error = 1;
>  			dirty++;
> -			count++;
>  			goto read_again;
>  		}

[Severity: Medium]
This is a pre-existing issue, but the rx_dropped count added here is added
to the device-wide counter at the end of stmmac_rx_zc():

	priv->xstats.rx_dropped += rx_dropped;
	priv->xstats.rx_errors += rx_errors;

stmmac_rx() does the same thing, and stmmac_dispatch_skb_zc() does
priv->xstats.rx_dropped++. These are plain unsigned long read-modify-writes
with no lock, no atomic and no per-queue split.

NAPI polls for different RX queues can finish at the same time on
different CPUs. Can they lose increments here, so that stmmac_get_stats64()
under-reports rx_dropped and rx_errors?

[ ... ]

> @@ -5898,7 +5902,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  
>  		if (priv->extend_desc)
>  			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
> -		if (unlikely(status == discard_frame)) {
> +
> +		if (unlikely(status & discard_frame)) {
>  			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
>  			buf->page = NULL;
>  			error = 1;

[Severity: Medium]
The commit message does not seem to describe this change or explain the
second Fixes tag:

    Fixes: 644b8437ccef9e ("net: stmmac: dwmac4: report Rx checksum errors in status")

Is this change from == to & the actual fix for 644b8437ccef9e?
dwmac4_wrback_get_rx_status() sets ret = discard_frame on
RDES3_ERROR_SUMMARY. It then ORs in csum_none on RDES1_IP_HDR_ERROR or
RDES1_IP_PAYLOAD_ERROR, so it can return discard_frame | csum_none.

The old status == discard_frame test missed that value. Frames with CRC,
overflow, watchdog or giant errors that also failed the IP checksum were
passed to the stack in stmmac_rx(), or to XDP/XSK in stmmac_rx_zc().

The fix looks correct. However, the body only covers the AF_XDP zero-copy
1:1 drop and the dwmac100/dwmac1000 descriptor change, and this hunk
changes the regular non-XDP RX path on every GMAC4/XGMAC system.

Could the commit message describe this change and the 644b8437ccef9e
regression it fixes?

[Severity: High]
This is a pre-existing issue, but does stmmac_rx() in native XDP mode
still mishandle split frames on GMAC4/XGMAC? For a non-last descriptor,
dwmac4_wrback_get_rx_status() returns rx_not_ls without discard_frame:

	if (likely(!(rdes3 & RDES3_LAST_DESCRIPTOR)))
		return rx_not_ls;

So the discard_frame drain above never starts, and the XDP program sees
the first fragment as a complete frame. On XDP_DROP:

				if (xdp_res & STMMAC_XDP_CONSUMED) {
					...
					skb = NULL;

					if (unlikely((status & rx_not_ls)))
						goto read_again;

skb is NULL and error is still 0. The tail then goes through XDP again
and can be passed to the stack as a new packet.

On XDP_TX or XDP_REDIRECT the code does goto next, which clears
in_progress. The next iteration resets len to 0, so stmmac_rx_buf1_len()
reports the full dma_buf_sz for the tail. Can that expose stale page_pool
contents that DMA did not write for this frame?

This is the same kind of bug the patch fixes for stmmac_rx_zc(), but here
stmmac_rx() is only covered for dwmac100/dwmac1000. The next patch in the
series, "net: stmmac: drop multi-buffer frames in XDP mode", appears to
fix it. That patch sets error on rx_not_ls in XDP mode, drops frames that
use buf2_len, and makes the XDP_CONSUMED path goto next.

[Severity: Medium]
This is a pre-existing issue, but stmmac_rx() has a similar problem when
napi_build_skb() fails on the first descriptor of a multi-descriptor
frame:

			skb = napi_build_skb(page_address(buf->page),
					     rx_q->napi_skb_frag_size);
			if (!skb) {
				page_pool_recycle_direct(rx_q->page_pool,
							 buf->page);
				buf->page = NULL;
				rx_dropped++;
				count++;
				goto drain_data;
			}

error is not set, so drain_data jumps to read_again with skb == NULL and
error == 0. If allocation succeeds on the next descriptor of the same
frame, the if (!skb) branch builds a new skb from only the tail fragment.

At the last descriptor, pskb_trim(skb, len) does nothing. len still
includes the dropped first fragment, so it is larger than skb->len.

Does this pass the tail to the stack as a standalone packet with its FCS
still attached, and count the frame against the budget twice? Should this
path set error so the rest of the frame is drained?

At the end of the series this only happens in non-XDP mode, because XDP
mode now sets error on rx_not_ls.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd%40oss.qualcomm.com