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.
At this point, with nfs_uuid->net set to NULL, nothing can be added to
nfs_uuid->files. Things can only be removed.
So if list_first_entry_or_null stops being nfl, then we know that nfl
has been removed from the list and cannot possibly be added again.
This must have been done by nfs_close_local_fh() which set ->nfs_uuid to
NULL so that nfs_uuid_put() decided to wait.
>
> Also change the event variable to be nfs_uuid, for the same reason that
> no reference is held to nfl.
The event variable is never dereferenced so we don't need to hold a
reference to it. The address is hashed to choose a wait queue - that is
all it is used for.
So it is perfectly safe to wait on &nfl->nfs_uuid or to wake it up
without a reference to nfl. All that matters is that the waker and the
waiter use the same address.
So I believe this patch is wrong. The extra test on nfl->nfs_uuid is
incorrect and not needed. The change to the event variable is not needed
except that they must both be the same (which is what my earlier patch
did).
Thanks,
NeilBrown
>
> Acked-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> Tested-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> Fixes: 21fb44034695 ("nfs_localio: protect race between nfs_uuid_put() and nfs_close_local_fh()")
> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> ---
> fs/nfs_common/nfslocalio.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c
> index 64949c46c174..d157fdc068d7 100644
> --- a/fs/nfs_common/nfslocalio.c
> +++ b/fs/nfs_common/nfslocalio.c
> @@ -177,12 +177,13 @@ static bool nfs_uuid_put(nfs_uuid_t *nfs_uuid)
> /* nfs_close_local_fh() is doing the
> * close and we must wait. until it unlinks
> */
> - wait_var_event_spinlock(nfl,
> - list_first_entry_or_null(
> - &nfs_uuid->files,
> - struct nfs_file_localio,
> - list) != nfl,
> - &nfs_uuid->lock);
> + wait_var_event_spinlock(
> + nfs_uuid,
> + list_first_entry_or_null(
> + &nfs_uuid->files,
> + struct nfs_file_localio, list) != nfl ||
> + rcu_access_pointer(nfl->nfs_uuid),
> + &nfs_uuid->lock);
> continue;
> }
>
> @@ -338,7 +339,7 @@ void nfs_close_local_fh(struct nfs_file_localio *nfl)
> */
> spin_lock(&nfs_uuid->lock);
> list_del_init(&nfl->list);
> - wake_up_var_locked(&nfl->nfs_uuid, &nfs_uuid->lock);
> + wake_up_var_locked(nfs_uuid, &nfs_uuid->lock);
> spin_unlock(&nfs_uuid->lock);
> }
> EXPORT_SYMBOL_GPL(nfs_close_local_fh);
> --
> 2.50.1
>
>
