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

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

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

On Oct 27, netdev-bot+sashiko@kernel.org wrote:
> 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?

ack, I agree this is a real problem. I will fix it in v2.

Regards,
Lorenzo

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

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