Re: [PATCH net-next 2/3] net: stmmac: Use pagepool instead of streaming buffers for Tx

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

New issues:
- [High] `page_pool_recycle_direct()` is called outside of NAPI context, leading to potential lockless ring corruption.
- [Medium] Unnecessary `PP_FLAG_DMA_SYNC_DEV` flag causes redundant full-page DMA syncs, defeating performance goals.

Pre-existing issues:
- [High] Unconditional NULL pointer dereference in `dma_free_tx_skbufs` if `tx_skbuff_dma` allocation fails.
- [High] DMA mapping and `page_pool` memory leak when `skb_frag_dma_map()` fails during transmission.
- [High] Double free and Use-After-Free of TX queue resources due to dangling pointers on teardown.
--

--- Patch [2]: [PATCH net-next 2/3] net: stmmac: Use pagepool instead of streaming buffers for Tx ---
commit a91ef91a303112e59c32621c9138a02b37e37769
Author: Nazim Amirul <[email protected]>
Subject: net: stmmac: Use pagepool instead of streaming buffers for Tx

Use page pool buffers instead of streaming skb buffers in stmmac_xmit
when snps,pagepool-tx-buf-quirk is enabled. As page pool buffers are
recycled, the overhead of dma map/unmap for each transmit can be avoided.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ee44bd6f4d48e..4aa6b1d1507bf 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1712,7 +1712,10 @@ static void stmmac_free_tx_buffer(struct stmmac_priv *priv,
>  
>  	if (tx_q->tx_skbuff_dma[i].buf &&
>  	    tx_q->tx_skbuff_dma[i].buf_type != STMMAC_TXBUF_T_XDP_TX) {

[Severity: High]
This is a pre-existing issue, but is it possible for this code to trigger a
NULL pointer dereference?

If the memory allocation for tx_q->tx_skbuff_dma fails in
__alloc_dma_tx_desc_resources() (in stmmac_main.c), it returns -ENOMEM. The
error path then calls free_dma_tx_desc_resources(), which loops over the
queues and calls __free_dma_tx_desc_resources(). This ultimately calls
dma_free_tx_skbufs(), which unconditionally indexes into
tx_q->tx_skbuff_dma[i] via stmmac_free_tx_buffer().

Since tx_q->tx_skbuff_dma is NULL, would this result in a panic during
device setup or XDP re-initialization on memory pressure?

> -		if (tx_q->tx_skbuff_dma[i].map_as_page)
> +		if (tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_DMA)
> +			page_pool_recycle_direct(tx_q->page_pool,
> +						 tx_q->tx_skbuff_dma[i].page);

[Severity: High]
Is it safe to call page_pool_recycle_direct() here outside of NAPI context?

The page_pool_recycle_direct() API requires execution in NAPI softirq context
on the same CPU, as it bypasses locking (allow_direct=true). However,
stmmac_free_tx_buffer() is called by dma_free_tx_skbufs() during device
teardown (process context) and by stmmac_tx_err(), which is triggered from
stmmac_napi_check() directly inside the hardirq handler when a TX error
occurs.

Could calling this lockless API from hardirq or process contexts race with
NAPI polling and lead to internal page pool ring corruption?

> +		else if (tx_q->tx_skbuff_dma[i].map_as_page)
>  			dma_unmap_page(priv->device,
>  				       tx_q->tx_skbuff_dma[i].buf,
>  				       tx_q->tx_skbuff_dma[i].len,
[ ... ]
> @@ -2201,6 +2205,8 @@ static void __free_dma_tx_desc_resources(struct stmmac_priv *priv,
>  
>  	kfree(tx_q->tx_skbuff_dma);
>  	kfree(tx_q->tx_skbuff);
> +	if (tx_q->page_pool)
> +		page_pool_destroy(tx_q->page_pool);

[Severity: High]
This is a pre-existing issue, but does this code leave dangling pointers that
could cause a double free or use-after-free on teardown?

Memory arrays (tx_q->tx_skbuff, tx_q->tx_skbuff_dma) and the newly added
tx_q->page_pool are freed or destroyed, but their pointers are not nullified.
If a subsequent resource allocation fails (e.g. during stmmac_xdp_open()),
the error path calls free_dma_tx_desc_resources(), which loops over all
queues.

For queues that were not successfully re-initialized, it re-executes kfree()
and page_pool_destroy() on the stale, dangling pointers.

>  }
>  
>  static void free_dma_tx_desc_resources(struct stmmac_priv *priv,
[ ... ]
> @@ -2335,12 +2341,32 @@ static int __alloc_dma_tx_desc_resources(struct stmmac_priv *priv,
>  					 u32 queue)
>  {
>  	struct stmmac_tx_queue *tx_q = &dma_conf->tx_queue[queue];
> +	struct page_pool_params pp_params = { 0 };
> +	unsigned int num_pages;
>  	size_t size;
>  	void *addr;
> +	int ret;
>  
>  	tx_q->queue_index = queue;
>  	tx_q->priv_data = priv;
>  
> +	pp_params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;

[Severity: Medium]
Does using PP_FLAG_DMA_SYNC_DEV cause redundant full-page DMA syncs?

This flag instructs the page pool to automatically synchronize the entire
allocated size (up to 16KB) for the device. However, as seen later in
stmmac_xmit(), the driver manually calls dma_sync_single_for_cpu() and
dma_sync_single_for_device() on just the packet payload length (nopaged_len).

Would this result in double-syncing where the page pool redundantly syncs the
full page on every allocation, reducing the intended performance benefit?

> +	pp_params.pool_size = dma_conf->dma_tx_size;
> +	num_pages = DIV_ROUND_UP(dma_conf->dma_buf_sz, PAGE_SIZE);
[ ... ]
> @@ -4742,12 +4773,14 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  	bool enh_desc, has_vlan, set_ic, is_jumbo = false;
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  	unsigned int nopaged_len = skb_headlen(skb);
> +	gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN;
>  	u32 queue = skb_get_queue_mapping(skb);
>  	int nfrags = skb_shinfo(skb)->nr_frags;
>  	unsigned int first_entry, tx_packets;
>  	struct stmmac_txq_stats *txq_stats;
>  	struct dma_desc *desc, *first_desc;
>  	struct stmmac_tx_queue *tx_q;
> +	struct page *tx_buf_page;
>  	int i, csum_insertion = 0;
>  	int entry, first_tx;
>  	dma_addr_t dma_addr;
> @@ -4814,13 +4847,33 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  	} else {
>  		bool last_segment = (nfrags == 0);
>  
> -		dma_addr = dma_map_single(priv->device, skb->data,
> -					  nopaged_len, DMA_TO_DEVICE);
> -		if (dma_mapping_error(priv->device, dma_addr))
> -			goto dma_map_err;
> +		if (!(last_segment && priv->plat->tx_buf_quirk)) {
> +			dma_addr = dma_map_single(priv->device, skb->data,
> +						  nopaged_len, DMA_TO_DEVICE);
> +			if (dma_mapping_error(priv->device, dma_addr))
> +				goto dma_map_err;
>  
> -		stmmac_set_tx_skb_dma_entry(tx_q, first_entry, dma_addr,
> -					    nopaged_len, false);
> +			stmmac_set_tx_skb_dma_entry(tx_q, first_entry, dma_addr,
> +						    nopaged_len, false);
> +		} else {
> +			tx_buf_page = page_pool_alloc_pages(tx_q->page_pool, gfp);
> +			if (!tx_buf_page)
> +				goto dma_map_err;
> +
> +			dma_addr = page_pool_get_dma_addr(tx_buf_page);
> +			stmmac_set_tx_dma_entry(tx_q, first_entry,
> +						STMMAC_TXBUF_T_DMA, dma_addr,
> +						nopaged_len, false);
> +			tx_q->tx_skbuff_dma[first_entry].page = tx_buf_page;
> +
> +			dma_sync_single_for_cpu(priv->device, dma_addr,
> +						nopaged_len, DMA_BIDIRECTIONAL);
> +			skb_copy_from_linear_data(skb, page_address(tx_buf_page),
> +						  nopaged_len);
> +			dma_sync_single_for_device(priv->device, dma_addr,
> +						   nopaged_len,
> +						   DMA_BIDIRECTIONAL);
> +		}
>  
>  		stmmac_set_desc_addr(priv, first_desc, dma_addr);
>  

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
stmmac_xmit() leak DMA mappings and memory when skb_frag_dma_map() fails?

If mapping an SKB fragment fails lower down in stmmac_xmit() via
skb_frag_dma_map(), the code jumps to the dma_map_err label, frees the SKB,
and returns. However, it fails to unmap previously mapped fragments or the
linear segment, and does not advance the tx_q->cur_tx ring index.

Because the index is not advanced, stmmac_tx_clean() will never process these
leaked entries, and the next packet transmitted will silently overwrite
tx_q->tx_skbuff_dma[first_entry]. For the newly added page pool buffer, this
permanently leaks the underlying physical page allocated from
page_pool_alloc_pages(), which could deplete system memory over time.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803070513.4657-1-muhammad.nazim.amirul.nazle.asmade@altera.com?part=2