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

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

[NeilBrown]

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.

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
> 
> 
> 

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

[Trond Myklebust]

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

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

[trondmy]

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.

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