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@xxxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx, trond.myklebust@xxxxxxxxxxxxxxx
>
