Re: [PATCH net 1/6] net: stmmac: take ownership of saved RX state at poll entry

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

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

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 (1) · 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] The commit message says that on resume a saved partial frame
>   would be resumed "with descriptors from the freshly initialized ring",…
> 
> Pre-existing issues:
> - [Medium] In stmmac_rx()
>   (drivers/net/ethernet/stmicro/stmmac/stmmac_main.c), the…
> 
> --
> 
> net: stmmac: take ownership of saved RX state at poll entry
> 
> The saved partial-frame RX state is now taken and cleared at the start of
> stmmac_rx() and stmmac_rx_zc(). It is saved again only if the frame is
> still incomplete when the poll ends. The saved skb is also dropped in
> stmmac_reset_rx_queue() and __free_dma_rx_desc_resources().
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index b20e5038eeadd..fba21599a5e15 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> 
> [ ... ]
> 
> > @@ -6032,10 +6042,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> >  		/* Remove FCS if needed */
> >  		if (!fcs_stripped && pskb_trim(skb, len)) {
> >  			dev_kfree_skb_any(skb);
> > -			skb = NULL;
> >  			rx_dropped++;
> > -			count++;
> > -			continue;
> > +			goto next;
> >  		}
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but can a multi-descriptor
> frame reach the stack as a truncated fragment when napi_build_skb() fails
> on a descriptor that is not the last one?

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

> 
> The allocation failure path in stmmac_rx() does not set error:
> 
> 	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;
> 	}
> 
> When status & rx_not_ls is set, drain_data jumps back to read_again. At
> that point skb == NULL and error == 0, and len still holds the bytes of
> the dropped prefix.
> 
> With no XDP program attached, stmmac_xdp_run_prog() returns NULL. The next
> descriptor of the same frame then builds a new skb from a mid-frame
> buffer.
> 
> On the last descriptor, len still includes the dropped prefix. So
> pskb_trim() does nothing, because len is not smaller than skb->len:
> 
> 	return (len < skb->len) ? __pskb_trim(skb, len) : 0;
> 
> Wouldn't that pass a mid-payload fragment, with its FCS still attached,
> to napi_gro_receive()? Its ip_summed would come from the status of the
> whole frame, and rx_bytes would be counted with the full-frame len.
> 
> The same thing seems to happen across a poll boundary. In that case
> in_progress saves skb == NULL, error == 0 and len.
> 
> The old status & rx_not_ls save condition behaved the same way. Later in
> the series, "net: stmmac: drop multi-buffer frames in XDP mode" sets
> error for rx_not_ls when XDP is enabled. The non-XDP path is unchanged.
> 
> [ ... ]
> 
> > @@ -8412,6 +8421,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: High]
> The commit message says that without this drain, a partial frame saved
> before suspend would be resumed "with descriptors from the freshly
> initialized ring". Is the ring actually freshly initialized on the
> stmmac_resume() path?
> 
> stmmac_resume() does:
> 
> 	stmmac_reset_queues_param(priv);
> 
> 	stmmac_free_tx_skbufs(priv);
> 	stmmac_clear_descriptors(priv, &priv->dma_conf);
> 
> 	ret = stmmac_hw_setup(ndev);
> 
> stmmac_clear_rx_descriptors() calls stmmac_init_rx_desc() on every RX
> descriptor, and that only sets ownership bits:
> 
> - dwmac4_rd_init_rx_desc() and dwxgmac2_init_rx_desc() OR OWN|BUF1V into
>   des3.
> - enh_desc_init_rx_desc() and ndesc_init_rx_desc() OR RDES0_OWN into
>   des0.
> 
> Buffer addresses are never reprogrammed and empty slots are not refilled.
> stmmac_init_dma_engine() then sets the RX tail pointer to buf_alloc_num,
> which gives every descriptor back to the DMA.
> 
> Suppose page_pool_alloc_pages() failed in stmmac_rx_refill() for the
> slots used by the saved partial frame. The loop then breaks before
> stmmac_set_desc_addr():
> 
> 	if (!buf->page) {
> 		buf->page = page_pool_alloc_pages(rx_q->page_pool, gfp);
> 		if (!buf->page)
> 			break;
> 	}
> 
> Those descriptors keep the DMA addresses of pages that state.skb now
> owns, and buf->page == NULL.
> 
> With this change, stmmac_reset_rx_queue() returns those pages to the
> page_pool through dev_kfree_skb_any(). The same descriptors are then
> re-armed and the DMA is restarted.
> 
> Can the device then DMA into pages that have already been recycled or
> freed?
> 
> There is a separate problem on dwmac4 and xgmac. A descriptor that was
> written back but not refilled before suspend still holds write-back data
> in des0/des1. For example, dwmac4_wrback_get_rx_vlan_tci() reads the VLAN
> tag from des0. Re-arming that descriptor would make the DMA treat the
> write-back data as a buffer address.
> 
> The re-arm-without-refill behaviour predates this patch: an earlier
> upstream re-init on resume was reverted. Before this patch, the stale skb
> would also have been completed and freed on the first poll after resume,
> while those descriptors were still armed. So this change frees the pages
> earlier rather than creating the problem.
> 
> Since the resume handling depends on the ring being freshly initialized,
> should the commit message be corrected? And should the resume path refill
> and reprogram the RX descriptors before the DMA is restarted?

I will rework the commit message in v2 but the reported issue has not been
introduced by 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 --]