Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <25749d7d-7036-0b71-3dd8-7b04dcc430e4@linaro.org>
Date:   Fri, 25 Feb 2022 11:19:19 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: BUG in ext4_ind_remove_space
Content-Language: en-US
To:     Ritesh Harjani <riteshh@linux.ibm.com>
Cc:     Theodore Ts'o <tytso@mit.edu>,
        Andreas Dilger <adilger.kernel@dilger.ca>,
        linux-ext4@vger.kernel.org
References: <48308b02-149b-1c47-115a-1a268dac6e24@linaro.org>
 <20220225171016.zwhp62b3yzgewk6l@riteshh-domain>
From:   Tadeusz Struk <tadeusz.struk@linaro.org>
In-Reply-To: <20220225171016.zwhp62b3yzgewk6l@riteshh-domain>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/25/22 09:10, Ritesh Harjani wrote:
> On 22/02/24 05:12PM, Tadeusz Struk wrote:
>>
>> Hi,
>> Syzbot found an issue [1] in fallocate() that looks to me like a loss of precision.
>> The C reproducer [2] calls fallocate() and passes the size 0xffeffeff000ul, and
>> offset 0x1000000ul, which is then used to calculate the first_block and
>> stop_block using ext4_lblk_t type (u32). I think this gets the MSB of the size
>> truncated and leads to invalid calculations, and eventually his BUG() in
>> https://elixir.bootlin.com/linux/v5.16.11/source/fs/ext4/indirect.c#L1244
>> The issue can be reproduced on 5.17.0-rc5, but I don't think it's a new
>> regression. I spent some time debugging it, but could spot anything obvious.
>> Can someone have a look please.
> 
> I did look into it a little. Below are some of my observations.
> If nobody gets to it before me, I can spend sometime next week to verify it's
> correctness.
> 
> So I think based on the warning log before kernel BUG_ON() [1], it looks like it
> has the problem with ext4_block_to_path() calculation with end offset.
> It seems it is not fitting into triple block ptrs calculation.
> 
> <log>
> ======
> EXT4-fs warning (device sda1): ext4_block_to_path:107: block 1074791436 > max in inode 1137
> 
> But ideally it should fit in (right?) since we do make sure if end >= max_block;
> then we set it to end = max_block.
> 
> Then looking at how we calculate max_block is below
> 	max_block = (EXT4_SB(inode->i_sb)->s_bitmap_maxbytes + blocksize-1) >> EXT4_BLOCK_SIZE_BITS(inode->i_sb);
> 
> So looking closely I think there _could be_ a off by 1 calculation error in
> above. So I think above calculation is for max_len and not max_end_block.
> 
> I think max_block should be max_end_block which should be this -
> 	max_end_block = ((EXT4_SB(inode->i_sb)->s_bitmap_maxbytes + blocksize-1) >> EXT4_BLOCK_SIZE_BITS(inode->i_sb))-1;
> 
> 
> But I haven't yet verified it completely. This is just my initial thought.
> Maybe others can confirm too. Or maybe there is more then one problem.
> But somehow above looks more likely to me.

I tried the above and it does help.

> 
> I can verify this sometime next week when I get back to it.
> But thanks for reporting the issue :)

Next week is perfectly fine. Thanks for looking into it.

-- 
Thanks,
Tadeusz