Re: [PATCH] NFS/localio: Fix a race in nfs_local_open_fh()

[Trond Myklebust <trondmy@hammerspace.com>]

On Wed, 2025-04-23 at 08:00 +1000, NeilBrown wrote:
> On Tue, 22 Apr 2025, trondmy@kernel.org wrote:
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > Once the clp->cl_uuid.lock has been dropped, another CPU could come
> > in
> > and free the struct nfsd_file that was just added. To prevent that
> > from
> > happening, take the RCU read lock before dropping the spin lock.
> 
> I think there is a race here but I think the better fix would be to
> use
> nfs_local_file_get() to get an extra reference earlier.  That ensures
> we
> won't lose the nfsd_file.
> 

Yes. This patch only ensures that the address pointed to by "nf" still
contains a valid object. It doesn't guarantee that the reference count
won't be zero, nor does it fix the issue that the initial dereference
to '*pnf' might be non-zero, yet point to an object that has a zero
refcount.

I'm not sure how much we really care, since those races should be
extremely rare and will only have a performance impact (as opposed to
the existing potential use-after-free).
That said, alternative patches are definitely welcome.

> I'm working on a patch in this area which I hope to post soon.  It
> will
> address this.
> 
> Thanks,
> NeilBrown
> 
> 
> 
> > 
> > Fixes: 86e00412254a ("nfs: cache all open LOCALIO nfsd_file(s) in
> > client")
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  fs/nfs/localio.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
> > index 5c21caeae075..4ec952f9f47d 100644
> > --- a/fs/nfs/localio.c
> > +++ b/fs/nfs/localio.c
> > @@ -278,6 +278,7 @@ nfs_local_open_fh(struct nfs_client *clp, const
> > struct cred *cred,
> >  		new = __nfs_local_open_fh(clp, cred, fh, nfl,
> > mode);
> >  		if (IS_ERR(new))
> >  			return NULL;
> > +		rcu_read_lock();
> >  		/* try to swap in the pointer */
> >  		spin_lock(&clp->cl_uuid.lock);
> >  		nf = rcu_dereference_protected(*pnf, 1);
> > @@ -287,7 +288,6 @@ nfs_local_open_fh(struct nfs_client *clp, const
> > struct cred *cred,
> >  			rcu_assign_pointer(*pnf, nf);
> >  		}
> >  		spin_unlock(&clp->cl_uuid.lock);
> > -		rcu_read_lock();
> >  	}
> >  	nf = nfs_local_file_get(nf);
> >  	rcu_read_unlock();
> > -- 
> > 2.49.0
> > 
> > 
> > 

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com