Messages in this thread |  | | Date | Wed, 4 Dec 2024 19:01:14 +0800 | | Subject | Re: [PATCH RFC v4 1/3] page_pool: fix timing for checking and disabling napi_local | | From | Yunsheng Lin <> |
| |
On 2024/12/3 10:49, Jakub Kicinski wrote: > 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/ > > 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). > >> @@ -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() ?
It is in page_pool_destroy() mostly because: 1. Only call synchronize_rcu() when there is inflight pages, which should be an unlikely case, and synchronize_rcu() might need to be called at least for the case of pool->p.napi not being NULL if it is called inside page_pool_disable_direct_recycling(). 2. the above synchronize_rcu() in page_pool_destroy() is also reused for the fixing of dma unmappings.
> > 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. >
|  |