[PATCH 0/3] md: Fix three raid5/raid10 bugs

[PATCH 0/3] md: Fix three raid5/raid10 bugs

[Zhihao Cheng]

Fix three raid5/raid10 bugs in error injection case.

Zhihao Cheng (3):
  md/raid5: Hide the origin mddev->thread before takeover
  md/raid5: Don't free conf on raid5_run failure
  md/raid10: Don't free conf on raid10_run failure

 drivers/md/raid10.c | 22 ++++++++++------------
 drivers/md/raid5.c  | 39 ++++++++++++++++++++++++++++-----------
 2 files changed, 38 insertions(+), 23 deletions(-)

-- 
2.52.0

[PATCH 1/3] md/raid5: Hide the origin mddev->thread before takeover

[Zhihao Cheng]

The raid5 takeover invokes setup_conf and allocates strip heads, but
it wakes up the wrong thread, which lefts strip heads in the list
'conf->released_stripes' and not being processed. If raid5_run fails,
the strip heads won't be released, which triggers the following slab
warnings (CONFIG_SLUB_DEBUG):
 BUG raid5-md0 (Not tainted): Objects remaining on __kmem_cache_shutdown()
 Object 0x0000000062fad548 @offset=3968
 Object 0x000000007f74683c @offset=4960
 WARNING: mm/slub.c:1268 at __slab_err+0x31/0x40, CPU#0: bash/865
 RIP: 0010:__slab_err+0x31
 Call Trace:
  __kmem_cache_shutdown.cold+0x15b
  kmem_cache_destroy+0x71
  free_conf+0xf8
  raid5_run.cold+0x463
  level_store+0x64e
  md_attr_store+0xd7

The detailed triggering process is as follows:
 mdadm --create /dev/md0 --level=1 --raid-devices=2 /dev/sda /dev/sdb
 --force --assume-clean # create raid1, mddev->thread is raid1d
 echo 5 > /sys/block/md0/md/level
  level_store
   raid5_takeover_raid1
     setup_conf
      grow_stripes
       grow_one_stripe
        sh = alloc_stripe
        raid5_release_stripe
          md_wakeup_thread(conf->mddev->thread) // wakeup raid1d
     raid5_run
      ENOMEM = raid5_create_ctx_pool
      free_conf
       shrink_stripes
        drop_one_stripe // no strips found from the conf->inactive_list
        kmem_cache_destroy(conf->slab_cache)
	 __kmem_cache_shutdown
	  free_partial
	   list_slab_objects // some entries are not released !

Fix it by hiding the origin mddev->thread before takeover, so that
new allocating strip heads can be put into 'conf->inactive_list',
which can be found by drop_one_stripe().

Fixes: 773ca82fa1ee ("raid5: make release_stripe lockless")
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 drivers/md/raid5.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index c091bba95c31..7e87e8a60f5f 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -9039,19 +9039,38 @@ static void *raid5_takeover(struct mddev *mddev)
 	 *  raid4 - trivial - just use a raid4 layout.
 	 *  raid6 - Providing it is a *_6 layout
 	 */
-	if (mddev->level == 0)
-		return raid45_takeover_raid0(mddev, 5);
-	if (mddev->level == 1)
-		return raid5_takeover_raid1(mddev);
-	if (mddev->level == 4) {
+	void *ret = ERR_PTR(-EINVAL);
+	struct md_thread *thread;
+
+	thread = rcu_dereference_protected(mddev->thread,
+				lockdep_is_held(&mddev->reconfig_mutex));
+	/*
+	 * Set mddev->thread to NULL before setup_conf() to avoid waking up
+	 * wrong thread(eg. raid1), which can prevent the strips from being
+	 * left unreleased in the error handling path(free_conf) of raid5_run.
+	 */
+	rcu_assign_pointer(mddev->thread, NULL);
+
+	switch (mddev->level) {
+	case 0:
+		ret = raid45_takeover_raid0(mddev, 5);
+		break;
+	case 1:
+		ret = raid5_takeover_raid1(mddev);
+		break;
+	case 4:
 		mddev->new_layout = ALGORITHM_PARITY_N;
 		mddev->new_level = 5;
-		return setup_conf(mddev);
+		ret = setup_conf(mddev);
+		break;
+	case 6:
+		ret = raid5_takeover_raid6(mddev);
+		break;
 	}
-	if (mddev->level == 6)
-		return raid5_takeover_raid6(mddev);
 
-	return ERR_PTR(-EINVAL);
+	rcu_assign_pointer(mddev->thread, thread);
+
+	return ret;
 }
 
 static void *raid4_takeover(struct mddev *mddev)
-- 
2.52.0

[PATCH 2/3] md/raid5: Don't free conf on raid5_run failure

[Zhihao Cheng]

Set 'raid_disks' after raid5_run() failure will trigger an
null-ptr-deref problem:
 BUG: kernel NULL pointer dereference, address: 0000000000000038
 RIP: 0010:raid5_check_reshape+0xad
 Call Trace:
  update_raid_disks+0x124
  raid_disks_store+0x145
  md_attr_store+0xd7
  sysfs_kf_write+0x7c

The trigger process is simple:
 mdadm --create /dev/md0 --level=1 --raid-devices=2 /dev/sda /dev/sdb
 --force --assume-clean # create raid1
 echo 5 > /sys/block/md0/md/level
  level_store
   mddev->pers = pers
   mddev->private = priv
   raid5_run
    fail to abort (eg. raid5_create_ctx_pool fails)
    mddev->private = NULL
 echo 10 > /sys/block/md0/md/raid_disks
  raid_disks_store
   if (mddev->pers) // true
    update_raid_disks
     raid5_check_reshape
      conf = mddev->private
       conf->algorithm = mddev->new_layout // null-ptr-deref !
 // similar process in do_md_stop->__md_stop_writes->raid5_quiesce

Just like commit 35f20acaa358 ("md/raid0: don't free conf on raid0_run
failure") does, fix it by not free conf on raid5_run failure.

Fixes: 245f46c2c221e ("md: add ->takeover method to support changing the personality managing an array")
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 drivers/md/raid5.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 7e87e8a60f5f..913e5a709e35 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -8263,8 +8263,6 @@ static int raid5_run(struct mddev *mddev)
 abort:
 	md_unregister_thread(mddev, &mddev->thread);
 	print_raid5_conf(conf);
-	free_conf(conf);
-	mddev->private = NULL;
 	pr_warn("md/raid:%s: failed to run raid set.\n", mdname(mddev));
 	return ret;
 }
-- 
2.52.0

[PATCH 3/3] md/raid10: Don't free conf on raid10_run failure

[Zhihao Cheng]

Set 'raid_disks' after raid5_run() failure will trigger an
null-ptr-deref problem:
 BUG: kernel NULL pointer dereference, address: 00000000000000dc
 RIP: 0010:_raw_spin_lock_irq+0x3d
 Call Trace:
  raise_barrier+0x3a
  raid10_quiesce+0x20
  __md_stop_writes+0x6c
  do_md_stop+0xaa

The trigger process is simple:
 mdadm --create /dev/md0 --level=0 --raid-devices=2 /dev/sda /dev/sdb
 --force --assume-clean # create raid0
 echo 10 > /sys/block/md0/md/level
  level_store
   mddev->pers = pers
   mddev->private = priv
   raid10_run
   fail to out_free_conf (eg. enough fails)
    raid10_free_conf(conf)
    mddev->private = NULL
 echo inactive > /sys/block/md0/md/array_state
  array_state_store
   do_md_stop
    __md_stop_writes
     raid10_quiesce
      raise_barrier
       write_seqlock_irq(&conf->resync_lock) // null-ptr-deref !

Just like commit 35f20acaa358 ("md/raid0: don't free conf on raid0_run
failure") does, fix it by not free conf on raid10_run failure.

Fixes: 245f46c2c221e ("md: add ->takeover method to support changing the personality managing an array")
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 drivers/md/raid10.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 1093c798d9dd..9e4d4202f35c 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -3972,7 +3972,7 @@ static int raid10_run(struct mddev *mddev)
 		if (fc > 1 || fo > 0) {
 			pr_err("only near layout is supported by clustered"
 				" raid10\n");
-			goto out_free_conf;
+			goto out_unregister_thread;
 		}
 	}
 
@@ -3989,11 +3989,11 @@ static int raid10_run(struct mddev *mddev)
 
 		if (test_bit(Replacement, &rdev->flags)) {
 			if (disk->replacement)
-				goto out_free_conf;
+				goto out_unregister_thread;
 			disk->replacement = rdev;
 		} else {
 			if (disk->rdev)
-				goto out_free_conf;
+				goto out_unregister_thread;
 			disk->rdev = rdev;
 		}
 		diff = (rdev->new_data_offset - rdev->data_offset);
@@ -4013,7 +4013,7 @@ static int raid10_run(struct mddev *mddev)
 
 		if (err) {
 			ret = err;
-			goto out_free_conf;
+			goto out_unregister_thread;
 		}
 	}
 
@@ -4021,17 +4021,17 @@ static int raid10_run(struct mddev *mddev)
 	if (!enough(conf, -1)) {
 		pr_err("md/raid10:%s: not enough operational mirrors.\n",
 		       mdname(mddev));
-		goto out_free_conf;
+		goto out_unregister_thread;
 	}
 
 	if (conf->reshape_progress != MaxSector) {
 		/* must ensure that shape change is supported */
 		if (conf->geo.far_copies != 1 &&
 		    conf->geo.far_offset == 0)
-			goto out_free_conf;
+			goto out_unregister_thread;
 		if (conf->prev.far_copies != 1 &&
 		    conf->prev.far_offset == 0)
-			goto out_free_conf;
+			goto out_unregister_thread;
 	}
 
 	mddev->degraded = 0;
@@ -4081,7 +4081,7 @@ static int raid10_run(struct mddev *mddev)
 	set_bit(MD_FAILFAST_SUPPORTED, &mddev->flags);
 
 	if (md_integrity_register(mddev))
-		goto out_free_conf;
+		goto out_unregister_thread;
 
 	if (conf->reshape_progress != MaxSector) {
 		unsigned long before_length, after_length;
@@ -4094,7 +4094,7 @@ static int raid10_run(struct mddev *mddev)
 		if (max(before_length, after_length) > min_offset_diff) {
 			/* This cannot work */
 			pr_warn("md/raid10: offset difference not enough to continue reshape\n");
-			goto out_free_conf;
+			goto out_unregister_thread;
 		}
 		conf->offset_diff = min_offset_diff;
 
@@ -4106,10 +4106,8 @@ static int raid10_run(struct mddev *mddev)
 
 	return 0;
 
-out_free_conf:
+out_unregister_thread:
 	md_unregister_thread(mddev, &mddev->thread);
-	raid10_free_conf(conf);
-	mddev->private = NULL;
 out:
 	return ret;
 }
-- 
2.52.0

Re: [PATCH 3/3] md/raid10: Don't free conf on raid10_run failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] md/raid10: NULL ptr deref and I/O hang on raid10_run failure
--

commit 1bebf502d0ba7be4661dc272d82091e48c0924ff
Author: Zhihao Cheng <chengzhihao1@huawei.com>

md/raid10: Don't free conf on raid10_run failure

This patch prevents freeing the configuration on raid10_run() failure to
avoid a null pointer dereference later during shutdown. It modifies the
failure path to skip raid10_free_conf() and clearing mddev->private, leaving
the conf structure allocated.

> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 1093c798d9dde..9e4d4202f35cf 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
[ ... ]
> @@ -4106,10 +4106,8 @@ static int raid10_run(struct mddev *mddev)
>  
>  	return 0;
>  
> -out_free_conf:
> +out_unregister_thread:
>  	md_unregister_thread(mddev, &mddev->thread);
> -	raid10_free_conf(conf);
> -	mddev->private = NULL;

[Severity: High]
Does leaving mddev->private allocated here expose the array to panics and
I/O hangs when raid10_run() fails?

Since the sysfs interface ignores personality run() failures:

drivers/md/md.c:level_store() {
    ...
    pers->run(mddev);
    ...
}

The array remains pseudo-active. If subsequent I/O requests enter
raid10_make_request(), they can hang indefinitely because they attempt
to wake a NULL thread:

drivers/md/raid10.c:raid10_make_request() {
    ...
    bio_list_add(&conf->pending_bio_list, mbio);
    spin_unlock_irqrestore(&conf->device_lock, flags);
    md_wakeup_thread(mddev->thread);
    ...
}

Furthermore, if a concurrent teardown or flush raises conf->barrier,
wait_barrier() will call stop_waiting_barrier(), which unconditionally
dereferences the NULL thread pointer, causing a panic:

drivers/md/raid10.c:stop_waiting_barrier() {
    ...
    if (thread->tsk == current) {
    ...
}

>  out:
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921071537.2902362-1-chengzhihao1@huawei.com?part=3