Re: [PATCH] md/raid10: honor REQ_NOWAIT in wait_blocked_dev()

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

Pre-existing issues:
- [Critical] Dropping the barrier in `wait_blocked_dev()` allows concurrent reshape to invalidate the physical mapping, leading to data corruption.
- [High] Pre-existing memory leak of `r10_bio` in `raid10_write_request()` when returning early due to `REQ_NOWAIT`.
- [High] `wait_blocked_dev()` incorrectly checks the first `conf->copies` physical disks for blocked state, rather than the dynamically calculated target disks.
- [High] Allocation of `r10_bio` unconditionally sleeps, violating `REQ_NOWAIT` semantics.
- [High] TOCTOU Use-After-Free of `md_rdev` due to missing `rcu_read_lock()` in `raid10_write_request()` and `wait_blocked_dev()`.
--

--- Patch [1]: [PATCH] md/raid10: honor REQ_NOWAIT in wait_blocked_dev() ---
commit 4ef20896ea5200b8191d2a0afa7c08883eacb0e6
Author: liyouhong <[email protected]>

md/raid10: honor REQ_NOWAIT in wait_blocked_dev()

The patch modifies wait_blocked_dev() in the raid10 driver to honor the
REQ_NOWAIT flag. It returns false immediately if a blocked device is
encountered during a nowait write request, instead of sleeping. The write
path then releases the barrier, frees the bio, and returns a wouldblock error.

> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 0a3cfdd3f5df8..2ccca42e07029 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1293,7 +1293,8 @@ static void raid10_write_one_disk(struct mddev *mddev, struct r10bio *r10_bio,
>  	}
>  }
>  
> -static void wait_blocked_dev(struct mddev *mddev, struct r10bio *r10_bio)
> +static bool wait_blocked_dev(struct mddev *mddev, struct r10bio *r10_bio,
> +			     bool nowait)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but how does wait_blocked_dev()
identify the correct mapped target disks?

The loop examines conf->mirrors[i] instead of using the dynamically
calculated target disk indices:

wait_blocked_dev() {
    ...
    for (i = 0; i < conf->copies; i++) {
        struct md_rdev *rdev, *rrdev;
        rdev = conf->mirrors[i].rdev;
    ...
}

Might this loop hardcode the check for the first conf->copies physical
disks, ignoring the actual target disks stored in r10_bio->devs[i].devnum
by raid10_find_phys()? This could risk bypassing blocked checks for disks
that the IO will actually touch.

>  	struct r10conf *conf = mddev->private;
>  	struct md_rdev *blocked_rdev;
[ ... ]
> @@ -1323,6 +1324,8 @@ static void wait_blocked_dev(struct mddev *mddev, struct r10bio *r10_bio)
>  				set_bit(BlockedBadBlocks, &rdev->flags);
>  
>  			if (rdev_blocked(rdev)) {
> +				if (nowait)
> +					return false;
>  				blocked_rdev = rdev;
>  				atomic_inc(&rdev->nr_pending);

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk for
md_rdev here?

Because atomic_inc(&rdev->nr_pending) is executed without holding
rcu_read_lock(), could a concurrent raid10_remove_disk() check the
pending count, find it zero, and call md_kick_rdev_from_array(),
freeing the device before the increment occurs?

A similar pattern happens in raid10_write_request() where
atomic_inc(&rdev->nr_pending) is also called without rcu_read_lock()
protection.

>  				break;
>  			}
[ ... ]
> @@ -1347,6 +1352,7 @@ static void wait_blocked_dev(struct mddev *mddev, struct r10bio *r10_bio)
>  		wait_barrier(conf, false);

[Severity: Critical]
This isn't a bug introduced by this patch, but can dropping the barrier
allow a concurrent reshape to invalidate the physical mapping?

When a blocked device is found, the synchronization barrier is dropped:

wait_blocked_dev() {
    ...
    allow_barrier(conf);
    md_wait_for_blocked_rdev(blocked_rdev, mddev);
    wait_barrier(conf, false);
    ...
}

Since raid10_find_phys() was already called before wait_blocked_dev(),
could a waiting reshape thread freeze the array, move data, and advance
reshape_progress while the barrier is dropped? If that happens,
raid10_write_request() might use a stale physical mapping in
r10_bio->devs to submit the IO after the barrier is re-acquired.

>  		goto retry_wait;
>  	}
> +	return true;
>  }
>  
>  static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
> @@ -1421,7 +1427,12 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
>  	r10_bio->read_slot = -1; /* make sure repl_bio gets freed */
>  	raid10_find_phys(conf, r10_bio);
>  
> -	wait_blocked_dev(mddev, r10_bio);
> +	if (!wait_blocked_dev(mddev, r10_bio, bio->bi_opf & REQ_NOWAIT)) {

[Severity: High]
This is a pre-existing issue, but are we leaking the r10_bio structure
on early returns in raid10_write_request()?

When REQ_NOWAIT is set and the IO overlaps with a clustered resync or
reshape, the function correctly bails out early, for example:

raid10_write_request() {
    ...
    if (bio->bi_opf & REQ_NOWAIT) {
        allow_barrier(conf);
        bio_wouldblock_error(bio);
        return false;
    }
    ...
}

If the function returns false without calling free_r10bio(r10_bio), will
this permanently leak the slab memory allocated for r10_bio?

[Severity: High]
This isn't a bug introduced by this patch, but would the initial allocation
of r10_bio violate REQ_NOWAIT semantics?

Before reaching the newly added nowait logic, the caller (__make_request)
unconditionally allocates r10_bio using GFP_NOIO:

__make_request() {
    ...
    r10_bio = mempool_alloc(&conf->r10bio_pool, GFP_NOIO);
    ...
}

Since GFP_NOIO allows direct reclaim, could this allocation sleep if the
mempool is exhausted, regardless of whether bio->bi_opf & REQ_NOWAIT
is set?

> +		allow_barrier(conf);
> +		free_r10bio(r10_bio);
> +		bio_wouldblock_error(bio);
> +		return false;
> +	}
>  
>  	max_sectors = r10_bio->sectors;

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