Re: [PATCH V3 7/9] dax: fix holder_ops race in fs_put_dax()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The `WARN_ON` in `fs_put_dax()` will falsely trigger during normal filesystem unmount if the DAX device was hot-unplugged.
- [High] Unconditional clear of `holder_ops` in `fs_put_dax()` before validating ownership can corrupt the state of concurrent valid holders.

Pre-existing issues:
- [High] Lockless double-fetch of `dax_dev->holder_ops` in `dax_holder_notify_failure()` can lead to a NULL pointer dereference.
- [High] Missing SRCU synchronization in `fs_put_dax()` allows use-after-free and module use-after-unload during unmount.
- [High] Inode reference leak in `fs_dax_get_by_bdev()` when a concurrent holder already owns the DAX device.
--

commit 182e9ccdc90a15375d69ff8ae0ad8f75bda03aed
Author: John Groves <John@Groves.net>

dax: fix holder_ops race in fs_put_dax()

Clear holder_ops before holder_data so that a concurrent fs_dax_get()
cannot have its newly installed holder_ops overwritten.

> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index 25cf99dd9360b..4c56ac2faacdb 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c

[ ... ]

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

[ ... ]

> +		WRITE_ONCE(dax_dev->holder_ops, NULL);
[Severity: High]
Could this unconditional write corrupt the state of concurrent valid holders?

By clearing dax_dev->holder_ops unconditionally before checking ownership
with cmpxchg(), a caller passing the wrong holder could inadvertently
overwrite the holder_ops installed by a legitimate subsystem that just
acquired the DAX device.

> +		WARN_ON(cmpxchg(&dax_dev->holder_data, holder, NULL) != holder);
[Severity: High]
Will this WARN_ON falsely trigger during a normal filesystem unmount if the
DAX device was hot-unplugged?

When a DAX device is hot-unplugged, the driver calls kill_dax(), which sets
dax_dev->holder_data = NULL.

If the user subsequently unmounts the filesystem, fs_put_dax() is called, but
the cmpxchg will see NULL instead of the expected holder. This would cause
the WARN_ON to fire and potentially panic systems configured with
panic_on_warn during normal administrative operations.

> +	}
>  	put_dax(dax_dev);
[Severity: High]
This isn't a bug introduced by this patch, but does fs_put_dax() need SRCU
synchronization before returning?

dax_holder_notify_failure() runs asynchronously under the dax_read_lock()
SRCU critical section. Since fs_put_dax() clears the holder registration but
returns immediately without calling synchronize_srcu(&dax_srcu), could a
filesystem unmount proceed, destroying in-memory structures and unloading
the module while the notify_failure callback is still actively executing?

>  }

[Severity: High]
This isn't a bug introduced by this patch, but does fs_dax_get_by_bdev() leak
an inode reference when a concurrent holder already owns the DAX device?

drivers/dax/super.c:fs_dax_get_by_bdev() {
    ...
	else if (holder) {
		if (!cmpxchg(&dax_dev->holder_data, NULL, holder))
			dax_dev->holder_ops = ops;
		else
			dax_dev = NULL;
	}
    ...
}

When a dax_dev is found, its inode reference count is incremented via
igrab(&dax_dev->inode). If cmpxchg() fails because the device is already
held, dax_dev is set to NULL, but the function appears to return without
calling iput(&dax_dev->inode) to release the reference.

[Severity: High]
This isn't a bug introduced by this patch, but is there a potential lockless
double-fetch of dax_dev->holder_ops in dax_holder_notify_failure() that could
lead to a NULL pointer dereference?

drivers/dax/super.c:dax_holder_notify_failure() {
    ...
	if (!dax_dev->holder_ops) {
		rc = -EOPNOTSUPP;
		goto out;
	}

	rc = dax_dev->holder_ops->notify_failure(dax_dev, off, len, mf_flags);
    ...
}

Since dax_dev->holder_ops is checked for NULL and then immediately
dereferenced without READ_ONCE(), the compiler might load the value twice.
If a concurrent fs_put_dax() executes WRITE_ONCE(dax_dev->holder_ops, NULL)
between these two loads, the second load could fetch NULL and cause a crash.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019e79caead2-5795328c-af48-4a93-b147-c11df7446e1a-000000@email.amazonses.com?part=7