Re: [PATCH 5/4] sunrpc: protect the svc_pool_map pool_to[] array with RCU
NeilBrown <[email protected]>
| Newsgroups | gmane.linux.nfs,gmane.linux.kernel |
|---|---|
| Message-ID | <[email protected]> |
On Tue, 30 Jun 2026, Jeff Layton wrote: > svc_pool_map_get_node() reads the global svc_pool_map without holding > svc_pool_map_mutex and dereferences m->pool_to[]. The array was both > published and torn down without any synchronisation against that > lockless reader: > > - svc_pool_map_get() incremented m->count to one before > svc_pool_map_init_pernode() allocated and filled the arrays, so a > reader observing the map as "in use" could see a NULL (or partially > built) pool_to[] and oops. > > - svc_pool_map_put() freed the arrays as soon as the last reference > went away, so a reader that had already started dereferencing > pool_to[] could use it after free. > > svc_new_thread() takes this lockless path for every service, including > unpooled ones that hold no map reference, so the reader genuinely can > run concurrently with another service's startup or shutdown. > > Publish pool_to[] with rcu_assign_pointer() only after it is fully > built in a private allocation, and have svc_pool_map_get_node() > dereference it under rcu_read_lock(). On teardown, clear the pointer > and defer the free past a grace period with kfree_rcu_mightsleep(). > > svc_pool_map_set_cpumask() also reads pool_to[], but its caller holds a > map reference (it checks sv_nrpools > 1) so the array is stable; it uses > rcu_dereference_protected() rather than taking the read lock. > > to_pool[] needs no such treatment: it is only read by services that > hold a map reference, so it cannot be freed under a reader. I don't think this is the best approach. The problem only occurs when svc_create() is used as when svc_create_pooled() is used, svc_pool_map_get() is called early so a reference is held to svc_pool_map. svc_new_thread() is the only caller of svc_pool_map_get_node(), and it can easily check pool->sv_is_pooled and only call svc_pool_map_get_node() is sv_is_pooled is true. When false it should probably use NUMA_NO_NODE. e.g. diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c index ae9ec4bf34f7..063826702f46 100644 --- a/net/sunrpc/svc.c +++ b/net/sunrpc/svc.c @@ -800,7 +800,10 @@ int svc_new_thread(struct svc_serv *serv, struct svc_pool *pool) int node; int err = 0; - node = svc_pool_map_get_node(pool->sp_id); + if (serv->sv_is_pooled) + node = svc_pool_map_get_node(pool->sp_id); + else + node = NUMA_NO_NODE; rqstp = svc_prepare_thread(serv, pool, node); if (!rqstp) NeilBrown