Re: [PATCH V5 8/9] dax: fix holder_ops race in fs_put_dax()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Unconditional `WRITE_ONCE()` in `fs_put_dax()` corrupts `holder_ops` of concurrent mounts due to a pre-existing race in `fs_dax_get()`.

Pre-existing issues:
- [High] Lack of SRCU synchronization in `fs_put_dax()` leads to NULL pointer dereference or use-after-free in memory failure handlers.
--

commit 4554a08ddf0ff95891c1710cb432fdf796290f3d
Author: John Groves <John@Groves.net>

dax: fix holder_ops race in fs_put_dax()

This patch clears holder_ops before holder_data in fs_put_dax() to
prevent a concurrent fs_dax_get() from having its newly installed
operations overwritten. It adds a WARN_ON for API contract violations.

> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index 6b5ee6589e39b..af5c1e14f7e39 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c
> @@ -116,11 +116,47 @@ EXPORT_SYMBOL_GPL(fs_dax_get_by_bdev);
>  
>  #if IS_ENABLED(CONFIG_FS_DAX)
>  
[ ... ]
>  void fs_put_dax(struct dax_device *dax_dev, void *holder)
>  {
> -	if (dax_dev && holder &&
> -	    cmpxchg(&dax_dev->holder_data, holder, NULL) == holder)
> -		dax_dev->holder_ops = NULL;
> +	if (dax_dev && holder) {
> +		void *prev;
> +
> +		/*
> +		 * Clear holder_ops before releasing holder_data. A concurrent
[ ... ]
> +		 * its subsequent store of new ops will not be overwritten.
> +		 */
> +		WRITE_ONCE(dax_dev->holder_ops, NULL);

[Severity: High]
Does this unconditional write corrupt concurrent mounts?

Because the SRCU lock is dropped in fs_dax_get() before claiming
holder_data, kill_dax() can run concurrently and clear the device state.
This allows fs_dax_get() to mount a new filesystem on a dead device.

If the original filesystem performs a forced unmount and calls
fs_put_dax(), this WRITE_ONCE() unconditionally destroys the new
filesystem's holder_ops before verifying ownership via cmpxchg().

> +		prev = cmpxchg(&dax_dev->holder_data, holder, NULL);

[Severity: High]
This is a pre-existing issue, but does the lack of SRCU synchronization
here lead to use-after-free or NULL pointer dereferences in memory failure
handlers?

dax_holder_notify_failure() executes under an SRCU read lock and reads
holder_ops before calling the filesystem's callback. If fs_put_dax()
clears holder_data and completes the unmount without waiting for an SRCU
grace period, it can cause problems if a hardware memory failure runs
concurrently.

For instance, looking at xfs_dax_notify_failure():

    struct xfs_mount *mp = dax_holder(dax_dev);
    if (!(mp->m_super->s_flags & SB_BORN)) {
        ...

If holder_data is cleared right after holder_ops is checked, can the
callback read a cleared holder_data, resulting in a NULL mp dereference
when accessing mp->m_super, or access a fully freed mp if the filesystem
has finished unmounting?

> +
> +		/*
> +		 * prev == holder: normal release.
[ ... ]
> +		 *                 instead since violating the contract is
> +		 *                 a bug.
> +		 */
> +		WARN_ON(prev && prev != holder);
> +	}
>  	put_dax(dax_dev);
>  }
>  EXPORT_SYMBOL_GPL(fs_put_dax);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019eb7bcda4b-3f8edae9-d7a4-4bfa-aaea-fcef77fdbbc3-000000@email.amazonses.com?part=8