Re: [PATCH v5 03/10] md/raid1: serialize non-write-behind writes on CollisionCheck rdevs

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

New issues:
- [High] Hot-added devices silently bypass the array's `MD_SERIALIZE_POLICY`, leaving them vulnerable to overlap write data corruption.

Pre-existing issues:
- [High] Dynamic evaluation of `CollisionCheck` during write-behind I/O completion races with sysfs state changes, leading to use-after-free and memory leaks.
--

--- Patch [3]: [PATCH v5 03/10] md/raid1: serialize non-write-behind writes on CollisionCheck rdevs ---
commit b12be35453573f37c0313dbe2cfc0eb2808cb133
Author: Mykola Marzhan <[email protected]>
md/raid1: serialize non-write-behind writes on CollisionCheck rdevs

This patch changes the serialization gate in raid1 to use CollisionCheck
instead of MD_SERIALIZE_POLICY. It aims to correctly serialize on
write-mostly members when write-behind is armed, and remove a latent NULL
dereference.

Cc: [email protected]

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index afe2ca96ad8c2..997f79c05db7b 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -564,7 +564,7 @@ static void raid1_end_write_request(struct bio *bio)
>  				call_bio_endio(r1_bio);
>  			}
>  		}
> -	} else if (test_bit(MD_SERIALIZE_POLICY, &rdev->mddev->flags))
> +	} else if (test_bit(CollisionCheck, &rdev->flags))

[Severity: High]
This is a pre-existing issue, but does this unlocked dynamic check of
CollisionCheck create a time-of-check to time-of-use race against sysfs
state changes?

When -writemostly is echoed to sysfs, mddev_suspend() is called. For
write-behind I/O, mddev->active_io is decremented early when non-writemostly
clones finish, which allows mddev_suspend() to return while the write-mostly
clone is still in flight.

Then state_store() in drivers/md/md.c calls mddev_destroy_serial_pool()
which frees rdev->serial and clears CollisionCheck:

drivers/md/md.c:rdev_uninit_serial() {
    if (!test_and_clear_bit(CollisionCheck, &rdev->flags))
        return;

    kvfree(rdev->serial);
    rdev->serial = NULL;
}

When the in-flight write-mostly clone subsequently completes here in
raid1_end_write_request(), could it check CollisionCheck and either call
remove_serial() on the freed pointer, or skip it and leak the serial_info
elements?

>  		remove_serial(rdev, lo, hi);
>  	if (r1_bio->bios[mirror] == NULL)
>  		rdev_dec_pending(rdev, conf->mddev);

[ ... ]

> @@ -1677,7 +1677,11 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
>  			mbio = bio_alloc_clone(rdev->bdev, bio, GFP_NOIO,
>  					       &mddev->bio_set);
>  
> -			if (test_bit(MD_SERIALIZE_POLICY, &mddev->flags))
> +			/*
> +			 * CollisionCheck marks every rdev with a serial
> +			 * tree; order against in-flight write-behind I/O.
> +			 */
> +			if (test_bit(CollisionCheck, &rdev->flags))

[Severity: High]
By gating serialization exclusively on CollisionCheck, do hot-added devices
now silently bypass serialization?

When an array has MD_SERIALIZE_POLICY enabled and a new device is hot-added,
mddev_create_serial_pool() skips initializing the device's serial tree
because it doesn't check the global policy flag.

Since CollisionCheck remains unset for the new device, this change masks
the previous NULL pointer dereference crash by skipping serialization for
the hot-added device entirely. Will this leave the array vulnerable to
overlap write data corruption when stale data persists?

>  				wait_for_serialization(rdev, r1_bio);
>  		}

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