[RFC 1/2] page_pool: fix timing for checking and disabling napi_local

From: Yunsheng Lin
Date: Thu Sep 12 2024 - 08:51:22 EST


page_pool page may be freed from skb_defer_free_flush() to
softirq context, it may cause concurrent access problem for
pool->alloc cache due to the below time window, as below,
both CPU0 and CPU1 may access the pool->alloc cache
concurrently in page_pool_empty_alloc_cache_once() and
page_pool_recycle_in_cache():

CPU 0 CPU1
page_pool_destroy() skb_defer_free_flush()
. .
. page_pool_put_unrefed_page()
. .
. allow_direct = page_pool_napi_local()
. .
page_pool_disable_direct_recycling() .
. .
page_pool_empty_alloc_cache_once() page_pool_recycle_in_cache()

Use rcu mechanism to avoid the above concurrent access problem.

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

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

Signed-off-by: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
CC: Alexander Lobakin <aleksander.lobakin@xxxxxxxxx>
---
net/core/page_pool.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index a813d30d2135..18645907cec2 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -818,6 +818,8 @@ static bool page_pool_napi_local(const struct page_pool *pool)
void page_pool_put_unrefed_netmem(struct page_pool *pool, netmem_ref netmem,
unsigned int dma_sync_size, bool allow_direct)
{
+ rcu_read_lock();
+
if (!allow_direct)
allow_direct = page_pool_napi_local(pool);

@@ -828,6 +830,8 @@ void page_pool_put_unrefed_netmem(struct page_pool *pool, netmem_ref netmem,
recycle_stat_inc(pool, ring_full);
page_pool_return_page(pool, netmem);
}
+
+ rcu_read_unlock();
}
EXPORT_SYMBOL(page_pool_put_unrefed_netmem);

@@ -861,6 +865,7 @@ void page_pool_put_page_bulk(struct page_pool *pool, void **data,
bool allow_direct;
bool in_softirq;

+ rcu_read_lock();
allow_direct = page_pool_napi_local(pool);

for (i = 0; i < count; i++) {
@@ -876,8 +881,10 @@ void page_pool_put_page_bulk(struct page_pool *pool, void **data,
data[bulk_len++] = (__force void *)netmem;
}

- if (!bulk_len)
+ if (!bulk_len) {
+ rcu_read_unlock();
return;
+ }

/* Bulk producer into ptr_ring page_pool cache */
in_softirq = page_pool_producer_lock(pool);
@@ -892,14 +899,18 @@ void page_pool_put_page_bulk(struct page_pool *pool, void **data,
page_pool_producer_unlock(pool, in_softirq);

/* Hopefully all pages was return into ptr_ring */
- if (likely(i == bulk_len))
+ if (likely(i == bulk_len)) {
+ rcu_read_unlock();
return;
+ }

/* ptr_ring cache full, free remaining pages outside producer lock
* since put_page() with refcnt == 1 can be an expensive operation
*/
for (; i < bulk_len; i++)
page_pool_return_page(pool, (__force netmem_ref)data[i]);
+
+ rcu_read_unlock();
}
EXPORT_SYMBOL(page_pool_put_page_bulk);

@@ -1121,6 +1132,12 @@ void page_pool_destroy(struct page_pool *pool)
return;

page_pool_disable_direct_recycling(pool);
+
+ /* Wait for the freeing side see the disabling direct recycling setting
+ * to avoid the concurrent access to the pool->alloc cache.
+ */
+ synchronize_rcu();
+
page_pool_free_frag(pool);

if (!page_pool_release(pool))
--
2.33.0