Re: [PATCH RFC v4 1/3] page_pool: fix timing for checking and disabling napi_local

[Jakub Kicinski]

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@xxxxxxxxxx/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() ?

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.