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

[Julian Sun <sunjunchao2870@gmail.com>]

On Fri, 2024-08-30 at 17:40 +0800, Joseph Qi wrote:
> 
> 
> 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)) {
>         ...
> }
Hi, Joseph, thanks for your review and comments. 

Yeah! It's absolutely the simplest and cleanest way to fix this issue.
Thanks for your suggestion.
> 
> BTW, could you please also replace 'osb->journal->j_num_trans' to
> 'journal->j_num_trans'?
Sure.
> 
> >                 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.
> 
Agreed. And now OCFS2_JOURNAL_LOADED is set when ocfs2_journal_init()
succeed, it may led to some misunderstanding: the journal was not
really loaded when OCFS2_JOURNAL_LOADED was set. I would like to rename
it to OCFS2_JOURNAL_INITED in another patch to make it clearer.
> 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,
> >  };

Thanks,
-- 
Julian Sun <sunjunchao2870@gmail.com>