Re: [PATCH] ext2: Handle fiemap on empty files to prevent EINVAL

[Jan Kara]

On Thu 12-06-25 10:28:55, Wei Gao wrote:
> Previously, ext2_fiemap would unconditionally apply "len = min_t(u64, len,
> i_size_read(inode));", When inode->i_size was 0 (for an empty file), this
> would reduce the requested len to 0. Passing len = 0 to iomap_fiemap could
> then result in an -EINVAL error, even for valid queries on empty files.
> The new validation logic directly references ext4_fiemap_check_ranges.
> 
> Link: https://github.com/linux-test-project/ltp/issues/1246
> Signed-off-by: Wei Gao <wegao@xxxxxxxx>

Thanks for the fix. Some comments below:

> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index 30f8201c155f..e5cc61088f21 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -895,10 +895,30 @@ int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>  		u64 start, u64 len)
>  {
>  	int ret;
> +	u64 maxbytes;
>  
>  	inode_lock(inode);
> -	len = min_t(u64, len, i_size_read(inode));

So I agree this 'len' reduction to i_size looks bogus and can be removed.
But why are you adding those maxbytes checks when they are done inside
iomap_fiemap() shorly later?

								Honza


> +	maxbytes = inode->i_sb->s_maxbytes;
> +
> +	if (len == 0) {
> +		ret = -EINVAL;
> +		goto unlock_inode;
> +	}
> +
> +	if (start > maxbytes) {
> +		ret = -EFBIG;
> +		goto unlock_inode;
> +	}
> +
> +	/*
> +	 * Shrink request scope to what the fs can actually handle.
> +	 */
> +	if (len > maxbytes || (maxbytes - len) < start)
> +		len = maxbytes - start;
> +
>  	ret = iomap_fiemap(inode, fieinfo, start, len, &ext2_iomap_ops);
> +
> +unlock_inode:
>  	inode_unlock(inode);
>  
>  	return ret;
> -- 
> 2.49.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR