linux-ext4 - Re: [PATCH] ext4: Fix incorrect group count in ext4_fill

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]

Date:   Sat, 28 Mar 2020 16:55:47 -0700
From:   Josh Triplett <josh@...htriplett.org>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
        Theodore Ts'o <tytso@....edu>,
        Andreas Dilger <adilger.kernel@...ger.ca>
Subject: Re: [PATCH] ext4: Fix incorrect group count in ext4_fill_super error
 message

On Sat, Mar 28, 2020 at 05:07:55PM -0600, Andreas Dilger wrote:
> On Mar 28, 2020, at 3:54 PM, Josh Triplett <josh@...htriplett.org> wrote:
> > 
> > ext4_fill_super doublechecks the number of groups before mounting; if
> > that check fails, the resulting error message prints the group count
> > from the ext4_sb_info sbi, which hasn't been set yet. Print the freshly
> > computed group count instead (which at that point has just been computed
> > in "blocks_count").
> > 
> > Signed-off-by: Josh Triplett <josh@...htriplett.org>
> > Fixes: 4ec1102813798 ("ext4: Add sanity checks for the superblock before mounting the filesystem")
> 
> Modulo the compiler warning pointed out by kbuild test robot, I think the
> patch is correct, but was definitely confusing to read within the shown
> context, since "blocks_count" definitely doesn't seem to be "groups count"
> (it *is* the "groups count", but is just used as a temporary variable).

I agree that the code and patch read confusingly due to the (lack of)
context. The commit message attempted to explain that, but clearer code
seems preferable to confusing-but-explained code.

I sent a new version of this patch, which instead just moves the
assignment to sbi's group count earlier, so that the error message can
continue referencing it that way. (That also addresses the warning.)

- Josh Triplett