Re: [PATCH net-next v9] net: stmmac: rework stmmac_rx to support XDP rx multi-buff

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 1

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 skb head truesize grows on every received frame, including
  with no XDP program loaded, and the commit message does not say so.
- [Low] The commit message overstates one change.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  makes it wider.

--

net: stmmac: rework stmmac_rx to support XDP rx multi-buff

This reworks stmmac_rx() to collect all the descriptors of a frame into an
xdp_buff, so programs with frags support see the whole packet. On XDP_PASS
the skb is built from the xdp_buff head and its fragments. The patch also
saves the in-progress xdp_buff in rx_q->state across NAPI polls, advertises
NETDEV_XDP_ACT_RX_SG, and strips the FCS with a driver-local tail shrink.

> - carry the XDP metadata written with bpf_xdp_adjust_meta() into the skb
>   via skb_metadata_set(), so it is no longer dropped and TC/BPF programs
>   can read it through data_meta;

[Severity: Low]
Is this item accurate? The XDP_PASS path in stmmac_rx() already did this
before the patch. The patch removes these lines from stmmac_rx():

-			metasize = ctx.xdp.data - ctx.xdp.data_meta;
-			if (metasize)
-				skb_metadata_set(skb, metasize);

The same logic now lives in stmmac_build_skb().

Metadata on linear frames was not being dropped before. The only frames
that never got metadata were multi-descriptor or buf2 frames, and XDP mode
dropped those outright. The commit message already covers that elsewhere.

Could this item be reworded so it does not read as a fix for a metadata
loss bug?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index a2177777727ef..c215cc70caa19 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -2290,10 +2294,6 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
>  			 SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>  	num_pages = DIV_ROUND_UP(dma_buf_sz_pad, PAGE_SIZE);
>  
> -	rx_q->queue_index = queue;
> -	rx_q->priv_data = priv;
> -	rx_q->napi_skb_frag_size = num_pages * PAGE_SIZE;
> -
>  	pp_params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
>  	pp_params.pool_size = dma_conf->dma_rx_size;
>  	pp_params.order = order_base_2(num_pages);
> @@ -2321,6 +2321,10 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
>  	if (!addr)
>  		return -ENOMEM;
>  
> +	rx_q->queue_index = queue;
> +	rx_q->priv_data = priv;
> +	rx_q->napi_skb_frag_size = PAGE_SIZE << pp_params.order;
> +

[Severity: Medium]
Does this change the skb head truesize for every received frame, even when
no XDP program is attached?

Before the patch the head size was num_pages * PAGE_SIZE. Now it is rounded
up to the page_pool allocation order, and skb->truesize is derived from it
here:

stmmac_rx()
	bufsz = rx_q->napi_skb_frag_size;
	...
	xdp_init_buff(&ctx.xdp, bufsz, &rx_q->xdp_rxq);

stmmac_build_skb()
	skb = napi_build_skb(xdp->data_hard_start, xdp->frame_sz);

On 4K pages, a dma_buf_sz of 8188 (BUF_SIZE_8KiB) gives num_pages = 3, so
the head grows from 12K to 16K. A dma_buf_sz of 16368 (BUF_SIZE_16KiB)
gives num_pages = 5, so the head grows from 20K to 32K.

That shrinks the effective socket receive buffer for jumbo MTU users who
never load XDP. The commit message calls out the fragment truesize change
as a behavior change, but not this one. Should this be mentioned too?

[ ... ]

> @@ -5962,120 +6073,79 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)

[ ... ]

> +		skb = stmmac_xdp_run_prog(priv, &ctx.xdp);
> +		/* For Not XDP_PASS verdict */
> +		if (IS_ERR(skb)) {
> +			unsigned int xdp_res = -PTR_ERR(skb);
> +
> +			if (xdp_res & STMMAC_XDP_CONSUMED) {
> +				stmmac_xdp_put_buff(rx_q, &ctx.xdp, true);
> +				rx_dropped++;
> +			} else if (xdp_res & (STMMAC_XDP_TX |
> +					      STMMAC_XDP_REDIRECT)) {
> +				xdp_status |= xdp_res;
> +			}
> +			goto next;
> +		}

[Severity: High]
This isn't a bug introduced by this patch, but multi-buffer frames can now
reach XDP_TX, which makes it worse. Does XDP_TX completion recycle pages
into this RX queue's page_pool direct cache from the wrong NAPI context?

stmmac_xdp_xmit_back() picks the TX queue by CPU, not by RX queue:

stmmac_xdp_get_tx_queue()
	while (index >= priv->plat->tx_queues_to_use)
		index -= priv->plat->tx_queues_to_use;

Completion then runs in that channel's tx_napi:

stmmac_napi_poll_tx()
  stmmac_tx_clean()
    xdp_return_frame_rx_napi(xdpf)
      __xdp_return(..., napi_direct=true)    /* head and every frag */
        page_pool_put_unrefed_netmem(..., allow_direct=true)
          page_pool_recycle_in_cache()

stmmac_tx_clean() {
	...
		if (xdpf &&
		    tx_q->tx_skbuff_dma[entry].buf_type == STMMAC_TXBUF_T_XDP_TX) {
			xdp_return_frame_rx_napi(xdpf);
	...
}

When allow_direct is already true, page_pool_put_unrefed_netmem() skips the
page_pool_napi_local() check. That means pool->alloc.cache and
pool->alloc.count are updated without a lock.

The tx_napi is a separate NAPI instance from the rx_napi that owns the
pool. Meanwhile the owning rx_napi can be on another CPU running
stmmac_rx_refill() or the allow_direct=true puts in stmmac_xdp_put_buff(),
stmmac_build_xdp_frags() and stmmac_xdp_shrink_tail().

Can this corrupt the alloc cache? If so, the same page could be handed to
two RX descriptors, or recycled while an skb still holds it.

The race dates back to XDP_TX support. With this patch, though, each
XDP_TX frame can push the head plus several fragment pages down this path.
Would returning XDP_TX frames with xdp_return_frame() in stmmac_tx_clean()
avoid this?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-stmmac-rx-mb-v9-1-c099c923664b%40oss.qualcomm.com