On Wed, 16 Jul 2025, Trond Myklebust wrote:
> From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>
> After setting nfl->nfs_uuid to NULL, dereferences of nfl should be
> avoided, since there are no further guarantees that the memory is still
> allocated.
nfl is not being dereferenced here. The difference between using "nfl"
and "&nfl->nfs_uuid" as the event variable is simply some address
arithmetic. As long as both are the same it doesn't matter which is
used.
> Also change the wake_up_var_locked() to be a regular wake_up_var(),
> since nfs_close_local_fh() cannot retake the nfs_uuid->lock after
> dropping it.
The point of wake_up_var_locked() is to document why wake_up_var() is
safe. In general you need a barrier between an assignment and a
wake_up_var(). I would like to eventually remove all wake_up_var()
calls, replacing them with other calls which document why the wakeup is
safe (e.g. store_release_wake_up(), atomic_dec_and_wake_up()). In this
case it is safe because both the waker and the waiter hold the same
spinlock. I would like that documentation to remain.
Also the change from RCU_INIT_POINTER() to rcu_assign_pointer() is not
documented and not needed. The primary purpose of rcu_assign_pointer()
is to include a barer so that anything that the new value points to will
be visible to a concurrent reader that uses rcu_dereference_pointer().
As NULL doesn't point to anything, no possible barrier is needed and
RCU_INIT_POINTER() is the correct interface to use.
So I think this patch is unnecessary and while it doesn't change
behaviour in any important way, it does make the intention of the code a
little less clear.
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 | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c
> index d157fdc068d7..27ad5263e400 100644
> --- a/fs/nfs_common/nfslocalio.c
> +++ b/fs/nfs_common/nfslocalio.c
> @@ -199,8 +199,8 @@ static bool nfs_uuid_put(nfs_uuid_t *nfs_uuid)
> /* Now we can allow racing nfs_close_local_fh() to
> * skip the locking.
> */
> - RCU_INIT_POINTER(nfl->nfs_uuid, NULL);
> - wake_up_var_locked(&nfl->nfs_uuid, &nfs_uuid->lock);
> + rcu_assign_pointer(nfl->nfs_uuid, NULL);
> + wake_up_var(nfl);
> }
>
> /* Remove client from nn->local_clients */
> @@ -321,8 +321,7 @@ void nfs_close_local_fh(struct nfs_file_localio *nfl)
> */
> spin_unlock(&nfs_uuid->lock);
> rcu_read_unlock();
> - wait_var_event(&nfl->nfs_uuid,
> - rcu_access_pointer(nfl->nfs_uuid) == NULL);
> + wait_var_event(nfl, rcu_access_pointer(nfl->nfs_uuid) == NULL);
> return;
> }
> /* tell nfs_uuid_put() to wait for us */
> --
> 2.50.1
>
>
