Re: [PATCH v6 2/3] md/raid10: resize r10bio_pool for reshape

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly allocated `new_pool` is leaked on the `reshape_backwards` error path.
--

commit 2142096170ffdd8a1caabfde6f2461f6e7d64f61
Author: Chen Cheng <[email protected]>

md/raid10: resize r10bio_pool for reshape
    
When reshape grows raid_disks, the pool must also switch to new geometry
object size, and allocate a new geometry size pool and replace the old.

But not for shrinking reshape, because regular I/O can still use the
prev geo for sectors that have not crossed reshape_progress yet.

> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index cee5a253a281d..d740744a97461 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
[ ... ]
> @@ -4411,6 +4422,11 @@ static int raid10_start_reshape(struct mddev *mddev)
>  		return -EINVAL;
>  
>  	conf->offset_diff = min_offset_diff;
> +	if (mddev->delta_disks > 0) {
> +		new_pool = create_r10bio_pool(new.raid_disks);
> +		if (!new_pool)
> +			return -ENOMEM;
> +	}
>  	spin_lock_irq(&conf->device_lock);
>  	if (conf->mirrors_new) {
>  		memcpy(conf->mirrors_new, conf->mirrors,

[Severity: Medium]
Will this newly allocated new_pool be leaked if the reshape_backwards
error path is triggered?

If an array is configured for a reshape that expands the number of disks
(delta_disks > 0), the new_pool is allocated here. However, if the
component device sizes are also reduced such that the array is set to
reshape backwards, it can hit a pre-existing error check a few lines
down:

	if (mddev->reshape_backwards) {
		sector_t size = raid10_size(mddev, 0, 0);
		if (size < mddev->array_sectors) {
			spin_unlock_irq(&conf->device_lock);
			pr_warn("md/raid10:%s: array size must be reduce before number of disks\n",
				mdname(mddev));
			return -EINVAL;
		}

This direct return skips the standard cleanup path at the abort label,
causing the mempool_t structure and its preallocated elements to leak.
Should we add a mempool_destroy(new_pool) here, or change the return to
a goto abort?

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