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

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

Pre-existing issues:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition exists during concurrent device removal, leading to a NULL pointer dereference in both the read and write I/O submission paths.
--

--- Patch [3]: [PATCH v6 03/12] md/raid1: serialize non-write-behind writes on CollisionCheck rdevs ---
commit c5339979d08defedb52fda8881d721fd1180c172
Author: Mykola Marzhan <[email protected]>
    
md/raid1: serialize non-write-behind writes on CollisionCheck rdevs
    
This commit serializes non-write-behind writes on CollisionCheck rdevs
to prevent older behind data from landing last and leaving stale data.
It replaces the previous MD_SERIALIZE_POLICY check with CollisionCheck,
which properly accounts for write-mostly members.

Link: https://lore.kernel.org/linux-raid/[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

[ ... ]

> @@ -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);
>  

[Severity: High]
This is a pre-existing issue, but can this second loop in
raid1_write_request() encounter a NULL pointer dereference during
concurrent device removal?

The loop re-fetches rdev from conf->mirrors[i].rdev which might have
been set to NULL by a concurrent raid1_remove_conf(). If
raid1_remove_conf() executes after the first loop incremented
nr_pending on the old pointer, re-fetching it here and accessing
rdev->bdev in bio_alloc_clone() could trigger a kernel panic.

A similar issue appears to exist in the read path where
update_read_sectors() implicitly re-fetches info->rdev after an active
disk is selected in choose_best_rdev(), potentially crashing on
atomic_inc(&info->rdev->nr_pending).

> -			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))
>  				wait_for_serialization(rdev, r1_bio);
>  		}

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