Re: [RFC PATCH 4/5] md/raid1: use rot policy when no nonrot disk is readable

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

New issues:
- [Low] The patch leaves behind dead code related to `conf->nonrot_disks`.

Pre-existing issues:
- [High] Lockless iteration over `conf->mirrors` lacks `READ_ONCE()` and RCU protection, leading to TOCTOU NULL pointer dereferences and Use-After-Free during concurrent device removal.
--

--- Patch [4]: [RFC PATCH 4/5] md/raid1: use rot policy when no nonrot disk is readable ---
commit 53dd92b2fd0628bc1951a803d06d7fa324cf3f85
Author: Chen Cheng <[email protected]>

md/raid1: use rot policy when no nonrot disk is readable

has_nonrot selects mixed policy or rot-only policy. Currently, has_nonrot
is true if conf->nonrot_disks > 0, which counts every nonrot disk including
faulty or WriteMostly disks. This commit updates the logic to look at disks
that can actually take the read, and if none are nonrot, uses the rot-only
policy.

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 523b55d427791..f476d4dea4be5 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -795,6 +795,24 @@ static int raid1_rr_pos(int disk, int start, int n)
>  	return ((disk % n) - start + n) % n;
>  }
>  
> +static bool raid1_has_readable_nonrot(struct r1conf *conf,
> +				      struct r1bio *r1_bio)
> +{
> +	int disk;
> +
> +	for (disk = 0; disk < conf->raid_disks * 2; disk++) {
> +		struct md_rdev *rdev;
> +
> +		if (r1_bio->bios[disk] == IO_BLOCKED)
> +			continue;
> +		rdev = conf->mirrors[disk].rdev;
> +		if (rdev_readable(rdev, r1_bio) &&
> +		    test_bit(Nonrot, &rdev->flags))
> +			return true;

[Severity: High]
This is a pre-existing issue, but does this lockless iteration over
conf->mirrors lack READ_ONCE() and RCU protection?

In raid1_has_readable_nonrot(), we read conf->mirrors[disk].rdev locklessly.
If HOT_REMOVE_DISK runs concurrently, raid1_remove_conf() can zero the rdev
pointer. Without READ_ONCE(), the compiler might reload the pointer between
the NULL check inside rdev_readable() and test_bit(Nonrot, &rdev->flags),
causing a NULL pointer dereference.

Additionally, since raid1_read_request() (which calls read_balance() and then
raid1_has_readable_nonrot()) does not hold rcu_read_lock(), could the reader
be preempted, allowing md_kick_rdev_from_array() to complete synchronize_rcu()
and free the rdev, resulting in a Use-After-Free?

This identical pattern is also present in choose_best_rdev().

> +	}
> +	return false;
> +}
> +

[ ... ]

> @@ -816,7 +834,7 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
>  {
>  	int disk;
>  	int rr_start = 0;
> -	bool has_nonrot = READ_ONCE(conf->nonrot_disks);
> +	bool has_nonrot = raid1_has_readable_nonrot(conf, r1_bio);

[Severity: Low]
Does this change leave conf->nonrot_disks as dead code?

By removing the only reader of conf->nonrot_disks in choose_best_rdev(),
should the nonrot_disks field also be removed from struct r1conf in
drivers/md/raid1.h, as well as the updates to it in raid1_add_conf() and
raid1_remove_conf()?

>  	struct read_balance_ctl ctl = {
>  		.closest_dist_disk      = -1,
>  		.closest_dist           = MaxSector,

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