Re: [PATCH 2/3] NFS/localio: nfs_uuid_put() fix the wait for file unlink events

NeilBrown
2025-07-16T03:51:53Z

On Wed, 16 Jul 2025, Trond Myklebust wrote:
> On Wed, 2025-07-16 at 11:22 +1000, NeilBrown wrote:
> > On Wed, 16 Jul 2025, Trond Myklebust wrote:
> > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > >
> > > No reference to nfl is held when waiting in nfs_uuid_put(), so not
> > > only
> > > must the event condition check if the first entry in the list has
> > > changed, it must also check if the nfl->nfs_uuid field is still
> > > NULL,
> > > in case the old entry was replaced.
> >
> > As no reference is held to nfl, it cannot be safe to check if
> > nfl->nfs_uuid is still NULL.  It could be freed and the memory reused
> > for anything.
>
> It is quite safe.
>
> The test first checks if the nfs_uuid->files list first entry still
> points to 'nfl'. Then it checks the value of nfl->nfs_uuid.
>
> All this happens while the nfs_uuid->lock is held.

Ok, agreed. It is safe.

>
> >
> > At this point, with nfs_uuid->net set to NULL, nothing can be added
> > to
> > nfs_uuid->files.  Things can only be removed.
>
> There is nothing in either nfs_open_local_fh() or nfs_uuid_add_file()
> that stops anyone from adding a new entry to nfs_uuid->files in the
> case where nfs_uuid->net is NULL.

nfs_open_local_fh() starts:
rcu_read_lock();
net = rcu_dereference(uuid->net);
if (!net || !nfs_to->nfsd_net_try_get(net)) {
rcu_read_unlock();
return ERR_PTR(-ENXIO);
}
rcu_read_unlock();

so if uuid->net is NULL, it will return and not add to the list.
Of course there could be races with nfs_put_uuid() called after that
check. The nfsd_net_try_get(net) check is supposed to handle that case.

But I cannot convince myself that it does. In particular expkey_flush()
calls nfsd_file_cache_purge() which calls
nfs_localio_invalidate_clients(). This can happen when
nfsd_net_try_get() will succeed.

So I agree that things could get added to the nfs_uuid->list after ->net
has been set to NULL, but that is a bug. nfs_uuid_put() shouldn't be
checking for that.

I don't think nfsd_file_cache_purge() should be invalidating the clients
in this case - just the files that the clients have open so they are
forced to re-open.

I'm not sure how best to resolve this at present.

Thanks,
NeilBrown

>
> If that was the intention, then I agree that this patch is wrong, but
> not for the reasons you listed.
>
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trondmy@kernel.org, trond.myklebust@hammerspace.com
>