Re: [PATCH] md/raid1: release barrier when REQ_NOWAIT write would block

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

Pre-existing issues:
- [Critical] Missing RCU read lock around `conf->mirrors[i].rdev` access in `wait_blocked_rdev()`.
- [High] Early failure paths in `raid1_write_request()` leak the `mddev->writes_pending` reference.
- [High] Allocating bios and r1bios can sleep even when the `REQ_NOWAIT` flag is set.
--

commit 3db9da0ec804800aa3afb93361ccd18de39ce5f0
Author: Abd-Alrhman Masalkhi <[email protected]>

md/raid1: release barrier when REQ_NOWAIT write would block

raid1_write_request() leaks the barrier reference taken by wait_barrier()
if wait_blocked_rdev() fails for a REQ_NOWAIT write. This patch releases
the barrier on this failure path to prevent array freeze hangs.

Closes: https://sashiko.dev/#/patchset/[email protected]?part=1

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 5b9368bd9e700..f3cc30932ba2b 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1512,6 +1512,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>  	int max_sectors;
>  	bool write_behind = false;
>  	bool is_discard = (bio_op(bio) == REQ_OP_DISCARD);
> +	sector_t sector = bio->bi_iter.bi_sector;
>  
>  	if (mddev_is_clustered(mddev) &&
>  	    mddev->cluster_ops->area_resyncing(mddev, WRITE,
> @@ -1540,6 +1541,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>  
>  	if (!wait_blocked_rdev(mddev, bio)) {
>  		bio_wouldblock_error(bio);
> +		allow_barrier(conf, sector);

[Severity: High]
This is a pre-existing issue, but does this early return path also leak the
mddev->writes_pending reference?

In raid1_make_request(), md_write_start() unconditionally takes a per-cpu
reference on mddev->writes_pending before calling raid1_write_request().
Normally, this is released via md_write_end() inside close_write().

By directly returning here after bio_wouldblock_error(bio), we never call
md_write_end(). Does this permanently leak the writes_pending counter and
prevent the array from transitioning to a clean state?

It looks like this same leak exists on the clustered area_resyncing and
wait_barrier REQ_NOWAIT failure paths earlier in this function, as well as
the err_handle path if bio_submit_split_bioset() fails.

>  		return;
>  	}

[Severity: Critical]
This is a pre-existing issue, but is it safe to call wait_blocked_rdev()
here without holding the RCU read lock?

Looking at wait_blocked_rdev():

	for (i = 0; i < disks; i++) {
		struct md_rdev *rdev = conf->mirrors[i].rdev;

		if (!rdev)
			continue;

		/* don't write here until the bad block is acknowledged */
		if (test_bit(WriteErrorSeen, &rdev->flags) &&

It accesses the rdev pointer and its fields without rcu_read_lock().
Could a concurrent device removal (via md_ioctl -> raid1_remove_conf)
replace the pointer with NULL and free the rdev structure while we are
dereferencing it here, resulting in a use-after-free?

[Severity: High]
This is also a pre-existing issue, but if we proceed past this point for a
REQ_NOWAIT bio, we call alloc_r1bio() and eventually bio_alloc_clone(),
both of which use GFP_NOIO.

Since memory pool allocations with GFP_NOIO can block when the mempool is
exhausted under memory pressure, could this cause REQ_NOWAIT writes to
sleep, defeating the purpose of the REQ_NOWAIT flag?

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