linux-kernel - Re: [PATCH v2 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: <3y7za2sfohzeifijsnlxdp7zb7vfkltjgeqcca57tw6uqzlxtt@hngy3b6up3ch>
Date: Fri, 7 Mar 2025 15:26:54 +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>, linux-kernel@...r.kernel.org, 
	Mahesh Kumar <maheshkumar657g@...il.com>
Subject: Re: [PATCH v2 2/3] ext4: avoid journaling sb update on error if
 journal is destroying

On Thu 06-03-25 19:58:33, 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 sbi flag s_journal_destroying to indicate journal is
> destroying only do a journaled (and deferred) update of sb if this flag is not
> set. Otherwise, just fallback to an un-journaled commit.
> 
> We set sbi->s_journal_destroying = true only after all the FS updates are done
> during ext4_put_super() (except a running transaction that will get commited
> during jbd2_journal_destroy()). After this point, it is safe to commit the sb
> outside the journal as it won't race with a journaled update (refer
> 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>
> Suggested-by: Jan Kara <jack@...e.cz>
> Signed-off-by: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
> ---
>  fs/ext4/ext4.h      | 2 ++
>  fs/ext4/ext4_jbd2.h | 8 ++++++++
>  fs/ext4/super.c     | 4 +++-
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 2b7d781bfcad..d48e93bd5690 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1728,6 +1728,8 @@ struct ext4_sb_info {
>  	 */
>  	struct work_struct s_sb_upd_work;
>  
> +	bool s_journal_destorying;
> +

Not that it would matter much but why not make this a flag in
sbi->s_mount_flags?

> diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
> index 9b3c9df02a39..6bd3ca84410d 100644
> --- a/fs/ext4/ext4_jbd2.h
> +++ b/fs/ext4/ext4_jbd2.h
> @@ -437,6 +437,14 @@ static inline int ext4_journal_destroy(struct ext4_sb_info *sbi, journal_t *jour
>  {
>  	int err = 0;
>  
> +	/*
> +	 * At this point all pending FS updates should be done except a possible
> +	 * running transaction (which will commit in jbd2_journal_destroy). It
> +	 * is now safe for any new errors to directly commit superblock rather
> +	 * than going via journal.
> +	 */
> +	sbi->s_journal_destorying = true;
> +

So as you already uncovered with Zhang Yi, this does not work. What I meant
was that we move flush_work(&sbi->s_sb_upd_work) into
ext4_journal_destroy() and set s_journal_destorying *before* calling
flush_work(). By the time ext4_journal_destroy() gets called, the
filesystem is quiescent, there cannot be new handles started (except for sb
update itself from the workqueue) and thus if we hit some error, the
journal will be aborted anyway and in that case non-journaled sb update is
safe.

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