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

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

New issues:
- [High] Acquiring a global configuration mutex (`reconfig_mutex`) in the I/O fast path serializes all RAID1 writes and violates non-blocking semantics.
- [High] Using an interruptible lock (`mutex_lock_interruptible`) in the block I/O submission path causes spurious block I/O errors (`BLK_STS_IOERR`) if the submitting process receives a signal.
- [High] Acquiring `reconfig_mutex` in the block I/O submission path (`raid1_make_request`) introduces a classic memory reclaim deadlock between in-flight IOs, `kswapd`, and array management.

Pre-existing issues:
- [Critical] The patch leaves the READ path completely unprotected against the concurrent `do_md_stop` Use-After-Free race that it was intended to fix for writes.
--

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

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

The commit aims to resolve a use-after-free triggered by a race condition
between write requests and array stop requests. It does this by taking the
array's reconfig_mutex inside the write I/O submission path.

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: Critical]
This is a pre-existing issue, but does this bypass leave the read path
completely unprotected against the exact same race condition?

If an array stop operation clears mddev->private concurrently, wouldn't
raid1_read_request() still dereference a freed or NULL conf pointer at:

wait_read_barrier(conf, bio->bi_iter.bi_sector);

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

[Severity: High]
Does acquiring a global configuration mutex (reconfig_mutex) in the I/O
fast path forcibly serialize all concurrent RAID1 writes and severely degrade
block I/O performance?

Will taking a sleeping lock here also violate non-blocking semantics for
asynchronous I/O if REQ_NOWAIT is set?

[Severity: High]
Can acquiring reconfig_mutex in the block I/O submission path introduce a
memory reclaim deadlock?

If memory pressure forces kswapd to write out dirty pages to the RAID1 array,
will those memory-reclaim writes attempt to acquire reconfig_mutex in
raid1_make_request()?

Since configuration pathways like do_md_stop() hold reconfig_mutex while
waiting for in-flight IOs to drain or performing memory allocations, wouldn't
this create an ABBA deadlock that hangs the I/O subsystem?

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

[Severity: High]
Does this incorrectly convert a signal interruption into a hard I/O error?

Since mddev_lock() uses mutex_lock_interruptible() to acquire the lock, it
returns -EINTR if the submitting process receives a signal.

Will converting this directly to BLK_STS_IOERR cause spurious block I/O
errors for user space applications and potentially abort filesystem journals?

> +			goto done;
> +		}

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