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

On 5/30/26 5:48 PM, Jeff Layton wrote:
> 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.
Yeah. It's too clever by half.


-- 
Chuck Lever