| Message ID | 20230330182427.19013-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sunrpc: only free unix grouplist after RCU settles | expand |
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") ? > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2183056 This bug isn't publicly accessible, fwiw. > 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
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. > > > 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 > >
> 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
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)
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> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2183056 Signed-off-by: Jeff Layton <jlayton@kernel.org> --- net/sunrpc/svcauth_unix.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)