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] [day] [month] [year] [list]

Date:   Tue, 20 Jun 2023 09:12:46 +0800
From:   Zhang Yi <yi.zhang@...weicloud.com>
To:     Theodore Ts'o <tytso@....edu>
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 2023/6/18 2:50, Theodore Ts'o wrote:
> 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.  :-)
> 

Thanks for the suggestion and historical details, we could do the cleanup
in jbd2 layer after the merge window.

Thanks,
Yi.