Re: [PATCH V5 7/9] 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 fixes a TOCTOU race on `holder_ops` in `dax_holder_notify_failure()` but fails to address a missing SRCU synchronization in `fs_put_dax()`. Concurrently, `fs_put_dax()` clears `holder_ops` and `holder_data` but returns immediately without waiting for `dax_srcu` readers (unlike `kill_dax()`). This enables a filesystem callback to dereference a NULL `holder_data` pointer, or execute unloaded module memory if the module is unloaded before the preempted callback resumes.
--

commit fb9deacd32044f5bf6a6a9877ee95b7eb436b68b
Author: John Groves <John@Groves.net>
dax: read holder_ops once in dax_holder_notify_failure()

This commit updates dax_holder_notify_failure() to read dax_dev->holder_ops
once using READ_ONCE(). This prevents a concurrent clear of holder_ops from
causing a NULL pointer dereference during the indirect function call.

> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c

[ ... ]

> @@ -311,12 +312,18 @@ 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() or kill_dax() can
> +	 * clear it. 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 this fully address the race with
holder_data being cleared?

If fs_put_dax() clears both holder_ops and holder_data concurrently, it
returns without calling synchronize_srcu(&dax_srcu) to wait for dax_srcu
readers.

If a thread is preempted right before calling ops->notify_failure() here,
and fs_put_dax() executes, the callback will proceed with a valid ops pointer
but a NULL holder_data.

For example, xfs_dax_notify_failure() calls dax_holder() which returns NULL,
and then unconditionally dereferences the mount pointer:

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

Could this result in a NULL pointer dereference, or an execution-after-free
if the filesystem module is subsequently unloaded?

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

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