linux-kernel - Re: [PATCH 4/4] ext4: fix incorrect tid assumption in ext4_fc_mark

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]

Message-ID: <20240724151350.wkdxrqg6bf6jzh3t@quack3>
Date: Wed, 24 Jul 2024 17:13:50 +0200
From: Jan Kara <jack@...e.cz>
To: Luis Henriques <luis.henriques@...ux.dev>
Cc: Jan Kara <jack@...e.cz>, Theodore Ts'o <tytso@....edu>,
	Andreas Dilger <adilger@...ger.ca>,
	Harshad Shirwadkar <harshadshirwadkar@...il.com>,
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] ext4: fix incorrect tid assumption in
 ext4_fc_mark_ineligible()

On Wed 24-07-24 15:02:49, Luis Henriques wrote:
> On Wed, Jul 24 2024, Jan Kara wrote:
> 
> > On Tue 23-07-24 16:44:02, Luis Henriques (SUSE) wrote:
> >> Function jbd2_journal_shrink_checkpoint_list() assumes that '0' is not a
> >> valid value for transaction IDs, which is incorrect.
> >> 
> >> Furthermore, the sbi->s_fc_ineligible_tid handling also makes the same
> >> assumption by being initialised to '0'.  Fortunately, the sb flag
> >> EXT4_MF_FC_INELIGIBLE can be used to check whether sbi->s_fc_ineligible_tid
> >> has been previously set instead of comparing it with '0'.
> >> 
> >> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@...ux.dev>
> >
> > Just one style nit below, otherwise looks good. Feel free to add:
> >
> > Reviewed-by: Jan Kara <jack@...e.cz>
> >
> > BTW, the ineligibility handling looks flaky to me, in particular the cases
> > where we call ext4_fc_mark_ineligible() with NULL handle seem racy to me as
> > fastcommit can happen *before* we mark the filesystem as ineligible.  But
> > that's not really related to your changes, they just made me look at that
> > code in detail and I couldn't resist complaining :)
> 
> Heh, fair enough.  Regarding this race, I may try to look into it but I'll
> need to dig a bit more.  And yeah it's probably better to separate that
> from this patch.

I suspect all the places that mark the fs as ineligible with NULL handle
need to actually mark corresponding transactions as ineligible using handle
instead. This is going to require a bit of churn e.g. for stuff like
resize or __track_dentry_update() but shouldn't be hard to do.

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