On 26/05/26 05:16PM, Dave Jiang wrote:
>
>
> On 5/22/26 12:19 PM, John Groves wrote:
> > From: John Groves <[email protected]>
> >
> > 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 <[email protected]>
>
> 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
> */
Updating to WRITE_ONCE(), which I think is the right choice
>
> 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.
Will study this and post a separate patch for kill_dax if I think it's
warranted
>
> 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.
This is a bit of a sticky wicket. The API contract is that the caller
of fs_dax_put() is the holder. To get the ordering right AND guard against
non-holder callers would require a lock.
Instead, I think the right answer is:
WRITE_ONCE(dax_dev->holder_ops, NULL);
WARN_ON(cmpxchg(&dax_dev->holder_data, holder, NULL) != holder);
If a non-holder calls this function, that's a bug and we'll get the
WARN_ON(). If a holder calls this function twice, we'll get the WARN_ON()
(the second time).
And when the API contract is honored, we have correct ordering.
>
> DJ
Thanks Dave!
John
<snip>
