Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <353b44f3-fb95-ac43-53ba-0d3b45fff574@gmx.com>
Date:   Fri, 7 Apr 2023 08:35:40 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH] btrfs: Avoid potential integer overflow when
 left-shifting 32-bit int
Content-Language: en-US
To:     Nur Hussein <hussein@unixcat.org>, clm@fb.com,
        josef@toxicpanda.com, dsterba@suse.com,
        linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230406192406.2300379-1-hussein@unixcat.org>
From:   Qu Wenruo <quwenruo.btrfs@gmx.com>
In-Reply-To: <20230406192406.2300379-1-hussein@unixcat.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
UI-OutboundReport: notjunk:1;M01:P0:67SDILSAIMQ=;mB37kQs9m1OHlz3IExDdj30CGQX
 MNzG8WyGWeJZu1LXSEfEPj7rN8CmyHFKTvWHi6sfS+ApG+T6NDhI6uyt1ooq1ZpBMuXU1PLcE
 qD3wSxg3R9V7hRRm66dBj8IHO1VNeZbF9gxhXBIQRVZT+GMnJ/lB5lwmPUuxjpgKGGra/3z3Q
 h2Y9X/TayGaeAgrKJ6B+adBCLjviv9GguBHlUEaV85fAkQwZamhlfBsXeEGgANsvOnNvE/UYY
 nGyKQeyOiSQK8bjz8qKB6K3/Ax2Ivxohu5xRF/Vr9ri0aaPHEoRksSqAIoCDKBF8iwbtHO739
 4xXBtZ1kAXphdfKkLYlh/hdAy57I/d6UftJl/w/d+4cw7b47OKxxaQjb9AMwPvmShHlMMGC21
 9v5G0UBt3ZxGxveAVcZRITBoiNEZPQYWico76km2WuM+MtP3wNabHh9wGOfW38+51PIBJqSEK
 3wBzYgVn35U7dPjqGw5TyOyAls+EuhZV6VEljTspiGco7CFLxN36blJcjUs0SSF0EioxRDiMn
 DcmNPk5h5LfLY7m5787QeTRvdBuve2beIki6tIwPWrkWH2XBxHrKIG2WZU3v+GV9gPei6cjo7
 xa4LQnj7xStNEo2Az0eY0CUp1p2pZa9B5mrdswyVJIB8x1LEEnQuVOlsuI85j/V3wiysesBBS
 pc9PC/wmsHqRB0GY4/H3Y//EugaBER/ZhziRPOEpizF2U+dyJ7Yn0gfxhqj9lBh1rxxxpWu7E
 HvPmA2wNCJafMC6p9ChWOk4h/6m1XR6yPyu8nc5Vm+NNeGi1c9sojeln8IYEPwExXs192lDa8
 Zs5yn3k+6+Hd1hCyuPrgg3fG80/BrVzQ5hAzsPRZZlSwRizMRpEVgjII/bIzws+d7jb3oLF9J
 7IhpQ21FDg9doZ9hnGYOdZCDl+3XJU2b/fFGmuG3VXzGEQaMxOSuw1O2mOyxdKG7dnRX2/AKu
 VhBDD6/qi+uK1pDIyzb+WY0y4V0=
Precedence: bulk


On 2023/4/7 03:24, Nur Hussein wrote:
> In scrub_stripe(), the 32-bit signed value returned by the
> nr_data_stripes(map) function call should be cast to u64
> before being shifted left by BTRFS_STRIPE_LEN_SHIFT (16),
> as a cautionary measure to avoid potential overflows. We
> then assign it to a u64 value anyway, so a cast before a
> shift seems prudent.

I'd say it's a little overkilled.

For nr_data_stripes(), it's at most hundreds of stripes (which is 
already insane).
Even with 16 bits left shift, we need to get 2 ** 16 stripes to overflow 
32bits.

Thanks,
Qu
> 
> Signed-off-by: Nur Hussein <hussein@unixcat.org>
> ---
>   fs/btrfs/scrub.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index ccb4f58ae307..4de1665fcd52 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -2187,7 +2187,7 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
>   
>   	/* Initialize @offset in case we need to go to out: label */
>   	get_raid56_logic_offset(physical, stripe_index, map, &offset, NULL);
> -	increment = nr_data_stripes(map) << BTRFS_STRIPE_LEN_SHIFT;
> +	increment = (u64)nr_data_stripes(map) << BTRFS_STRIPE_LEN_SHIFT;
>   
>   	/*
>   	 * Due to the rotation, for RAID56 it's better to iterate each stripe