| Message ID | 20231030011247.9794-2-neilb@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sunrpc: not refcounting svc_serv | expand |
On Mon, 2023-10-30 at 12:08 +1100, NeilBrown wrote: > If write_ports_addfd or write_ports_addxprt fail, they call nfsD_put() > without calling nfsd_last_thread(). This leaves nn->nfsd_serv pointing > to a structure that has been freed. > > So export nfsd_last_thread() and call it when the nfsd_serv is about to > be destroy. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/nfsctl.c | 9 +++++++-- > fs/nfsd/nfsd.h | 1 + > fs/nfsd/nfssvc.c | 2 +- > 3 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > index 739ed5bf71cd..79efb1075f38 100644 > --- a/fs/nfsd/nfsctl.c > +++ b/fs/nfsd/nfsctl.c > @@ -705,8 +705,10 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred > > err = svc_addsock(nn->nfsd_serv, net, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred); > > - if (err >= 0 && > - !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1)) > + if (err < 0 && !nn->nfsd_serv->sv_nrthreads && !nn->keep_active) > + nfsd_last_thread(net); > + else if (err >= 0 && > + !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1)) > svc_get(nn->nfsd_serv); > > nfsd_put(net); > @@ -757,6 +759,9 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr > svc_xprt_put(xprt); > } > out_err: > + if (!nn->nfsd_serv->sv_nrthreads && !nn->keep_active) > + nfsd_last_thread(net); > + > nfsd_put(net); > return err; > } > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h > index f5ff42f41ee7..3286ffacbc56 100644 > --- a/fs/nfsd/nfsd.h > +++ b/fs/nfsd/nfsd.h > @@ -155,6 +155,7 @@ int nfsd_vers(struct nfsd_net *nn, int vers, enum vers_op change); > int nfsd_minorversion(struct nfsd_net *nn, u32 minorversion, enum vers_op change); > void nfsd_reset_versions(struct nfsd_net *nn); > int nfsd_create_serv(struct net *net); > +void nfsd_last_thread(struct net *net); > > extern int nfsd_max_blksize; > > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c > index d6122bb2d167..6c968c02cc29 100644 > --- a/fs/nfsd/nfssvc.c > +++ b/fs/nfsd/nfssvc.c > @@ -542,7 +542,7 @@ static struct notifier_block nfsd_inet6addr_notifier = { > /* Only used under nfsd_mutex, so this atomic may be overkill: */ > static atomic_t nfsd_notifier_refcount = ATOMIC_INIT(0); > > -static void nfsd_last_thread(struct net *net) > +void nfsd_last_thread(struct net *net) > { > struct nfsd_net *nn = net_generic(net, nfsd_net_id); > struct svc_serv *serv = nn->nfsd_serv; This patch should fix the problem that I was seeing with write_ports, but it won't fix the hinky error cleanup in nfsd_svc. It looks like that does get fixed in patch #4 though, so I'm not too concerned. Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Mon, Oct 30, 2023 at 09:15:17AM -0400, Jeff Layton wrote: > On Mon, 2023-10-30 at 12:08 +1100, NeilBrown wrote: > > If write_ports_addfd or write_ports_addxprt fail, they call nfsD_put() > > without calling nfsd_last_thread(). This leaves nn->nfsd_serv pointing > > to a structure that has been freed. > > > > So export nfsd_last_thread() and call it when the nfsd_serv is about to > > be destroy. > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > fs/nfsd/nfsctl.c | 9 +++++++-- > > fs/nfsd/nfsd.h | 1 + > > fs/nfsd/nfssvc.c | 2 +- > > 3 files changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > > index 739ed5bf71cd..79efb1075f38 100644 > > --- a/fs/nfsd/nfsctl.c > > +++ b/fs/nfsd/nfsctl.c > > @@ -705,8 +705,10 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred > > > > err = svc_addsock(nn->nfsd_serv, net, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred); > > > > - if (err >= 0 && > > - !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1)) > > + if (err < 0 && !nn->nfsd_serv->sv_nrthreads && !nn->keep_active) > > + nfsd_last_thread(net); > > + else if (err >= 0 && > > + !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1)) > > svc_get(nn->nfsd_serv); > > > > nfsd_put(net); > > @@ -757,6 +759,9 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr > > svc_xprt_put(xprt); > > } > > out_err: > > + if (!nn->nfsd_serv->sv_nrthreads && !nn->keep_active) > > + nfsd_last_thread(net); > > + > > nfsd_put(net); > > return err; > > } > > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h > > index f5ff42f41ee7..3286ffacbc56 100644 > > --- a/fs/nfsd/nfsd.h > > +++ b/fs/nfsd/nfsd.h > > @@ -155,6 +155,7 @@ int nfsd_vers(struct nfsd_net *nn, int vers, enum vers_op change); > > int nfsd_minorversion(struct nfsd_net *nn, u32 minorversion, enum vers_op change); > > void nfsd_reset_versions(struct nfsd_net *nn); > > int nfsd_create_serv(struct net *net); > > +void nfsd_last_thread(struct net *net); > > > > extern int nfsd_max_blksize; > > > > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c > > index d6122bb2d167..6c968c02cc29 100644 > > --- a/fs/nfsd/nfssvc.c > > +++ b/fs/nfsd/nfssvc.c > > @@ -542,7 +542,7 @@ static struct notifier_block nfsd_inet6addr_notifier = { > > /* Only used under nfsd_mutex, so this atomic may be overkill: */ > > static atomic_t nfsd_notifier_refcount = ATOMIC_INIT(0); > > > > -static void nfsd_last_thread(struct net *net) > > +void nfsd_last_thread(struct net *net) > > { > > struct nfsd_net *nn = net_generic(net, nfsd_net_id); > > struct svc_serv *serv = nn->nfsd_serv; > > This patch should fix the problem that I was seeing with write_ports, Then let's add Fixes: ec52361df99b ("SUNRPC: stop using ->sv_nrthreads as a refcount") to this one, since it addresses a crasher seen in the wild. > but it won't fix the hinky error cleanup in nfsd_svc. It looks like that > does get fixed in patch #4 though, so I'm not too concerned. Does that fix also need to be backported?
On Mon, 2023-10-30 at 11:52 -0400, Chuck Lever wrote: > On Mon, Oct 30, 2023 at 09:15:17AM -0400, Jeff Layton wrote: > > On Mon, 2023-10-30 at 12:08 +1100, NeilBrown wrote: > > > If write_ports_addfd or write_ports_addxprt fail, they call nfsD_put() > > > without calling nfsd_last_thread(). This leaves nn->nfsd_serv pointing > > > to a structure that has been freed. > > > > > > So export nfsd_last_thread() and call it when the nfsd_serv is about to > > > be destroy. > > > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > --- > > > fs/nfsd/nfsctl.c | 9 +++++++-- > > > fs/nfsd/nfsd.h | 1 + > > > fs/nfsd/nfssvc.c | 2 +- > > > 3 files changed, 9 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > > > index 739ed5bf71cd..79efb1075f38 100644 > > > --- a/fs/nfsd/nfsctl.c > > > +++ b/fs/nfsd/nfsctl.c > > > @@ -705,8 +705,10 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred > > > > > > err = svc_addsock(nn->nfsd_serv, net, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred); > > > > > > - if (err >= 0 && > > > - !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1)) > > > + if (err < 0 && !nn->nfsd_serv->sv_nrthreads && !nn->keep_active) > > > + nfsd_last_thread(net); > > > + else if (err >= 0 && > > > + !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1)) > > > svc_get(nn->nfsd_serv); > > > > > > nfsd_put(net); > > > @@ -757,6 +759,9 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr > > > svc_xprt_put(xprt); > > > } > > > out_err: > > > + if (!nn->nfsd_serv->sv_nrthreads && !nn->keep_active) > > > + nfsd_last_thread(net); > > > + > > > nfsd_put(net); > > > return err; > > > } > > > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h > > > index f5ff42f41ee7..3286ffacbc56 100644 > > > --- a/fs/nfsd/nfsd.h > > > +++ b/fs/nfsd/nfsd.h > > > @@ -155,6 +155,7 @@ int nfsd_vers(struct nfsd_net *nn, int vers, enum vers_op change); > > > int nfsd_minorversion(struct nfsd_net *nn, u32 minorversion, enum vers_op change); > > > void nfsd_reset_versions(struct nfsd_net *nn); > > > int nfsd_create_serv(struct net *net); > > > +void nfsd_last_thread(struct net *net); > > > > > > extern int nfsd_max_blksize; > > > > > > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c > > > index d6122bb2d167..6c968c02cc29 100644 > > > --- a/fs/nfsd/nfssvc.c > > > +++ b/fs/nfsd/nfssvc.c > > > @@ -542,7 +542,7 @@ static struct notifier_block nfsd_inet6addr_notifier = { > > > /* Only used under nfsd_mutex, so this atomic may be overkill: */ > > > static atomic_t nfsd_notifier_refcount = ATOMIC_INIT(0); > > > > > > -static void nfsd_last_thread(struct net *net) > > > +void nfsd_last_thread(struct net *net) > > > { > > > struct nfsd_net *nn = net_generic(net, nfsd_net_id); > > > struct svc_serv *serv = nn->nfsd_serv; > > > > This patch should fix the problem that I was seeing with write_ports, > > Then let's add > > Fixes: ec52361df99b ("SUNRPC: stop using ->sv_nrthreads as a refcount") > > to this one, since it addresses a crasher seen in the wild. > > Sounds good. > > but it won't fix the hinky error cleanup in nfsd_svc. It looks like that > > does get fixed in patch #4 though, so I'm not too concerned. > > Does that fix also need to be backported? > I think so, but we might want to split that out into a more targeted patch and apply it ahead of the rest of the series. Our QA folks seem to be able to hit the problem somehow, so it's likely to be triggered by people in the field too.
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c index 739ed5bf71cd..79efb1075f38 100644 --- a/fs/nfsd/nfsctl.c +++ b/fs/nfsd/nfsctl.c @@ -705,8 +705,10 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred err = svc_addsock(nn->nfsd_serv, net, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred); - if (err >= 0 && - !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1)) + if (err < 0 && !nn->nfsd_serv->sv_nrthreads && !nn->keep_active) + nfsd_last_thread(net); + else if (err >= 0 && + !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1)) svc_get(nn->nfsd_serv); nfsd_put(net); @@ -757,6 +759,9 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr svc_xprt_put(xprt); } out_err: + if (!nn->nfsd_serv->sv_nrthreads && !nn->keep_active) + nfsd_last_thread(net); + nfsd_put(net); return err; } diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h index f5ff42f41ee7..3286ffacbc56 100644 --- a/fs/nfsd/nfsd.h +++ b/fs/nfsd/nfsd.h @@ -155,6 +155,7 @@ int nfsd_vers(struct nfsd_net *nn, int vers, enum vers_op change); int nfsd_minorversion(struct nfsd_net *nn, u32 minorversion, enum vers_op change); void nfsd_reset_versions(struct nfsd_net *nn); int nfsd_create_serv(struct net *net); +void nfsd_last_thread(struct net *net); extern int nfsd_max_blksize; diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c index d6122bb2d167..6c968c02cc29 100644 --- a/fs/nfsd/nfssvc.c +++ b/fs/nfsd/nfssvc.c @@ -542,7 +542,7 @@ static struct notifier_block nfsd_inet6addr_notifier = { /* Only used under nfsd_mutex, so this atomic may be overkill: */ static atomic_t nfsd_notifier_refcount = ATOMIC_INIT(0); -static void nfsd_last_thread(struct net *net) +void nfsd_last_thread(struct net *net) { struct nfsd_net *nn = net_generic(net, nfsd_net_id); struct svc_serv *serv = nn->nfsd_serv;
If write_ports_addfd or write_ports_addxprt fail, they call nfsD_put() without calling nfsd_last_thread(). This leaves nn->nfsd_serv pointing to a structure that has been freed. So export nfsd_last_thread() and call it when the nfsd_serv is about to be destroy. Signed-off-by: NeilBrown <neilb@suse.de> --- fs/nfsd/nfsctl.c | 9 +++++++-- fs/nfsd/nfsd.h | 1 + fs/nfsd/nfssvc.c | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-)