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.
Signed-off-by: Nur Hussein <[email protected]>
---
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
--
2.34.1
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 <[email protected]>
> ---
> 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
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.
Perhaps so, but it was flagged by Coverity, and it's a little safer with
the cast, with no cost. It's up to y'all if you want it though.
- Nur
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.