lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Fri, 8 Oct 2021 10:04:59 +0800
From:   yangerkun <yangerkun@...wei.com>
To:     Jan Kara <jack@...e.cz>
CC:     <tytso@....edu>, <linux-ext4@...r.kernel.org>, <yukuai3@...wei.com>
Subject: Re: [PATCH 3/3] ext4: stop use path once restart journal in
 ext4_ext_shift_path_extents



在 2021/10/1 0:43, Jan Kara 写道:
> Let me improve English a bit:
> 
> On Fri 03-09-21 14:27:48, yangerkun wrote:
>> We get a BUG as follow:
> 
> We hit the following bug:
> 
>>
>> [52117.465187] ------------[ cut here ]------------
>> [52117.465686] kernel BUG at fs/ext4/extents.c:1756!
>> ...
>> [52117.478306] Call Trace:
>> [52117.478565]  ext4_ext_shift_extents+0x3ee/0x710
>> [52117.479020]  ext4_fallocate+0x139c/0x1b40
>> [52117.479405]  ? __do_sys_newfstat+0x6b/0x80
>> [52117.479805]  vfs_fallocate+0x151/0x4b0
>> [52117.480177]  ksys_fallocate+0x4a/0xa0
>> [52117.480533]  __x64_sys_fallocate+0x22/0x30
>> [52117.480930]  do_syscall_64+0x35/0x80
>> [52117.481277]  entry_SYSCALL_64_after_hwframe+0x44/0xae
>> [52117.481769] RIP: 0033:0x7fa062f855ca
>>
>> static int ext4_ext_try_to_merge_right(struct inode *inode,
>>                                   struct ext4_ext_path *path,
>>                                   struct ext4_extent *ex)
>> {
>>          struct ext4_extent_header *eh;
>>          unsigned int depth, len;
>>          int merge_done = 0, unwritten;
>>
>>          depth = ext_depth(inode);
>>          BUG_ON(path[depth].p_hdr == NULL); <=== trigger here
>>          eh = path[depth].p_hdr;
>>
>> Normally, we protect extent tree with i_data_sem, and once we really
>> need drop i_data_sem, we should reload the ext4_ext_path array after we
>> recatch i_data_sem since extent tree may has changed, the 'again' in
>> ext4_ext_remove_space give us a sample. But the other case
>> ext4_ext_shift_path_extents seems forget to do this(ext4_access_path may
>> drop i_data_sem and recatch it with not enough credits), and will lead
>> the upper BUG when there is a parallel extents split which will grow the
>> extent tree.
> 
> Normally, the extent tree is protected by i_data_sem and if we drop
> i_data_sem in ext4_datasem_ensure_credits(), we need to reload
> ext4_ext_path array after reacquiring i_data_sem since the extent tree may
> have changed. The 'again' label in ext4_ext_remove_space() is an example of
> this. But ext4_ext_shift_path_extents() forgets to reload ext4_ext_path and
> thus can cause the above mentioned BUG when there is a parallel extents
> split which will grow the extent tree.
> 
>>
>> Fix it by introduce the again in ext4_ext_shift_extents.
>>
>> Signed-off-by: yangerkun <yangerkun@...wei.com>
>> ---
>>   fs/ext4/extents.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index a6fb0350f062..0aa14f6ca914 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -5009,8 +5009,11 @@ ext4_ext_shift_path_extents(struct ext4_ext_path *path, ext4_lblk_t shift,
>>   			restart_credits = ext4_writepage_trans_blocks(inode);
>>   			err = ext4_datasem_ensure_credits(handle, inode, credits,
>>   					restart_credits, 0);
>> -			if (err)
>> +			if (err) {
>> +				if (err > 0)
>> +					err = -EAGAIN;
>>   				goto out;
>> +			}
> 
> Hum, I'd note that the previous patch actually broke
> ext4_ext_shift_path_extents() which could return 1 after patch 2/3 and
> probably confuse code upwards in the stack and now you fix it up in this
> patch. Can you perhaps fixup the previous patch by changing the condition
> to:
> 	if (err < 0)
> 
> and then change it here?

Thanks for your review! Ted has fix this by add some comments in patch 2/3!


> 
>>   
>>   			err = ext4_ext_get_access(handle, inode, path + depth);
>>   			if (err)
>> @@ -5084,6 +5087,7 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,
>>   	int ret = 0, depth;
>>   	struct ext4_extent *extent;
>>   	ext4_lblk_t stop, *iterator, ex_start, ex_end;
>> +	ext4_lblk_t tmp = EXT_MAX_BLOCKS;
> 
> Can you perhaps name this more descriptively than 'tmp'? Something like
> restart_lblk or something like that?

Agree. I'll pay attention next time!

>    
>>   	/* Let path point to the last extent */
>>   	path = ext4_find_extent(inode, EXT_MAX_BLOCKS - 1, NULL,
>> @@ -5137,11 +5141,15 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,
>>   	 * till we reach stop. In case of right shift, iterator points to stop
>>   	 * and it is decreased till we reach start.
>>   	 */
>> +again:
>>   	if (SHIFT == SHIFT_LEFT)
>>   		iterator = &start;
>>   	else
>>   		iterator = &stop;
>>   
>> +	if (tmp != EXT_MAX_BLOCKS)
>> +		*iterator = tmp;
>> +
>>   	/*
>>   	 * Its safe to start updating extents.  Start and stop are unsigned, so
>>   	 * in case of right shift if extent with 0 block is reached, iterator
>> @@ -5170,6 +5178,7 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,
>>   			}
>>   		}
>>   
>> +		tmp = *iterator;
>>   		if (SHIFT == SHIFT_LEFT) {
>>   			extent = EXT_LAST_EXTENT(path[depth].p_hdr);
>>   			*iterator = le32_to_cpu(extent->ee_block) +
>> @@ -5188,6 +5197,9 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,
>>   		}
>>   		ret = ext4_ext_shift_path_extents(path, shift, inode,
>>   				handle, SHIFT);
>> +		/* iterator can be NULL which means we should break */
>> +		if (ret == -EAGAIN)
>> +			goto again;
> 
> Hum, but while we dropped i_data_sem, the extent depth may have increased
> so we may need larger 'path' now?
> 
> Otherwise the patch looks good.

The ext4_find_extent in ext4_ext_shift_extents can handle this case. It 
seems OK.


> 
> 								Honza
> 

Powered by blists - more mailing lists