linux-ext4 - Re: [PATCH] ext4: fix check to prevent false positive report of incorrect used inodes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5d6e4215-4d42-6621-1004-517caf3d3ebf@huawei.com>
Date:   Wed, 31 Mar 2021 11:12:58 +0800
From:   Zhang Yi <yi.zhang@...wei.com>
To:     Jan Kara <jack@...e.cz>
CC:     <linux-ext4@...r.kernel.org>, <tytso@....edu>,
        <adilger.kernel@...ger.ca>
Subject: Re: [PATCH] ext4: fix check to prevent false positive report of
 incorrect used inodes

On 2021/3/29 22:26, Jan Kara wrote:
> On Mon 29-03-21 14:19:55, Zhang Yi wrote:
>> Commit <50122847007> ("ext4: fix check to prevent initializing reserved
>> inodes") check the block group zero and prevent initializing reserved
>> inodes. But in some special cases, the reserved inode may not all belong
>> to the group zero, it may exist into the second group if we format
>> filesystem below.
>>
>>   mkfs.ext4 -b 4096 -g 8192 -N 1024 -I 4096 /dev/sda
>>
>> So, it will end up triggering a false positive report of a corrupted
>> file system. This patch fix it by avoid check reserved inodes if no free
>> inode blocks will be zeroed.
>>
>> Fixes: 50122847007 ("ext4: fix check to prevent initializing reserved inodes")
>> Signed-off-by: Zhang Yi <yi.zhang@...wei.com>
> 
> Thanks! The patch looks correct but maybe the code can be made more
> comprehensible like I suggest below?
> 
>> @@ -1543,22 +1544,25 @@ int ext4_init_inode_table(struct super_block *sb, ext4_group_t group,
>>  	 * used inodes so we need to skip blocks with used inodes in
>>  	 * inode table.
>>  	 */
>> -	if (!(gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_UNINIT)))
>> -		used_blks = DIV_ROUND_UP((EXT4_INODES_PER_GROUP(sb) -
>> -			    ext4_itable_unused_count(sb, gdp)),
>> -			    sbi->s_inodes_per_block);
>> -
>> -	if ((used_blks < 0) || (used_blks > sbi->s_itb_per_group) ||
>> -	    ((group == 0) && ((EXT4_INODES_PER_GROUP(sb) -
>> -			       ext4_itable_unused_count(sb, gdp)) <
>> -			      EXT4_FIRST_INO(sb)))) {
>> -		ext4_error(sb, "Something is wrong with group %u: "
>> -			   "used itable blocks: %d; "
>> -			   "itable unused count: %u",
>> -			   group, used_blks,
>> -			   ext4_itable_unused_count(sb, gdp));
>> -		ret = 1;
>> -		goto err_out;
>> +	if (!(gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_UNINIT))) {
>> +		used_inos = EXT4_INODES_PER_GROUP(sb) -
>> +			    ext4_itable_unused_count(sb, gdp);
>> +		used_blks = DIV_ROUND_UP(used_inos, sbi->s_inodes_per_block);
>> +
>> +		if (used_blks >= 0 && used_blks <= sbi->s_itb_per_group)
>> +			used_inos += group * EXT4_INODES_PER_GROUP(sb);
> 
> Maybe if would be more comprehensible like:
> 
> 		/* Bogus inode unused count? */
> 		if (used_blks < 0 || used_blks > sbi->s_itb_per_group) {
> 			ext4_error(...);
> 			ret = 1;
> 			goto err_out;
> 		}
> 
> 		used_inos += EXT4_INODES_PER_GROUP(sb);
> 		/*
> 		 * Are there some uninitialized inodes in the inode table
> 		 * before the first normal inode?
> 		 */
> 		if (used_blks != sbi->s_itb_per_group &&
> 		    used_inos < EXT4_FIRST_INO(sb)) {
> 			ext4_error(...);
> 			ret = 1;
> 			goto err_out;
> 		}

Yes, it looks more comprehensible, I will send v2 as you suggested.

Thanks,
Yi.