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

[Jiufei Xue <xuejiufei@gmail.com>]

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