Re: [PATCH 2/2] SUNRPC: Check svc pool percpu counter allocation

On Sat, 2026-05-30 at 17:45 -0400, Jeff Layton wrote:
> On Sat, 2026-05-30 at 16:21 -0400, Chuck Lever wrote:
> > From: Chuck Lever <[email protected]>
> > 
> > __svc_create() initializes three per-pool percpu_counter stats and
> > ignores every return value. On SMP, percpu_counter_init() fails when
> > __alloc_percpu_gfp() cannot satisfy the allocation, leaving the failed
> > counter with fbc->counters == NULL and its embedded raw_spinlock_t,
> > list_head, and count never initialized. __svc_create() returns the
> > half-constructed svc_serv to nfsd, lockd, or the NFS callback service
> > anyway.
> > 
> > Once that service is live, the hot-path increments in
> > svc_xprt_enqueue(), svc_handle_xprt(), and
> > svc_pool_wake_idle_thread() reach a counter whose backing pointer is
> > NULL. The pointer is a per-cpu offset, so the access does not fault:
> > it resolves to offset zero of the current CPU's per-cpu area and
> > silently corrupts whatever variable lives there. A
> > /proc/fs/nfsd/pool_stats read walks the same NULL per-cpu storage and
> > returns garbage, and on CONFIG_DEBUG_SPINLOCK or lockdep it splats on
> > the never-initialized lock.
> > 
> > Creating the broken service requires a percpu allocation failure during
> > RPC server startup, so it is reachable only by a local administrator
> > under memory pressure or fault injection; a remote peer cannot induce
> > the bad state on its own.
> > 
> > Initialize the three adjacent pool counters with one checked
> > percpu_counter_init_many() call and fail __svc_create() when the
> > allocation fails, unwinding the counters for pools already set up. Use
> > the matching percpu_counter_destroy_many() at teardown so the single
> > per-cpu allocation is freed exactly once.
> > 
> > Fixes: ccf08bed6e7a ("SUNRPC: Replace pool stats with per-CPU variables")
> > Signed-off-by: Chuck Lever <[email protected]>
> > ---
> >  net/sunrpc/svc.c | 32 ++++++++++++++++++++++++++------
> >  1 file changed, 26 insertions(+), 6 deletions(-)
> > 
> > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> > index ae9ec4bf34f7..aeb6e631848c 100644
> > --- a/net/sunrpc/svc.c
> > +++ b/net/sunrpc/svc.c
> > @@ -476,6 +476,22 @@ __svc_init_bc(struct svc_serv *serv)
> >  }
> >  #endif
> >  
> > +enum {
> > +	SVC_POOL_COUNTERS = 3,
> > +};
> > +
> > +static int svc_pool_init_counters(struct svc_pool *pool)
> > +{
> > +	return percpu_counter_init_many(&pool->sp_messages_arrived, 0,
> > +					GFP_KERNEL, SVC_POOL_COUNTERS);
> > +}
> > 
> > 
> 
> Switching to this looks like a good thing, but it means that the
> svc_pool struct fields now have some strict ordering requirements. The
> percpu_counters all need to be snuggled up together.
> 
> That deserves a comment to that effect in the struct svc_pool, so that
> we don't inadvertently break it later.
> 

Actually, sashiko points out that struct randomization could break here
too. Given that service creation isn't a hot codepath, I'd keep these
as discrete percpu_counter_init/destroy() calls and avoid the hassle.

> > +
> > +static void svc_pool_destroy_counters(struct svc_pool *pool)
> > +{
> > +	percpu_counter_destroy_many(&pool->sp_messages_arrived,
> > +				    SVC_POOL_COUNTERS);
> > +}
> > +
> >  /*
> >   * Create an RPC service
> >   */
> > @@ -540,12 +556,18 @@ __svc_create(struct svc_program *prog, int nprogs, struct svc_stat *stats,
> >  		INIT_LIST_HEAD(&pool->sp_all_threads);
> >  		init_llist_head(&pool->sp_idle_threads);
> >  
> > -		percpu_counter_init(&pool->sp_messages_arrived, 0, GFP_KERNEL);
> > -		percpu_counter_init(&pool->sp_sockets_queued, 0, GFP_KERNEL);
> > -		percpu_counter_init(&pool->sp_threads_woken, 0, GFP_KERNEL);
> > +		if (svc_pool_init_counters(pool))
> > +			goto out_err;
> >  	}
> >  
> >  	return serv;
> > +
> > +out_err:
> > +	while (i--)
> > +		svc_pool_destroy_counters(&serv->sv_pools[i]);
> > +	kfree(serv->sv_pools);
> > +	kfree(serv);
> > +	return NULL;
> >  }
> >  
> >  /**
> > @@ -624,9 +646,7 @@ svc_destroy(struct svc_serv **servp)
> >  	for (i = 0; i < serv->sv_nrpools; i++) {
> >  		struct svc_pool *pool = &serv->sv_pools[i];
> >  
> > -		percpu_counter_destroy(&pool->sp_messages_arrived);
> > -		percpu_counter_destroy(&pool->sp_sockets_queued);
> > -		percpu_counter_destroy(&pool->sp_threads_woken);
> > +		svc_pool_destroy_counters(pool);
> >  	}
> >  	kfree(serv->sv_pools);
> >  	kfree(serv);
> 
> Patch itself looks fine though.
> 
> Reviewed-by: Jeff Layton <[email protected]>

-- 
Jeff Layton <[email protected]>