Re: [PATCH V6 09/10] dax: fix holder_ops race in fs_put_dax()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Lack of synchronization between fs_put_dax() and dax_holder_notify_failure() leads to a Use-After-Free or NULL pointer dereference in the filesystem notification callback.
--

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

dax: fix holder_ops race in fs_put_dax()

This commit attempts to fix a race in fs_put_dax() by clearing holder_ops
before releasing holder_data. It also introduces a warning to catch cases
where fs_put_dax() is called by something other than the current holder,
while intentionally tolerating NULL results during device removal.

> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index 433cd431a6c06..45f84b0eb909a 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
> +		 * dax_holder_notify_failure() that sees NULL ops returns
> +		 * -EOPNOTSUPP cleanly. A concurrent fs_dax_get() that acquires
> +		 * holder_data after the cmpxchg below is guaranteed to observe
> +		 * holder_ops=NULL first (cmpxchg provides release ordering), so
> +		 * its subsequent store of new ops will not be overwritten.
> +		 */
> +		WRITE_ONCE(dax_dev->holder_ops, NULL);
> +		prev = cmpxchg(&dax_dev->holder_data, holder, NULL);

[Severity: High]
This is a pre-existing issue, but does this lack of synchronization lead to a
Use-After-Free or NULL pointer dereference in the filesystem notification
callback?

dax_holder_notify_failure() executes under srcu_read_lock(&dax_srcu) and reads
dax_dev->holder_ops to invoke the filesystem callback. Concurrently, during
unmount, fs_put_dax() clears holder_ops and holder_data but returns without
calling synchronize_srcu(&dax_srcu).

This allows the filesystem tear-down to proceed and free the xfs_mount
structure while the notification callback is still running.

Inside xfs_dax_notify_failure(), dax_holder() is called:

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)) {
	...
}

If it sees the cleared holder_data, it returns NULL, leading to an immediate
NULL pointer dereference on mp->m_super. If it fetches the pointer before it
is cleared, it will access a freed xfs_mount, leading to a Use-After-Free.

Is it necessary to add synchronize_srcu(&dax_srcu) here to wait for in-flight
SRCU readers before returning?

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