Re: [PATCH v4 1/2] md/raid5: track disks array size to fix extra_page leak on error paths

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

Pre-existing issues:
- [High] Retrying `resize_stripes()` after a partial failure leaks `extra_page` allocations from the previous attempt.
- [High] `resize_stripes()` leaks `stripe_head` pages if Step 4 fails (on architectures where PAGE_SIZE == DEFAULT_STRIPE_SIZE).
- [High] Use-After-Free race condition on `conf->disks` array between `raid5_status()` and `resize_stripes()`
--

commit c7f0ab9943c7136150bf0d1225ef069780dedbe5
Author: Cao Guanghui <[email protected]>

md/raid5: track disks array size to fix extra_page leak on error paths

This commit introduces disks_cnt to struct r5conf to track the actual number
of entries in conf->disks and uses it in free_conf() to fix memory leaks
when pool_size doesn't match the actual allocated array size.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index b91545ce090de..e5838bf516e38 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 can freeing the disks array directly here 
cause a use-after-free race with raid5_status()?

raid5_status() runs with only mddev->lock held and iterates over
conf->disks[i].rdev:

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

Would calling kfree() here (without kfree_rcu or synchronize_rcu) allow the
array to be freed while raid5_status() is actively reading from it?

[Severity: High]
This is also a pre-existing issue, but will this kfree() leak the 
extra_page allocations on retry if a previous reshape failed in Step 4?

If resize_stripes() succeeds in Step 3 (updating conf->disks) but fails
in Step 4, conf->pool_size is not updated. On a retry, the earlier copy
loop only copies elements up to the outdated conf->pool_size:

drivers/md/raid5.c:resize_stripes() {
    ...
    for (i = 0; i < conf->pool_size; i++)
        ndisks[i] = conf->disks[i];
    ...
}

Will this cause the extra_page allocations at indices >= pool_size from
the previous attempt to be permanently abandoned and leaked when this
kfree(conf->disks) is called?

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

[Severity: High]
This isn't a bug introduced by this patch, but could resize_stripes()
leak stripe_head pages if a failure happens later in Step 4?

In Step 4, pages are allocated for the new stripe size:

drivers/md/raid5.c:resize_stripes() {
    ...
    for (i = conf->raid_disks; i < newsize; i++)
        if (nsh->dev[i].page == NULL) {
            struct page *p = alloc_page(GFP_NOIO);
            nsh->dev[i].page = p;
    ...
}

If an allocation fails, err = -ENOMEM is set, but the loop continues and
previously allocated pages remain attached. Because err is set,
conf->pool_size is never updated.

Later, shrink_buffers() bounds itself strictly to 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++) {
    ...
}

Will this unconditionally leak all orphaned pages residing at higher
indices (>= pool_size) when the stripe_head structures are dropped?

>  		}
>  	} else
>  		err = -ENOMEM;

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