linux-kernel - Re: [PATCH v3 2/3] ext4: avoid journaling sb update on error if journal is destroying

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: <2ptbzrdbw2ttnd2ik5y4qq5py7ho7kusescdhtfzwg4ft7ldgs@qnkbcdoxvpfs>
Date: Fri, 14 Mar 2025 11:10:40 +0100
From: Jan Kara <jack@...e.cz>
To: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
Cc: linux-ext4@...r.kernel.org, Theodore Ts'o <tytso@....edu>, 
	Jan Kara <jack@...e.cz>, Baokun Li <libaokun1@...wei.com>, 
	Ritesh Harjani <ritesh.list@...il.com>, linux-kernel@...r.kernel.org, 
	Mahesh Kumar <maheshkumar657g@...il.com>
Subject: Re: [PATCH v3 2/3] ext4: avoid journaling sb update on error if
 journal is destroying

On Fri 14-03-25 13:14:10, Ojaswin Mujoo wrote:
> Presently we always BUG_ON if trying to start a transaction on a journal marked
> with JBD2_UNMOUNT, since this should never happen. However, while ltp running
> stress tests, it was observed that in case of some error handling paths, it is
> possible for update_super_work to start a transaction after the journal is
> destroyed eg:
> 
> (umount)
> ext4_kill_sb
>   kill_block_super
>     generic_shutdown_super
>       sync_filesystem /* commits all txns */
>       evict_inodes
>         /* might start a new txn */
>       ext4_put_super
> 	flush_work(&sbi->s_sb_upd_work) /* flush the workqueue */
>         jbd2_journal_destroy
>           journal_kill_thread
>             journal->j_flags |= JBD2_UNMOUNT;
>           jbd2_journal_commit_transaction
>             jbd2_journal_get_descriptor_buffer
>               jbd2_journal_bmap
>                 ext4_journal_bmap
>                   ext4_map_blocks
>                     ...
>                     ext4_inode_error
>                       ext4_handle_error
>                         schedule_work(&sbi->s_sb_upd_work)
> 
>                                                /* work queue kicks in */
>                                                update_super_work
>                                                  jbd2_journal_start
>                                                    start_this_handle
>                                                      BUG_ON(journal->j_flags &
>                                                             JBD2_UNMOUNT)
> 
> Hence, introduce a new mount flag to indicate journal is destroying and only do
> a journaled (and deferred) update of sb if this flag is not set. Otherwise, just
> fallback to an un-journaled commit.
> 
> Further, in the journal destroy path, we have the following sequence:
> 
>   1. Set mount flag indicating journal is destroying
>   2. force a commit and wait for it
>   3. flush pending sb updates
> 
> This sequence is important as it ensures that, after this point, there is no sb
> update that might be journaled so it is safe to update the sb outside the
> journal. (To avoid race discussed in 2d01ddc86606)
> 
> Also, we don't need a similar check in ext4_grp_locked_error since it is only
> called from mballoc and AFAICT it would be always valid to schedule work here.
> 
> Fixes: 2d01ddc86606 ("ext4: save error info to sb through journal if available")
> Reported-by: Mahesh Kumar <maheshkumar657g@...il.com>
> Signed-off-by: Ojaswin Mujoo <ojaswin@...ux.ibm.com>

Mostly looks good. Couple of small comments below:

> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1179,6 +1179,7 @@ struct ext4_inode_info {
>  #define	EXT4_ERROR_FS			0x0002	/* Errors detected */
>  #define	EXT4_ORPHAN_FS			0x0004	/* Orphans being recovered */
>  #define EXT4_FC_REPLAY			0x0020	/* Fast commit replay ongoing */
> +#define EXT4_JOURNAL_DESTORY		0x0040	/* Journal is in process of destroying */

This should be defined as part of the following enum:

/*
 * run-time mount flags
 */
enum {
        EXT4_MF_MNTDIR_SAMPLED,
        EXT4_MF_FC_INELIGIBLE   /* Fast commit ineligible */
};

Also you have a typo in the flag name. I guess it should be
EXT4_MF_JOURNAL_DESTROY.

> @@ -4957,8 +4961,6 @@ static int ext4_load_and_init_journal(struct super_block *sb,
>  	return 0;
>  
>  out:
> -	/* flush s_sb_upd_work before destroying the journal. */
> -	flush_work(&sbi->s_sb_upd_work);
>  	ext4_journal_destroy(sbi, sbi->s_journal);
>  	return -EINVAL;
>  }
> @@ -5648,8 +5650,6 @@ failed_mount8: __maybe_unused
>  	sbi->s_ea_block_cache = NULL;
>  
>  	if (sbi->s_journal) {
> -		/* flush s_sb_upd_work before journal destroy. */
> -		flush_work(&sbi->s_sb_upd_work);
>  		ext4_journal_destroy(sbi, sbi->s_journal);
>  	}
>  failed_mount3a:

These are good. I would also move the flush_work() in ext4_put_super()
into else branch of:

	if (sbi->s_journal)

								Honza

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