Re: [PATCH v2] md/raid1: Fix stack memory use after return in raid1_reshape

[Yu Kuai <yukuai1@huaweicloud.com>]

Hi,

在 2025/06/11 16:55, Wang Jinchao 写道:
> In the raid1_reshape function, newpool is
> allocated on the stack and assigned to conf->r1bio_pool.
> This results in conf->r1bio_pool.wait.head pointing
> to a stack address.
> Accessing this address later can lead to a kernel panic.
> 
> Example access path:
> 
> raid1_reshape()
> {
> 	// newpool is on the stack
> 	mempool_t newpool, oldpool;
> 	// initialize newpool.wait.head to stack address
> 	mempool_init(&newpool, ...);
> 	conf->r1bio_pool = newpool;
> }
> 
> raid1_read_request() or raid1_write_request()
> {
> 	alloc_r1bio()
> 	{
> 		mempool_alloc()
> 		{
> 			// if pool->alloc fails
> 			remove_element()
> 			{
> 				--pool->curr_nr;
> 			}
> 		}
> 	}
> }
> 
> mempool_free()
> {
> 	if (pool->curr_nr < pool->min_nr) {
> 		// pool->wait.head is a stack address
> 		// wake_up() will try to access this invalid address
> 		// which leads to a kernel panic
> 		return;
> 		wake_up(&pool->wait);
> 	}
> }
> 
> Fix:
> The solution is to avoid using a stack-based newpool.
> Instead, directly initialize conf->r1bio_pool.
> 
> Signed-off-by: Wang Jinchao <wangjinchao600@gmail.com>
> ---
> v1 -> v2:
> - change subject
> - use mempool_init(&conf->r1bio_pool) instead of reinitializing the list on stack
> ---
>   drivers/md/raid1.c | 34 +++++++++++++++++++---------------
>   1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 19c5a0ce5a40..f2436262092a 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -3366,7 +3366,7 @@ static int raid1_reshape(struct mddev *mddev)
>   	 * At the same time, we "pack" the devices so that all the missing
>   	 * devices have the higher raid_disk numbers.
>   	 */
> -	mempool_t newpool, oldpool;
> +	mempool_t oldpool;
>   	struct pool_info *newpoolinfo;
>   	struct raid1_info *newmirrors;
>   	struct r1conf *conf = mddev->private;
> @@ -3375,9 +3375,6 @@ static int raid1_reshape(struct mddev *mddev)
>   	int d, d2;
>   	int ret;
>   
> -	memset(&newpool, 0, sizeof(newpool));
> -	memset(&oldpool, 0, sizeof(oldpool));
> -
>   	/* Cannot change chunk_size, layout, or level */
>   	if (mddev->chunk_sectors != mddev->new_chunk_sectors ||
>   	    mddev->layout != mddev->new_layout ||
> @@ -3408,26 +3405,33 @@ static int raid1_reshape(struct mddev *mddev)
>   	newpoolinfo->mddev = mddev;
>   	newpoolinfo->raid_disks = raid_disks * 2;
>   
> -	ret = mempool_init(&newpool, NR_RAID_BIOS, r1bio_pool_alloc,
> -			   rbio_pool_free, newpoolinfo);
> -	if (ret) {
> -		kfree(newpoolinfo);
> -		return ret;
> -	}
>   	newmirrors = kzalloc(array3_size(sizeof(struct raid1_info),
> -					 raid_disks, 2),
> -			     GFP_KERNEL);
> +	raid_disks, 2),
> +	GFP_KERNEL);
>   	if (!newmirrors) {
>   		kfree(newpoolinfo);
> -		mempool_exit(&newpool);
>   		return -ENOMEM;
>   	}
>   
> +	/* stop everything before switching the pool */
>   	freeze_array(conf, 0);
>   
> -	/* ok, everything is stopped */
> +	/* backup old pool in case restore is needed */
>   	oldpool = conf->r1bio_pool;
> -	conf->r1bio_pool = newpool;
> +
> +	ret = mempool_init(&conf->r1bio_pool, NR_RAID_BIOS, r1bio_pool_alloc,
> +			   rbio_pool_free, newpoolinfo);
> +	if (ret) {
> +		kfree(newpoolinfo);
> +		kfree(newmirrors);
> +		mempool_exit(&conf->r1bio_pool);
> +		/* restore the old pool */
> +		conf->r1bio_pool = oldpool;
> +		unfreeze_array(conf);
> +		pr_err("md/raid1:%s: cannot allocate r1bio_pool for reshape\n",
> +			mdname(mddev));
> +		return ret;
> +	}
>   
>   	for (d = d2 = 0; d < conf->raid_disks; d++) {
>   		struct md_rdev *rdev = conf->mirrors[d].rdev;
> 

Any specific reason not to use mempool_resize() and krealloc() here?
In the case if new raid_disks is greater than the old one.

Thanks,
Kuai