Message-ID: <d7022eb3-4dee-8d15-1018-051b882467b2@huaweicloud.com>
Date: Mon, 16 Jun 2025 19:32:38 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: Wang Jinchao <wangjinchao600@...il.com>, Yu Kuai <yukuai1@...weicloud.com>
Cc: linux-raid@...r.kernel.org, linux-kernel@...r.kernel.org,
 Song Liu <song@...nel.org>, "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH v2] md/raid1: Fix stack memory use after return in
 raid1_reshape

Hi,

在 2025/06/16 18:11, Wang Jinchao 写道:
> On 6/13/25 19:53, Wang Jinchao wrote:
>> On 2025/6/13 17:15, Yu Kuai wrote:
>>> Hi,
>>>
>>> 在 2025/06/12 20:21, Wang Jinchao 写道:
>>>> BTW, I feel raid1_reshape can be better coding with following：
>>>
>>> And another hint:
>>>
>>> The poolinfo can be removed directly, the only other place to use it
>>> is r1buf_pool, and can covert to pass in conf or &conf->raid_disks.
>> Thanks, I'll review these relationships carefully before refactoring.
>>>
>>> Thanks,
>>> Kuai
>>>
>>
> Hi Kuai,
> 
> After reading the related code, I’d like to discuss the possibility of 
> rewriting raid1_reshape.
> 
> I am considering converting r1bio_pool to use 
> mempool_create_kmalloc_pool. However, mempool_create_kmalloc_pool 
> requires the element size as a parameter, but the size is calculated 
> dynamically in r1bio_pool_alloc(). Because of this, I feel that 
> mempool_create() may be more suitable here.

You only need raid_disks to calculate the size, the value will not
change.
> 
> I noticed that mempool_create() was used historically and was later 
> replaced by mempool_init() in commit afeee514ce7f. Using 
> mempool_create() would essentially be a partial revert of that commit, 
> so I’m not sure whether this is appropriate.

This is fine, the commit introduce the porblem.
> 
> Regarding raid1_info and pool_info, I feel the original design might be 
> more suitable for the reshape process.
> 
> The goals of raid1_reshape() are:
> 
> - Keep the array usable for as long as possible.
This is not needed, reshape is a slow path, just don't introduce
problems.

> - Be able to restore the previous state if reshape fails.
Yes.

> So I think we need to follow some constraints:
> 
> - conf should not be modified before freeze_array().
> - We should try to prepare everything possible before freeze_array().
> - If an error occurs after freeze_array(), it must be possible to roll 
> back.
> 
> Now, regarding the idea of rewriting raid1_info or pool_info:
> 
> Convert raid1_info using krealloc:

1) If raid_disks decreases, you don't need to realloc it;
2) If raid_disks increases, call krealloc after freezing the array, you
can't call it before in order to prevent concurrent access.
> 
> According to rule 1, krealloc() must be called after freeze_array(). 
> According to rule 2, it should be called before freeze_array(). → So 
> this approach seems to violate one of the rules.
> 
> Use conf instead of pool_info:

I'm suggesting to remove pool_info, you should change conf->raid_disks
as it is now, while the array is freezed.

Thanks,
Kuai

> 
> According to rule 1, conf->raid_disks must be modified after 
> freeze_array(). According to rule 2, conf->raid_disks needs to be 
> updated before calling mempool_create(), i.e., before freeze_array().
> These also seem to conflict.
> 
> For now, I’m not considering rule 3, as that would make the logic even 
> more complex.
> 
> I’d really appreciate your thoughts on whether this direction makes 
> sense or if there’s a better approach.
> 
> Thank you for your time and guidance.
> .
> 

Powered by blists - more mailing lists