* Re: [PATCH v2] ocfs2: fix null-ptr-deref when journal load failed. [not found] <20240823083150.17590-1-sunjunchao2870@gmail.com> @ 2024-08-30 9:40 ` Joseph Qi 2024-08-30 10:11 ` Julian Sun 0 siblings, 1 reply; 4+ messages in thread From: Joseph Qi @ 2024-08-30 9:40 UTC (permalink / raw) To: Julian Sun, ocfs2-devel Cc: lbec, mark, syzbot+05b9b39d8bdfe1a0861f, linux-kernel, Heming Zhao 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, > }; ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ocfs2: fix null-ptr-deref when journal load failed. 2024-08-30 9:40 ` [PATCH v2] ocfs2: fix null-ptr-deref when journal load failed Joseph Qi @ 2024-08-30 10:11 ` Julian Sun 2024-08-30 10:58 ` Joseph Qi 0 siblings, 1 reply; 4+ messages in thread From: Julian Sun @ 2024-08-30 10:11 UTC (permalink / raw) To: Joseph Qi, ocfs2-devel Cc: lbec, mark, syzbot+05b9b39d8bdfe1a0861f, linux-kernel, Heming Zhao 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> ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ocfs2: fix null-ptr-deref when journal load failed. 2024-08-30 10:11 ` Julian Sun @ 2024-08-30 10:58 ` Joseph Qi 2024-08-30 11:22 ` Julian Sun 0 siblings, 1 reply; 4+ messages in thread From: Joseph Qi @ 2024-08-30 10:58 UTC (permalink / raw) To: Julian Sun, ocfs2-devel Cc: lbec, mark, syzbot+05b9b39d8bdfe1a0861f, linux-kernel, Heming Zhao On 8/30/24 6:11 PM, Julian Sun wrote: > 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. Umm... You can treat it as ocfs2 journal state only, not jbd2. Also jbd2 doesn't have 'initialized' state either. So I don't think we have to do this change in the fix. Joseph ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ocfs2: fix null-ptr-deref when journal load failed. 2024-08-30 10:58 ` Joseph Qi @ 2024-08-30 11:22 ` Julian Sun 0 siblings, 0 replies; 4+ messages in thread From: Julian Sun @ 2024-08-30 11:22 UTC (permalink / raw) To: Joseph Qi, ocfs2-devel Cc: lbec, mark, syzbot+05b9b39d8bdfe1a0861f, linux-kernel, Heming Zhao On Fri, 2024-08-30 at 18:58 +0800, Joseph Qi wrote: > > > On 8/30/24 6:11 PM, Julian Sun wrote: > > 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. > > Umm... You can treat it as ocfs2 journal state only, not jbd2. > Also jbd2 doesn't have 'initialized' state either. > So I don't think we have to do this change in the fix. > > Joseph Got it. Thanks for your explanation. Thanks, -- Julian Sun <sunjunchao2870@gmail.com> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-08-30 11:22 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20240823083150.17590-1-sunjunchao2870@gmail.com>
2024-08-30 9:40 ` [PATCH v2] ocfs2: fix null-ptr-deref when journal load failed Joseph Qi
2024-08-30 10:11 ` Julian Sun
2024-08-30 10:58 ` Joseph Qi
2024-08-30 11:22 ` Julian Sun
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®