linux-kernel - Re: [PATCH 11/13] ext4: correct gdblock calculation in add_new_gdb_meta

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <797c2dff-1858-9e4b-bda7-d6106d5ff844@huaweicloud.com>
Date:   Fri, 18 Aug 2023 15:09:35 +0800
From:   Kemeng Shi <shikemeng@...weicloud.com>
To:     Theodore Ts'o <tytso@....edu>
Cc:     adilger.kernel@...ger.ca, linux-ext4@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 11/13] ext4: correct gdblock calculation in
 add_new_gdb_meta_bg to support non first group



on 8/18/2023 12:07 PM, Theodore Ts'o wrote:
> On Fri, Aug 18, 2023 at 10:29:52AM +0800, Kemeng Shi wrote:
>> Actually, there seems a functional change to add_new_gdb_meta_bg.
>> Assume 'group' is the new added group, 'first_group' is the first group
>> of meta_bg which contains 'group',
>> Original way to calculate gdbblock:
>> gdbblock = group_first_block('first_group') + bg_has_super(*'group'*)
>> New ay to calculate gdbblock
>> gdbblock = group_first_block('first_group') + bg_has_super(*'first_group'*)
>> If new added group is not the first group of meta_bg, add_new_gdb_meta_bg
>> get a wrong gdbblock.
> 
> If you look at the ext4_add_new_descs() function,
> add_new_gdb_meta_bg() is only called when the group is a multiple of
> EXT4_DESC_PER_BLOCK --- that is, when group % EXT4_DESC_PER_BLOCK == 0.
>
> As such, it is only called when with group is the first group in the
> meta_bg.  So there is no bug here.  The code is bit confusing, I agree
> --- I myself got confused because it's been years since I last looked
> at the code, and it's not particularly commented well, which is my fault.
> 
Yes, add_new_gdb_meta_bg is only called with first group of mebg and no real
bug here.
> This also makes the commit description "... to support non-first
> group" incorrect, since it never gets called as with a "non-first
> group".
> 
Ah, what I want to say is "support non-frist group in fulture". And if there
is definely no need in fulture, it's more intuitive just treat group from caller
as first group in meta_bg.
> The patch makes things a little simpler, but the commit description
> would confuse anyone who looked at it while doing code archeology.
> The change is fine, although at this point, given how we both
> misunderstood how the code worked without doing some deep mind-melds
> with the C code in question, it's clear that we need some better
> comments in the code.
> 
> For example, the comment "add_new_gdb_meta_bg is the sister of
> add_new_gdb" is clearly insufficient.
> Is following comment looks good to you:
When all reserved primary blocks are consumed, we create meta_bg group and
allocate new primary block at first block or block after backup superblock
(if exsiting) in first group of meta_bg group.
This function is only called when first group of meta_bg is added.
> 						- Ted
>