| Message ID | ca680432cb92db7b57345fbd919e47032a78edf5.1687242592.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix a u32 overflow when writing into RAID56 chunks | expand |
Please discard this patch. There are some other u32 stripe_nr << BTRFS_STRIPE_LEN situations which also need to be fixed. Would send out an update to address them all. Thanks, Qu On 2023/6/20 14:37, Qu Wenruo wrote: > [BUG] > David reported an ASSERT() get triggered during certain fio load. > > The ASSERT() is from rbio_add_bio() of raid56.c: > > ASSERT(orig_logical >= full_stripe_start && > orig_logical + orig_len <= full_stripe_start + > rbio->nr_data * BTRFS_STRIPE_LEN); > > Which is checking if the target rbio is crossing the full stripe > boundary. > > [CAUSE] > Commit a97699d1d610 ("btrfs: replace map_lookup->stripe_len by > BTRFS_STRIPE_LEN") changes how we calculate the map length, to reduce > u64 division. > > Function btrfs_max_io_len() is to get the length to the stripe boundary. > > It calculates the full stripe start offset (inside the chunk) by the > following command: > > *full_stripe_start = > rounddown(*stripe_nr, nr_data_stripes(map)) << > BTRFS_STRIPE_LEN_SHIFT; > > The calculation itself is fine, but the value returned by rounddown() is > dependent on both @stripe_nr (which is u32) and nr_data_stripes() (which > returned int). > > Thus the result is also u32, then we do the left shift, which can > overflow u32. > > If such overflow happens, @full_stripe_start will be a value way smaller > than @offset, causing later "full_stripe_len - (offset - > *full_stripe_start)" to underflow, thus make later length calculation to > have no stripe boundary limit, resulting a write bio to exceed stripe > boundary. > > [FIX] > Convert the result of rounddown() to u64 before the left shift. > > Reported-by: David Sterba <dsterba@suse.com> > Fixes: a97699d1d610 ("btrfs: replace map_lookup->stripe_len by BTRFS_STRIPE_LEN") > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/volumes.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index b8540af6e136..b9cd41ac9d5e 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -6199,15 +6199,17 @@ static u64 btrfs_max_io_len(struct map_lookup *map, enum btrfs_map_op op, > * not ensured to be power of 2. > */ > *full_stripe_start = > - rounddown(*stripe_nr, nr_data_stripes(map)) << > + (u64)rounddown(*stripe_nr, nr_data_stripes(map)) << > BTRFS_STRIPE_LEN_SHIFT; > > /* > * For writes to RAID56, allow to write a full stripe set, but > * no straddling of stripe sets. > */ > - if (op == BTRFS_MAP_WRITE) > + if (op == BTRFS_MAP_WRITE) { > + ASSERT(*full_stripe_start + full_stripe_len > offset); > return full_stripe_len - (offset - *full_stripe_start); > + } > } > > /*
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index b8540af6e136..b9cd41ac9d5e 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -6199,15 +6199,17 @@ static u64 btrfs_max_io_len(struct map_lookup *map, enum btrfs_map_op op, * not ensured to be power of 2. */ *full_stripe_start = - rounddown(*stripe_nr, nr_data_stripes(map)) << + (u64)rounddown(*stripe_nr, nr_data_stripes(map)) << BTRFS_STRIPE_LEN_SHIFT; /* * For writes to RAID56, allow to write a full stripe set, but * no straddling of stripe sets. */ - if (op == BTRFS_MAP_WRITE) + if (op == BTRFS_MAP_WRITE) { + ASSERT(*full_stripe_start + full_stripe_len > offset); return full_stripe_len - (offset - *full_stripe_start); + } } /*
[BUG] David reported an ASSERT() get triggered during certain fio load. The ASSERT() is from rbio_add_bio() of raid56.c: ASSERT(orig_logical >= full_stripe_start && orig_logical + orig_len <= full_stripe_start + rbio->nr_data * BTRFS_STRIPE_LEN); Which is checking if the target rbio is crossing the full stripe boundary. [CAUSE] Commit a97699d1d610 ("btrfs: replace map_lookup->stripe_len by BTRFS_STRIPE_LEN") changes how we calculate the map length, to reduce u64 division. Function btrfs_max_io_len() is to get the length to the stripe boundary. It calculates the full stripe start offset (inside the chunk) by the following command: *full_stripe_start = rounddown(*stripe_nr, nr_data_stripes(map)) << BTRFS_STRIPE_LEN_SHIFT; The calculation itself is fine, but the value returned by rounddown() is dependent on both @stripe_nr (which is u32) and nr_data_stripes() (which returned int). Thus the result is also u32, then we do the left shift, which can overflow u32. If such overflow happens, @full_stripe_start will be a value way smaller than @offset, causing later "full_stripe_len - (offset - *full_stripe_start)" to underflow, thus make later length calculation to have no stripe boundary limit, resulting a write bio to exceed stripe boundary. [FIX] Convert the result of rounddown() to u64 before the left shift. Reported-by: David Sterba <dsterba@suse.com> Fixes: a97699d1d610 ("btrfs: replace map_lookup->stripe_len by BTRFS_STRIPE_LEN") Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/volumes.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)