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

[Yu Kuai]

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.
> .