linux-ext4 - 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240724101504.e2t4pvgw6td7rrmm@quack3>
Date: Wed, 24 Jul 2024 12:15:04 +0200
From: Jan Kara <jack@...e.cz>
To: "Luis Henriques (SUSE)" <luis.henriques@...ux.dev>
Cc: Theodore Ts'o <tytso@....edu>, Andreas Dilger <adilger@...ger.ca>,
	Jan Kara <jack@...e.cz>,
	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 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 :)

> ---
>  fs/ext4/fast_commit.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 3926a05eceee..3e0793cfea38 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -339,22 +339,29 @@ void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handl
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
>  	tid_t tid;
> +	bool has_transaction = true;
> +	bool is_ineligible;
>  
>  	if (ext4_fc_disabled(sb))
>  		return;
>  
> -	ext4_set_mount_flag(sb, EXT4_MF_FC_INELIGIBLE);
>  	if (handle && !IS_ERR(handle))
>  		tid = handle->h_transaction->t_tid;
>  	else {
>  		read_lock(&sbi->s_journal->j_state_lock);
> -		tid = sbi->s_journal->j_running_transaction ?
> -				sbi->s_journal->j_running_transaction->t_tid : 0;
> +		if (sbi->s_journal->j_running_transaction)
> +			tid = sbi->s_journal->j_running_transaction->t_tid;
> +		else
> +			has_transaction = false;
>  		read_unlock(&sbi->s_journal->j_state_lock);
>  	}
>  	spin_lock(&sbi->s_fc_lock);
> -	if (tid_gt(tid, sbi->s_fc_ineligible_tid))
> +	is_ineligible = ext4_test_mount_flag(sb, EXT4_MF_FC_INELIGIBLE);
> +	if (has_transaction &&
> +	    ((!is_ineligible) ||
	     ^^ these extra braces look strange

> +	     (is_ineligible && tid_gt(tid, sbi->s_fc_ineligible_tid))))
>  		sbi->s_fc_ineligible_tid = tid;
> +	ext4_set_mount_flag(sb, EXT4_MF_FC_INELIGIBLE);

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