Re: [PATCH RFC v4 1/3] page_pool: fix timing for checking and disabling napi_local
From: Jakub Kicinski <kuba@kernel.org>
Date: 2024-12-03 02:49:56
Also in:
lkml
On Wed, 20 Nov 2024 18:34:53 +0800 Yunsheng Lin wrote:
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
. .
Use rcu mechanism to avoid the above problem.
Note, the above was found during code reviewing on how to fix
the problem in [1].
1. https://lore.kernel.org/lkml/8067f204-1380-4d37-8ffd-007fc6f26738@kernel.org/T/ (local)Please split it from the rest of the series, it's related but not really logically connected (dma mappings and NAPI recycling are different features of page pool).
quoted hunk ↗ jump to hunk
@@ -1126,6 +1133,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();
I don't think this is in the right place. Why not inside page_pool_disable_direct_recycling() ? Hopefully this doesn't cause long stalls during reconfig, normally NAPIs and page pools are freed together, and NAPI removal implies synchronize_rcu(). That's why it's not really a problem for majority of drivers. You should perhaps make a note of this in the commit message.