Message-id: <169844539954.20306.1375894928598489617@noble.neil.brown.name>
Date:   Sat, 28 Oct 2023 09:23:19 +1100
From:   "NeilBrown" <neilb@...e.de>
To:     "Jeff Layton" <jlayton@...nel.org>
Cc:     "Chuck Lever" <chuck.lever@...cle.com>,
        "Olga Kornievskaia" <kolga@...app.com>,
        "Dai Ngo" <Dai.Ngo@...cle.com>, "Tom Talpey" <tom@...pey.com>,
        linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Zhi Li" <yieli@...hat.com>, "Jeff Layton" <jlayton@...nel.org>
Subject: Re: [PATCH] nfsd: ensure the nfsd_serv pointer is cleared when svc is
 torn down

On Fri, 27 Oct 2023, Jeff Layton wrote:
> Zhi Li reported a refcount_t use-after-free when bringing up nfsd.
> 
> We set the nn->nfsd_serv pointer in nfsd_create_serv, but it's only ever
> cleared in nfsd_last_thread. When setting up a new socket, if there is
> an error, this can leave nfsd_serv pointer set after it has been freed.
> We need to better couple the existence of the object with the value of
> the nfsd_serv pointer.
> 
> Since we always increment and decrement the svc_serv references under
> mutex, just test for whether the next put will destroy it in nfsd_put,
> and clear the pointer beforehand if so. Add a new nfsd_get function for
> better clarity and so that we can enforce that the mutex is held via
> lockdep. Remove the clearing of the pointer from nfsd_last_thread.
> Finally, change all of the svc_get and svc_put calls to use the updated
> wrappers.
> 
> Reported-by: Zhi Li <yieli@...hat.com>
> Signed-off-by: Jeff Layton <jlayton@...nel.org>
> ---
> When using their test harness, the RHQA folks would sometimes see the
> nfsv3 portmapper registration fail with -ERESTARTSYS, and that would
> trigger this bug. I could never reproduce that easily on my own, but I
> was able to validate this by hacking some fault injection into
> svc_register.

Maybe you could share that fault-injection with us?

> ---
>  fs/nfsd/nfsctl.c |  4 ++--
>  fs/nfsd/nfsd.h   |  8 ++-----
>  fs/nfsd/nfssvc.c | 72 ++++++++++++++++++++++++++++++++++++--------------------
>  3 files changed, 51 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 7ed02fb88a36..f8c0fed99c7f 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -706,7 +706,7 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
>  
>  	if (err >= 0 &&
>  	    !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
> -		svc_get(nn->nfsd_serv);
> +		nfsd_get(net);
>  
>  	nfsd_put(net);
>  	return err;
> @@ -745,7 +745,7 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
>  		goto out_close;
>  
>  	if (!nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
> -		svc_get(nn->nfsd_serv);
> +		nfsd_get(net);
>  
>  	nfsd_put(net);
>  	return 0;
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index 11c14faa6c67..c9cb70bf2a6d 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -96,12 +96,8 @@ 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);
> -}
> +struct svc_serv	*nfsd_get(struct net *net);
> +void		nfsd_put(struct net *net);
>  
>  bool		i_am_nfsd(void);
>  
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index c7af1095f6b5..4c00478c28dd 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -66,7 +66,7 @@ static __be32			nfsd_init_request(struct svc_rqst *,
>   * ->sv_pools[].
>   *
>   * Each active thread holds a counted reference on nn->nfsd_serv, as does
> - * the nn->keep_active flag and various transient calls to svc_get().
> + * the nn->keep_active flag and various transient calls to nfsd_get().
>   *
>   * Finally, the nfsd_mutex also protects some of the global variables that are
>   * accessed when nfsd starts and that are settable via the write_* routines in
> @@ -477,6 +477,39 @@ static void nfsd_shutdown_net(struct net *net)
>  }
>  
>  static DEFINE_SPINLOCK(nfsd_notifier_lock);
> +
> +struct svc_serv *nfsd_get(struct net *net)
> +{
> +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> +	struct svc_serv *serv = nn->nfsd_serv;
> +
> +	lockdep_assert_held(&nfsd_mutex);
> +	if (serv)
> +		svc_get(serv);
> +	return serv;
> +}

As far as I can tell, the addition of nfsd_get() is pure noise here.
Maybe it is a useful abstraction to add, but it makes the patch so noisy
that I cannot see where the actual fix is.

> +
> +void nfsd_put(struct net *net)
> +{
> +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> +	struct svc_serv *serv = nn->nfsd_serv;
> +
> +	/*
> +	 * The notifiers expect that if the nfsd_serv pointer is
> +	 * set that it's safe to access, so we must clear that
> +	 * pointer first before putting the last reference. Because
> +	 * we always increment and decrement the refcount under the
> +	 * mutex, it's safe to determine this via kref_read.
> +	 */
> +	lockdep_assert_held(&nfsd_mutex);
> +	if (kref_read(&serv->sv_refcnt) == 1) {

This is horrible.  Using kref_read() is OK for debug messages, but
almost always wrong anywhere else.
In this case it is actually safe but only because the use of kref has
become pointless.  We now ONLY change sv_refcnt while holding a mutex so
we may as well make it a simple integer (which I'd been thinking of
doing, but hadn't got to yet).  So this isn't wrong, but it looks wrong.

Also I think we should only have one place where we special-case
sv_refcnt going from 1 to 0.  nfsd_last_thread() must be called at that
time.  So if we are going to have nfsd_put() like this, it should be
calling nfsd_last_thread().

I'm certainly open to the possibility that these are still bugs here.  I
don't think patch helps me be confident that there are fewer.  Sorry.

NeilBrown



> +		spin_lock(&nfsd_notifier_lock);
> +		nn->nfsd_serv = NULL;
> +		spin_unlock(&nfsd_notifier_lock);
> +	}
> +	svc_put(serv);
> +}
> +
>  static int nfsd_inetaddr_event(struct notifier_block *this, unsigned long event,
>  	void *ptr)
>  {
> @@ -547,10 +580,6 @@ static void nfsd_last_thread(struct net *net)
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>  	struct svc_serv *serv = nn->nfsd_serv;
>  
> -	spin_lock(&nfsd_notifier_lock);
> -	nn->nfsd_serv = NULL;
> -	spin_unlock(&nfsd_notifier_lock);
> -
>  	/* check if the notifier still has clients */
>  	if (atomic_dec_return(&nfsd_notifier_refcount) == 0) {
>  		unregister_inetaddr_notifier(&nfsd_inetaddr_notifier);
> @@ -638,21 +667,19 @@ static int nfsd_get_default_max_blksize(void)
>  
>  void nfsd_shutdown_threads(struct net *net)
>  {
> -	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>  	struct svc_serv *serv;
>  
>  	mutex_lock(&nfsd_mutex);
> -	serv = nn->nfsd_serv;
> +	serv = nfsd_get(net);
>  	if (serv == NULL) {
>  		mutex_unlock(&nfsd_mutex);
>  		return;
>  	}
>  
> -	svc_get(serv);
>  	/* Kill outstanding nfsd threads */
>  	svc_set_num_threads(serv, NULL, 0);
>  	nfsd_last_thread(net);
> -	svc_put(serv);
> +	nfsd_put(net);
>  	mutex_unlock(&nfsd_mutex);
>  }
>  
> @@ -663,15 +690,13 @@ bool i_am_nfsd(void)
>  
>  int nfsd_create_serv(struct net *net)
>  {
> -	int error;
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>  	struct svc_serv *serv;
> +	int error;
>  
> -	WARN_ON(!mutex_is_locked(&nfsd_mutex));
> -	if (nn->nfsd_serv) {
> -		svc_get(nn->nfsd_serv);
> +	serv = nfsd_get(net);
> +	if (serv)
>  		return 0;
> -	}
>  	if (nfsd_max_blksize == 0)
>  		nfsd_max_blksize = nfsd_get_default_max_blksize();
>  	nfsd_reset_versions(nn);
> @@ -731,8 +756,6 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)
>  	int err = 0;
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>  
> -	WARN_ON(!mutex_is_locked(&nfsd_mutex));
> -
>  	if (nn->nfsd_serv == NULL || n <= 0)
>  		return 0;
>  
> @@ -766,7 +789,7 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)
>  		nthreads[0] = 1;
>  
>  	/* apply the new numbers */
> -	svc_get(nn->nfsd_serv);
> +	nfsd_get(net);
>  	for (i = 0; i < n; i++) {
>  		err = svc_set_num_threads(nn->nfsd_serv,
>  					  &nn->nfsd_serv->sv_pools[i],
> @@ -774,7 +797,7 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)
>  		if (err)
>  			break;
>  	}
> -	svc_put(nn->nfsd_serv);
> +	nfsd_put(net);
>  	return err;
>  }
>  
> @@ -826,8 +849,8 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred)
>  out_put:
>  	/* Threads now hold service active */
>  	if (xchg(&nn->keep_active, 0))
> -		svc_put(serv);
> -	svc_put(serv);
> +		nfsd_put(net);
> +	nfsd_put(net);
>  out:
>  	mutex_unlock(&nfsd_mutex);
>  	return error;
> @@ -1067,14 +1090,14 @@ bool nfssvc_encode_voidres(struct svc_rqst *rqstp, struct xdr_stream *xdr)
>  int nfsd_pool_stats_open(struct inode *inode, struct file *file)
>  {
>  	int ret;
> +	struct net *net = inode->i_sb->s_fs_info;
>  	struct nfsd_net *nn = net_generic(inode->i_sb->s_fs_info, nfsd_net_id);
>  
>  	mutex_lock(&nfsd_mutex);
> -	if (nn->nfsd_serv == NULL) {
> +	if (nfsd_get(net) == NULL) {
>  		mutex_unlock(&nfsd_mutex);
>  		return -ENODEV;
>  	}
> -	svc_get(nn->nfsd_serv);
>  	ret = svc_pool_stats_open(nn->nfsd_serv, file);
>  	mutex_unlock(&nfsd_mutex);
>  	return ret;
> @@ -1082,12 +1105,11 @@ int nfsd_pool_stats_open(struct inode *inode, struct file *file)
>  
>  int nfsd_pool_stats_release(struct inode *inode, struct file *file)
>  {
> -	struct seq_file *seq = file->private_data;
> -	struct svc_serv *serv = seq->private;
> +	struct net *net = inode->i_sb->s_fs_info;
>  	int ret = seq_release(inode, file);
>  
>  	mutex_lock(&nfsd_mutex);
> -	svc_put(serv);
> +	nfsd_put(net);
>  	mutex_unlock(&nfsd_mutex);
>  	return ret;
>  }
> 
> ---
> base-commit: 80eea12811ab8b32e3eac355adff695df5b4ba8e
> change-id: 20231026-kdevops-3c18d260bf7c
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@...nel.org>
> 
> 

Powered by blists - more mailing lists