| Message ID | 20240823083150.17590-1-sunjunchao2870@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ocfs2: fix null-ptr-deref when journal load failed. | expand |
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, > };
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,
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
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,
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) { 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; 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, };
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(-)