Re: [PATCH 1/2] md/raid5: fix pool_size leak in resize_stripes error path

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

New issues:
- [High] Premature update of `pool_size` causes subsequent reshape retries to bypass stripe cache resizing, leading to a NULL pointer dereference.

Pre-existing issues:
- [High] Lockless access to `conf->disks` in `raid5_status()` can lead to a Use-After-Free if it races with `resize_stripes()` freeing the array.
--

commit 021dd13adb2ffa7a0e256f63ee764cd6ee6f46ab
Author: Cao Guanghui <[email protected]>

md/raid5: fix pool_size leak in resize_stripes error path

This commit updates conf->pool_size immediately after replacing conf->disks
in resize_stripes(). It intends to ensure pool_size matches the allocated
array size so free_conf() frees the correct number of extra page allocations
on teardown.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index b91545ce090de..fa64cf059b8ea 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -2642,6 +2642,7 @@ static int resize_stripes(struct r5conf *conf, int newsize)
>  		} else {
>  			kfree(conf->disks);

[Severity: High]
This is a pre-existing issue, but does freeing the old disks array here
without synchronization (e.g., an RCU grace period) allow a use-after-free?

Concurrent readers like raid5_status() access the freed disks array locklessly
since md_seq_show() only holds mddev->lock (a spinlock), while this code
executes under mddev->reconfig_mutex and conf->cache_size_mutex.

For example, in raid5_status():

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

If an unprivileged user reads /proc/mdstat exactly when root initiates a
reshape, it appears the kernel will dereference the memory freed here.

>  			conf->disks = ndisks;
> +			conf->pool_size = newsize;

[Severity: High]
Does moving the pool_size update here cause a NULL pointer dereference on
reshape retries?

If the alloc_page() calls in Step 4 of resize_stripes() fail due to memory
pressure, the function sets err = -ENOMEM and returns, but conf->pool_size
has already been updated to newsize. The stripes are then released into the
cache with missing (NULL) pages.

If a user retries the array reshape operation after the -ENOMEM failure,
check_reshape() will evaluate this condition:

drivers/md/raid5.c:check_reshape() {
    ...
    if (conf->previous_raid_disks + mddev->delta_disks <= conf->pool_size)
        return 0; /* never bother to shrink */
    ...
}

This evaluates to true because pool_size was already updated. The retry
bypasses resize_stripes() and proceeds with the reshape. Later, when
operations like ops_run_io() access the unallocated pages for the new
devices, it results in a NULL pointer dereference.

>  		}
>  	} else
>  		err = -ENOMEM;

[ ... ]

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