Re: [RFC PATCH 4/5] md/raid1: use rot policy when no nonrot disk is readable
From: sashiko-bot@kernel.org
Date: 2026-08-18 07:34:29
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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; +} +
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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/20260818070646.1029149-1-chencheng@fnnas.com?part=4