Re: [PATCH] md/raid1: prevent a race between write and stop request

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

New issues:
- [Critical] Using an interruptible mutex in the I/O path causes normal user-space signals to spuriously fail legitimate writes with I/O errors.
- [High] Global lock acquisition in the high-frequency block I/O fast path completely serializes all RAID1 write requests.
- [High] Acquiring `reconfig_mutex` in the block I/O submission path causes a fatal lock-inversion deadlock during memory reclaim.
- [High] Holding `reconfig_mutex` while sleeping in `wait_blocked_rdev()` indefinitely blocks the MD management thread from clearing the blocked state.

Pre-existing issues:
- [High] The patch only protects WRITE requests, leaving the READ path entirely unpatched and vulnerable to the original race condition causing a NULL pointer dereference.
--

commit 75f3c8447cce610e8b9e2c583ca2b414877078df
Author: Edward Adam Davis <[email protected]>

md/raid1: prevent a race between write and stop request

This commit attempts to fix a race condition between write and stop requests
that leads to a null pointer dereference. It introduces synchronization in
the write path using the mddev lock to ensure the consistent state of mddev
members during a write request.

Closes: https://syzkaller.appspot.com/bug?extid=3fe892ea5fc292e1353f

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index f0646fb24371f..3b9f1fa65e654 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1692,11 +1693,36 @@ static bool raid1_make_request(struct mddev *mddev, struct bio *bio)
>  	if (bio_data_dir(bio) == READ)
>  		raid1_read_request(mddev, bio, sectors, NULL);

[Severity: High]
This is a pre-existing issue, but it appears the read path is still
unprotected and vulnerable to the same race condition.

Since the patch only wraps the write path, if a concurrent do_md_stop() runs
while a READ request is processed, raid1_read_request() will fetch
mddev->private without protection. Could this result in the exact same null
pointer dereference crash on reads when wait_read_barrier() is called?

>  	else {
> +		int err;
> +
>  		md_write_start(mddev, bio);
> -		if (!raid1_write_request(mddev, bio, sectors))
> +		err = mddev_lock(mddev);

[Severity: High]
By acquiring mddev_lock() (which takes the global reconfig_mutex) in the
hot I/O submission path, are all RAID1 write requests completely serialized?
Taking a global reconfiguration mutex on every single write request could
degrade array scalability and I/O concurrency.

[Severity: High]
Could acquiring reconfig_mutex in the block I/O submission path also cause
a lock-inversion deadlock during memory reclaim?

If a thread holds reconfig_mutex (for example, during md_import_device())
and performs a GFP_KERNEL allocation, direct reclaim could write dirty pages
to the RAID array. Since raid1_make_request() attempts to acquire
reconfig_mutex via mddev_lock(), this might cause a direct self-deadlock.

> +
> +		if (err < 0) {
> +			md_write_end(mddev);
> +			status = BLK_STS_IOERR;
> +			goto done;
> +		}

[Severity: Critical]
By translating a failed interruptible mutex lock into BLK_STS_IOERR, could
normal user-space signals spuriously fail legitimate writes?

mddev_lock() uses mutex_lock_interruptible(), which returns -EINTR if a
signal is pending. If a userspace process performing I/O receives a benign
signal (like SIGALRM) while submitting a write, this path seems to fail the
write with an I/O error instead of transparently retrying or blocking.

> +
> +		if (!mddev->private) {
> +			mddev_unlock(mddev);
> +			md_write_end(mddev);
> +			status = BLK_STS_OFFLINE;
> +			goto done;
> +		}
> +
> +		err = raid1_write_request(mddev, bio, sectors);
> +		mddev_unlock(mddev);

[Severity: High]
Since reconfig_mutex is held across the entirety of raid1_write_request(),
could this block the MD management thread indefinitely?

If a write request encounters a blocked device and sleeps in
wait_blocked_rdev() inside raid1_write_request(), the I/O thread will be
holding reconfig_mutex. The MD management thread (md_check_recovery()) needs
to clear the blocked state, but it uses mddev_trylock() to acquire
reconfig_mutex. Since the I/O thread holds the lock, the management thread
will fail to acquire it, potentially leaving the I/O thread blocked forever.

> +
> +		if (!err)
>  			md_write_end(mddev);
>  	}
> +out:
>  	return true;
> +done:
> +	bio->bi_status = status;
> +	bio_endio(bio);
> +	goto out;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1