Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <12c783873abcd0f9c631e2f367cc1dd97949e0fb.camel@kernel.org>
Subject: Re: [PATCH] sunrpc: only free unix grouplist after RCU settles
From:   Jeff Layton <jlayton@kernel.org>
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Benjamin Coddington <bcodding@redhat.com>,
        Zhi Li <yieli@redhat.com>
Date:   Thu, 30 Mar 2023 14:49:26 -0400
In-Reply-To: <49AC9CF4-71A8-48D7-BF21-41FFFEFBE4C8@oracle.com>
References: <20230330182427.19013-1-jlayton@kernel.org>
         <49AC9CF4-71A8-48D7-BF21-41FFFEFBE4C8@oracle.com>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Thu, 2023-03-30 at 18:31 +0000, Chuck Lever III wrote:
> Hi Jeff-
>=20
> > On Mar 30, 2023, at 2:24 PM, Jeff Layton <jlayton@kernel.org> wrote:
> >=20
> > 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.
> >=20
> > Reported-by: Zhi Li <yieli@redhat.com>
>=20
> Should we also add
>=20
> Fixes: fd5d2f78261b ("SUNRPC: Make server side AUTH_UNIX use lockless loo=
kups") ?
>=20
>=20

Sure. That does look like when that particular bug crept in.
=20
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=3D2183056
>=20
> This bug isn't publicly accessible, fwiw.
>=20

Thanks. It should be now.

>=20
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > net/sunrpc/svcauth_unix.c | 17 +++++++++++++----
> > 1 file changed, 13 insertions(+), 4 deletions(-)
> >=20
> > 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);
> > }
> >=20
> > -static void unix_gid_put(struct kref *kref)
> > +static void unix_gid_free(struct rcu_head *rcu)
> > {
> > -	struct cache_head *item =3D container_of(kref, struct cache_head, ref=
);
> > -	struct unix_gid *ug =3D container_of(item, struct unix_gid, h);
> > +	struct unix_gid *ug =3D container_of(rcu, struct unix_gid, rcu);
> > +	struct cache_head *item =3D &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 =3D container_of(kref, struct cache_head, ref=
);
> > +	struct unix_gid *ug =3D container_of(item, struct unix_gid, h);
> > +
> > +	call_rcu(&ug->rcu, unix_gid_free);
> > }
> >=20
> > static int unix_gid_match(struct cache_head *corig, struct cache_head *=
cnew)
> > --=20
> > 2.39.2
> >=20
>=20
> --
> Chuck Lever
>=20
>=20

--=20
Jeff Layton <jlayton@kernel.org>