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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5ygal3ht47dcpftsxxksmk4lid47al2g4xzlbennmtteeqqsed@uswr3gimu3wc>
Date: Wed, 12 Mar 2025 18:15:44 +0100
From: Jan Kara <jack@...e.cz>
To: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
Cc: Jan Kara <jack@...e.cz>, Ritesh Harjani <ritesh.list@...il.com>, 
	linux-ext4@...r.kernel.org, Theodore Ts'o <tytso@....edu>, 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 Wed 12-03-25 19:56:36, Ojaswin Mujoo wrote:
> On Wed, Mar 12, 2025 at 11:51:03AM +0100, Jan Kara wrote:
> > On Mon 10-03-25 10:13:36, Ritesh Harjani wrote:
> > > Ojaswin Mujoo <ojaswin@...ux.ibm.com> writes:
> > > > On Sun, Mar 09, 2025 at 12:11:22AM +0530, Ritesh Harjani wrote:
> > > >> Ojaswin Mujoo <ojaswin@...ux.ibm.com> writes:
> > > >> > On Sat, Mar 08, 2025 at 06:56:23PM +0530, Ritesh Harjani wrote:
> > > >> >> Ojaswin Mujoo <ojaswin@...ux.ibm.com> writes:
> > > >> >> > On Sat, Mar 08, 2025 at 03:25:04PM +0530, Ritesh Harjani (IBM) wrote:
> > > >> >> >> Ojaswin Mujoo <ojaswin@...ux.ibm.com> writes:
> > > >> >> >> > 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;
> > > >> >> >> > +
> > > >> >> >> >  	/* Atomic write unit values in bytes */
> > > >> >> >> >  	unsigned int s_awu_min;
> > > >> >> >> >  	unsigned int s_awu_max;
> > > >> >> >> > 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;
> > > >> >> >> 
> > > >> >> >> This is not correct right. I think what we decided to set this flag
> > > >> >> >> before we flush the workqueue. So that we don't schedule any new
> > > >> >> >> work after this flag has been set. At least that is what I understood.
> > > >> >> >> 
> > > >> >> >> [1]: https://lore.kernel.org/all/87eczc6rlt.fsf@gmail.com/
> > > >> >> >> 
> > > >> >> >> -ritesh
> > > >> >> >
> > > >> >> > Hey Ritesh,
> > > >> >> >
> > > >> >> > Yes that is not correct, I missed that in my patch however we realised
> > > >> >> > that adding it before flush_work() also has issues [1]. More
> > > >> >> > specifically:
> > > >> >> 
> > > >> >> Ohk. right. 
> > > >> >> 
> > > >> >> >
> > > >> >> >                      **kjournald2**
> > > >> >> >                      jbd2_journal_commit_transaction()
> > > >> >> >                      ...
> > > >> >> >                      ext4_handle_error()
> > > >> >> >                         /* s_journal_destorying is not set */
> > > >> >> >                         if (journal && !s_journal_destorying)
> > > >> >> 
> > > >> >> Then maybe we should not schedule another work to update the superblock
> > > >> >> via journalling, it the error itself occurred while were trying to
> > > >> >> commit the journal txn? 
> > > >> >> 
> > > >> >> 
> > > >> >> -ritesh
> > > >> >
> > > >> > Hmm, ideally yes that should not happen, but how can we achieve that?
> > > >> > For example with the trace we saw:
> > > >> >
> > > >> >    **kjournald2**
> > > >> >    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)
> > > >> >
> > > >> > How do we tell ext4_handle_error that it is in the context of a
> > > >> > committing txn.
> > 
> > So I was thinking about this. It is not a problem to determine we are
> > running in kjournald context - it is enough to check
> > 
> > 	current == EXT4_SB(sb)->s_journal->j_task
> 
> Oh, right :) 
> 
> > 
> > But I'm not sure checking this in ext4_handle_error() and doing direct sb
> > update instead of scheduling a journalled one is always correct. For
> > example kjournald does also writeback of ordered data and if that hits an
> > error, we do not necessarily abort the journal (well, currently we do as
> > far as I'm checking but it seems a bit fragile to rely on this).
> 
> Okay so IIUC your concern is there might be some codepaths, now or in
> the future, where kjournald might call the FS layer, hit an error and
> still decide to not abort. In which case we would still want to update
> the sb via journal.

Yeah. The reason why I'm a bit concerned about it is mostly the case of
kjournald also handling ordered data and situations like
!(journal->j_flags & JBD2_ABORT_ON_SYNCDATA_ERR) where people want to
continue although ordered data had issues. Or situations where something in
j_commit_callback or another jbd2 hook ends up calling ext4_error()...

> > static void ext4_journal_destroy(struct super_block *sb)
> > {
> > 	/*
> > 	 * At this point only two things can be operating on the journal.
> > 	 * JBD2 thread performing transaction commit and s_sb_upd_work
> > 	 * issuing sb update through the journal. Once we set
> > 	 * EXT4_FLAGS_JOURNAL_DESTROY, new ext4_handle_error() calls will not
> > 	 * queue s_sb_upd_work and ext4_force_commit() makes sure any
> > 	 * ext4_handle_error() calls from the running transaction commit are
> > 	 * finished. Hence no new s_sb_upd_work can be queued after we
> > 	 * flush it here.
> > 	 */
> > 	set_bit(EXT4_FLAGS_JOURNAL_DESTROY, &EXT4_SB(sb)->s_ext4_flags);
> 
> Offtopic, how are s_ext4_flags different from s_mount_flags, since in a
> draft patchset for this, I am using:
> 
> 	ext4_set_mount_flag(sbi->s_sb, EXT4_JBD2_DESTORYING);
> 
> so just curious.

I don't think there's a difference and I think we can unify them. For now
pick whatever you like :)

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ