Re: [PATCH v2 2/3] md/raid5: fix leak and use-after-free in resize_stripes error path

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

Pre-existing issues:
- [Critical] Concurrent lockless read of `conf->disks` in IO and error paths races with synchronous `kfree(old_disks)`.
- [High] Memory leak of `stripe_head` pages (`nsh->dev[i].page`) when Step 4 of `resize_stripes()` fails.
--

commit 42a858726c79b14d53c892c406bd10d16e4932f8
Author: Cao Guanghui [off-list ref]

md/raid5: fix leak and use-after-free in resize_stripes error path

This commit attempts to fix a memory leak and a use-after-free issue
in the resize_stripes error path. It defers the conf->disks replacement
and performs it under mddev->lock to prevent concurrent reads in
raid5_status from seeing a freed array.

Cc: stable@vger.kernel.org

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but the patch's attempt to fix the conf->disks
use-after-free is incomplete and leaves critical race windows open. This
kfree() is synchronous and doesn't wait for an RCU grace period.

Because check_reshape() invokes resize_stripes() before reshape_position is
updated, chunk_aligned_read is still enabled. Does this allow
raid5_read_one_chunk() to run concurrently and read the freed conf->disks
array completely locklessly?

drivers/md/raid5.c:raid5_read_one_chunk() {
    ...
    rdev = conf->disks[dd_idx].replacement;
    if (!rdev || test_bit(Faulty, &rdev->flags) ||
        READ_ONCE(rdev->recovery_offset) < end_sector) {
        rdev = conf->disks[dd_idx].rdev;
    ...
}

Additionally, raid5_calc_degraded() iterates over conf->disks holding only
rcu_read_lock(). Will this synchronous kfree() cause a use-after-free for
those RCU readers as well?

drivers/md/raid5.c:raid5_calc_degraded() {
    ...
    struct md_rdev *rdev = READ_ONCE(conf->disks[i].rdev);
    ...
}

 		conf->pool_size = newsize;
+	} else if (ndisks) {

[Severity: High]
This isn't a bug introduced by this patch, but the patch attempts to fix
leaks in this error path and structurally misses this one. When Step 4 of
resize_stripes() fails to allocate a page for the new stripe slots
(nsh->dev[i].page), err is set to -ENOMEM and the pool_size update is
skipped here.

During array teardown, shrink_buffers() iterates only up to
sh->raid_conf->pool_size:

drivers/md/raid5.c:shrink_buffers() {
    ...
    int num = sh->raid_conf->pool_size;

#if PAGE_SIZE == DEFAULT_STRIPE_SIZE
    for (i = 0; i < num ; i++) {
    ...
}

Are the successfully allocated pages at indices >= pool_size permanently
leaked when free_stripe() destroys the slab object, because the cleanup loop
misses them?

+		for (i = conf->pool_size; i < newsize; i++)
+			if (ndisks[i].extra_page)
+				put_page(ndisks[i].extra_page);
+		kfree(ndisks);
+	}
 	mutex_unlock(&conf->cache_size_mutex);
 
 	return err;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827060554.77849-1-ghuicao@163.com?part=2