wangyan
2020-Jan-08 12:14 UTC
[Ocfs2-devel] [PATCH] ocfs2: fix a NULL pointer dereference when call ocfs2_update_inode_fsync_trans()
On 2020/1/8 19:31, Joseph Qi wrote:> > > On 20/1/8 17:23, wangyan wrote: >> I found a NULL pointer dereference in ocfs2_update_inode_fsync_trans(), >> handle->h_transaction may be NULL in this situation: >> ocfs2_file_write_iter >> ->__generic_file_write_iter >> ->generic_perform_write >> ->ocfs2_write_begin >> ->ocfs2_write_begin_nolock >> ->ocfs2_write_cluster_by_desc >> ->ocfs2_write_cluster >> ->ocfs2_mark_extent_written >> ->ocfs2_change_extent_flag >> ->ocfs2_split_extent >> ->ocfs2_try_to_merge_extent >> ->ocfs2_extend_rotate_transaction >> ->ocfs2_extend_trans >> ->jbd2_journal_restart >> ->jbd2__journal_restart >> // handle->h_transaction is NULL here >> ->handle->h_transaction = NULL; >> ->start_this_handle >> /* journal aborted due to storage >> network disconnection, return error */ >> ->return -EROFS; >> /* line 3806 in ocfs2_try_to_merge_extent (), >> it will ignore ret error. */ >> ->ret = 0; >> ->... >> ->ocfs2_write_end >> ->ocfs2_write_end_nolock >> ->ocfs2_update_inode_fsync_trans >> // NULL pointer dereference >> ->oi->i_sync_tid = handle->h_transaction->t_tid; >> >> The information of NULL pointer dereference as follows: >> JBD2: Detected IO errors while flushing file data on dm-11-45 >> Aborting journal on device dm-11-45. >> JBD2: Error -5 detected when updating journal superblock for dm-11-45. >> (dd,22081,3):ocfs2_extend_trans:474 ERROR: status = -30 >> (dd,22081,3):ocfs2_try_to_merge_extent:3877 ERROR: status = -30 >> Unable to handle kernel NULL pointer dereference at >> virtual address 0000000000000008 >> Mem abort info: >> ESR = 0x96000004 >> Exception class = DABT (current EL), IL = 32 bits >> SET = 0, FnV = 0 >> EA = 0, S1PTW = 0 >> Data abort info: >> ISV = 0, ISS = 0x00000004 >> CM = 0, WnR = 0 >> user pgtable: 4k pages, 48-bit VAs, pgdp = 00000000e74e1338 >> [0000000000000008] pgd=0000000000000000 >> Internal error: Oops: 96000004 [#1] SMP >> Process dd (pid: 22081, stack limit = 0x00000000584f35a9) >> CPU: 3 PID: 22081 Comm: dd Kdump: loaded >> Hardware name: Huawei TaiShan 2280 V2/BC82AMDD, BIOS 0.98 08/25/2019 >> pstate: 60400009 (nZCv daif +PAN -UAO) >> pc : ocfs2_write_end_nolock+0x2b8/0x550 [ocfs2] >> lr : ocfs2_write_end_nolock+0x2a0/0x550 [ocfs2] >> sp : ffff0000459fba70 >> x29: ffff0000459fba70 x28: 0000000000000000 >> x27: ffff807ccf7f1000 x26: 0000000000000001 >> x25: ffff807bdff57970 x24: ffff807caf1d4000 >> x23: ffff807cc79e9000 x22: 0000000000001000 >> x21: 000000006c6cd000 x20: ffff0000091d9000 >> x19: ffff807ccb239db0 x18: ffffffffffffffff >> x17: 000000000000000e x16: 0000000000000007 >> x15: ffff807c5e15bd78 x14: 0000000000000000 >> x13: 0000000000000000 x12: 0000000000000000 >> x11: 0000000000000000 x10: 0000000000000001 >> x9 : 0000000000000228 x8 : 000000000000000c >> x7 : 0000000000000fff x6 : ffff807a308ed6b0 >> x5 : ffff7e01f10967c0 x4 : 0000000000000018 >> x3 : d0bc661572445600 x2 : 0000000000000000 >> x1 : 000000001b2e0200 x0 : 0000000000000000 >> Call trace: >> ocfs2_write_end_nolock+0x2b8/0x550 [ocfs2] >> ocfs2_write_end+0x4c/0x80 [ocfs2] >> generic_perform_write+0x108/0x1a8 >> __generic_file_write_iter+0x158/0x1c8 >> ocfs2_file_write_iter+0x668/0x950 [ocfs2] >> __vfs_write+0x11c/0x190 >> vfs_write+0xac/0x1c0 >> ksys_write+0x6c/0xd8 >> __arm64_sys_write+0x24/0x30 >> el0_svc_common+0x78/0x130 >> el0_svc_handler+0x38/0x78 >> el0_svc+0x8/0xc >> >> To prevent NULL pointer dereference in this situation, we use >> is_handle_aborted() before using handle->h_transaction->t_tid. >> >> Signed-off-by: Yan Wang <wangyan122 at huawei.com> >> Reviewed-by: Jun Piao <piaojun at huawei.com> >> --- >> fs/ocfs2/journal.h | 8 +++++--- >> fs/ocfs2/namei.c | 3 +-- >> 2 files changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/fs/ocfs2/journal.h b/fs/ocfs2/journal.h >> index 3103ba7f97a2..bfe611ed1b1d 100644 >> --- a/fs/ocfs2/journal.h >> +++ b/fs/ocfs2/journal.h >> @@ -597,9 +597,11 @@ static inline void ocfs2_update_inode_fsync_trans(handle_t *handle, >> { >> struct ocfs2_inode_info *oi = OCFS2_I(inode); >> >> - oi->i_sync_tid = handle->h_transaction->t_tid; >> - if (datasync) >> - oi->i_datasync_tid = handle->h_transaction->t_tid; >> + if (!is_handle_aborted(handle)) { >> + oi->i_sync_tid = handle->h_transaction->t_tid; >> + if (datasync) >> + oi->i_datasync_tid = handle->h_transaction->t_tid; > > Use tab instead of space, please.Thanks, I will resend it later.> >> + } >> } >> >> #endif /* OCFS2_JOURNAL_H */ >> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c >> index 8ea51cf27b97..da65251ef815 100644 >> --- a/fs/ocfs2/namei.c >> +++ b/fs/ocfs2/namei.c >> @@ -586,8 +586,7 @@ static int __ocfs2_mknod_locked(struct inode *dir, >> mlog_errno(status); >> } >> >> - oi->i_sync_tid = handle->h_transaction->t_tid; >> - oi->i_datasync_tid = handle->h_transaction->t_tid; >> + ocfs2_update_inode_fsync_trans(handle, inode, 1); >> > I don't see any reason why we have to check handle here. > > Thanks, > JosephYes, it doesn't need to check handle here. handle->h_transaction can not be NULL here. But?other functions use ocfs2_update_inode_fsync_trans() to access t_tid in handle->h_transaction. I modify it for the uniform format. Thanks, Yan Wang> >> leave: >> if (status < 0) { > > . >
Joseph Qi
2020-Jan-09 01:25 UTC
[Ocfs2-devel] [PATCH] ocfs2: fix a NULL pointer dereference when call ocfs2_update_inode_fsync_trans()
On 20/1/8 20:14, wangyan wrote:> On 2020/1/8 19:31, Joseph Qi wrote: >> >> >> On 20/1/8 17:23, wangyan wrote: >>> I found a NULL pointer dereference in ocfs2_update_inode_fsync_trans(), >>> handle->h_transaction may be NULL in this situation: >>> ocfs2_file_write_iter >>> ? ->__generic_file_write_iter >>> ????? ->generic_perform_write >>> ??????? ->ocfs2_write_begin >>> ????????? ->ocfs2_write_begin_nolock >>> ??????????? ->ocfs2_write_cluster_by_desc >>> ????????????? ->ocfs2_write_cluster >>> ??????????????? ->ocfs2_mark_extent_written >>> ????????????????? ->ocfs2_change_extent_flag >>> ??????????????????? ->ocfs2_split_extent >>> ????????????????????? ->ocfs2_try_to_merge_extent >>> ??????????????????????? ->ocfs2_extend_rotate_transaction >>> ????????????????????????? ->ocfs2_extend_trans >>> ??????????????????????????? ->jbd2_journal_restart >>> ????????????????????????????? ->jbd2__journal_restart >>> ??????????????????????????????? // handle->h_transaction is NULL here >>> ??????????????????????????????? ->handle->h_transaction = NULL; >>> ??????????????????????????????? ->start_this_handle >>> ????????????????????????????????? /* journal aborted due to storage >>> ???????????????????????????????????? network disconnection, return error */ >>> ????????????????????????????????? ->return -EROFS; >>> ???????????????????????? /* line 3806 in ocfs2_try_to_merge_extent (), >>> ??????????????????????????? it will ignore ret error. */ >>> ??????????????????????? ->ret = 0; >>> ??????? ->... >>> ??????? ->ocfs2_write_end >>> ????????? ->ocfs2_write_end_nolock >>> ??????????? ->ocfs2_update_inode_fsync_trans >>> ????????????? // NULL pointer dereference >>> ????????????? ->oi->i_sync_tid = handle->h_transaction->t_tid; >>> >>> The information of NULL pointer dereference as follows: >>> ??? JBD2: Detected IO errors while flushing file data on dm-11-45 >>> ??? Aborting journal on device dm-11-45. >>> ??? JBD2: Error -5 detected when updating journal superblock for dm-11-45. >>> ??? (dd,22081,3):ocfs2_extend_trans:474 ERROR: status = -30 >>> ??? (dd,22081,3):ocfs2_try_to_merge_extent:3877 ERROR: status = -30 >>> ??? Unable to handle kernel NULL pointer dereference at >>> ??? virtual address 0000000000000008 >>> ??? Mem abort info: >>> ????? ESR = 0x96000004 >>> ????? Exception class = DABT (current EL), IL = 32 bits >>> ????? SET = 0, FnV = 0 >>> ????? EA = 0, S1PTW = 0 >>> ??? Data abort info: >>> ????? ISV = 0, ISS = 0x00000004 >>> ????? CM = 0, WnR = 0 >>> ??? user pgtable: 4k pages, 48-bit VAs, pgdp = 00000000e74e1338 >>> ??? [0000000000000008] pgd=0000000000000000 >>> ??? Internal error: Oops: 96000004 [#1] SMP >>> ??? Process dd (pid: 22081, stack limit = 0x00000000584f35a9) >>> ??? CPU: 3 PID: 22081 Comm: dd Kdump: loaded >>> ??? Hardware name: Huawei TaiShan 2280 V2/BC82AMDD, BIOS 0.98 08/25/2019 >>> ??? pstate: 60400009 (nZCv daif +PAN -UAO) >>> ??? pc : ocfs2_write_end_nolock+0x2b8/0x550 [ocfs2] >>> ??? lr : ocfs2_write_end_nolock+0x2a0/0x550 [ocfs2] >>> ??? sp : ffff0000459fba70 >>> ??? x29: ffff0000459fba70 x28: 0000000000000000 >>> ??? x27: ffff807ccf7f1000 x26: 0000000000000001 >>> ??? x25: ffff807bdff57970 x24: ffff807caf1d4000 >>> ??? x23: ffff807cc79e9000 x22: 0000000000001000 >>> ??? x21: 000000006c6cd000 x20: ffff0000091d9000 >>> ??? x19: ffff807ccb239db0 x18: ffffffffffffffff >>> ??? x17: 000000000000000e x16: 0000000000000007 >>> ??? x15: ffff807c5e15bd78 x14: 0000000000000000 >>> ??? x13: 0000000000000000 x12: 0000000000000000 >>> ??? x11: 0000000000000000 x10: 0000000000000001 >>> ??? x9 : 0000000000000228 x8 : 000000000000000c >>> ??? x7 : 0000000000000fff x6 : ffff807a308ed6b0 >>> ??? x5 : ffff7e01f10967c0 x4 : 0000000000000018 >>> ??? x3 : d0bc661572445600 x2 : 0000000000000000 >>> ??? x1 : 000000001b2e0200 x0 : 0000000000000000 >>> ??? Call trace: >>> ???? ocfs2_write_end_nolock+0x2b8/0x550 [ocfs2] >>> ???? ocfs2_write_end+0x4c/0x80 [ocfs2] >>> ???? generic_perform_write+0x108/0x1a8 >>> ???? __generic_file_write_iter+0x158/0x1c8 >>> ???? ocfs2_file_write_iter+0x668/0x950 [ocfs2] >>> ???? __vfs_write+0x11c/0x190 >>> ???? vfs_write+0xac/0x1c0 >>> ???? ksys_write+0x6c/0xd8 >>> ???? __arm64_sys_write+0x24/0x30 >>> ???? el0_svc_common+0x78/0x130 >>> ???? el0_svc_handler+0x38/0x78 >>> ???? el0_svc+0x8/0xc >>> >>> To prevent NULL pointer dereference in this situation, we use >>> is_handle_aborted() before using handle->h_transaction->t_tid. >>> >>> Signed-off-by: Yan Wang <wangyan122 at huawei.com> >>> Reviewed-by: Jun Piao <piaojun at huawei.com> >>> --- >>> ?fs/ocfs2/journal.h | 8 +++++--- >>> ?fs/ocfs2/namei.c?? | 3 +-- >>> ?2 files changed, 6 insertions(+), 5 deletions(-) >>> >>> diff --git a/fs/ocfs2/journal.h b/fs/ocfs2/journal.h >>> index 3103ba7f97a2..bfe611ed1b1d 100644 >>> --- a/fs/ocfs2/journal.h >>> +++ b/fs/ocfs2/journal.h >>> @@ -597,9 +597,11 @@ static inline void ocfs2_update_inode_fsync_trans(handle_t *handle, >>> ?{ >>> ???? struct ocfs2_inode_info *oi = OCFS2_I(inode); >>> >>> -??? oi->i_sync_tid = handle->h_transaction->t_tid; >>> -??? if (datasync) >>> -??????? oi->i_datasync_tid = handle->h_transaction->t_tid; >>> +??? if (!is_handle_aborted(handle)) { >>> +??????? oi->i_sync_tid = handle->h_transaction->t_tid; >>> +??????? if (datasync) >>> +??????????? oi->i_datasync_tid = handle->h_transaction->t_tid; >> >> Use tab instead of space, please. > Thanks, I will resend it later. >> >>> +??? } >>> ?} >>> >>> ?#endif /* OCFS2_JOURNAL_H */ >>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c >>> index 8ea51cf27b97..da65251ef815 100644 >>> --- a/fs/ocfs2/namei.c >>> +++ b/fs/ocfs2/namei.c >>> @@ -586,8 +586,7 @@ static int __ocfs2_mknod_locked(struct inode *dir, >>> ???????????? mlog_errno(status); >>> ???? } >>> >>> -??? oi->i_sync_tid = handle->h_transaction->t_tid; >>> -??? oi->i_datasync_tid = handle->h_transaction->t_tid; >>> +??? ocfs2_update_inode_fsync_trans(handle, inode, 1); >>> >> I don't see any reason why we have to check handle here. >> >> Thanks, >> Joseph > Yes, it doesn't need to check handle here. > handle->h_transaction can not be NULL here. But?other > functions use ocfs2_update_inode_fsync_trans() to access > t_tid in handle->h_transaction. > I modify it for the uniform format. >I suggest we split the above cleanup into another patch. Since it doesn't have relations with the subject. Thanks, Joseph