| Message ID | 20240830110853.101060-1-sunjunchao2870@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] ocfs2: fix null-ptr-deref when journal load failed. | expand |
On 8/30/24 7:08 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, we should check the JBD2_LOADED flag to > ensure the journal was properly loaded. Additionly, refine > code to make it cleaner. s/Additionly/Additionally Additionally, use journal instead of osb->journal directly to simplify the code. > > Link: https://syzkaller.appspot.com/bug?extid=05b9b39d8bdfe1a0861f > Reported-by: syzbot+05b9b39d8bdfe1a0861f@syzkaller.appspotmail.com > Suggested-by: Joseph Qi <joseph.qi@linux.alibaba.com> > Signed-off-by: Julian Sun <sunjunchao2870@gmail.com> > --- > fs/ocfs2/journal.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c > index 530fba34f6d3..ff2a6538b46e 100644 > --- a/fs/ocfs2/journal.c > +++ b/fs/ocfs2/journal.c > @@ -1074,9 +1074,10 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb) > osb->commit_task = NULL; > } > > - BUG_ON(atomic_read(&(osb->journal->j_num_trans)) != 0); > + BUG_ON(atomic_read(&(journal->j_num_trans)) != 0); > Please do the same change for the above one: num_running_trans = atomic_read(&(journal->j_num_trans)); > - if (ocfs2_mount_local(osb)) { > + if (ocfs2_mount_local(osb) && > + (journal->j_journal->j_flags & JBD2_LOADED)) { Align it to the right place, please. Thanks, Joseph > jbd2_journal_lock_updates(journal->j_journal); > status = jbd2_journal_flush(journal->j_journal, 0); > jbd2_journal_unlock_updates(journal->j_journal);
On Mon, 2024-09-02 at 09:32 +0800, Joseph Qi wrote: > > > On 8/30/24 7:08 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, we should check the JBD2_LOADED flag to > > ensure the journal was properly loaded. Additionly, refine > > code to make it cleaner. > > s/Additionly/Additionally > > Additionally, use journal instead of osb->journal directly to > simplify > the code. Yeah, sorry for the typo.. > > > > > Link: https://syzkaller.appspot.com/bug?extid=05b9b39d8bdfe1a0861f > > Reported-by: syzbot+05b9b39d8bdfe1a0861f@syzkaller.appspotmail.com > > Suggested-by: Joseph Qi <joseph.qi@linux.alibaba.com> > > Signed-off-by: Julian Sun <sunjunchao2870@gmail.com> > > --- > > fs/ocfs2/journal.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c > > index 530fba34f6d3..ff2a6538b46e 100644 > > --- a/fs/ocfs2/journal.c > > +++ b/fs/ocfs2/journal.c > > @@ -1074,9 +1074,10 @@ void ocfs2_journal_shutdown(struct > > ocfs2_super *osb) > > osb->commit_task = NULL; > > } > > > > - BUG_ON(atomic_read(&(osb->journal->j_num_trans)) != 0); > > + BUG_ON(atomic_read(&(journal->j_num_trans)) != 0); > > > Please do the same change for the above one: > > num_running_trans = atomic_read(&(journal->j_num_trans)); Ok. > > > - if (ocfs2_mount_local(osb)) { > > + if (ocfs2_mount_local(osb) && > > + (journal->j_journal->j_flags & JBD2_LOADED)) { > Align it to the right place, please. Sure. > > Thanks, > Joseph > > jbd2_journal_lock_updates(journal->j_journal); > > status = jbd2_journal_flush(journal->j_journal, 0); > > jbd2_journal_unlock_updates(journal->j_journal); > Thanks,
diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c index 530fba34f6d3..ff2a6538b46e 100644 --- a/fs/ocfs2/journal.c +++ b/fs/ocfs2/journal.c @@ -1074,9 +1074,10 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb) osb->commit_task = NULL; } - BUG_ON(atomic_read(&(osb->journal->j_num_trans)) != 0); + BUG_ON(atomic_read(&(journal->j_num_trans)) != 0); - if (ocfs2_mount_local(osb)) { + if (ocfs2_mount_local(osb) && + (journal->j_journal->j_flags & JBD2_LOADED)) { jbd2_journal_lock_updates(journal->j_journal); status = jbd2_journal_flush(journal->j_journal, 0); jbd2_journal_unlock_updates(journal->j_journal);
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, we should check the JBD2_LOADED flag to ensure the journal was properly loaded. Additionly, refine code to make it cleaner. Link: https://syzkaller.appspot.com/bug?extid=05b9b39d8bdfe1a0861f Reported-by: syzbot+05b9b39d8bdfe1a0861f@syzkaller.appspotmail.com Suggested-by: Joseph Qi <joseph.qi@linux.alibaba.com> Signed-off-by: Julian Sun <sunjunchao2870@gmail.com> --- fs/ocfs2/journal.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)