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

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

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

> 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:

This issue has not been introduced in this patch and it needs to be fixed with
a dedicated patch.

> 
> 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?

This issue has not been introduced in this patch and it needs to be fixed with
a dedicated patch.

> 
> [ ... ]
> 
> > @@ -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.

I will fix the commit message in v2.

> 
> [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.

This issue has not been introduced in this patch and it needs to be fixed with
a dedicated patch.

Regards,
Lorenzo

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

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