linux-kernel - Re: [RFC PATCH] jbd2: make '0' an invalid transaction sequence

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: <20240716095201.o7kkrhfdy2bps3rw@quack3>
Date: Tue, 16 Jul 2024 11:52:01 +0200
From: Jan Kara <jack@...e.cz>
To: Luis Henriques <luis.henriques@...ux.dev>
Cc: Andreas Dilger <adilger@...ger.ca>,
	Wang Jianjian <wangjianjian0@...mail.com>,
	"wangjianjian (C)" <wangjianjian3@...wei.com>,
	Theodore Ts'o <tytso@....edu>, Jan Kara <jack@...e.cz>,
	Harshad Shirwadkar <harshadshirwadkar@...il.com>,
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] jbd2: make '0' an invalid transaction sequence

On Fri 12-07-24 10:53:02, Luis Henriques wrote:
> Since there's code (in fast-commit) that already handles a '0' tid as a
> special case, it's better to ensure that jbd2 never sets it to that value
> when journal->j_transaction_sequence increment wraps.
> 
> Suggested-by: Andreas Dilger <adilger@...ger.ca>
> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@...ux.dev>

Well, sadly it isn't so simple. If nothing else, journal replay
(do_one_pass()) will get broken by the skipped tid as we do check:

                if (sequence != next_commit_ID) {
                        brelse(bh);
                        break;
                }

So we'd abort journal replay too early. Secondly, there's also code
handling journal replay in libext2fs which would need to be checked and
fixed up. Finally, I've found code in mballoc which alternates between two
lists based on tid & 1, so this logic would get broken by skipping 0 tid
as well.

Overall, I think we might be better off to go and fix places that assume
tid 0 is not valid. I can see those assumptions in:

ext4_fc_mark_ineligible()
ext4_wait_for_tail_page_commit()
__jbd2_log_wait_for_space()
jbd2_journal_shrink_checkpoint_list()

Now I don't see it as urgent to fix all these right now. Just for this
series let's not add another place making tid 0 special. Later we can fixup
the other places...

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR