BUG in ext4_ind_remove_space

BUG in ext4_ind_remove_space

[Tadeusz Struk]

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.


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

-- 
Thanks,
Tadeusz

Re: BUG in ext4_ind_remove_space

[Ritesh Harjani]

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

Re: BUG in ext4_ind_remove_space

[Tadeusz Struk]

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

Re: BUG in ext4_ind_remove_space

[Tadeusz Struk]

On 2/25/22 11:19, Tadeusz Struk wrote:
>> 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.

Hi Ritesh,
Did you have chance to look into this?
If you want I can send a patch that fixes the off by 1 calculation error.

-- 
Thanks,
Tadeusz

Re: BUG in ext4_ind_remove_space

[Ritesh Harjani]

On 22/03/02 03:14PM, Tadeusz Struk wrote:
> On 2/25/22 11:19, Tadeusz Struk wrote:
> > > 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.
>
> Hi Ritesh,
> Did you have chance to look into this?
> If you want I can send a patch that fixes the off by 1 calculation error.

Hi Tadeusz,

I wanted to look at that path a bit more before sending that patch.
Last analysis by me was more of a cursory look at the kernel dmesg log which you
had shared.

In case if you want to pursue that issue and spend time on it, then feel free to
do it.

I got pulled into number of other things last week and this week. So didn't get
a chance to look into it yet. I hope to look into this soon (if no one else
picks up :))

-ritesh


>
> --
> Thanks,
> Tadeusz

Re: BUG in ext4_ind_remove_space

[Tadeusz Struk]

On 3/3/22 06:56, Ritesh Harjani wrote:
> On 22/03/02 03:14PM, Tadeusz Struk wrote:
>> On 2/25/22 11:19, Tadeusz Struk wrote:
>>>> 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.
>>
>> Hi Ritesh,
>> Did you have chance to look into this?
>> If you want I can send a patch that fixes the off by 1 calculation error.
> 
> Hi Tadeusz,
> 
> I wanted to look at that path a bit more before sending that patch.
> Last analysis by me was more of a cursory look at the kernel dmesg log which you
> had shared.
> 
> In case if you want to pursue that issue and spend time on it, then feel free to
> do it.
> 
> I got pulled into number of other things last week and this week. So didn't get
> a chance to look into it yet. I hope to look into this soon (if no one else
> picks up :))

I'm not familiar with the internals of ext4 implementation so I would rather
have someone who knows it look at it.
I will wait till the end of this week and if there will be fix for it by then
I will send a patch fixing the off by 1 issue to get the ball rolling.

-- 
Thanks,
Tadeusz

Re: BUG in ext4_ind_remove_space

[Ritesh Harjani]

On 22/03/03 07:37AM, Tadeusz Struk wrote:
> On 3/3/22 06:56, Ritesh Harjani wrote:
> > On 22/03/02 03:14PM, Tadeusz Struk wrote:
> > > On 2/25/22 11:19, Tadeusz Struk wrote:
> > > > > 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.
> > >
> > > Hi Ritesh,
> > > Did you have chance to look into this?
> > > If you want I can send a patch that fixes the off by 1 calculation error.
> >
> > Hi Tadeusz,
> >
> > I wanted to look at that path a bit more before sending that patch.
> > Last analysis by me was more of a cursory look at the kernel dmesg log which you
> > had shared.
> >
> > In case if you want to pursue that issue and spend time on it, then feel free to
> > do it.
> >
> > I got pulled into number of other things last week and this week. So didn't get
> > a chance to look into it yet. I hope to look into this soon (if no one else
> > picks up :))
>
> I'm not familiar with the internals of ext4 implementation so I would rather
> have someone who knows it look at it.

No problem. I am willing to look into this anyways.
btw, this issue could be seen easily with below cmd on non-extent ext4 FS.

sudo xfs_io -f -c "truncate 0x4010040c000" -c "fsync" -c "fpunch 0x1000000 0xffefffff000" testfile

-ritesh

Re: BUG in ext4_ind_remove_space

[Ritesh Harjani]

On 22/03/04 01:38AM, Ritesh Harjani wrote:
> On 22/03/03 07:37AM, Tadeusz Struk wrote:
> > On 3/3/22 06:56, Ritesh Harjani wrote:
> > > On 22/03/02 03:14PM, Tadeusz Struk wrote:
> > > > On 2/25/22 11:19, Tadeusz Struk wrote:
> > > > > > 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.
> > > >
> > > > Hi Ritesh,
> > > > Did you have chance to look into this?
> > > > If you want I can send a patch that fixes the off by 1 calculation error.
> > >
> > > Hi Tadeusz,
> > >
> > > I wanted to look at that path a bit more before sending that patch.
> > > Last analysis by me was more of a cursory look at the kernel dmesg log which you
> > > had shared.
> > >
> > > In case if you want to pursue that issue and spend time on it, then feel free to
> > > do it.
> > >
> > > I got pulled into number of other things last week and this week. So didn't get
> > > a chance to look into it yet. I hope to look into this soon (if no one else
> > > picks up :))
> >
> > I'm not familiar with the internals of ext4 implementation so I would rather
> > have someone who knows it look at it.
>
> No problem. I am willing to look into this anyways.
> btw, this issue could be seen easily with below cmd on non-extent ext4 FS.
>
> sudo xfs_io -f -c "truncate 0x4010040c000" -c "fsync" -c "fpunch 0x1000000 0xffefffff000" testfile

Just FYI - The change which we discussed to fix the max_block to max_end_block, is not correct.
Since it will still leave 1 block at the end after punch operation, if the file has s_bitmap_maxbytes size.
This is due to the fact that, "end" is expected to be 1 block after the end of last block.

Will try look into it to see how can we fix this.

1210 /**
1211  *      ext4_ind_remove_space - remove space from the range
1212  *      @handle: JBD handle for this transaction
1213  *      @inode: inode we are dealing with
1214  *      @start: First block to remove
1215  *      @end:   One block after the last block to remove (exclusive)
1216  *
1217  *      Free the blocks in the defined range (end is exclusive endpoint of
1218  *      range). This is used by ext4_punch_hole().
1219  */
1220 int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
1221                           ext4_lblk_t start, ext4_lblk_t end)

-ritesh

Re: BUG in ext4_ind_remove_space

[Tadeusz Struk]

On 3/6/22 21:45, Ritesh Harjani wrote:
> On 22/03/04 01:38AM, Ritesh Harjani wrote:
>> On 22/03/03 07:37AM, Tadeusz Struk wrote:
>>> On 3/3/22 06:56, Ritesh Harjani wrote:
>>>> On 22/03/02 03:14PM, Tadeusz Struk wrote:
>>>>> On 2/25/22 11:19, Tadeusz Struk wrote:
>>>>>>> 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.
>>>>>
>>>>> Hi Ritesh,
>>>>> Did you have chance to look into this?
>>>>> If you want I can send a patch that fixes the off by 1 calculation error.
>>>>
>>>> Hi Tadeusz,
>>>>
>>>> I wanted to look at that path a bit more before sending that patch.
>>>> Last analysis by me was more of a cursory look at the kernel dmesg log which you
>>>> had shared.
>>>>
>>>> In case if you want to pursue that issue and spend time on it, then feel free to
>>>> do it.
>>>>
>>>> I got pulled into number of other things last week and this week. So didn't get
>>>> a chance to look into it yet. I hope to look into this soon (if no one else
>>>> picks up :))
>>>
>>> I'm not familiar with the internals of ext4 implementation so I would rather
>>> have someone who knows it look at it.
>>
>> No problem. I am willing to look into this anyways.
>> btw, this issue could be seen easily with below cmd on non-extent ext4 FS.
>>
>> sudo xfs_io -f -c "truncate 0x4010040c000" -c "fsync" -c "fpunch 0x1000000 0xffefffff000" testfile
> 
> Just FYI - The change which we discussed to fix the max_block to max_end_block, is not correct.
> Since it will still leave 1 block at the end after punch operation, if the file has s_bitmap_maxbytes size.
> This is due to the fact that, "end" is expected to be 1 block after the end of last block.
> 
> Will try look into it to see how can we fix this.
> 
> 1210 /**
> 1211  *      ext4_ind_remove_space - remove space from the range
> 1212  *      @handle: JBD handle for this transaction
> 1213  *      @inode: inode we are dealing with
> 1214  *      @start: First block to remove
> 1215  *      @end:   One block after the last block to remove (exclusive)
> 1216  *
> 1217  *      Free the blocks in the defined range (end is exclusive endpoint of
> 1218  *      range). This is used by ext4_punch_hole().
> 1219  */
> 1220 int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
> 1221                           ext4_lblk_t start, ext4_lblk_t end)
> 

Hi Ritesh,
Thanks for update. Let me know if I can be of any help to you.

-- 
Thanks,
Tadeusz

Re: BUG in ext4_ind_remove_space

[Tadeusz Struk]

On 3/6/22 21:45, Ritesh Harjani wrote:
> Just FYI - The change which we discussed to fix the max_block to max_end_block, is not correct.
> Since it will still leave 1 block at the end after punch operation, if the file has s_bitmap_maxbytes size.
> This is due to the fact that, "end" is expected to be 1 block after the end of last block.
> 
> Will try look into it to see how can we fix this.
> 
> 1210 /**
> 1211  *      ext4_ind_remove_space - remove space from the range
> 1212  *      @handle: JBD handle for this transaction
> 1213  *      @inode: inode we are dealing with
> 1214  *      @start: First block to remove
> 1215  *      @end:   One block after the last block to remove (exclusive)
> 1216  *
> 1217  *      Free the blocks in the defined range (end is exclusive endpoint of
> 1218  *      range). This is used by ext4_punch_hole().
> 1219  */
> 1220 int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
> 1221                           ext4_lblk_t start, ext4_lblk_t end)
> 
> -ritesh

Hi Ritesh,
I tried to bisect it and went as far back as 4.4 and the issue still triggers there.
I couldn't build anything older than that with my compiler, but I suspect that
the issue exists even before 3.0 where ext4_fallocate() has been introduced.

-- 
Thanks,
Tadeusz

Re: BUG in ext4_ind_remove_space

[Tadeusz Struk]

On 3/6/22 21:45, Ritesh Harjani wrote:
> Just FYI - The change which we discussed to fix the max_block to max_end_block, is not correct.
> Since it will still leave 1 block at the end after punch operation, if the file has s_bitmap_maxbytes size.
> This is due to the fact that, "end" is expected to be 1 block after the end of last block.
> 
> Will try look into it to see how can we fix this.
> 
> 1210 /**
> 1211  *      ext4_ind_remove_space - remove space from the range
> 1212  *      @handle: JBD handle for this transaction
> 1213  *      @inode: inode we are dealing with
> 1214  *      @start: First block to remove
> 1215  *      @end:   One block after the last block to remove (exclusive)

So in that case, in ext4_punch_hole(), what should be done is:

if offset + length > sbi->s_bitmap_maxbytes - inode->i_sb->s_blocksize

it either needs to update the length to:
length = sbi->s_bitmap_maxbytes - inode->i_sb->s_blocksize - offset;

or -ENOSPC should be returned, which would be more consistent with the `man 2 
fallocate`:

"ERRORS:
...
  ENOSPC There is not enough space left on the device containing the file 
referred to by fd."

Please let me know if my reckoning is correct, and if so which option you
prefer and I will follow with a patch.

-- 
Thanks,
Tadeusz

[PATCH] ext4: check if offset+length is within a valid range in fallocate

[Tadeusz Struk]

Syzbot found an issue [1] in ext4_fallocate().
The C reproducer [2] calls fallocate(), passing size 0xffeffeff000ul,
and offset 0x1000000ul, which, when added together exceed the disk size,
and trigger a BUG in ext4_ind_remove_space() [3].
According to the comment doc in ext4_ind_remove_space() the 'end'
parameter needs to be one block after the last block to remove.
In the case when the BUG is triggered it points to the last block on
a 4GB virtual disk image. This is calculated in
ext4_ind_remove_space() in [4].
This patch adds a check that ensure the length + offest to be
within the valid range and returns -ENOSPC error code in case
it is invalid.

LINK: [1] https://syzkaller.appspot.com/bug?id=b80bd9cf348aac724a4f4dff251800106d721331
LINK: [2] https://syzkaller.appspot.com/text?tag=ReproC&x=14ba0238700000
LINK: [3] https://elixir.bootlin.com/linux/v5.17-rc8/source/fs/ext4/indirect.c#L1244
LINK: [4] https://elixir.bootlin.com/linux/v5.17-rc8/source/fs/ext4/indirect.c#L1234

Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Andreas Dilger <adilger.kernel@dilger.ca>
Cc: Ritesh Harjani <riteshh@linux.ibm.com>
Cc: <linux-ext4@vger.kernel.org>
Cc: <stable@vger.kernel.org>
Cc: <linux-kernel@vger.kernel.org>

Fixes: a4bb6b64e39a ("ext4: enable "punch hole" functionality")
Reported-by: syzbot+7a806094edd5d07ba029@syzkaller.appspotmail.com
Signed-off-by: Tadeusz Struk <tadeusz.struk@linaro.org>
---
 fs/ext4/inode.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 01c9e4f743ba..dd9c35113efe 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3924,7 +3924,8 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
 	struct super_block *sb = inode->i_sb;
 	ext4_lblk_t first_block, stop_block;
 	struct address_space *mapping = inode->i_mapping;
-	loff_t first_block_offset, last_block_offset;
+	loff_t first_block_offset, last_block_offset, max_length;
+	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 	handle_t *handle;
 	unsigned int credits;
 	int ret = 0, ret2 = 0;
@@ -3967,6 +3968,16 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
 		   offset;
 	}
 
+	/*
+	 * For punch hole the length + offset needs to be at least within
+	 * one block before last
+	 */
+	max_length = sbi->s_bitmap_maxbytes - sbi->s_blocksize;
+	if (offset + length >= max_length) {
+		ret = -ENOSPC;
+		goto out_mutex;
+	}
+
 	if (offset & (sb->s_blocksize - 1) ||
 	    (offset + length) & (sb->s_blocksize - 1)) {
 		/*
-- 
2.35.1

Re: [PATCH] ext4: check if offset+length is within a valid range in fallocate

[Tadeusz Struk]

On 3/15/22 12:15, Tadeusz Struk wrote:
> @@ -3967,6 +3968,16 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
>   		   offset;
>   	}
>   
> +	/*
> +	 * For punch hole the length + offset needs to be at least within
> +	 * one block before last
> +	 */
> +	max_length = sbi->s_bitmap_maxbytes - sbi->s_blocksize;

that supposed to be:
max_length = sbi->s_bitmap_maxbytes - inode->i_sb->s_blocksize;

Please ignore this one. I will send a new version soon.
Sorry for the noise.

> +	if (offset + length >= max_length) {
> +		ret = -ENOSPC;
> +		goto out_mutex;
> +	}
> +


-- 
Thanks,
Tadeusz

Re: [PATCH] ext4: check if offset+length is within a valid range in fallocate

[kernel test robot]

Hi Tadeusz,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tytso-ext4/dev]
[also build test ERROR on linus/master v5.17-rc8 next-20220315]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Tadeusz-Struk/ext4-check-if-offset-length-is-within-a-valid-range-in-fallocate/20220316-031841
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
config: mips-cu1830-neo_defconfig (https://download.01.org/0day-ci/archive/20220316/202203160955.fFhAfodc-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a6b2f50fb47da3baeee10b1906da6e30ac5d26ec)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/0day-ci/linux/commit/bc1fdc20f07523e970c9dea4f0fbabbc437fb0d5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Tadeusz-Struk/ext4-check-if-offset-length-is-within-a-valid-range-in-fallocate/20220316-031841
        git checkout bc1fdc20f07523e970c9dea4f0fbabbc437fb0d5
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> fs/ext4/inode.c:4002:45: error: no member named 's_blocksize' in 'struct ext4_sb_info'
           max_length = sbi->s_bitmap_maxbytes - sbi->s_blocksize;
                                                 ~~~  ^
   1 error generated.


vim +4002 fs/ext4/inode.c

  3937	
  3938	/*
  3939	 * ext4_punch_hole: punches a hole in a file by releasing the blocks
  3940	 * associated with the given offset and length
  3941	 *
  3942	 * @inode:  File inode
  3943	 * @offset: The offset where the hole will begin
  3944	 * @len:    The length of the hole
  3945	 *
  3946	 * Returns: 0 on success or negative on failure
  3947	 */
  3948	
  3949	int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
  3950	{
  3951		struct super_block *sb = inode->i_sb;
  3952		ext4_lblk_t first_block, stop_block;
  3953		struct address_space *mapping = inode->i_mapping;
  3954		loff_t first_block_offset, last_block_offset, max_length;
  3955		struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
  3956		handle_t *handle;
  3957		unsigned int credits;
  3958		int ret = 0, ret2 = 0;
  3959	
  3960		trace_ext4_punch_hole(inode, offset, length, 0);
  3961	
  3962		ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
  3963		if (ext4_has_inline_data(inode)) {
  3964			filemap_invalidate_lock(mapping);
  3965			ret = ext4_convert_inline_data(inode);
  3966			filemap_invalidate_unlock(mapping);
  3967			if (ret)
  3968				return ret;
  3969		}
  3970	
  3971		/*
  3972		 * Write out all dirty pages to avoid race conditions
  3973		 * Then release them.
  3974		 */
  3975		if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
  3976			ret = filemap_write_and_wait_range(mapping, offset,
  3977							   offset + length - 1);
  3978			if (ret)
  3979				return ret;
  3980		}
  3981	
  3982		inode_lock(inode);
  3983	
  3984		/* No need to punch hole beyond i_size */
  3985		if (offset >= inode->i_size)
  3986			goto out_mutex;
  3987	
  3988		/*
  3989		 * If the hole extends beyond i_size, set the hole
  3990		 * to end after the page that contains i_size
  3991		 */
  3992		if (offset + length > inode->i_size) {
  3993			length = inode->i_size +
  3994			   PAGE_SIZE - (inode->i_size & (PAGE_SIZE - 1)) -
  3995			   offset;
  3996		}
  3997	
  3998		/*
  3999		 * For punch hole the length + offset needs to be at least within
  4000		 * one block before last
  4001		 */
> 4002		max_length = sbi->s_bitmap_maxbytes - sbi->s_blocksize;
  4003		if (offset + length >= max_length) {
  4004			ret = -ENOSPC;
  4005			goto out_mutex;
  4006		}
  4007	
  4008		if (offset & (sb->s_blocksize - 1) ||
  4009		    (offset + length) & (sb->s_blocksize - 1)) {
  4010			/*
  4011			 * Attach jinode to inode for jbd2 if we do any zeroing of
  4012			 * partial block
  4013			 */
  4014			ret = ext4_inode_attach_jinode(inode);
  4015			if (ret < 0)
  4016				goto out_mutex;
  4017	
  4018		}
  4019	
  4020		/* Wait all existing dio workers, newcomers will block on i_rwsem */
  4021		inode_dio_wait(inode);
  4022	
  4023		/*
  4024		 * Prevent page faults from reinstantiating pages we have released from
  4025		 * page cache.
  4026		 */
  4027		filemap_invalidate_lock(mapping);
  4028	
  4029		ret = ext4_break_layouts(inode);
  4030		if (ret)
  4031			goto out_dio;
  4032	
  4033		first_block_offset = round_up(offset, sb->s_blocksize);
  4034		last_block_offset = round_down((offset + length), sb->s_blocksize) - 1;
  4035	
  4036		/* Now release the pages and zero block aligned part of pages*/
  4037		if (last_block_offset > first_block_offset) {
  4038			ret = ext4_update_disksize_before_punch(inode, offset, length);
  4039			if (ret)
  4040				goto out_dio;
  4041			truncate_pagecache_range(inode, first_block_offset,
  4042						 last_block_offset);
  4043		}
  4044	
  4045		if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
  4046			credits = ext4_writepage_trans_blocks(inode);
  4047		else
  4048			credits = ext4_blocks_for_truncate(inode);
  4049		handle = ext4_journal_start(inode, EXT4_HT_TRUNCATE, credits);
  4050		if (IS_ERR(handle)) {
  4051			ret = PTR_ERR(handle);
  4052			ext4_std_error(sb, ret);
  4053			goto out_dio;
  4054		}
  4055	
  4056		ret = ext4_zero_partial_blocks(handle, inode, offset,
  4057					       length);
  4058		if (ret)
  4059			goto out_stop;
  4060	
  4061		first_block = (offset + sb->s_blocksize - 1) >>
  4062			EXT4_BLOCK_SIZE_BITS(sb);
  4063		stop_block = (offset + length) >> EXT4_BLOCK_SIZE_BITS(sb);
  4064	
  4065		/* If there are blocks to remove, do it */
  4066		if (stop_block > first_block) {
  4067	
  4068			down_write(&EXT4_I(inode)->i_data_sem);
  4069			ext4_discard_preallocations(inode, 0);
  4070	
  4071			ret = ext4_es_remove_extent(inode, first_block,
  4072						    stop_block - first_block);
  4073			if (ret) {
  4074				up_write(&EXT4_I(inode)->i_data_sem);
  4075				goto out_stop;
  4076			}
  4077	
  4078			if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
  4079				ret = ext4_ext_remove_space(inode, first_block,
  4080							    stop_block - 1);
  4081			else
  4082				ret = ext4_ind_remove_space(handle, inode, first_block,
  4083							    stop_block);
  4084	
  4085			up_write(&EXT4_I(inode)->i_data_sem);
  4086		}
  4087		ext4_fc_track_range(handle, inode, first_block, stop_block);
  4088		if (IS_SYNC(inode))
  4089			ext4_handle_sync(handle);
  4090	
  4091		inode->i_mtime = inode->i_ctime = current_time(inode);
  4092		ret2 = ext4_mark_inode_dirty(handle, inode);
  4093		if (unlikely(ret2))
  4094			ret = ret2;
  4095		if (ret >= 0)
  4096			ext4_update_inode_fsync_trans(handle, inode, 1);
  4097	out_stop:
  4098		ext4_journal_stop(handle);
  4099	out_dio:
  4100		filemap_invalidate_unlock(mapping);
  4101	out_mutex:
  4102		inode_unlock(inode);
  4103		return ret;
  4104	}
  4105	

---
0-DAY CI Kernel Test Service
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH] ext4: check if offset+length is within a valid range in fallocate

[kernel test robot]

Hi Tadeusz,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tytso-ext4/dev]
[also build test ERROR on linus/master v5.17-rc8 next-20220315]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Tadeusz-Struk/ext4-check-if-offset-length-is-within-a-valid-range-in-fallocate/20220316-031841
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
config: arc-randconfig-r043-20220313 (https://download.01.org/0day-ci/archive/20220316/202203160919.MtfBk5N0-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/bc1fdc20f07523e970c9dea4f0fbabbc437fb0d5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Tadeusz-Struk/ext4-check-if-offset-length-is-within-a-valid-range-in-fallocate/20220316-031841
        git checkout bc1fdc20f07523e970c9dea4f0fbabbc437fb0d5
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash fs/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   fs/ext4/inode.c: In function 'ext4_punch_hole':
>> fs/ext4/inode.c:4002:50: error: 'struct ext4_sb_info' has no member named 's_blocksize'
    4002 |         max_length = sbi->s_bitmap_maxbytes - sbi->s_blocksize;
         |                                                  ^~


vim +4002 fs/ext4/inode.c

  3937	
  3938	/*
  3939	 * ext4_punch_hole: punches a hole in a file by releasing the blocks
  3940	 * associated with the given offset and length
  3941	 *
  3942	 * @inode:  File inode
  3943	 * @offset: The offset where the hole will begin
  3944	 * @len:    The length of the hole
  3945	 *
  3946	 * Returns: 0 on success or negative on failure
  3947	 */
  3948	
  3949	int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
  3950	{
  3951		struct super_block *sb = inode->i_sb;
  3952		ext4_lblk_t first_block, stop_block;
  3953		struct address_space *mapping = inode->i_mapping;
  3954		loff_t first_block_offset, last_block_offset, max_length;
  3955		struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
  3956		handle_t *handle;
  3957		unsigned int credits;
  3958		int ret = 0, ret2 = 0;
  3959	
  3960		trace_ext4_punch_hole(inode, offset, length, 0);
  3961	
  3962		ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
  3963		if (ext4_has_inline_data(inode)) {
  3964			filemap_invalidate_lock(mapping);
  3965			ret = ext4_convert_inline_data(inode);
  3966			filemap_invalidate_unlock(mapping);
  3967			if (ret)
  3968				return ret;
  3969		}
  3970	
  3971		/*
  3972		 * Write out all dirty pages to avoid race conditions
  3973		 * Then release them.
  3974		 */
  3975		if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
  3976			ret = filemap_write_and_wait_range(mapping, offset,
  3977							   offset + length - 1);
  3978			if (ret)
  3979				return ret;
  3980		}
  3981	
  3982		inode_lock(inode);
  3983	
  3984		/* No need to punch hole beyond i_size */
  3985		if (offset >= inode->i_size)
  3986			goto out_mutex;
  3987	
  3988		/*
  3989		 * If the hole extends beyond i_size, set the hole
  3990		 * to end after the page that contains i_size
  3991		 */
  3992		if (offset + length > inode->i_size) {
  3993			length = inode->i_size +
  3994			   PAGE_SIZE - (inode->i_size & (PAGE_SIZE - 1)) -
  3995			   offset;
  3996		}
  3997	
  3998		/*
  3999		 * For punch hole the length + offset needs to be at least within
  4000		 * one block before last
  4001		 */
> 4002		max_length = sbi->s_bitmap_maxbytes - sbi->s_blocksize;
  4003		if (offset + length >= max_length) {
  4004			ret = -ENOSPC;
  4005			goto out_mutex;
  4006		}
  4007	
  4008		if (offset & (sb->s_blocksize - 1) ||
  4009		    (offset + length) & (sb->s_blocksize - 1)) {
  4010			/*
  4011			 * Attach jinode to inode for jbd2 if we do any zeroing of
  4012			 * partial block
  4013			 */
  4014			ret = ext4_inode_attach_jinode(inode);
  4015			if (ret < 0)
  4016				goto out_mutex;
  4017	
  4018		}
  4019	
  4020		/* Wait all existing dio workers, newcomers will block on i_rwsem */
  4021		inode_dio_wait(inode);
  4022	
  4023		/*
  4024		 * Prevent page faults from reinstantiating pages we have released from
  4025		 * page cache.
  4026		 */
  4027		filemap_invalidate_lock(mapping);
  4028	
  4029		ret = ext4_break_layouts(inode);
  4030		if (ret)
  4031			goto out_dio;
  4032	
  4033		first_block_offset = round_up(offset, sb->s_blocksize);
  4034		last_block_offset = round_down((offset + length), sb->s_blocksize) - 1;
  4035	
  4036		/* Now release the pages and zero block aligned part of pages*/
  4037		if (last_block_offset > first_block_offset) {
  4038			ret = ext4_update_disksize_before_punch(inode, offset, length);
  4039			if (ret)
  4040				goto out_dio;
  4041			truncate_pagecache_range(inode, first_block_offset,
  4042						 last_block_offset);
  4043		}
  4044	
  4045		if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
  4046			credits = ext4_writepage_trans_blocks(inode);
  4047		else
  4048			credits = ext4_blocks_for_truncate(inode);
  4049		handle = ext4_journal_start(inode, EXT4_HT_TRUNCATE, credits);
  4050		if (IS_ERR(handle)) {
  4051			ret = PTR_ERR(handle);
  4052			ext4_std_error(sb, ret);
  4053			goto out_dio;
  4054		}
  4055	
  4056		ret = ext4_zero_partial_blocks(handle, inode, offset,
  4057					       length);
  4058		if (ret)
  4059			goto out_stop;
  4060	
  4061		first_block = (offset + sb->s_blocksize - 1) >>
  4062			EXT4_BLOCK_SIZE_BITS(sb);
  4063		stop_block = (offset + length) >> EXT4_BLOCK_SIZE_BITS(sb);
  4064	
  4065		/* If there are blocks to remove, do it */
  4066		if (stop_block > first_block) {
  4067	
  4068			down_write(&EXT4_I(inode)->i_data_sem);
  4069			ext4_discard_preallocations(inode, 0);
  4070	
  4071			ret = ext4_es_remove_extent(inode, first_block,
  4072						    stop_block - first_block);
  4073			if (ret) {
  4074				up_write(&EXT4_I(inode)->i_data_sem);
  4075				goto out_stop;
  4076			}
  4077	
  4078			if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
  4079				ret = ext4_ext_remove_space(inode, first_block,
  4080							    stop_block - 1);
  4081			else
  4082				ret = ext4_ind_remove_space(handle, inode, first_block,
  4083							    stop_block);
  4084	
  4085			up_write(&EXT4_I(inode)->i_data_sem);
  4086		}
  4087		ext4_fc_track_range(handle, inode, first_block, stop_block);
  4088		if (IS_SYNC(inode))
  4089			ext4_handle_sync(handle);
  4090	
  4091		inode->i_mtime = inode->i_ctime = current_time(inode);
  4092		ret2 = ext4_mark_inode_dirty(handle, inode);
  4093		if (unlikely(ret2))
  4094			ret = ret2;
  4095		if (ret >= 0)
  4096			ext4_update_inode_fsync_trans(handle, inode, 1);
  4097	out_stop:
  4098		ext4_journal_stop(handle);
  4099	out_dio:
  4100		filemap_invalidate_unlock(mapping);
  4101	out_mutex:
  4102		inode_unlock(inode);
  4103		return ret;
  4104	}
  4105	

---
0-DAY CI Kernel Test Service
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH] ext4: check if offset+length is within a valid range in fallocate

[Tadeusz Struk]

On 3/15/22 18:22, kernel test robot wrote:
> Hi Tadeusz,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on tytso-ext4/dev]
> [also build test ERROR on linus/master v5.17-rc8 next-20220315]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:https://github.com/0day-ci/linux/commits/Tadeusz-Struk/ext4-check-if-offset-length-is-within-a-valid-range-in-fallocate/20220316-031841
> base:https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git  dev
> config: arc-randconfig-r043-20220313 (https://download.01.org/0day-ci/archive/20220316/202203160919.MtfBk5N0-lkp@intel.com/config)
> compiler: arc-elf-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
>          wgethttps://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross  -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          #https://github.com/0day-ci/linux/commit/bc1fdc20f07523e970c9dea4f0fbabbc437fb0d5
>          git remote add linux-reviewhttps://github.com/0day-ci/linux
>          git fetch --no-tags linux-review Tadeusz-Struk/ext4-check-if-offset-length-is-within-a-valid-range-in-fallocate/20220316-031841
>          git checkout bc1fdc20f07523e970c9dea4f0fbabbc437fb0d5
>          # save the config file to linux build tree
>          mkdir build_dir
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash fs/
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot<lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>     fs/ext4/inode.c: In function 'ext4_punch_hole':
>>> fs/ext4/inode.c:4002:50: error: 'struct ext4_sb_info' has no member named 's_blocksize'
>      4002 |         max_length = sbi->s_bitmap_maxbytes - sbi->s_blocksize;
>           |                                                  ^~

Thanks for report, but I've already sent v2:
https://lore.kernel.org/linux-ext4/20220315215439.269122-1-tadeusz.struk@linaro.org

-- 
Thanks,
Tadeusz