| Message ID | 20240819131120.746077-1-sunjunchao2870@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ocfs2: fix null-ptr-deref when journal load failed. | expand |
CC stable Julian Sun <sunjunchao2870@gmail.com> 于2024年8月19日周一 21:11写道： > > During the mounting process, if the jbd2_journal_load() > call fails, it will internally invoke journal_reset() > ->journal_fail_superblock(), which sets journal->j_sb_buffer > to NULL. 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..6f837296048f 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) > 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, > }; > -- > 2.39.2 >
On 8/19/24 9:11 PM, Julian Sun wrote: > During the mounting process, if the jbd2_journal_load() > call fails, it will internally invoke journal_reset() > ->journal_fail_superblock(), which sets journal->j_sb_buffer This description is not right. journal_reset() fails because of too short journal, then lead to jbd2_journal_load() fails with NULL j_sb_buffer. > to NULL. 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. Or set OCFS2_JOURNAL_LOADED only after JBD2_LOADED? Thanks, Joseph > 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..6f837296048f 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) > 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, > };
Joseph Qi <joseph.qi@linux.alibaba.com> 于2024年8月20日周二 21:03写道： > > > > On 8/19/24 9:11 PM, Julian Sun wrote: > > During the mounting process, if the jbd2_journal_load() > > call fails, it will internally invoke journal_reset() > > ->journal_fail_superblock(), which sets journal->j_sb_buffer > > > > This description is not right. > > journal_reset() fails because of too short journal, then lead to > > jbd2_journal_load() fails with NULL j_sb_buffer. yeah. That's exactly what I described. > > > to NULL. 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. > > > > Or set OCFS2_JOURNAL_LOADED only after JBD2_LOADED? I don't think this is correct. We first call ocfs2_journal_init(), which allocates some resources, before calling jbd2_journal_load(). If ocfs2_journal_init() succeeds but jbd2_journal_load() fails, this solution may lead to a resource leak. If there is anything important I'm missing, please let me know, thanks. > > Thanks, > Joseph > > > 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..6f837296048f 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) > > 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, > > }; Thanks,
On 8/20/24 11:19 PM, Julian Sun wrote: > Joseph Qi <joseph.qi@linux.alibaba.com> 于2024年8月20日周二 21:03写道： >> >> >> >> On 8/19/24 9:11 PM, Julian Sun wrote: >>> During the mounting process, if the jbd2_journal_load() >>> call fails, it will internally invoke journal_reset() >>> ->journal_fail_superblock(), which sets journal->j_sb_buffer >> >> >>> This description is not right. >>> journal_reset() fails because of too short journal, then lead to >>> jbd2_journal_load() fails with NULL j_sb_buffer. > yeah. That's exactly what I described. >> >>> to NULL. 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. >> >> >>> Or set OCFS2_JOURNAL_LOADED only after JBD2_LOADED? > I don't think this is correct. We first call ocfs2_journal_init(), > which allocates some resources, before calling jbd2_journal_load(). If > ocfs2_journal_init() succeeds but jbd2_journal_load() fails, this > solution may lead to a resource leak. > If there is anything important I'm missing, please let me know, thanks. > Okay, seems except iput(inode) and kfree(journal), we may have to do the following cleanup: 1) ocfs2_inode_unlock(journal->j_inode); 2) brelse(journal->j_bh); 3) OCFS2_I(inode)->ip_open_count-- 4) jbd2_journal_destroy() ... So it seems that introducing a new state will be more clear. >>> 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..6f837296048f 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) This is not right. What if journal has already been loaded? Thanks, Joseph >>> 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, >>> }; > > > Thanks,
On Fri, 2024-08-23 at 09:59 +0800, Joseph Qi wrote: > > > On 8/20/24 11:19 PM, Julian Sun wrote: > > Joseph Qi <joseph.qi@linux.alibaba.com> 于2024年8月20日周二 21:03写道： > > > > > > > > > > > > On 8/19/24 9:11 PM, Julian Sun wrote: > > > > During the mounting process, if the jbd2_journal_load() > > > > call fails, it will internally invoke journal_reset() > > > > ->journal_fail_superblock(), which sets journal->j_sb_buffer > > > > > > > > > > This description is not right. > > > > journal_reset() fails because of too short journal, then lead > > > > to > > > > jbd2_journal_load() fails with NULL j_sb_buffer. > > yeah. That's exactly what I described. > > > > > > > to NULL. 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. > > > > > > > > > > Or set OCFS2_JOURNAL_LOADED only after JBD2_LOADED? > > I don't think this is correct. We first call ocfs2_journal_init(), > > which allocates some resources, before calling jbd2_journal_load(). > > If > > ocfs2_journal_init() succeeds but jbd2_journal_load() fails, this > > solution may lead to a resource leak. > > If there is anything important I'm missing, please let me know, > > thanks. > > > > Okay, seems except iput(inode) and kfree(journal), we may have to do > the > following cleanup: > 1) ocfs2_inode_unlock(journal->j_inode); > 2) brelse(journal->j_bh); > 3) OCFS2_I(inode)->ip_open_count-- > 4) jbd2_journal_destroy() > ... > > So it seems that introducing a new state will be more clear. > > > > > 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..6f837296048f 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) > > This is not right. > What if journal has already been loaded? Hi, Joseph Thanks for your review and comments. I'm not sure if I fully understand what you mean. Because the functionality of OCFS2_JOURNAL_INITED is completely equivalent to the original OCFS2_JOURNAL_LOADED, so do you mean that there might be an issue with the original OCFS2_JOURNAL_LOADED? If so, I will dig it into and try to fix. But in any case, that should be a separate patch. If there is any misunderstanding, please let me know, thanks. > > Thanks, > Joseph > > > > > 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, > > > > }; > > > > > > Thanks,
On 8/23/24 10:22 AM, Julian Sun wrote: > On Fri, 2024-08-23 at 09:59 +0800, Joseph Qi wrote: >> >> >> On 8/20/24 11:19 PM, Julian Sun wrote: >>> Joseph Qi <joseph.qi@linux.alibaba.com> 于2024年8月20日周二 21:03写道： >>>> >>>> >>>> >>>> On 8/19/24 9:11 PM, Julian Sun wrote: >>>>> During the mounting process, if the jbd2_journal_load() >>>>> call fails, it will internally invoke journal_reset() >>>>> ->journal_fail_superblock(), which sets journal->j_sb_buffer >>>> >>>> >>>>> This description is not right. >>>>> journal_reset() fails because of too short journal, then lead >>>>> to >>>>> jbd2_journal_load() fails with NULL j_sb_buffer. >>> yeah. That's exactly what I described. >>>> >>>>> to NULL. 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. >>>> >>>> >>>>> Or set OCFS2_JOURNAL_LOADED only after JBD2_LOADED? >>> I don't think this is correct. We first call ocfs2_journal_init(), >>> which allocates some resources, before calling jbd2_journal_load(). >>> If >>> ocfs2_journal_init() succeeds but jbd2_journal_load() fails, this >>> solution may lead to a resource leak. >>> If there is anything important I'm missing, please let me know, >>> thanks. >>> >> >> Okay, seems except iput(inode) and kfree(journal), we may have to do >> the >> following cleanup: >> 1) ocfs2_inode_unlock(journal->j_inode); >> 2) brelse(journal->j_bh); >> 3) OCFS2_I(inode)->ip_open_count-- >> 4) jbd2_journal_destroy() >> ... >> >> So it seems that introducing a new state will be more clear. >> >>>>> 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..6f837296048f 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) >> >> This is not right. >> What if journal has already been loaded? > Hi, Joseph > > Thanks for your review and comments. > > I'm not sure if I fully understand what you mean. > Because the functionality of OCFS2_JOURNAL_INITED is completely > equivalent to the original OCFS2_JOURNAL_LOADED, so do you mean that > there might be an issue with the original OCFS2_JOURNAL_LOADED? If so, > I will dig it into and try to fix. > But in any case, that should be a separate patch. > > If there is any misunderstanding, please let me know, thanks. Now you separate original OCFS2_JOURNAL_LOADED into OCFS2_JOURNAL_INITED and OCFS2_JOURNAL_LOADED. And ocfs2_journal_shutdown() will be called after OCFS2_JOURNAL_LOADED is set, e.g. normal umount, or error happens after ocfs2_check_volume(). You changes break the this logic. BTW, cc linux-kernel as well. Thanks, Joseph
On Fri, 2024-08-23 at 14:13 +0800, Joseph Qi wrote: > > > On 8/23/24 10:22 AM, Julian Sun wrote: > > On Fri, 2024-08-23 at 09:59 +0800, Joseph Qi wrote: > > > > > > > > > On 8/20/24 11:19 PM, Julian Sun wrote: > > > > Joseph Qi <joseph.qi@linux.alibaba.com> 于2024年8月20日周二 21:03写道： > > > > > > > > > > > > > > > > > > > > On 8/19/24 9:11 PM, Julian Sun wrote: > > > > > > During the mounting process, if the jbd2_journal_load() > > > > > > call fails, it will internally invoke journal_reset() > > > > > > ->journal_fail_superblock(), which sets journal- > > > > > > >j_sb_buffer > > > > > > > > > > > > > > > > This description is not right. > > > > > > journal_reset() fails because of too short journal, then > > > > > > lead > > > > > > to > > > > > > jbd2_journal_load() fails with NULL j_sb_buffer. > > > > yeah. That's exactly what I described. > > > > > > > > > > > to NULL. 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. > > > > > > > > > > > > > > > > Or set OCFS2_JOURNAL_LOADED only after JBD2_LOADED? > > > > I don't think this is correct. We first call > > > > ocfs2_journal_init(), > > > > which allocates some resources, before calling > > > > jbd2_journal_load(). > > > > If > > > > ocfs2_journal_init() succeeds but jbd2_journal_load() fails, > > > > this > > > > solution may lead to a resource leak. > > > > If there is anything important I'm missing, please let me know, > > > > thanks. > > > > > > > > > > Okay, seems except iput(inode) and kfree(journal), we may have to > > > do > > > the > > > following cleanup: > > > 1) ocfs2_inode_unlock(journal->j_inode); > > > 2) brelse(journal->j_bh); > > > 3) OCFS2_I(inode)->ip_open_count-- > > > 4) jbd2_journal_destroy() > > > ... > > > > > > So it seems that introducing a new state will be more clear. > > > > > > > > > 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..6f837296048f 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) > > > > > > This is not right. > > > What if journal has already been loaded? > > Hi, Joseph > > > > Thanks for your review and comments. > > > > I'm not sure if I fully understand what you mean. > > Because the functionality of OCFS2_JOURNAL_INITED is completely > > equivalent to the original OCFS2_JOURNAL_LOADED, so do you mean > > that > > there might be an issue with the original OCFS2_JOURNAL_LOADED? If > > so, > > I will dig it into and try to fix. > > But in any case, that should be a separate patch. > > > > If there is any misunderstanding, please let me know, thanks. > > Now you separate original OCFS2_JOURNAL_LOADED into > OCFS2_JOURNAL_INITED > and OCFS2_JOURNAL_LOADED. And ocfs2_journal_shutdown() will be called > after OCFS2_JOURNAL_LOADED is set, e.g. normal umount, or error > happens > after ocfs2_check_volume(). You changes break the this logic. > > BTW, cc linux-kernel as well. I see. Thanks very much for the clarification. I will send patch v2 to fix it. > > Thanks, > Joseph Thanks,
diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c index 530fba34f6d3..6f837296048f 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) 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 the jbd2_journal_load() call fails, it will internally invoke journal_reset() ->journal_fail_superblock(), which sets journal->j_sb_buffer to NULL. 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(-)