Re: [PATCH v2 2/3] e2fsprogs/tune2fs: rewrite metadata checksums when resizing inode size

[Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>]

Hi,
On 11/13/2014 06:39 AM, Darrick J. Wong wrote:
> On Wed, Nov 12, 2014 at 05:49:39PM +0800, Xiaoguang Wang wrote:
>> When we use tune2fs -I new_ino_size to change inode size, if everything is OK,
>> the corresponding ext4_group_desc.bg_free_blocks_count will be decreased, so
>> obviously, we need to re-compute the group descriptor checksums, fix this. If
>> not doing this, mount operation will fail.
>>
>> Meanwhile, the patch will trigger an existing memory write overflow, which will
>> casue segfault, please see the next patch.
>>
>> Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
>> ---
>>  misc/tune2fs.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
>> index 065b483..91dc7c1 100644
>> --- a/misc/tune2fs.c
>> +++ b/misc/tune2fs.c
>> @@ -2908,8 +2908,7 @@ retry_open:
>>  				EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
>>  			rewrite_checksums = 1;
>>  	}
>> -	if (rewrite_checksums)
>> -		rewrite_metadata_checksums(fs);
>> +
>>  	if (I_flag) {
>>  		if (mount_flags & EXT2_MF_MOUNTED) {
>>  			fputs(_("The inode size may only be "
>> @@ -2935,6 +2934,7 @@ retry_open:
>>  		if (resize_inode(fs, new_inode_size) == 0) {
>>  			printf(_("Setting inode size %lu\n"),
>>  							new_inode_size);
>> +			rewrite_checksums = 1;
>>  		} else {
>>  			printf("%s", _("Failed to change inode size\n"));
>>  			rc = 1;
>> @@ -2942,6 +2942,9 @@ retry_open:
>>  		}
>>  	}
>>  
>> +	if (rewrite_checksums)
>> +		rewrite_metadata_checksums(fs);
>> +
> 
> Aha!  expand_inode_table() fails to recompute the checksums of the inode blocks
> it's moving around, and happily your change takes care of recomputing the inode
> checksums for a metadata_csum FS.  The changelog for this patch doesn't mention
> this, but it should.
Oh, I had not realized the inode checksum, thanks for pointing this.
So we agree to call a rewrite_metadata_checksums() here, in new version patch, i'll
update the changelog :)

> 
> There ought to be a regression test for this.  Can you send one along, please?
Sure, I spent some time about how to write a test, will send this test soon.

Regards,
Xiaoguang Wang
> 
> I crafted my own test case for the metadata_csum failure while trying to figure
> out what this patchset does, so I'll simply send it out.  Unfortunately, the
> test requires a fix for a bug in the hugefile code that I'll cram onto the
> patchbomb.
> 
> Other than that, you can add for all three patches:
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> --D
> 
>>  	if (l_flag)
>>  		list_super(sb);
>>  	if (stride_set) {
>> -- 
>> 1.8.2.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> .
>