| Message ID | 20190311063528.112996-1-jiufei.xue@linux.alibaba.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | ext4: fix NULL pointer dereference while journal is aborted | expand |
On 19/3/11 14:35, Jiufei Xue wrote: > We see the following NULL pointer dereference while running xfstests > generic/475: > BUG: unable to handle kernel NULL pointer dereference at 0000000000000008 > PGD 8000000c84bad067 P4D 8000000c84bad067 PUD c84e62067 PMD 0 > Oops: 0000 [#1] SMP PTI > CPU: 7 PID: 9886 Comm: fsstress Kdump: loaded Not tainted 5.0.0-rc8 #10 > RIP: 0010:ext4_do_update_inode+0x4ec/0x760 > ... > Call Trace: > ? jbd2_journal_get_write_access+0x42/0x50 > ? __ext4_journal_get_write_access+0x2c/0x70 > ? ext4_truncate+0x186/0x3f0 > ext4_mark_iloc_dirty+0x61/0x80 > ext4_mark_inode_dirty+0x62/0x1b0 > ext4_truncate+0x186/0x3f0 > ? unmap_mapping_pages+0x56/0x100 > ext4_setattr+0x817/0x8b0 > notify_change+0x1df/0x430 > do_truncate+0x5e/0x90 > ? generic_permission+0x12b/0x1a0 > > This is triggered because the NULL pointer handle->h_transaction was > dereferenced in function ext4_update_inode_fsync_trans(). > I found that the h_transaction was set to NULL in jbd2__journal_restart > but failed to attached to a new transaction while the journal is aborted. > > Fix this by checking the handle before updating the inode. > > Signed-off-by: Jiufei Xue <jiufei.xue@linux.alibaba.com> Looks good. Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com> > --- > fs/ext4/ext4_jbd2.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h > index a1ac7e9245ec..75a5309f2231 100644 > --- a/fs/ext4/ext4_jbd2.h > +++ b/fs/ext4/ext4_jbd2.h > @@ -384,7 +384,7 @@ static inline void ext4_update_inode_fsync_trans(handle_t *handle, > { > struct ext4_inode_info *ei = EXT4_I(inode); > > - if (ext4_handle_valid(handle)) { > + if (ext4_handle_valid(handle) && !is_handle_aborted(handle)) { > ei->i_sync_tid = handle->h_transaction->t_tid; > if (datasync) > ei->i_datasync_tid = handle->h_transaction->t_tid; >
Hi Ted, Would you please review this patch? Thanks, Jiufei On 2019/3/11 下午2:45, Joseph Qi wrote: > > > On 19/3/11 14:35, Jiufei Xue wrote: >> We see the following NULL pointer dereference while running xfstests >> generic/475: >> BUG: unable to handle kernel NULL pointer dereference at 0000000000000008 >> PGD 8000000c84bad067 P4D 8000000c84bad067 PUD c84e62067 PMD 0 >> Oops: 0000 [#1] SMP PTI >> CPU: 7 PID: 9886 Comm: fsstress Kdump: loaded Not tainted 5.0.0-rc8 #10 >> RIP: 0010:ext4_do_update_inode+0x4ec/0x760 >> ... >> Call Trace: >> ? jbd2_journal_get_write_access+0x42/0x50 >> ? __ext4_journal_get_write_access+0x2c/0x70 >> ? ext4_truncate+0x186/0x3f0 >> ext4_mark_iloc_dirty+0x61/0x80 >> ext4_mark_inode_dirty+0x62/0x1b0 >> ext4_truncate+0x186/0x3f0 >> ? unmap_mapping_pages+0x56/0x100 >> ext4_setattr+0x817/0x8b0 >> notify_change+0x1df/0x430 >> do_truncate+0x5e/0x90 >> ? generic_permission+0x12b/0x1a0 >> >> This is triggered because the NULL pointer handle->h_transaction was >> dereferenced in function ext4_update_inode_fsync_trans(). >> I found that the h_transaction was set to NULL in jbd2__journal_restart >> but failed to attached to a new transaction while the journal is aborted. >> >> Fix this by checking the handle before updating the inode. >> >> Signed-off-by: Jiufei Xue <jiufei.xue@linux.alibaba.com> > > Looks good. > Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com> > >> --- >> fs/ext4/ext4_jbd2.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h >> index a1ac7e9245ec..75a5309f2231 100644 >> --- a/fs/ext4/ext4_jbd2.h >> +++ b/fs/ext4/ext4_jbd2.h >> @@ -384,7 +384,7 @@ static inline void ext4_update_inode_fsync_trans(handle_t *handle, >> { >> struct ext4_inode_info *ei = EXT4_I(inode); >> >> - if (ext4_handle_valid(handle)) { >> + if (ext4_handle_valid(handle) && !is_handle_aborted(handle)) { >> ei->i_sync_tid = handle->h_transaction->t_tid; >> if (datasync) >> ei->i_datasync_tid = handle->h_transaction->t_tid; >> >
Hi Jan, Could you spend some time to review this patch and give some feedback? Thanks a lot. Jiufei On 2019/3/12 下午7:00, Jiufei Xue wrote: > Hi Ted, > > Would you please review this patch? > > Thanks, > Jiufei > > On 2019/3/11 下午2:45, Joseph Qi wrote: >> >> On 19/3/11 14:35, Jiufei Xue wrote: >>> We see the following NULL pointer dereference while running xfstests >>> generic/475: >>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000008 >>> PGD 8000000c84bad067 P4D 8000000c84bad067 PUD c84e62067 PMD 0 >>> Oops: 0000 [#1] SMP PTI >>> CPU: 7 PID: 9886 Comm: fsstress Kdump: loaded Not tainted 5.0.0-rc8 #10 >>> RIP: 0010:ext4_do_update_inode+0x4ec/0x760 >>> ... >>> Call Trace: >>> ? jbd2_journal_get_write_access+0x42/0x50 >>> ? __ext4_journal_get_write_access+0x2c/0x70 >>> ? ext4_truncate+0x186/0x3f0 >>> ext4_mark_iloc_dirty+0x61/0x80 >>> ext4_mark_inode_dirty+0x62/0x1b0 >>> ext4_truncate+0x186/0x3f0 >>> ? unmap_mapping_pages+0x56/0x100 >>> ext4_setattr+0x817/0x8b0 >>> notify_change+0x1df/0x430 >>> do_truncate+0x5e/0x90 >>> ? generic_permission+0x12b/0x1a0 >>> >>> This is triggered because the NULL pointer handle->h_transaction was >>> dereferenced in function ext4_update_inode_fsync_trans(). >>> I found that the h_transaction was set to NULL in jbd2__journal_restart >>> but failed to attached to a new transaction while the journal is aborted. >>> >>> Fix this by checking the handle before updating the inode. >>> >>> Signed-off-by: Jiufei Xue <jiufei.xue@linux.alibaba.com> >> Looks good. >> Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com> >> >>> --- >>> fs/ext4/ext4_jbd2.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h >>> index a1ac7e9245ec..75a5309f2231 100644 >>> --- a/fs/ext4/ext4_jbd2.h >>> +++ b/fs/ext4/ext4_jbd2.h >>> @@ -384,7 +384,7 @@ static inline void ext4_update_inode_fsync_trans(handle_t *handle, >>> { >>> struct ext4_inode_info *ei = EXT4_I(inode); >>> >>> - if (ext4_handle_valid(handle)) { >>> + if (ext4_handle_valid(handle) && !is_handle_aborted(handle)) { >>> ei->i_sync_tid = handle->h_transaction->t_tid; >>> if (datasync) >>> ei->i_datasync_tid = handle->h_transaction->t_tid; >>>
On Mon, Mar 11, 2019 at 02:35:28PM +0800, Jiufei Xue wrote: > We see the following NULL pointer dereference while running xfstests > generic/475: > BUG: unable to handle kernel NULL pointer dereference at 0000000000000008 > PGD 8000000c84bad067 P4D 8000000c84bad067 PUD c84e62067 PMD 0 > Oops: 0000 [#1] SMP PTI > CPU: 7 PID: 9886 Comm: fsstress Kdump: loaded Not tainted 5.0.0-rc8 #10 > RIP: 0010:ext4_do_update_inode+0x4ec/0x760 > ... > Call Trace: > ? jbd2_journal_get_write_access+0x42/0x50 > ? __ext4_journal_get_write_access+0x2c/0x70 > ? ext4_truncate+0x186/0x3f0 > ext4_mark_iloc_dirty+0x61/0x80 > ext4_mark_inode_dirty+0x62/0x1b0 > ext4_truncate+0x186/0x3f0 > ? unmap_mapping_pages+0x56/0x100 > ext4_setattr+0x817/0x8b0 > notify_change+0x1df/0x430 > do_truncate+0x5e/0x90 > ? generic_permission+0x12b/0x1a0 > > This is triggered because the NULL pointer handle->h_transaction was > dereferenced in function ext4_update_inode_fsync_trans(). > I found that the h_transaction was set to NULL in jbd2__journal_restart > but failed to attached to a new transaction while the journal is aborted. > > Fix this by checking the handle before updating the inode. > > Signed-off-by: Jiufei Xue <jiufei.xue@linux.alibaba.com> Thanks, applied. - Ted
diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h index a1ac7e9245ec..75a5309f2231 100644 --- a/fs/ext4/ext4_jbd2.h +++ b/fs/ext4/ext4_jbd2.h @@ -384,7 +384,7 @@ static inline void ext4_update_inode_fsync_trans(handle_t *handle, { struct ext4_inode_info *ei = EXT4_I(inode); - if (ext4_handle_valid(handle)) { + if (ext4_handle_valid(handle) && !is_handle_aborted(handle)) { ei->i_sync_tid = handle->h_transaction->t_tid; if (datasync) ei->i_datasync_tid = handle->h_transaction->t_tid;
We see the following NULL pointer dereference while running xfstests generic/475: BUG: unable to handle kernel NULL pointer dereference at 0000000000000008 PGD 8000000c84bad067 P4D 8000000c84bad067 PUD c84e62067 PMD 0 Oops: 0000 [#1] SMP PTI CPU: 7 PID: 9886 Comm: fsstress Kdump: loaded Not tainted 5.0.0-rc8 #10 RIP: 0010:ext4_do_update_inode+0x4ec/0x760 ... Call Trace: ? jbd2_journal_get_write_access+0x42/0x50 ? __ext4_journal_get_write_access+0x2c/0x70 ? ext4_truncate+0x186/0x3f0 ext4_mark_iloc_dirty+0x61/0x80 ext4_mark_inode_dirty+0x62/0x1b0 ext4_truncate+0x186/0x3f0 ? unmap_mapping_pages+0x56/0x100 ext4_setattr+0x817/0x8b0 notify_change+0x1df/0x430 do_truncate+0x5e/0x90 ? generic_permission+0x12b/0x1a0 This is triggered because the NULL pointer handle->h_transaction was dereferenced in function ext4_update_inode_fsync_trans(). I found that the h_transaction was set to NULL in jbd2__journal_restart but failed to attached to a new transaction while the journal is aborted. Fix this by checking the handle before updating the inode. Signed-off-by: Jiufei Xue <jiufei.xue@linux.alibaba.com> --- fs/ext4/ext4_jbd2.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)