[PATCH] ext4: fix error when itable blocks is greater than s_itb_per_group

[PATCH] ext4: fix error when itable blocks is greater than s_itb_per_group

[Michael Wu]

The following error occurs when mounting the ext4 image made by image
making tool:
"ext4_init_inode_table:1301:comm ext4lazyinit:Something is wrong with group
0: used itable blocks: 491; itable unused count: 0."

Currently all the inodes in block group0 and ext4 image is divided by
s_inodes_per_group. That leads to a hazard: we can't ensure all
s_inodes_per_group are divisible by s_inodes_per_block. For example, when
the s_inodes_per_group (equals to 7851) is divided by s_inodes_per_block
(which is 16), because 7851 is undivisible by 16, we get the wrong result
490, while 491 is expected.

So, we suggest that s_itb_per_group should equal to
DIV_ROUND_UP(s_inodes_per_group, s_inodes_per_block) instead of directly
getting the result from s_inodes_per_group/s_inodes_per_block.

Signed-off-by: Michael Wu <michael@allwinnertech.com>
---
 fs/ext4/super.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 845f2f8aee5f..76cbd638ea10 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4796,8 +4796,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 			 sbi->s_inodes_per_group);
 		goto failed_mount;
 	}
-	sbi->s_itb_per_group = sbi->s_inodes_per_group /
-					sbi->s_inodes_per_block;
+	sbi->s_itb_per_group = DIV_ROUND_UP(sbi->s_inodes_per_group,
+					    sbi->s_inodes_per_block);
 	sbi->s_desc_per_block = blocksize / EXT4_DESC_SIZE(sb);
 	sbi->s_sbh = bh;
 	sbi->s_mount_state = le16_to_cpu(es->s_state) & ~EXT4_FC_REPLAY;
-- 
2.29.0

Re: [PATCH] ext4: fix error when itable blocks is greater than s_itb_per_group

[Lukas Czerner]

On Tue, Aug 02, 2022 at 10:10:29AM +0800, Michael Wu wrote:
> The following error occurs when mounting the ext4 image made by image
> making tool:
> "ext4_init_inode_table:1301:comm ext4lazyinit:Something is wrong with group
> 0: used itable blocks: 491; itable unused count: 0."
> 
> Currently all the inodes in block group0 and ext4 image is divided by
> s_inodes_per_group. That leads to a hazard: we can't ensure all
> s_inodes_per_group are divisible by s_inodes_per_block. For example, when
> the s_inodes_per_group (equals to 7851) is divided by s_inodes_per_block
> (which is 16), because 7851 is undivisible by 16, we get the wrong result
> 490, while 491 is expected.
> 
> So, we suggest that s_itb_per_group should equal to
> DIV_ROUND_UP(s_inodes_per_group, s_inodes_per_block) instead of directly
> getting the result from s_inodes_per_group/s_inodes_per_block.

Hi Michael,

mke2fs is making sure that we completely fill the inote table blocks.
This is a corrupted image and so AFAICT ext4 is doing the right thing
here. There does not seem to be a problem to fix, unless you can somehow
trick mke2fs to make a file system like this.

-Lukas

> 
> Signed-off-by: Michael Wu <michael@allwinnertech.com>
> ---
>  fs/ext4/super.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 845f2f8aee5f..76cbd638ea10 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4796,8 +4796,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  			 sbi->s_inodes_per_group);
>  		goto failed_mount;
>  	}
> -	sbi->s_itb_per_group = sbi->s_inodes_per_group /
> -					sbi->s_inodes_per_block;
> +	sbi->s_itb_per_group = DIV_ROUND_UP(sbi->s_inodes_per_group,
> +					    sbi->s_inodes_per_block);
>  	sbi->s_desc_per_block = blocksize / EXT4_DESC_SIZE(sb);
>  	sbi->s_sbh = bh;
>  	sbi->s_mount_state = le16_to_cpu(es->s_state) & ~EXT4_FC_REPLAY;
> -- 
> 2.29.0
> 

Re: [PATCH] ext4: fix error when itable blocks is greater than s_itb_per_group

[Theodore Ts'o]

On Wed, Aug 03, 2022 at 09:18:59AM +0200, Lukas Czerner wrote:
> 
> Hi Michael,
> 
> mke2fs is making sure that we completely fill the inote table blocks.
> This is a corrupted image and so AFAICT ext4 is doing the right thing
> here. There does not seem to be a problem to fix, unless you can somehow
> trick mke2fs to make a file system like this.

Several years ago, android was shipping a bogus/busted
reimeplementation of mke2fs, reportedly because a certain founder of
Android (cough, Andy Rubin, cough) was alergic to the GPL.  ("The
problem with GPL in embedded systems [such as smartphones and tablets]
is that it's viral...")  This bogus reimplementation would create file
systems where the number of inodes per block group was a multiple of 4
instead of 8.  But, it was under the BSD license, so it was all good!   :-/

This bogus reimplementation of mkfs would, 50% of the time, create
busted file systems which couldn't be fixed, if they got corrupted, by
e2fsck.  This is because e2fsprogs' allocation bitmap code assumes
that you can back the bitarray into a single contiguous memory block
--- and this doesn't work if the number of inodes per block group is
not a multiple of 8.  If the file system got corrupted, the only
recourse was to wipe the user partition and the user would lose any
data that wasn't backed up to the cloud.

This has since been fixed for quite some time, but if there is some
low-end Android manufacturer is using an ancient version of AOSP, this
could be happening even in 2022 --- but that doesn't mean we need to
support such broken file systems.  As far as I'm concerned the only
way to make valid Android ext4 system images is the combination of
mke2fs and e2fsdroid, which is what modern versions of AOSP do.

    	       	     	     	    	     	- Ted

Re: [PATCH] ext4: fix error when itable blocks is greater than s_itb_per_group

[Michael Wu]

On 8/4/2022 10:19 AM, Theodore Ts'o wrote:
> On Wed, Aug 03, 2022 at 09:18:59AM +0200, Lukas Czerner wrote:
>>
>> Hi Michael,
>>
>> mke2fs is making sure that we completely fill the inote table blocks.
>> This is a corrupted image and so AFAICT ext4 is doing the right thing
>> here. There does not seem to be a problem to fix, unless you can somehow
>> trick mke2fs to make a file system like this.
> 
> Several years ago, android was shipping a bogus/busted
> reimeplementation of mke2fs, reportedly because a certain founder of
> Android (cough, Andy Rubin, cough) was alergic to the GPL.  ("The
> problem with GPL in embedded systems [such as smartphones and tablets]
> is that it's viral...")  This bogus reimplementation would create file
> systems where the number of inodes per block group was a multiple of 4
> instead of 8.  But, it was under the BSD license, so it was all good!   :-/
> 
> This bogus reimplementation of mkfs would, 50% of the time, create
> busted file systems which couldn't be fixed, if they got corrupted, by
> e2fsck.  This is because e2fsprogs' allocation bitmap code assumes
> that you can back the bitarray into a single contiguous memory block
> --- and this doesn't work if the number of inodes per block group is
> not a multiple of 8.  If the file system got corrupted, the only
> recourse was to wipe the user partition and the user would lose any
> data that wasn't backed up to the cloud.
> 
> This has since been fixed for quite some time, but if there is some
> low-end Android manufacturer is using an ancient version of AOSP, this
> could be happening even in 2022 --- but that doesn't mean we need to
> support such broken file systems.  As far as I'm concerned the only
> way to make valid Android ext4 system images is the combination of
> mke2fs and e2fsdroid, which is what modern versions of AOSP do.
> 
>      	       	     	     	    	     	- Ted

Dear Ted & Lukas,
Thanks for your clarification. I did several tests, turned outs Ted was 
right. I'm clear now.

-- 
Regards,
Michael Wu