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

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

[Jiufei Xue]

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(-)

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

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

[Joseph Qi]

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

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

[Jiufei Xue]

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

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

[Jiufei Xue]

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

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

[Theodore Ts'o]

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