Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 20 Jul 2023 23:18:06 +0530
Message-Id: <87bkg6o4o9.fsf@doe.com>
From:   Ritesh Harjani (IBM) <ritesh.list@gmail.com>
To:     Baokun Li <libaokun1@huawei.com>, linux-ext4@vger.kernel.org
Cc:     tytso@mit.edu, adilger.kernel@dilger.ca, jack@suse.cz,
        ojaswin@linux.ibm.com, linux-kernel@vger.kernel.org,
        yi.zhang@huawei.com, yangerkun@huawei.com, yukuai3@huawei.com,
        libaokun1@huawei.com
Subject: Re: [PATCH 1/4] ext4: add two helper functions fex_end() and pa_end()
In-Reply-To: <20230718131052.283350-2-libaokun1@huawei.com>
Precedence: bulk

Baokun Li <libaokun1@huawei.com> writes:

> When we use lstart + len to calculate the end of free extent or prealloc
> space, it may exceed the maximum value of 4294967295(0xffffffff) supported
> by ext4_lblk_t and cause overflow, which may lead to various problems.
>
> Therefore, we add two helper functions, fex_end() and pa_end(), to limit
> the type of end to loff_t, and also convert lstart to loff_t for
> calculation to avoid overflow.
>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
>  fs/ext4/mballoc.c |  4 ++--
>  fs/ext4/mballoc.h | 13 +++++++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 456150ef6111..eb7f5d35ef96 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -4432,7 +4432,7 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
>  
>  	/* first, let's learn actual file size
>  	 * given current request is allocated */
> -	size = ac->ac_o_ex.fe_logical + EXT4_C2B(sbi, ac->ac_o_ex.fe_len);
> +	size = fex_end(sbi, &ac->ac_o_ex, NULL);
>  	size = size << bsbits;
>  	if (size < i_size_read(ac->ac_inode))
>  		size = i_size_read(ac->ac_inode);
> @@ -5665,7 +5665,7 @@ static void ext4_mb_group_or_file(struct ext4_allocation_context *ac)
>  
>  	group_pa_eligible = sbi->s_mb_group_prealloc > 0;
>  	inode_pa_eligible = true;
> -	size = ac->ac_o_ex.fe_logical + EXT4_C2B(sbi, ac->ac_o_ex.fe_len);
> +	size = fex_end(sbi, &ac->ac_o_ex, NULL);
>  	isize = (i_size_read(ac->ac_inode) + ac->ac_sb->s_blocksize - 1)
>  		>> bsbits;
>  
> diff --git a/fs/ext4/mballoc.h b/fs/ext4/mballoc.h
> index df6b5e7c2274..fe6bf381f30a 100644
> --- a/fs/ext4/mballoc.h
> +++ b/fs/ext4/mballoc.h
> @@ -233,6 +233,19 @@ static inline ext4_fsblk_t ext4_grp_offs_to_block(struct super_block *sb,
>  		(fex->fe_start << EXT4_SB(sb)->s_cluster_bits);
>  }
>  
> +static inline loff_t fex_end(struct ext4_sb_info *sbi,
> +			     struct ext4_free_extent *fex, ext4_grpblk_t *fe_len)
> +{
> +	return (loff_t)fex->fe_logical +
> +		EXT4_C2B(sbi, fe_len ? *fe_len : fex->fe_len);
> +}
> +
> +static inline loff_t pa_end(struct ext4_sb_info *sbi,
> +			    struct ext4_prealloc_space *pa)
> +{
> +	return (loff_t)pa->pa_lstart + EXT4_C2B(sbi, pa->pa_len);
> +}
> +

I didn't find anything else where we could hit any overflow due to
fex->fe_logical + xxx. 
So, looks good to me except these two helper functions.
I think we can directly just typecast those in place. There are many
other places in ext4 code where careful arithmatic of either adding or
bit shifting has been done. These helpers specially fex_end() is
really confusing.

-ritesh