Re: [PATCH v2 2/3] ext4: avoid journaling sb update on error if journal is destroying

[Baokun Li <libaokun1@huawei.com>]

On 2025/3/8 17:58, Ojaswin Mujoo wrote:
> On Sat, Mar 08, 2025 at 01:48:44PM +0530, Ojaswin Mujoo wrote:
>> On Sat, Mar 08, 2025 at 10:57:16AM +0800, Zhang Yi wrote:
>>> On 2025/3/8 1:26, Ojaswin Mujoo wrote:
>>>> On Fri, Mar 07, 2025 at 08:36:08PM +0800, Zhang Yi wrote:
>>>>> On 2025/3/7 18:27, Ojaswin Mujoo wrote:
>>>>>> On Fri, Mar 07, 2025 at 04:43:24PM +0800, Zhang Yi wrote:
>>>>>>> On 2025/3/7 16:13, Ojaswin Mujoo wrote:
>>>>>>>> On Fri, Mar 07, 2025 at 12:04:26PM +0530, Ojaswin Mujoo wrote:
>>>>>>>>> On Fri, Mar 07, 2025 at 10:49:28AM +0800, Zhang Yi wrote:
>>>>>>>>>> On 2025/3/6 22:28, 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@gmail.com>
>>>>>>>>>>> Suggested-by: Jan Kara <jack@suse.cz>
>>>>>>>>>>> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.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;
>>>>>>>>>>> +
>>>>>>>>>> Hi, Ojaswin!
>>>>>>>>>>
>>>>>>>>>> I'm afraid you still need to flush the superblock update work here,
>>>>>>>>>> otherwise I guess the race condition you mentioned in v1 could still
>>>>>>>>>> occur.
>>>>>>>>>>
>>>>>>>>>>   ext4_put_super()
>>>>>>>>>>    flush_work(&sbi->s_sb_upd_work)
>>>>>>>>>>
>>>>>>>>>>                      **kjournald2**
>>>>>>>>>>                      jbd2_journal_commit_transaction()
>>>>>>>>>>                      ...
>>>>>>>>>>                      ext4_inode_error()
>>>>>>>>>>                        /* JBD2_UNMOUNT not set */
>>>>>>>>>>                        schedule_work(s_sb_upd_work)
>>>>>>>>>>
>>>>>>>>>>                                    **workqueue**
>>>>>>>>>>                                     update_super_work
>>>>>>>>>>                                     /* s_journal_destorying is not set */
>>>>>>>>>>                              	   if (journal && !s_journal_destorying)
>>>>>>>>>>
>>>>>>>>>>    ext4_journal_destroy()
>>>>>>>>>>     /* set s_journal_destorying */
>>>>>>>>>>     sbi->s_journal_destorying = true;
>>>>>>>>>>     jbd2_journal_destroy()
>>>>>>>>>>      journal->j_flags |= JBD2_UNMOUNT;
>>>>>>>>>>
>>>>>>>>>>                                         jbd2_journal_start()
>>>>>>>>>>                                          start_this_handle()
>>>>>>>>>>                                            BUG_ON(JBD2_UNMOUNT)
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Yi.
>>>>>>>>> Hi Yi,
>>>>>>>>>
>>>>>>>>> Yes you are right, somehow missed this edge case :(
>>>>>>>>>
>>>>>>>>> Alright then, we have to move out sbi->s_journal_destroying outside the
>>>>>>>>> helper. Just wondering if I should still let it be in
>>>>>>>>> ext4_journal_destroy and just add an extra s_journal_destroying = false
>>>>>>>>> before schedule_work(s_sb_upd_work), because it makes sense.
>>>>>>>>>
>>>>>>>>> Okay let me give it some thought but thanks for pointing this out!
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> ojaswin
>>>>>>>> Okay so thinking about it a bit more, I see you also suggested to flush
>>>>>>>> the work after marking sbi->s_journal_destroying. But will that solve
>>>>>>>> it?
>>>>>>>>
>>>>>>>>    ext4_put_super()
>>>>>>>>     flush_work(&sbi->s_sb_upd_work)
>>>>>>>>   
>>>>>>>>                       **kjournald2**
>>>>>>>>                       jbd2_journal_commit_transaction()
>>>>>>>>                       ...
>>>>>>>>                       ext4_inode_error()
>>>>>>>>                         /* JBD2_UNMOUNT not set */
>>>>>>>>                         schedule_work(s_sb_upd_work)
>>>>>>>>   
>>>>>>>>                                      **workqueue**
>>>>>>>>                                      update_super_work
>>>>>>>>                                      /* s_journal_destorying is not set */
>>>>>>>>                               	      if (journal && !s_journal_destorying)
>>>>>>>>   
>>>>>>>>     ext4_journal_destroy()
>>>>>>>>      /* set s_journal_destorying */
>>>>>>>>      sbi->s_journal_destorying = true;
>>>>>>>>      flush_work(&sbi->s_sb_upd_work)
>>>>>>>>                                        schedule_work()
>>>>>>>                                          ^^^^^^^^^^^^^^^
>>>>>>>                                          where does this come from?
>>>>>>>
>>>>>>> After this flush_work, we can guarantee that the running s_sb_upd_work
>>>>>>> finishes before we set JBD2_UNMOUNT. Additionally, the journal will
>>>>>>> not commit transaction or call schedule_work() again because it has
>>>>>>> been aborted due to the previous error. Am I missing something?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Yi.
>>>>>> Hmm, so I am thinking of a corner case in ext4_handle_error() where
>>>>>>
>>>>>>   if(journal && !is_journal_destroying)
>>>>>>
>>>>>> is computed but schedule_work() not called yet, which is possible cause
>>>>>> the cmp followed by jump is not atomic in nature. If the schedule_work
>>>>>> is only called after we have done the flush then we end up with this:
>>>>>>
>>>>>>                                	      if (journal && !s_journal_destorying)
>>>>>>      ext4_journal_destroy()
>>>>>>       /* set s_journal_destorying */
>>>>>>       sbi->s_journal_destorying = true;
>>>>>>       flush_work(&sbi->s_sb_upd_work)
>>>>>>                                         schedule_work()
>>>>>>
>>>>>> Which is possible IMO, although the window is tiny.
>>>>> Yeah, right!
>>>>> Sorry for misread the location where you add the "!s_journal_destorying"
>>>>> check, the graph I provided was in update_super_work(), which was wrong.
>>>> Oh right, I also misread your trace but yes as discussed, even
>>>>
>>>>      sbi->s_journal_destorying = true;
>>>> 		flush_work()
>>>>      jbd2_journal_destroy()
>>>>
>>>> doesn't work.
>>>>
>>>>> The right one should be:
>>>>>
>>>>>   ext4_put_super()
>>>>>    flush_work(&sbi->s_sb_upd_work)
>>>>>
>>>>>                      **kjournald2**
>>>>>                      jbd2_journal_commit_transaction()
>>>>>                      ...
>>>>>                      ext4_inode_error()
>>>>>                        /* s_journal_destorying is not set */
>>>>>                        if (journal && !s_journal_destorying)
>>>>>                          (schedule_work(s_sb_upd_work))  //can be here
>>>>>
>>>>>    ext4_journal_destroy()
>>>>>     /* set s_journal_destorying */
>>>>>     sbi->s_journal_destorying = true;
>>>>>     jbd2_journal_destroy()
>>>>>      journal->j_flags |= JBD2_UNMOUNT;
>>>>>
>>>>>                          (schedule_work(s_sb_upd_work))  //also can be here
>>>>>
>>>>>                                    **workqueue**
>>>>>                                     update_super_work()
>>>>>                                     journal = sbi->s_journal //get journal
>>>>>      kfree(journal)
>>>>>                                       jbd2_journal_start(journal) //journal UAF
>>>>>                                         start_this_handle()
>>>>>                                           BUG_ON(JBD2_UNMOUNT) //bugon here
>>>>>
>>>>>
>>>>> So there are two problems here, the first one is the 'journal' UAF,
>>>>> the second one is triggering JBD2_UNMOUNT flag BUGON.
>>>> Indeed, there's a possible UAF here as well.
>>>>
>>>>>>>> As for the fix, how about we do something like this:
>>>>>>>>
>>>>>>>>    ext4_put_super()
>>>>>>>>
>>>>>>>>     flush_work(&sbi->s_sb_upd_work)
>>>>>>>>     destroy_workqueue(sbi->rsv_conversion_wq);
>>>>>>>>
>>>>>>>>     ext4_journal_destroy()
>>>>>>>>      /* set s_journal_destorying */
>>>>>>>>      sbi->s_journal_destorying = true;
>>>>>>>>
>>>>>>>>     /* trigger a commit and wait for it to complete */
>>>>>>>>
>>>>>>>>      flush_work(&sbi->s_sb_upd_work)
>>>>>>>>
>>>>>>>>      jbd2_journal_destroy()
>>>>>>>>       journal->j_flags |= JBD2_UNMOUNT;
>>>>>>>>   
>>>>>>>>                                          jbd2_journal_start()
>>>>>>>>                                           start_this_handle()
>>>>>>>>                                             BUG_ON(JBD2_UNMOUNT)
>>>>>>>>
>>>>>>>> Still giving this codepath some thought but seems like this might just
>>>>>>>> be enough to fix the race. Thoughts on this?
>>>>>>>>
>>>>> I think this solution should work, the forced commit and flush_work()
>>>>> should ensure that the last transaction is committed and that the
>>>>> potential work is done.
>>>>>
>>>>> Besides, the s_journal_destorying flag is set and check concurrently
>>>>> now, so we need WRITE_ONCE() and READ_ONCE() for it. Besides, what
>>>>> about adding a new flag into sbi->s_mount_state instead of adding
>>>>> new s_journal_destorying?
>>>> Right, that makes sence. I will incorporate these changes in the next
>>>> revision.
>>>>
>>> Think about this again, it seems that we no longer need the destroying
>>> flag. Because we force to commit and wait for the **last** transaction to
>>> complete, and the flush work should also ensure that the last sb_update
>>> work to complete. Regardless of whether it starts a new handle in the
>>> last update_super_work(), it will not commit since the journal should
>>> have aborted. What are your thoughts?
>>>
>>>   ext4_put_super()
>>>    flush_work(&sbi->s_sb_upd_work)
>>>    destroy_workqueue(sbi->rsv_conversion_wq)
>>>
>>>    ext4_journal_destroy()
>>>     /* trigger a commit (it will commit the last trnasaction) */
>>>
>>>                      **kjournald2**
>>>                      jbd2_journal_commit_transaction()
>>>                      ...
>>>                       ext4_inode_error()
>>>                        schedule_work(s_sb_upd_work))
>>>
>>>                                       **workqueue**
>>>                                        update_super_work()
>>>                                          jbd2_journal_start(journal)
>>>                                            start_this_handle()
>>>                                            //This new trans will
>>>                                            //not be committed.
>>>
>>>                       jbd2_journal_abort()
>>>
>>>     /* wait for it to complete */
>>>
>>>     flush_work(&sbi->s_sb_upd_work)
>>>     jbd2_journal_destroy()
>>>      journal->j_flags |= JBD2_UNMOUNT;
>>>     jbd2_journal_commit_transaction() //it will commit nothing
>>>
>>> Thanks,
>>> Yi.
>> Hi Yi,
>>
>> There's one more path for which we need the flag:
>>
>>     ext4_journal_destroy()
>>      /* trigger a commit (it will commit the last trnasaction) */
>>   
>>                       **kjournald2**
>>                       jbd2_journal_commit_transaction()
>> 										 	journal->j_commit_callback()
>> 											  ext4_journal_commit_callback()
>> 												  ext4_maybe_update_superblock()
>> 													  schedule_work()
>> 			/* start a transaction here */
>> 			flush_work()
>> 			  jbd2_journal_destroy()
>> 				  journal_kill_thread
>> 					  flags |= JBD2_UNMOUNT
>> 				  jbd2_journal_commit_transaction()
>> 					  ...
>>              ext4_inode_error()
>> 							schedule_work(s_sb_upd_work))
>> 							/* update_super_work_tries to start the txn */
>> 							BUG_ON()
> Oops the formatting is wrong, here's the trace:
>
>   ext4_journal_destroy()
>      /* trigger a commit (it will commit the last trnasaction) */
>
>                     **kjournald2**
>                     jbd2_journal_commit_transaction()
>                      journal->j_commit_callback()
>                        ext4_journal_commit_callback()
>                          ext4_maybe_update_superblock()
>                            schedule_work()
At this point, SB_ACTIVE should have been cleared,
so ext4_maybe_update_superblock() should do nothing.

With this in mind, it could be the case that an
additional flag is no longer needed.


Regards,
Baokun
>
>      /* update_super_work starts a new txn here */
>      flush_work()
>      jbd2_journal_destroy()
>        journal_kill_thread
>          flags |= JBD2_UNMOUNT
>        jbd2_journal_commit_transaction()
>          ...
>          ext4_inode_error()
>            schedule_work(s_sb_upd_work))
>            /* update_super_work_tries to start the txn */
>            BUG_ON()
>
>> I think this to protect against this path we do need a flag.
>>
>> Regards,
>> ojaswin