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: <20230817140328.GY2247938@mit.edu>
Date:   Thu, 17 Aug 2023 10:03:28 -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 Thu, Aug 17, 2023 at 11:38:34AM +0800, Kemeng Shi wrote:
> 
> 
> on 8/16/2023 11:45 AM, Theodore Ts'o wrote:
> > On Thu, Jun 29, 2023 at 08:00:42PM +0800, Kemeng Shi wrote:
> >> In add_new_gdb_meta_bg, we assume that group could be non first
> >> group in meta block group as we call ext4_meta_bg_first_block_no
> >> to get first block of meta block group rather than call
> >> ext4_group_first_block_no for passed group directly. Then ext4_bg_has_super
> >> should be called with first group in meta group rather than new added
> >> group. Or we can call ext4_group_first_block_no instead of
> >> ext4_meta_bg_first_block_no to assume only first group of
> >> meta group will be passed.
> >> Either way, ext4_meta_bg_first_block_no will be useless and
> >> could be removed.
> > 
> > Unfortunately, I spent more time trying to understand the commit
> > description than the C code.  Perhaps this might be a better way of
> > describing the situation?
> > 
> Sorry for my poor language again, :( I will try to improve this.
> > The ext4_new descs() function calls ext4_meta_bg_first_block_no() with
> > the group paramter when the group is the first group of a meta_bg
> > (e.g., when (group % EXT4_DESC_PER_BLOCK) is zero.  So we can simplify
> > things a bit by removing ext4_meta_bg_first_block_no() and an open
> > coding its logic.
> > 
> > Does this make more sense to tou?
> > 
> This patch tries to correct gdbblock calculation in add_new_gdb_meta_bg
> in case group from caller is not the first group of meta_bg which is
> supposed to be handled by add_new_gdb_meta_bg.
> We should call ext4_bg_has_super with first group in meta_bg instead
> of group which could be non first group in meta_bg to calculate gdb
> of meta_bg.
> Fortunately, the only caller ext4_add_new_descs always call
> add_new_gdb_meta_bg with first group of meta_bg and no real issue
> will happen.

To be clear, this doesn't have a functional change given how the code
is going to be used, right?  It's really more of a cleanup with a goal
of making the code easier to understand.  If so, we should make this
explicit at the beginning of the commit description, as opposed to
putting it at the end.

In journalism this is referred to as "burying the lede"[1], where the
"lede" the most important/key piece of information.  In general, it is
desirable not to "bury the lede".  That is, the most important
information, including why people should care, and what this is doing,
at the beginning of the commit description (or article in the case of
journalsm).

[1] https://www.masterclass.com/articles/bury-the-lede-explained

					- Ted