Re: [PATCH net-next v7 2/8] page_pool: fix timing for checking and disabling napi_local

On 1/10/2025 11:40 PM, Toke Høiland-Jørgensen wrote:

Yunsheng Lin [off-list ref] writes:

quoted

page_pool page may be freed from skb_defer_free_flush() in
softirq context without binding to any specific napi, it
may cause use-after-free problem due to the below time window,
as below, CPU1 may still access napi->list_owner after CPU0
free the napi memory:

             CPU 0                           CPU1
       page_pool_destroy()          skb_defer_free_flush()
              .                               .
              .                napi = READ_ONCE(pool->p.napi);
              .                               .
page_pool_disable_direct_recycling()         .
    driver free napi memory                   .
              .                               .
              .       napi && READ_ONCE(napi->list_owner) == cpuid
              .                               .

Have you actually observed this happen, or are you just speculating?

I did not actually observe this happen, but I added some delaying and
pr_err() debugging code in page_pool_napi_local()/page_pool_destroy(),
and modified the test module for page_pool in [1] to trigger that it is
indeed possible if the delay between reading napi and checking
napi->list_owner is long enough.

1. 
https://patchwork.kernel.org/project/netdevbpf/patch/20240909091913.987826-1-linyunsheng@huawei.com/

Because I don't think it can; deleting a NAPI instance already requires
observing an RCU grace period, cf netdevice.h:

/**
  *  __netif_napi_del - remove a NAPI context
  *  @napi: NAPI context
  *
  * Warning: caller must observe RCU grace period before freeing memory
  * containing @napi. Drivers might want to call this helper to combine
  * all the needed RCU grace periods into a single one.
  */
void __netif_napi_del(struct napi_struct *napi);

/**
  *  netif_napi_del - remove a NAPI context
  *  @napi: NAPI context
  *
  *  netif_napi_del() removes a NAPI context from the network device NAPI list
  */
static inline void netif_napi_del(struct napi_struct *napi)
{
	__netif_napi_del(napi);
	synchronize_net();
}

I am not sure we can reliably depend on the implicit synchronize_net()
above if netif_napi_del() might not be called before page_pool_destroy()
as there might not be netif_napi_del() before page_pool_destroy() for
the case of changing rx_desc_num for a queue, which seems to be the case
of hns3_set_ringparam() for hns3 driver.

quoted

Use rcu mechanism to avoid the above problem.

Note, the above was found during code reviewing on how to fix
the problem in [1].

As the following IOMMU fix patch depends on synchronize_rcu()
added in this patch and the time window is so small that it
doesn't seem to be an urgent fix, so target the net-next as
the IOMMU fix patch does.

1. https://lore.kernel.org/lkml/8067f204-1380-4d37-8ffd-007fc6f26738@kernel.org/T/ (local)

Fixes: dd64b232deb8 ("page_pool: unlink from napi during destroy")
Signed-off-by: Yunsheng Lin <redacted>
CC: Alexander Lobakin <aleksander.lobakin@intel.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
  net/core/page_pool.c | 15 ++++++++++++++-
  1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 9733206d6406..1aa7b93bdcc8 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -799,6 +799,7 @@ __page_pool_put_page(struct page_pool *pool, netmem_ref netmem,
  static bool page_pool_napi_local(const struct page_pool *pool)
  {
  	const struct napi_struct *napi;
+	bool napi_local;
  	u32 cpuid;
  
  	if (unlikely(!in_softirq()))

@@ -814,9 +815,15 @@ static bool page_pool_napi_local(const struct page_pool *pool)
  	if (READ_ONCE(pool->cpuid) == cpuid)
  		return true;
  
+	/* Synchronizated with page_pool_destory() to avoid use-after-free
+	 * for 'napi'.
+	 */
+	rcu_read_lock();
  	napi = READ_ONCE(pool->p.napi);
+	napi_local = napi && READ_ONCE(napi->list_owner) == cpuid;
+	rcu_read_unlock();

This rcu_read_lock/unlock() pair is redundant in the context you mention
above, since skb_defer_free_flush() is only ever called from softirq
context (within local_bh_disable()), which already function as an RCU
read lock.

I thought about it, but I am not sure if we need a explicit rcu lock
for different kernel PREEMPT and RCU config.
Perhaps use rcu_read_lock_bh_held() to ensure that we are in the
correct context?

quoted

-	return napi && READ_ONCE(napi->list_owner) == cpuid;
+	return napi_local;
  }
  
  void page_pool_put_unrefed_netmem(struct page_pool *pool, netmem_ref netmem,

@@ -1165,6 +1172,12 @@ void page_pool_destroy(struct page_pool *pool)
  	if (!page_pool_release(pool))
  		return;
  
+	/* Paired with rcu lock in page_pool_napi_local() to enable clearing
+	 * of pool->p.napi in page_pool_disable_direct_recycling() is seen
+	 * before returning to driver to free the napi instance.
+	 */
+	synchronize_rcu();

Most drivers call page_pool_destroy() in a loop for each RX queue, so
now you're introducing a full synchronize_rcu() wait for each queue.
That can delay tearing down the device significantly, so I don't think
this is a good idea.

synchronize_rcu() is called after page_pool_release(pool), which means
it is only called when there are some inflight pages, so there is not
necessarily a full synchronize_rcu() wait for each queue.

Anyway, it seems that there are some cases that need explicit
synchronize_rcu() and some cases depending on the other API providing
synchronize_rcu() semantics, maybe we provide two diffferent API for
both cases like the netif_napi_del()/__netif_napi_del() APIs do?