Re: [PATCH] md/raid1: create serial pool adding rdev to array with serialize_policy=1

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but `mddev_create_serial_pool` silently ignores memory allocation failures, leading to a guaranteed kernel panic on subsequent I/O.
- [High] This problem wasn't introduced by this patch, but echoing `-writemostly` to a device's sysfs state unconditionally destroys its serialization structures, even if the array-wide `MD_SERIALIZE_POLICY` is active, causing a NULL pointer dereference on the next I/O.
--

--- Patch [1]: [PATCH] md/raid1: create serial pool adding rdev to array with serialize_policy=1 ---
commit ac16670aca465de3c72a1f91b7b1bbb4e463a255
Author: Martin Wilck <[email protected]>

md/raid1: create serial pool adding rdev to array with serialize_policy=1
    
The raid1.c code calls wait_for_serialization() if the MD_SERIALIZE_POLICY
is set, and wait_for_serialization assumes that rdev->serial is
initialized. Normally this will be the case for arrays that have
the serialize_policy sysfs attribute set to 1.
    
But when a new rdev is added to an existing array in bind_rdev_to_array(),
the condition at mddev_create_serial_pool() causes creation of rdev->serial
to be skipped. Fix it.
    
Fixes: 69b00b5bb235 ("md: introduce a new struct for IO serialization")
Signed-off-by: Martin Wilck <[email protected]>

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d1465bcd86c81..baf79c501c9ec 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -234,7 +234,8 @@ void mddev_create_serial_pool(struct mddev *mddev, struct md_rdev *rdev)
>  {
>  	int ret = 0;
>  
> -	if (rdev && !rdev_need_serial(rdev) &&
> +	if (!test_bit(MD_SERIALIZE_POLICY, &mddev->flags) &&
> +	    rdev && !rdev_need_serial(rdev) &&
>  	    !test_bit(CollisionCheck, &rdev->flags))
>  		return;
>  

[Severity: High]
This isn't a bug introduced by this patch, but I noticed that
mddev_create_serial_pool() silently ignores memory allocation failures.

If rdev_init_serial() or mempool_create_kmalloc_pool() fails due to memory
pressure, mddev_create_serial_pool() catches the error but returns early
without propagating it:

drivers/md/md.c:mddev_create_serial_pool() {
	...
	if (ret)
		return;

	if (mddev->serial_info_pool == NULL) {
	...
}

Callers like bind_rdev_to_array() proceed as if successful, setting the
MD_SERIALIZE_POLICY bit. On the next I/O, raid1_make_request() invokes
serialization functions like check_and_add_serial() that unconditionally
dereference the unallocated rdev->serial pointers:

drivers/md/raid1.c:check_and_add_serial() {
	...
	struct serial_in_rdev *serial = &rdev->serial[idx];
	spin_lock_irqsave(&serial->serial_lock, flags);
	...
}

Should mddev_create_serial_pool() return an error code instead of void to
prevent this crash?

[Severity: High]
This is another pre-existing issue, but while reviewing the serialization
logic I noticed that echoing "-writemostly" to a device's state via sysfs
unconditionally destroys its serialization structures.

In state_store(), this destroys the serial pool for the specific rdev without
checking the array-wide policy:

drivers/md/md.c:state_store() {
	...
	} else if (cmd_match(buf, "-writemostly")) {
		mddev_destroy_serial_pool(rdev->mddev, rdev);
		clear_bit(WriteMostly, &rdev->flags);
	...
}

This unconditionally frees rdev->serial for the device:

drivers/md/md.c:mddev_destroy_serial_pool() {
	...
	if (rdev)
		rdev_uninit_serial(rdev);
	...
}

If the array-wide MD_SERIALIZE_POLICY is still active, the next I/O will
invoke wait_for_serialization() and remove_serial(), which dereference the
now-freed rdev->serial pointer:

drivers/md/raid1.c:check_and_add_serial() {
	...
	struct serial_in_rdev *serial = &rdev->serial[idx];
	spin_lock_irqsave(&serial->serial_lock, flags);
	...
}

Should state_store() check if MD_SERIALIZE_POLICY is still active before
destroying the serial pool?

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