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

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.

> +
> +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]>