Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] dm-raid: wrong role bound and continue bypass in super_init_validation() --
commit 46b04454594856163e75cd4dccd01fd7ff3efea6 Author: Jay Vadayath <[email protected]> dm-raid: bound array_position before indexing failed_devices This commit bounds the on-disk array_position (role) by rs->raid_disks in super_init_validation() before using it as a bit index into the on-stack failed_devices bitmap. > diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c > index ac019a0f5a929..b973181bdce24 100644 > --- a/drivers/md/dm-raid.c > +++ b/drivers/md/dm-raid.c > @@ -2420,7 +2420,8 @@ static int super_init_validation(struct raid_set *rs, > struct md_rdev *rdev) > */ > if (!test_bit(FirstUse, &r->flags) && (r->raid_disk >= 0)) { > role = le32_to_cpu(sb2->array_position); > - if (role < 0) > + /* Bound @role before it indexes the failed_devices > stack bitmap. */ > + if (role < 0 || role >= rs->raid_disks) > continue; [Severity: High] Does using continue when role >= rs->raid_disks in drivers/md/dm-raid.c:super_init_validation() introduce two regressions during table load (dm_ctl_ioctl() -> table_load() -> dm_table_add_target() -> raid_ctr() -> analyse_superblocks() -> super_validate() -> super_init_validation())? First, the commit message states that this change rejects anything outside [0, rs->raid_disks), but executing continue only skips the remainder of the per-rdev loop body and allows super_init_validation() to return 0: drivers/md/dm-raid.c:super_init_validation() { ... if (!test_bit(FirstUse, &r->flags) && (r->raid_disk >= 0)) { role = le32_to_cpu(sb2->array_position); /* Bound @role before it indexes the failed_devices stack bitmap. */ if (role < 0 || role >= rs->raid_disks) continue; if (role != r->raid_disk) { ... } else if (!(rs_is_raid10(rs) && rt_is_raid0(rs->raid_type)) && !(rs_is_raid0(rs) && rt_is_raid10(rs->raid_type)) && !rt_is_raid1(rs->raid_type)) { rs->ti->error = "Cannot change device positions in raid set"; return -EINVAL; } ... } Because r->raid_disk is in [0, rs->raid_disks), any device with role >= rs->raid_disks has role != r->raid_disk. Previously, on raid levels that forbid device reordering (such as raid4/5/6 or raid10 far/offset), a device with role in [rs->raid_disks, 255] entered the role != r->raid_disk block and failed validation with return -EINVAL. With this continue, shouldn't an out-of-bounds role return -EINVAL instead of silently bypassing both the device reordering check and the failed_devices check? Second, is rs->raid_disks the right upper bound for role here? In super_init_validation(), role is the device's previous position in the on-disk array of size mddev->raid_disks (le32_to_cpu(sb->num_devices)), and failed_devices is indexed by that previous role, whereas rs->raid_disks is the device count of the newly loaded table. dm-raid allows loading tables with rs->raid_disks < mddev->raid_disks when removing legs from a raid1 array without delta_disks or when removing mirror copies from a raid10_near array: drivers/md/dm-raid.c:rs_prepare_reshape() { if (rs_is_raid10(rs)) { if (rs->raid_disks != mddev->raid_disks && __is_raid10_near(mddev->layout) && rs->raid10_copies && rs->raid10_copies != __raid10_near_copies(mddev->layout)) { ... __reorder_raid_disk_indexes(rs); ... } ... } else if (rs_is_raid1(rs)) { if (rs->delta_disks) { ... } else { /* Process raid1 without delta_disks */ mddev->raid_disks = rs->raid_disks; reshape = false; } } } When a non-trailing leg or mirror copy is removed so that rs->raid_disks < mddev->raid_disks (for example, removing leg 0 from a 3-disk raid1 array so old device 2 moves to r->raid_disk = 1 with rs->raid_disks = 2, or reducing a 6-disk 3-copy raid10_near set to a 4-disk 2-copy set so old device 4 moves to r->raid_disk = 3 with rs->raid_disks = 4), surviving devices legitimately have rs->raid_disks <= role < mddev->raid_disks. If role >= rs->raid_disks takes the continue branch, won't super_init_validation() skip updating sb2->array_position for raid10_near and skip testing failed_devices to set Faulty on a returning failed device, preventing super_validate() from clearing In_sync and resetting recovery_offset to 0? > > if (role != r->raid_disk) { -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
