Subject: Re: [PATCH] ext2: add check for blocksize in ext2_fill_super
To:     Jan Kara <jack@suse.cz>
CC:     <linux-ext4@vger.kernel.org>, <miaoxie@huawei.com>,
        <houtao1@huawei.com>, <yi.zhang@huawei.com>
References: <20190110053145.138518-1-yangerkun@huawei.com>
 <422fbc79-12a1-62d4-ee4a-0bc9b652cd89@huawei.com>
 <20190122111700.GD13149@quack2.suse.cz>
From:   yangerkun <yangerkun@huawei.com>
Message-ID: <14606c49-e010-e624-0009-a2d95d7a8ef7@huawei.com>
Date:   Tue, 22 Jan 2019 20:29:39 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.4.0
MIME-Version: 1.0
In-Reply-To: <20190122111700.GD13149@quack2.suse.cz>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk


Jan Kara wrote on 2019/1/22 19:17:
> Hello!
> 
> On Tue 15-01-19 20:46:01, yangerkun wrote:
>> Could you please help me to check this patch?
>>
>> Besides, i have notice that EXT2_MAX_BLOCK_SIZE has never been used before.
>> So there maybe someone using ext2fs which blocksize exceed
>> EXT2_MAX_BLOCK_SIZE in the os that page size exceed 4K? And if it's true,
>> this patch will lead to a unused ext2fs.
>>
>> Also, we can fix the problem by modify ext2_max_size, but it's difficult to
>> get the accurate result, it may not a good idea too...
> 
> Thanks for your report and the patch! Ext2 is supposed to be usable for
> block sizes between 1k and min(PAGE_SIZE, 64k). So rather than limiting
> block size to 4k, we should fix ext2_max_size(). Will you send a new patch
> or should I fix the problem? I agree it is difficult to come up with the
> exact limit for a particular block size. But the computation does not have
> to be exact - even the current one is not. It just has to be approximately
> right and not allow any overflow. And BTW ext4 seems to have the same
> overflow problem in its ext4_max_bitmap_size().
> 
Hi,

Thanks for your reply!

I am working on this problem now, and the patch will coming soon.

Thanks,
Kun.

> 								Honza
> 
>> yangerkun wrote on 2019/1/10 13:31:
>>> While mkfs with '-b 65536' and then mount this fs with arm 64KB page
>>> size, function mount_fs will trigger WARNING because that ext2_max_size
>>> will return value less than 0. Also, we cannot write any file in this
>>> fs since the sb->s_maxbytes is less than 0. Avoid this by check blocksize
>>> in ext2_fill_super like ext4_fill_super.
>>>
>>> Signed-off-by: yangerkun <yangerkun@huawei.com>
>>> ---
>>>    fs/ext2/ext2.h  |  1 +
>>>    fs/ext2/super.c | 15 +++++++++++++++
>>>    2 files changed, 16 insertions(+)
>>>
>>> diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
>>> index e770cd1..ba95316 100644
>>> --- a/fs/ext2/ext2.h
>>> +++ b/fs/ext2/ext2.h
>>> @@ -177,6 +177,7 @@ static inline struct ext2_sb_info *EXT2_SB(struct super_block *sb)
>>>    #define EXT2_MIN_BLOCK_SIZE		1024
>>>    #define	EXT2_MAX_BLOCK_SIZE		4096
>>>    #define EXT2_MIN_BLOCK_LOG_SIZE		  10
>>> +#define EXT2_MAX_BLOCK_LOG_SIZE		  12
>>>    #define EXT2_BLOCK_SIZE(s)		((s)->s_blocksize)
>>>    #define	EXT2_ADDR_PER_BLOCK(s)		(EXT2_BLOCK_SIZE(s) / sizeof (__u32))
>>>    #define EXT2_BLOCK_SIZE_BITS(s)		((s)->s_blocksize_bits)
>>> diff --git a/fs/ext2/super.c b/fs/ext2/super.c
>>> index 73b2d52..029945e 100644
>>> --- a/fs/ext2/super.c
>>> +++ b/fs/ext2/super.c
>>> @@ -960,6 +960,21 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
>>>    	}
>>>    	blocksize = BLOCK_SIZE << le32_to_cpu(sbi->s_es->s_log_block_size);
>>> +	if (blocksize < EXT2_MIN_BLOCK_SIZE ||
>>> +		blocksize > EXT2_MAX_BLOCK_SIZE) {
>>> +		ext2_msg(sb, KERN_ERR,
>>> +				"Unsupported filesystem blocksize %d (%d log_block_size)",
>>> +				blocksize, le32_to_cpu(es->s_log_block_size));
>>> +		goto failed_mount;
>>> +	}
>>> +
>>> +	if (le32_to_cpu(es->s_log_block_size) >
>>> +		(EXT2_MAX_BLOCK_LOG_SIZE - EXT2_MIN_BLOCK_LOG_SIZE)) {
>>> +			ext2_msg(sb, KERN_ERR,
>>> +				"Invalid log block size: %u",
>>> +				le32_to_cpu(es->s_log_block_size));
>>> +			goto failed_mount;
>>> +		}
>>>    	if (sbi->s_mount_opt & EXT2_MOUNT_DAX) {
>>>    		if (!bdev_dax_supported(sb->s_bdev, blocksize)) {
>>>
>>