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

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

[Martin Wilck]

The following bug has been observed with kernel 7.1.3 after adding a new
rdev to an existing RAID1 array that had enabled serialize_policy:

[ 1754.091836][T19639] Oops: Oops: 0002 [#1] SMP NOPTI
[ 1754.092679][T19639] CPU: 0 UID: 0 PID: 19639 Comm: ext4lazyinit Not tainted 7.1.3-1-default #1 PREEMPT(full) openSUSE Tumbleweed  b041a6527f6e58424f4cd3de0fade8d408b378fd
[ 1754.095245][T19639] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS unknown 02/02/2022
[ 1754.096772][T19639] RIP: 0010:_raw_spin_lock_irqsave+0x27/0x50
[ 1754.097769][T19639] Code: 90 90 90 f3 0f 1e fa 0f 1f 44 00 00 53 9c 58 0f 1f 40 00 48 89 c3 fa 0f 1f 44 00 00 65 ff 05 c0 bc d8 01 31 c0 ba 01 00 00 00 <3e> 0f b1 17 75 09 48 89 d8 5b e9 85 8d fc fe 89 c6 e8 c3 08 00 00
[ 1754.100996][T19639] RSP: 0018:ffffcfbbc0a5bab8 EFLAGS: 00010046
[ 1754.102037][T19639] RAX: 0000000000000000 RBX: 0000000000000212 RCX: 61c8864680b583eb
[ 1754.103367][T19639] RDX: 0000000000000001 RSI: 000000000000092a RDI: 0000000000004960
[ 1754.104723][T19639] RBP: ffff8efae2b12700 R08: 00000000ffffffff R09: 0000000000000000
[ 1754.106038][T19639] R10: ffffffffa0605ac0 R11: 0000000000000001 R12: ffff8efad4199400
[ 1754.107343][T19639] R13: 0000000000004950 R14: ffff8efaf9164600 R15: 000000000004043e
[ 1754.108697][T19639] FS:  0000000000000000(0000) GS:ffff8efbdc754000(0000) knlGS:0000000000000000
[ 1754.110190][T19639] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1754.111280][T19639] CR2: 0000000000004960 CR3: 000000000d830001 CR4: 0000000000770ef0
[ 1754.112580][T19639] PKRU: 55555554
[ 1754.113182][T19639] Call Trace:
[ 1754.113754][T19639]  <TASK>
[ 1754.114256][T19639]  wait_for_serialization+0xb9/0x260 [raid1 9ad4618e4becc97c5a7140f7aa35cc4730d97dd9]
[ 1754.115869][T19639]  raid1_make_request+0x762/0xaff [raid1 9ad4618e4becc97c5a7140f7aa35cc4730d97dd9]
[ 1754.118368][T19639]  md_handle_request+0x1c9/0x2e0 [md_mod a261f96e3d61eb9d00ceb34dc9439cf6487db185]

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 <mwilck@suse.com>
---
 drivers/md/md.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index d1465bcd86c8..baf79c501c9e 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;
 
-- 
2.51.0

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

[sashiko-bot]

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 <martin.wilck@suse.com>

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 <mwilck@suse.com>

> 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/20260723112741.1206836-1-mwilck@suse.com?part=1