From: Jeff Layton <jlayton@kernel.org>
To: Chuck Lever <chuck.lever@oracle.com>
Cc: Neil Brown <neilb@suse.de>, Olga Kornievskaia <kolga@netapp.com>,
Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>,
linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org,
Zhi Li <yieli@redhat.com>
Subject: Re: [PATCH] nfsd: drop the nfsd_put helper
Date: Wed, 03 Jan 2024 10:20:45 -0500 [thread overview]
Message-ID: <f6e3444ca2b798d1f8d6b481ed6b848ec15d44c9.camel@kernel.org> (raw)
In-Reply-To: <ZZVz1wd/oTvBk3dU@tissot.1015granger.net>
On Wed, 2024-01-03 at 09:48 -0500, Chuck Lever wrote:
> On Wed, Jan 03, 2024 at 08:36:52AM -0500, Jeff Layton wrote:
> > It's not safe to call nfsd_put once nfsd_last_thread has been called, as
> > that function will zero out the nn->nfsd_serv pointer.
> >
> > Drop the nfsd_put helper altogether and open-code the svc_put in its
> > callers instead. That allows us to not be reliant on the value of that
> > pointer when handling an error.
> >
> > Fixes: 2a501f55cd64 ("nfsd: call nfsd_last_thread() before final nfsd_put()")
> > Reported-by: Zhi Li <yieli@redhat.com>
> > Cc: NeilBrown <neilb@suse.de>
> > Signed-off-by: Jeffrey Layton <jlayton@redhat.com>
> > ---
> > I know it's late, but it would be good to get this into v6.7 if
> > possible. I think it's fairly straightforward.
>
> How is Zhi finding these? Are there administrative API tests that
> can be shared with the community (say, by adding them to kdevops)?
>
>
I wish I knew. In most of these, we see these messages ahead of the
KASAN pops that he's reporting:
[ 643.617739] NFSD: Using UMH upcall client tracking operations.
[ 643.617755] NFSD: starting 90-second grace period (net f0000000)
[ 643.780487] svc: failed to register nfsdv3 RPC service (errno 512).
[ 643.780928] svc: failed to register nfsaclv3 RPC service (errno 512).
[ 643.794625] ==================================================================
[ 643.794635] BUG: KASAN: null-ptr-deref in __write_ports_addfd+0x3b9/0x6c0 [nfsd]
[ 643.794779] Write of size 4 at addr 0000000000000058 by task rpc.nfsd/16702
[ 643.794783]
...
The errno 512 makes me suspect that the rpcbind registration attempt is
catching a signal, and that failure then bubbles up when we're trying to
start the server. I've not been able to reproduce this myself though.
> > ---
> > fs/nfsd/nfsctl.c | 31 +++++++++++++++++--------------
> > fs/nfsd/nfsd.h | 7 -------
> > 2 files changed, 17 insertions(+), 21 deletions(-)
> >
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index 7cd513e59305..87fed75808ff 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -693,6 +693,7 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
> > char *mesg = buf;
> > int fd, err;
> > struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > + struct svc_serv *serv;
> >
> > err = get_int(&mesg, &fd);
> > if (err != 0 || fd < 0)
> > @@ -703,15 +704,15 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
> > if (err != 0)
> > return err;
> >
> > - err = svc_addsock(nn->nfsd_serv, net, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred);
> > + serv = nn->nfsd_serv;
> > + err = svc_addsock(serv, net, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred);
> >
> > - if (err < 0 && !nn->nfsd_serv->sv_nrthreads && !nn->keep_active)
> > + if (err < 0 && !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);
> > + else if (err >= 0 && !serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
> > + svc_get(serv);
> >
> > - nfsd_put(net);
> > + svc_put(serv);
> > return err;
> > }
> >
> > @@ -725,6 +726,7 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
> > struct svc_xprt *xprt;
> > int port, err;
> > struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > + struct svc_serv *serv;
> >
> > if (sscanf(buf, "%15s %5u", transport, &port) != 2)
> > return -EINVAL;
> > @@ -737,32 +739,33 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
> > if (err != 0)
> > return err;
> >
> > - err = svc_xprt_create(nn->nfsd_serv, transport, net,
> > + serv = nn->nfsd_serv;
> > + err = svc_xprt_create(serv, transport, net,
> > PF_INET, port, SVC_SOCK_ANONYMOUS, cred);
> > if (err < 0)
> > goto out_err;
> >
> > - err = svc_xprt_create(nn->nfsd_serv, transport, net,
> > + err = svc_xprt_create(serv, transport, net,
> > PF_INET6, port, SVC_SOCK_ANONYMOUS, cred);
> > if (err < 0 && err != -EAFNOSUPPORT)
> > goto out_close;
> >
> > - if (!nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
> > - svc_get(nn->nfsd_serv);
> > + if (!serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
> > + svc_get(serv);
> >
> > - nfsd_put(net);
> > + svc_put(serv);
> > return 0;
> > out_close:
> > - xprt = svc_find_xprt(nn->nfsd_serv, transport, net, PF_INET, port);
> > + xprt = svc_find_xprt(serv, transport, net, PF_INET, port);
> > if (xprt != NULL) {
> > svc_xprt_close(xprt);
> > svc_xprt_put(xprt);
> > }
> > out_err:
> > - if (!nn->nfsd_serv->sv_nrthreads && !nn->keep_active)
> > + if (!serv->sv_nrthreads && !nn->keep_active)
> > nfsd_last_thread(net);
> >
> > - nfsd_put(net);
> > + svc_put(serv);
> > return err;
> > }
> >
> > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> > index 3286ffacbc56..9ed0e08d16c2 100644
> > --- a/fs/nfsd/nfsd.h
> > +++ b/fs/nfsd/nfsd.h
> > @@ -113,13 +113,6 @@ int nfsd_pool_stats_open(struct inode *, struct file *);
> > int nfsd_pool_stats_release(struct inode *, struct file *);
> > void nfsd_shutdown_threads(struct net *net);
> >
> > -static inline void nfsd_put(struct net *net)
> > -{
> > - struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > -
> > - svc_put(nn->nfsd_serv);
> > -}
> > -
> > bool i_am_nfsd(void);
> >
> > struct nfsdfs_client {
> >
> > ---
> > base-commit: 610a9b8f49fbcf1100716370d3b5f6f884a2835a
> > change-id: 20240103-nfsd-fixes-1f1196134a11
> >
> > Best regards,
> > --
> > Jeff Layton <jlayton@kernel.org>
> >
>
--
Jeff Layton <jlayton@kernel.org>
prev parent reply other threads:[~2024-01-03 15:20 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-03 13:36 [PATCH] nfsd: drop the nfsd_put helper Jeff Layton
2024-01-03 14:48 ` Chuck Lever
2024-01-03 15:20 ` Jeff Layton [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f6e3444ca2b798d1f8d6b481ed6b848ec15d44c9.camel@kernel.org \
--to=jlayton@kernel.org \
--cc=Dai.Ngo@oracle.com \
--cc=chuck.lever@oracle.com \
--cc=kolga@netapp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=neilb@suse.de \
--cc=tom@talpey.com \
--cc=yieli@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.