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: <20230818040751.GF3464136@mit.edu>
Date:   Fri, 18 Aug 2023 00:07:51 -0400
From:   "Theodore Ts'o" <tytso@....edu>
To:     Kemeng Shi <shikemeng@...weicloud.com>
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 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.

This also makes the commit description "... to support non-first
group" incorrect, since it never gets called as with a "non-first
group".

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.

						- Ted