On Wed, 2025-04-23 at 08:00 +1000, NeilBrown wrote:
> On Tue, 22 Apr 2025, trondmy@xxxxxxxxxx wrote:
> > From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
