linux-ext4 - Re: [PATCH] jbd2: skip reading super block if it has been verified

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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 15 Jun 2023 01:26:54 -0400
From:   "Theodore Ts'o" <tytso@....edu>
To:     Zhang Yi <yi.zhang@...weicloud.com>
Cc:     linux-ext4@...r.kernel.org, adilger.kernel@...ger.ca, jack@...e.cz,
        yi.zhang@...wei.com, chengzhihao1@...wei.com, yukuai3@...wei.com
Subject: Re: [PATCH] jbd2: skip reading super block if it has been verified

On Thu, Jun 15, 2023 at 11:49:41AM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@...wei.com>
> 
> We got a NULL pointer dereference issue below while running generic/475
> I/O failure pressure test.

Have you been able to reproduce this failure without the "recheck
checkpoint" series applied?  I have not, so like with the e2fsck bug
fix, I can understand how the bug fix worked, but I still don't
understand why I wasn't seeing until I tried to apply the "recheck
chekcpoint" and the following patches in that patch series.

> If the journal super block had been read and verified, there is no need
> to call bh_read() read it again even if it has been failed to written
> out. So the fix could be simply move buffer_verified(bh) in front of
> bh_read().
> 
> Fixes: d9eafe0afafa ("jbd2: factor out journal initialization from journal_get_superblock()")

That works, but it's worth noting that commit d9eafe0afafa caused the
failure by removing the check on j_journal_version to determine
whether the superblock was read or not.  If the journal superblock had
been previously read, j_journal_version would be either 1 or 2.  If it
had been zero, then superblock was not read.  So from commit
d9eafe0afafa:

 	/* Load journal superblock if it is not loaded yet. */
-	if (journal->j_format_version == 0 &&
-	    journal_get_superblock(journal) != 0)
+	if (journal_get_superblock(journal))
 		return 0;
 	if (!jbd2_format_support_feature(journal))
 		return 0;

The comment "Load journal superblock if it is not loaded yet." should
be removed, since it no longer makes sense once the
"journal->j_format_version == 0" check was removed.

I'll also note that a problem with d9eafe0afafa is that by removing
the j_format_version check, every time we add a revoke header, and we
call jbd2_journal_set_features(), this was causing an unconditional
read of the journal superblock and that unnecessary I/O could slow
down certain workloads.

						- Ted