linux-ext4 - Re: [PATCH v2] 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20230617185057.GA343628@mit.edu>
Date:   Sat, 17 Jun 2023 14:50:57 -0400
From:   "Theodore Ts'o" <tytso@....edu>
To:     Zhang Yi <yi.zhang@...weicloud.com>
Cc:     Jan Kara <jack@...e.cz>, linux-ext4@...r.kernel.org,
        adilger.kernel@...ger.ca, yi.zhang@...wei.com,
        chengzhihao1@...wei.com, yukuai3@...wei.com
Subject: Re: [PATCH v2] jbd2: skip reading super block if it has been verified

On Sat, Jun 17, 2023 at 10:42:59AM +0800, Zhang Yi wrote:
> > This works as a workaround. It is a bit kludgy but for now I guess it is
> > good enough. Thanks for the fix and feel free to add:
> 
> Thanks for the review. Yes, I suppose it's better to find a way to adjust
> the sequence of journal load and feature checking in ocfs2_check_volume(),
> so that we could completely remove the journal_get_superblock() in
> jbd2_journal_check_used_features().

Indeed, thanks for the fix.

This is would be for after the merge window, but I think we can clean
this up in the jbd2 layer by simply moving the call to
load_superblock() from jbd2_journal_load() and jbd2_journal_wipe() to
journal_init_common().  This change would mean the journal superblock
gets read as part of the call to jbd2_journal_init_{dev,inode}.

That way, once the file system has a journal_t object, it's guaranteed
that the j_sb_buffer contains valid data, and so we can drop the call
to journal_get_superblock() from jbd2_journal_check_used_features().

And after we do that, we should be able to inline the code in
load_superblock() and journal_get_superblock() into
journal_init_common(), which would simplify things in
jfs/jbd2/journal.c

Finally, so we can provide better error handling, we could change
Jbd2_journal_init_{dev,inode} to return an ERR_PTR instead of a NULL
if there is a failure.  And since it's a good idea to change the
function name when changing the function signature, we could rename
those functions to something like jbd2_open_{dev,inode} at the same
time.

						- Ted

P.S.  The only reason why we don't load the superblock in
jbd2_journal_init_{dev,common} was that back in 2001, it was possible
to create the journal by creating a zero length file in the file
system, noting the inode number of the file system, unmounting the
file system from ext2, and then remounting it with "mount -t ext3 -o
journal=NNN ...".  In order to do this, the ext3 file system code
called journal_init_inode() with the inode, and then follow it up with
a call to journal_create(), which would actually write out the journal
superblock.  For that reason, journal_init_inode() had to avoid
reading the journal superblock, since it might not be initialized yet.

We removed jbd2_journal_create() from fs/jbd2 back in 2009, and it
hadn't been in use for quite a while before that --- in fact, I'm not
sure ext4 ever supported this ext3-style "let's create a journal
without e2fsprogs support because Stephen Tweedie was implementing the
ext3 journal kernel code without wanting to make changes to e2fsprogs
first" feature.  :-)