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;
Date:   Fri, 25 Feb 2022 22:40:16 +0530
From:   Ritesh Harjani <riteshh@linux.ibm.com>
To:     Tadeusz Struk <tadeusz.struk@linaro.org>
Cc:     "Theodore Ts'o" <tytso@mit.edu>,
        Andreas Dilger <adilger.kernel@dilger.ca>,
        linux-ext4@vger.kernel.org
Subject: Re: BUG in ext4_ind_remove_space
Message-ID: <20220225171016.zwhp62b3yzgewk6l@riteshh-domain>
References: <48308b02-149b-1c47-115a-1a268dac6e24@linaro.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <48308b02-149b-1c47-115a-1a268dac6e24@linaro.org>
MIME-Version: 1.0
Precedence: bulk

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 can verify this sometime next week when I get back to it.
But thanks for reporting the issue :)

-ritesh

[1] https://syzkaller.appspot.com/bug?id=b80bd9cf348aac724a4f4dff251800106d721331

>
>
> [1] https://syzkaller.appspot.com/bug?id=b80bd9cf348aac724a4f4dff251800106d721331
> [2] https://syzkaller.appspot.com/text?tag=ReproC&x=14ba0238700000
>
> --
> Thanks,
> Tadeusz