Re: [PATCH] sunrpc: only free unix grouplist after RCU settles

[Chuck Lever III <chuck.lever@oracle.com>]

> On Mar 30, 2023, at 2:49 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Thu, 2023-03-30 at 18:31 +0000, Chuck Lever III wrote:
>> Hi Jeff-
>> 
>>> On Mar 30, 2023, at 2:24 PM, Jeff Layton <jlayton@kernel.org> wrote:
>>> 
>>> While the unix_gid object is rcu-freed, the group_info list that it
>>> contains is not. Ensure that we only put the group list reference once
>>> we are really freeing the unix_gid object.
>>> 
>>> Reported-by: Zhi Li <yieli@redhat.com>
>> 
>> Should we also add
>> 
>> Fixes: fd5d2f78261b ("SUNRPC: Make server side AUTH_UNIX use lockless lookups") ?
>> 
>> 
> 
> Sure. That does look like when that particular bug crept in.
> 
>>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2183056
>> 
>> This bug isn't publicly accessible, fwiw.
>> 
> 
> Thanks. It should be now.

OK, applied to nfsd-fixes!


>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>> net/sunrpc/svcauth_unix.c | 17 +++++++++++++----
>>> 1 file changed, 13 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
>>> index 50e2eb579194..4485088ce27b 100644
>>> --- a/net/sunrpc/svcauth_unix.c
>>> +++ b/net/sunrpc/svcauth_unix.c
>>> @@ -416,14 +416,23 @@ static int unix_gid_hash(kuid_t uid)
>>> 	return hash_long(from_kuid(&init_user_ns, uid), GID_HASHBITS);
>>> }
>>> 
>>> -static void unix_gid_put(struct kref *kref)
>>> +static void unix_gid_free(struct rcu_head *rcu)
>>> {
>>> -	struct cache_head *item = container_of(kref, struct cache_head, ref);
>>> -	struct unix_gid *ug = container_of(item, struct unix_gid, h);
>>> +	struct unix_gid *ug = container_of(rcu, struct unix_gid, rcu);
>>> +	struct cache_head *item = &ug->h;
>>> +
>>> 	if (test_bit(CACHE_VALID, &item->flags) &&
>>> 	    !test_bit(CACHE_NEGATIVE, &item->flags))
>>> 		put_group_info(ug->gi);
>>> -	kfree_rcu(ug, rcu);
>>> +	kfree(ug);
>>> +}
>>> +
>>> +static void unix_gid_put(struct kref *kref)
>>> +{
>>> +	struct cache_head *item = container_of(kref, struct cache_head, ref);
>>> +	struct unix_gid *ug = container_of(item, struct unix_gid, h);
>>> +
>>> +	call_rcu(&ug->rcu, unix_gid_free);
>>> }
>>> 
>>> static int unix_gid_match(struct cache_head *corig, struct cache_head *cnew)
>>> -- 
>>> 2.39.2
>>> 
>> 
>> --
>> Chuck Lever
>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever