Re: [PATCH v4 3/9] md/raid1: serialize non-write-behind writes on CollisionCheck rdevs

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

New issues:
- [Medium] Hot-added devices silently bypass the MD_SERIALIZE_POLICY serialization.

Pre-existing issues:
- [Critical] Concurrent overlapping I/Os cause RB-tree double-insertion and corruption in `wait_for_serialization` and `remove_serial`.
- [High] Toggling `-writemostly` or `serialize_policy` via sysfs races with in-flight I/Os, causing a Use-After-Free of `rdev->serial` and leaking `serial_info`.
--

--- Patch [3]: [PATCH v4 3/9] md/raid1: serialize non-write-behind writes on CollisionCheck rdevs ---
commit cb65d8bde9fd6c1299852f1cafb33bef4da080a2
Author: Mykola Marzhan <[email protected]>

md/raid1: serialize non-write-behind writes on CollisionCheck rdevs

This commit changes the write serialization gating from the global
MD_SERIALIZE_POLICY flag to the per-device CollisionCheck flag. It fixes
a race with write-behind I/O on write-mostly members and avoids a latent
NULL dereference for hot-added devices.

Fixes: d0d2d8ba0494 ("md/raid1: introduce wait_for_serialization")
Cc: [email protected]

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> --- 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))
>  		remove_serial(rdev, lo, hi);

[Severity: High]
This is a pre-existing issue, but does toggling -writemostly or
serialize_policy via sysfs race with in-flight I/Os here?

When mddev_suspend() is called during reconfiguration, it waits for new I/O
submissions to finish, but it does not wait for in-flight cloned bios on
the underlying disks to complete.

If mddev_destroy_serial_pool() clears CollisionCheck and frees rdev->serial
concurrently, and an in-flight bio completes, raid1_end_write_request()
might read CollisionCheck as true and dereference the freed rdev->serial
inside remove_serial().

If it reads the flag as false, it skips remove_serial(), leaking the
serial_info mempool element.

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

[Severity: Medium]
Does this change allow hot-added devices to completely bypass serialization
when the global serialize policy is active?

If an administrator sets serialize_policy=1 and a new device is hot-added,
mddev_create_serial_pool() skips initializing the serial tree for the new
device because it checks rdev_need_serial() and CollisionCheck (which is
not set yet).

Since this gate in raid1_write_request() now only checks CollisionCheck
instead of MD_SERIALIZE_POLICY, the hot-added device skips serialization
entirely.

[Severity: Critical]
This isn't a bug introduced by this patch, but can concurrent overlapping
I/Os cause an RB-tree double-insertion during wait_for_serialization()?

If three overlapping I/Os (A, B, C) arrive in sequence (e.g. A=[25-35],
B=[20-30], C=[10-20]):

I/O A is inserted. I/O B overlaps A, and gets added to A's wait list.
I/O C overlaps B but not A, so C is inserted.

When A completes, it removes itself, inserts B into the tree, and wakes B.
B wakes up and calls check_and_add_serial(), which finds C. B is added to
C's wait list and sleeps again, but remains linked in the RB-tree.

When C completes, it removes itself, splices B's waiters, and calls
raid1_rb_insert() on B again, corrupting the tree structure.

>  		}

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