Received-SPF: pass (google.com: domain of linux-kernel+bounces-175694-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Subject: Re: [PATCH v3 03/26] ext4: correct the hole length returned by
 ext4_map_blocks()
To: Luis Henriques <luis.henriques@linux.dev>
Cc: Theodore Ts'o <tytso@mit.edu>, linux-ext4@vger.kernel.org,
 linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
 linux-kernel@vger.kernel.org, adilger.kernel@dilger.ca, jack@suse.cz,
 ritesh.list@gmail.com, hch@infradead.org, djwong@kernel.org,
 willy@infradead.org, zokeefe@google.com, yi.zhang@huawei.com,
 chengzhihao1@huawei.com, yukuai3@huawei.com, wangkefeng.wang@huawei.com
References: <20240127015825.1608160-1-yi.zhang@huaweicloud.com>
 <20240127015825.1608160-4-yi.zhang@huaweicloud.com>
 <87zfszuib1.fsf@brahms.olymp> <20240509163953.GI3620298@mit.edu>
 <87h6f6vqzj.fsf@brahms.olymp>
 <b9b93ad2-2253-6850-da38-afc42370303e@huaweicloud.com>
 <87seyquhpi.fsf@brahms.olymp>
From: Zhang Yi <yi.zhang@huaweicloud.com>
Message-ID: <ea7d4019-fec7-d30c-e62a-37e665ccd610@huaweicloud.com>
Date: Fri, 10 May 2024 19:40:52 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <87seyquhpi.fsf@brahms.olymp>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit

On 2024/5/10 17:41, Luis Henriques wrote:
> On Fri 10 May 2024 11:39:48 AM +08, Zhang Yi wrote;
> 
>> On 2024/5/10 1:23, Luis Henriques wrote:
>>> On Thu 09 May 2024 12:39:53 PM -04, Theodore Ts'o wrote;
>>>
>>>> On Thu, May 09, 2024 at 04:16:34PM +0100, Luis Henriques wrote:
>>>>>
>>>>> It's looks like it's easy to trigger an infinite loop here using fstest
>>>>> generic/039.  If I understand it correctly (which doesn't happen as often
>>>>> as I'd like), this is due to an integer overflow in the 'if' condition,
>>>>> and should be fixed with the patch below.
>>>>
>>>> Thanks for the report.  However, I can't reproduce the failure, and
>>>> looking at generic/039, I don't see how it could be relevant to the
>>>> code path in question.  Generic/039 creates a test symlink with two
>>>> hard links in the same directory, syncs the file system, and then
>>>> removes one of the hard links, and then drops access to the block
>>>> device using dmflakey.  So I don't see how the extent code would be
>>>> involved at all.  Are you sure that you have the correct test listed?
>>>
>>> Yep, I just retested and it's definitely generic/039.  I'm using a simple
>>> test environment, with virtme-ng.
>>>
>>>> Looking at the code in question in fs/ext4/extents.c:
>>>>
>>>> again:
>>>> 	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, hole_start,
>>>> 				  hole_start + len - 1, &es);
>>>> 	if (!es.es_len)
>>>> 		goto insert_hole;
>>>>
>>>>   	 * There's a delalloc extent in the hole, handle it if the delalloc
>>>>   	 * extent is in front of, behind and straddle the queried range.
>>>>   	 */
>>>>  -	if (lblk >= es.es_lblk + es.es_len) {
>>>>  +	if (lblk >= ((__u64) es.es_lblk) + es.es_len) {
>>>>   		/*
>>>>   		 * The delalloc extent is in front of the queried range,
>>>>   		 * find again from the queried start block.
>>>> 		len -= lblk - hole_start;
>>>> 		hole_start = lblk;
>>>> 		goto again;
>>>>
>>>> lblk and es.es_lblk are both __u32.  So the infinite loop is
>>>> presumably because es.es_lblk + es.es_len has overflowed.  This should
>>>> never happen(tm), and in fact we have a test for this case which
>>>
>>> If I instrument the code, I can see that es.es_len is definitely set to
>>> EXT_MAX_BLOCKS, which will overflow.
>>>
>>
>> Thanks for the report. After looking at the code, I think the root
>> cause of this issue is the variable es was not initialized on replaying
>> fast commit. ext4_es_find_extent_range() will return directly when
>> EXT4_FC_REPLAY flag is set, and then the es.len becomes stall.
>>
>> I can always reproduce this issue on generic/039 with
>> MKFS_OPTIONS="-O fast_commit".
>>
>> This uninitialization problem originally existed in the old
>> ext4_ext_put_gap_in_cache(), but it didn't trigger any real problem
>> since we never check and use extent cache when replaying fast commit.
>> So I suppose the correct fix would be to unconditionally initialize
>> the es variable.
> 
> Oh, you're absolutely right -- the extent_status 'es' struct isn't being
> initialized in that case.  I totally failed to see that.  And yes, I also
> failed to mention I had 'fast_commit' feature enabled, sorry!
> 
> Thanks a lot for figuring this out, Yi.  I'm looking at this code and
> trying to understand if it would be safe to call __es_find_extent_range()
> when EXT4_FC_REPLAY is in progress.  Probably not, and probably better to
> simply do:
> 
> 	es->es_lblk = es->es_len = es->es_pblk = 0;
> 
> in that case.  I'll send out a patch later today.
> 

Yeah, I'm glad it could help.

Thanks,
Yi.