Re: [PATCH V6 08/10] dax: read holder_ops once in dax_holder_notify_failure()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch uses READ_ONCE() to prevent a NULL pointer dereference on `holder_ops` but fails to address a critical race condition with `holder_data`, leaving the code vulnerable to an immediate NULL pointer dereference or Use-After-Free in the filesystem callback.
--

commit ee437cee4c275726ddf2ce4c7880e48e8f58eab5
Author: John Groves <John@Groves.net>

dax: read holder_ops once in dax_holder_notify_failure()

This commit uses READ_ONCE() to fetch dax_dev->holder_ops exactly once in
dax_holder_notify_failure(). This prevents a race condition where a concurrent
fs_put_dax() clears holder_ops between the NULL check and the indirect call.

> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index 25cf99dd9360b..433cd431a6c06 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c
> @@ -311,12 +312,19 @@ int dax_holder_notify_failure(struct dax_device *dax_dev, u64 off,
>  		goto out;
>  	}
>  
> -	if (!dax_dev->holder_ops) {
> +	/*
> +	 * Read holder_ops once: a concurrent fs_put_dax() can clear it without
> +	 * synchronizing against readers. Without the single fetch the compiler
> +	 * could reload between the NULL check and the call and dereference a
> +	 * NULL ops.
> +	 */
> +	ops = READ_ONCE(dax_dev->holder_ops);
> +	if (!ops) {
>  		rc = -EOPNOTSUPP;
>  		goto out;
>  	}
>  
> -	rc = dax_dev->holder_ops->notify_failure(dax_dev, off, len, mf_flags);
> +	rc = ops->notify_failure(dax_dev, off, len, mf_flags);

[Severity: High]
This is a pre-existing issue, but does fixing the holder_ops race still leave
a race with holder_data?

If a concurrent fs_put_dax() executes right after ops is fetched but before
ops->notify_failure() executes, holder_data could be cleared to NULL because
fs_put_dax() does not call synchronize_srcu(&dax_srcu) to wait for active
readers:

drivers/dax/super.c:fs_put_dax() {
	if (dax_dev && holder) {
		void *prev;
		WRITE_ONCE(dax_dev->holder_ops, NULL);
		prev = cmpxchg(&dax_dev->holder_data, holder, NULL);
		WARN_ON(prev && prev != holder);
	}
	put_dax(dax_dev);
}

The filesystem callback in XFS retrieves holder_data via dax_holder() and
expects it to be valid:

fs/xfs/xfs_notify_failure.c:xfs_dax_notify_failure() {
	struct xfs_mount	*mp = dax_holder(dax_dev);

	if (!(mp->m_super->s_flags & SB_BORN)) {
		xfs_warn(mp, "filesystem is not ready for notify_failure()!");
...
}

Because xfs_dax_notify_failure() unconditionally dereferences the retrieved
holder without a NULL check, could this result in an immediate NULL pointer
dereference or a use-after-free as the unmount proceeds?

A later patch in the series changes the ordering of how these pointers are
cleared, but it appears to still lack synchronization with dax_read_lock()
readers.

>  out:
>  	dax_read_unlock(id);
>  	return rc;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019ecc080a68-8dc0c99f-ab17-4aa9-83d9-490e9c97ac2e-000000@email.amazonses.com?part=8