Re: [PATCH V2 5/7] dax: fix holder_ops race in fs_put_dax()

[Dave Jiang <dave.jiang@intel.com>]

On 5/22/26 12:19 PM, John Groves wrote:
> From: John Groves <John@Groves.net>
> 
> Clear holder_ops before holder_data so that a concurrent fs_dax_get()
> cannot have its newly installed holder_ops overwritten. Also add a
> kerneldoc comment documenting that fs_put_dax() must only be called
> by the current holder.
> 
> Fixes: eec38f5d86d27 ("dax: add fs_dax_get() for devdax")
> Signed-off-by: John Groves <john@groves.net>

Couple things from Claude that may be worth taking a look at:

  1. Memory ordering is now load-bearing and missing

  The whole correctness argument depends on the reader observing holder_ops =
  NULL before observing holder_data = NULL. The patch uses a plain store
  followed by cmpxchg. On x86 plain stores are ordered, but on arm64/ppc they
  are not — the reader can observe cmpxchg's release of holder_data while still
  seeing the old holder_ops. That puts us back in the dangerous (holder_data ==
  NULL, holder_ops == old) state on weakly-ordered arches.

  Required:

  smp_store_release(&dax_dev->holder_ops, NULL);   /* publish ops=NULL first */
  cmpxchg(&dax_dev->holder_data, holder, NULL);    /* then release holder_data
  */

  And the reader in dax_holder_notify_failure should use
  smp_load_acquire/READ_ONCE because today it reads dax_dev->holder_ops twice
  (line 334 and line 339), allowing tearing or stale-cache reads. Pre-existing
  weakness, but this patch is what makes the ordering matter.

  kill_dax (line 461-462) has the same naked-store pattern — it should be made
  consistent.

  2. Unconditional holder_ops = NULL is a behavior regression

  Pre-patch was defensive: if a caller passed the wrong holder, the cmpxchg
  failed and nothing got cleared.

  Post-patch clears holder_ops unconditionally whenever dax_dev && holder is
  truthy. A wrong-holder fs_put_dax() now actively damages the legitimate
  holder's state — sets holder_ops to NULL while holder_data retains the
  legitimate holder's pointer. From that point, all dax_holder_notify_failure()
  calls return -EOPNOTSUPP, silently breaking the legitimate holder's
  poison-recovery path.

DJ


> ---
>  drivers/dax/super.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index 25cf99dd9360b..fa1d2a6eb2408 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c
> @@ -116,11 +116,31 @@ EXPORT_SYMBOL_GPL(fs_dax_get_by_bdev);
>  
>  #if IS_ENABLED(CONFIG_FS_DAX)
>  
> +/**
> + * fs_put_dax() - release holder ownership of a dax_device
> + * @dax_dev: dax device to release (may be NULL)
> + * @holder: the holder pointer previously passed to fs_dax_get() or
> + *          fs_dax_get_by_bdev(); must match exactly, as it is used
> + *          in a cmpxchg to atomically release ownership
> + *
> + * Must only be called by the current holder. Clears holder_ops before
> + * holder_data to avoid a race where a concurrent fs_dax_get() could have
> + * its newly installed holder_ops overwritten.
> + */
>  void fs_put_dax(struct dax_device *dax_dev, void *holder)
>  {
> -	if (dax_dev && holder &&
> -	    cmpxchg(&dax_dev->holder_data, holder, NULL) == holder)
> +	if (dax_dev && holder) {
> +		/*
> +		 * Clear holder_ops before holder_data so that a concurrent
> +		 * fs_dax_get() cannot have its newly installed holder_ops
> +		 * overwritten. holder_ops is only consulted when holder_data
> +		 * is non-NULL, so clearing ops first is safe — any in-flight
> +		 * holder_notify_failure() will see the old holder_data with
> +		 * NULL ops (a no-op) rather than new ops with wrong context.
> +		 */
>  		dax_dev->holder_ops = NULL;
> +		cmpxchg(&dax_dev->holder_data, holder, NULL);
> +	}
>  	put_dax(dax_dev);
>  }
>  EXPORT_SYMBOL_GPL(fs_put_dax);