Re: [PATCH v2] ocfs2: fix null-ptr-deref when journal load failed.

[Joseph Qi <joseph.qi@linux.alibaba.com>]

On 8/23/24 4:31 PM, Julian Sun wrote:
> During the mounting process, if journal_reset() fails
> because of too short journal, then lead to
> jbd2_journal_load() fails with NULL j_sb_buffer.
> Subsequently, ocfs2_journal_shutdown() calls
> jbd2_journal_flush()->jbd2_cleanup_journal_tail()->
> __jbd2_update_log_tail()->jbd2_journal_update_sb_log_tail()
> ->lock_buffer(journal->j_sb_buffer), resulting in a
> null-pointer dereference error.
> 
> To resolve this issue, a new state OCFS2_JOURNAL_INITED
> has been introduced to replace the previous functionality
> of OCFS2_JOURNAL_LOADED, the original OCFS2_JOURNAL_LOADED
> is only set when ocfs2_journal_load() is successful.
> The jbd2_journal_flush() function is allowed to be called
> only when this flag is set. The logic here is that if the
> journal has even not been successfully loaded, there is
> no need to flush the journal.
> 
> Link: https://syzkaller.appspot.com/bug?extid=05b9b39d8bdfe1a0861f
> Reported-by: syzbot+05b9b39d8bdfe1a0861f@syzkaller.appspotmail.com
> Signed-off-by: Julian Sun <sunjunchao2870@gmail.com>
> ---
>  fs/ocfs2/journal.c | 9 ++++++---
>  fs/ocfs2/journal.h | 1 +
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
> index 530fba34f6d3..da0ffcc5de0a 100644
> --- a/fs/ocfs2/journal.c
> +++ b/fs/ocfs2/journal.c
> @@ -968,7 +968,7 @@ int ocfs2_journal_init(struct ocfs2_super *osb, int *dirty)
>  
>  	ocfs2_set_journal_params(osb);
>  
> -	journal->j_state = OCFS2_JOURNAL_LOADED;
> +	journal->j_state = OCFS2_JOURNAL_INITED;
>  
>  	status = 0;
>  done:
> @@ -1039,6 +1039,7 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
>  	int status = 0;
>  	struct inode *inode = NULL;
>  	int num_running_trans = 0;
> +	enum ocfs2_journal_state state;
>  
>  	BUG_ON(!osb);
>  
> @@ -1047,8 +1048,9 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
>  		goto done;
>  
>  	inode = journal->j_inode;
> +	state = journal->j_state;
>  
> -	if (journal->j_state != OCFS2_JOURNAL_LOADED)
> +	if (state != OCFS2_JOURNAL_INITED && state != OCFS2_JOURNAL_LOADED)
>  		goto done;
>  
>  	/* need to inc inode use count - jbd2_journal_destroy will iput. */
> @@ -1076,7 +1078,7 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
>  
>  	BUG_ON(atomic_read(&(osb->journal->j_num_trans)) != 0);
>  
> -	if (ocfs2_mount_local(osb)) {
> +	if (ocfs2_mount_local(osb) && state == OCFS2_JOURNAL_LOADED) {

The only intent of the new introduced state is to identify if journal is
truly loaded or not.
So it seems that the simplest way to fix this is just check JBD2_LOADED
here.

if (ocfs2_mount_local(osb) &&
    (journal->j_journal->j_flags & JBD2_LOADED)) {
	...
}

BTW, could you please also replace 'osb->journal->j_num_trans' to
'journal->j_num_trans'?

>  		jbd2_journal_lock_updates(journal->j_journal);
>  		status = jbd2_journal_flush(journal->j_journal, 0);
>  		jbd2_journal_unlock_updates(journal->j_journal);
> @@ -1174,6 +1176,7 @@ int ocfs2_journal_load(struct ocfs2_journal *journal, int local, int replayed)
>  		}
>  	} else
>  		osb->commit_task = NULL;
> +	journal->j_state = OCFS2_JOURNAL_LOADED;

It seems that this has to be moved just after jbd2_journal_load().
Anyway, I don't think we have to introduce a new state. See above.

Joseph

>  
>  done:
>  	return status;
> diff --git a/fs/ocfs2/journal.h b/fs/ocfs2/journal.h
> index e3c3a35dc5e0..a80f76a8fa0e 100644
> --- a/fs/ocfs2/journal.h
> +++ b/fs/ocfs2/journal.h
> @@ -15,6 +15,7 @@
>  
>  enum ocfs2_journal_state {
>  	OCFS2_JOURNAL_FREE = 0,
> +	OCFS2_JOURNAL_INITED,
>  	OCFS2_JOURNAL_LOADED,
>  	OCFS2_JOURNAL_IN_SHUTDOWN,
>  };