2019-03-11 06:35:33

by Jiufei Xue

Subject: [PATCH] ext4: fix NULL pointer dereference while journal is aborted

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 <[email protected]>
---
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;
--
2.19.1.856.g8858448bb

2019-03-11 06:45:19

by Joseph Qi

Subject: Re: [PATCH] ext4: fix NULL pointer dereference while journal is aborted

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 <[email protected]>

Looks good.
Reviewed-by: Joseph Qi <[email protected]>

> ---
> 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;
>

2019-03-12 11:00:34

by Jiufei Xue

Subject: Re: [PATCH] ext4: fix NULL pointer dereference while journal is aborted

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 <[email protected]>
>
> Looks good.
> Reviewed-by: Joseph Qi <[email protected]>
>
>> ---
>> 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;
>>
>

2019-03-15 01:18:40

by Jiufei Xue

Subject: Re: [PATCH] ext4: fix NULL pointer dereference while journal is aborted

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 <[email protected]>
>> Looks good.
>> Reviewed-by: Joseph Qi <[email protected]>
>>
>>> ---
>>> 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;
>>>

2019-03-15 03:17:32

by Theodore Ts'o

Subject: Re: [PATCH] ext4: fix NULL pointer dereference while journal is aborted

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 <[email protected]>

Thanks, applied.

- Ted