Re: Patch "nfsd: fix oops when reading pool_stats before server is started" has been added to the 6.9-stable tree

Re: Patch "nfsd: fix oops when reading pool_stats before server is started" has been added to the 6.9-stable tree

[NeilBrown]

On Mon, 24 Jun 2024, stable@vger.kernel.org wrote:
> This is a note to let you know that I've just added the patch titled
> 
>     nfsd: fix oops when reading pool_stats before server is started
> 
> to the 6.9-stable tree which can be found at:
>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> 
> The filename of the patch is:
>      nfsd-fix-oops-when-reading-pool_stats-before-server-.patch
> and it can be found in the queue-6.9 subdirectory.
> 
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@vger.kernel.org> know about it.

I feel this should not be added to the stable tree.

It moves at test on a field protected by a mutex outside of the
protection of that mutex, and so is obviously racey.

Depending on how the race goes, si->serv might be NULL when dereferenced
in svc_pool_stats_start(), or svc_pool_stats_stop() might unlock a mutex
that hadn't been locked.

I'll post a revert and a better fix for mainline.

Thanks,
NeilBrown

> 
> 
> 
> commit 388a527c6cf55bde74bc0891d0b4c38f50d896be
> Author: Jeff Layton <jlayton@kernel.org>
> Date:   Mon Jun 17 07:54:08 2024 -0400
> 
>     nfsd: fix oops when reading pool_stats before server is started
>     
>     [ Upstream commit 8e948c365d9c10b685d1deb946bd833d6a9b43e0 ]
>     
>     Sourbh reported an oops that is triggerable by trying to read the
>     pool_stats procfile before nfsd had been started. Move the check for a
>     NULL serv in svc_pool_stats_start above the mutex acquisition, and fix
>     the stop routine not to unlock the mutex if there is no serv yet.
>     
>     Fixes: 7b207ccd9833 ("svc: don't hold reference for poolstats, only mutex.")
>     Reported-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>     Signed-off-by: Jeff Layton <jlayton@kernel.org>
>     Tested-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>     Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>     Signed-off-by: Sasha Levin <sashal@kernel.org>
> 
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index b4a85a227bd7d..1a2982051f986 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -1371,12 +1371,13 @@ static void *svc_pool_stats_start(struct seq_file *m, loff_t *pos)
>  
>  	dprintk("svc_pool_stats_start, *pidx=%u\n", pidx);
>  
> +	if (!si->serv)
> +		return NULL;
> +
>  	mutex_lock(si->mutex);
>  
>  	if (!pidx)
>  		return SEQ_START_TOKEN;
> -	if (!si->serv)
> -		return NULL;
>  	return pidx > si->serv->sv_nrpools ? NULL
>  		: &si->serv->sv_pools[pidx - 1];
>  }
> @@ -1408,7 +1409,8 @@ static void svc_pool_stats_stop(struct seq_file *m, void *p)
>  {
>  	struct svc_info *si = m->private;
>  
> -	mutex_unlock(si->mutex);
> +	if (si->serv)
> +		mutex_unlock(si->mutex);
>  }
>  
>  static int svc_pool_stats_show(struct seq_file *m, void *p)
> 

Re: Patch "nfsd: fix oops when reading pool_stats before server is started" has been added to the 6.9-stable tree

[Greg KH]

On Tue, Jun 25, 2024 at 08:55:44AM +1000, NeilBrown wrote:
> On Mon, 24 Jun 2024, stable@vger.kernel.org wrote:
> > This is a note to let you know that I've just added the patch titled
> > 
> >     nfsd: fix oops when reading pool_stats before server is started
> > 
> > to the 6.9-stable tree which can be found at:
> >     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> > 
> > The filename of the patch is:
> >      nfsd-fix-oops-when-reading-pool_stats-before-server-.patch
> > and it can be found in the queue-6.9 subdirectory.
> > 
> > If you, or anyone else, feels it should not be added to the stable tree,
> > please let <stable@vger.kernel.org> know about it.
> 
> I feel this should not be added to the stable tree.
> 
> It moves at test on a field protected by a mutex outside of the
> protection of that mutex, and so is obviously racey.
> 
> Depending on how the race goes, si->serv might be NULL when dereferenced
> in svc_pool_stats_start(), or svc_pool_stats_stop() might unlock a mutex
> that hadn't been locked.
> 
> I'll post a revert and a better fix for mainline.

Now dropped, thanks!

greg k-h