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;
Date:   Tue, 2 May 2023 18:11:38 +0200
From:   David Sterba <dsterba@suse.cz>
To:     Qu Wenruo <quwenruo.btrfs@gmx.com>
Cc:     Nur Hussein <hussein@unixcat.org>, clm@fb.com,
        josef@toxicpanda.com, dsterba@suse.com,
        linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] btrfs: Avoid potential integer overflow when
 left-shifting 32-bit int
Message-ID: <20230502161138.GK8111@twin.jikos.cz>
Reply-To: dsterba@suse.cz
References: <20230406192406.2300379-1-hussein@unixcat.org>
 <353b44f3-fb95-ac43-53ba-0d3b45fff574@gmx.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <353b44f3-fb95-ac43-53ba-0d3b45fff574@gmx.com>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)
Precedence: bulk

On Fri, Apr 07, 2023 at 08:35:40AM +0800, Qu Wenruo wrote:
> 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.

I don't like adding casts unless really necessary. That the values won't
overflow is what we know because there are other constraints.

In this case I'd rather switch the return type of nr_data_stripes to u32
as the return value from the helper is used for shifts by
BTRFS_STRIPE_LEN_SHIFT in several places.

For the struct map_lookup we should use u32 for all values that simply
count something and there's no semantic value for -1 (like uninitialzed
or invalid). I did a quick grep and the values are assigned from
unsigned types in most if not all cases.