Re: [PATCH net 6/6] net: stmmac: fix XDP RX buffer DMA sync length

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · 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] In stmmac_rx()
  (drivers/net/ethernet/stmicro/stmmac/stmmac_main.c), the length passed…

--

net: stmmac: fix XDP RX buffer DMA sync length

This commit changes the XDP path in stmmac_rx() so that pre_len and
sync_len are measured from data_hard_start, and buf->page_offset is no
longer subtracted. That matches the page_pool DMA window, which now
starts at offset 0. The goal is for page_pool_put_page() on the
consumed XDP path to sync the whole region the CPU touched.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 32b2c5834c340..c51358c51fae7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5980,8 +5980,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  			xdp_prepare_buff(&ctx.xdp, page_address(buf->page),
>  					 buf->page_offset, buf1_len, true);
>  
> -			pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start -
> -				  buf->page_offset;
> +			pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start;
>  
>  			ctx.priv = priv;
>  			ctx.desc = p;
> @@ -5991,8 +5990,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  			/* Due xdp_adjust_tail: DMA sync for_device
>  			 * cover max len CPU touch
>  			 */
> -			sync_len = ctx.xdp.data_end - ctx.xdp.data_hard_start -
> -				   buf->page_offset;
> +			sync_len = ctx.xdp.data_end - ctx.xdp.data_hard_start;
>  			sync_len = max(sync_len, pre_len);

[Severity: High]

Does this cover the furthest point the CPU touched if the XDP program
grows the tail and then shrinks it again before it returns?

The comment says the sync covers the "max len CPU touch". However,
max(sync_len, pre_len) only looks at data_end before the program runs
and after it returns. bpf_xdp_adjust_tail() writes to the grown area
when the tail grows:

net/core/filter.c:bpf_xdp_adjust_tail() {
    ...
	/* Clear memory area on grow, can contain uninit kernel memory */
	if (offset > 0)
		memset(xdp->data_end, 0, offset);

	xdp->data_end = data_end;
    ...
}

A later negative adjust only moves data_end back.

Take a 64 byte frame at page_offset 256 and a program that does
adjust_tail(+512), then adjust_tail(-512), then returns XDP_DROP. Both
pre_len and sync_len end up at the original end, around 316. The memset
dirtied the cache lines in [316, 828), but this call does not clean
them:

    page_pool_put_page(rx_q->page_pool,
                       virt_to_head_page(ctx.xdp.data),
                       sync_len, true);

__page_pool_dma_sync_for_device() caps the length at p.max_len but
never extends it. The direct recycle cache does no further sync.
stmmac_rx_refill() gives the buffer back to the device after dma_wmb(),
with no more cache maintenance.

max_len is dma_buf_sz + stmmac_rx_offset(priv), which is 1792 with
dma_buf_sz = 1536. So that range lies inside the window the device DMAs
into.

On a non-coherent platform, could one of those dirty lines be evicted
after the device has written the next frame into the recycled page, and
corrupt that frame? It looks like the same failure the commit message
describes:

    Dirty cache lines in the tail of the frame can be written back
    after the device has DMA'd a new frame into that page.

The commit message names "grow the tail" as a trigger and says "the
whole CPU-touched window is synced". The max(pre, post) approach was
already there before this patch.

Would it be better for the STMMAC_XDP_CONSUMED path to sync the full
pool window instead of checking only the two endpoints? Passing
p.max_len, or using page_pool_recycle_direct(), would do that.

>  
>  			/* For Not XDP_PASS verdict */

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