Re: btrfs zero divide

[Geert Uytterhoeven]

On Tue, Aug 13, 2013 at 6:32 PM, Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
> On Fri, 9 Aug 2013, Zach Brown wrote:
>> On Fri, Aug 09, 2013 at 02:26:36PM +0200, Andreas Schwab wrote:
>> > Josef Bacik <jbacik@xxxxxxxxxxxx> writes:
>> >
>> > > So stripe_len shouldn't be 0, if it is you have bigger problems :).
>> >
>> > The bigger problem is that stripe_nr is u64, this is completely bogus.
>> > The first operand of do_div must be u32.  This goes through the whole
>> > file.
>
> This was introduced by commit 53b381b3abeb86f12787a6c40fee9b2f71edc23b
> ("Btrfs: RAID5 and RAID6"), which changed the divisor from
> map->stripe_len (struct map_lookup.stripe_len is int) to a 64-bit
> temporary.
>
>> Definitely.  Can we get some typeof() tricks in the macros to have the
>> build fail if (when, evidently) someone gets it wrong?
>
> Not using typeof, as there are way too many callsites where int is used
> instead of u32.
>
> However, checking that sizeof() equals to 4 seems to work.
> Below is a patch for asm-generic, which is untested, but it works when
> adding the same checks to arch/m68k/include/asm/div64.h
>
> This is not something we just want to drop in, as it has the potential of
> breaking lots of things (yes, it breaks btrfs :-)

Found so far:
  - Several calls to sector_div() in blkdev_issue_discard()
  - Two calls to do_div() in sd_completed_bytes()

Some of these even operate on dividends that never exceed 32-bit, tss...

> >From 7ccabf41beae38da514f3e09624219a9362375d7 Mon Sep 17 00:00:00 2001
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Date: Tue, 13 Aug 2013 18:04:40 +0200
> Subject: [PATCH] asm-generic: Check divisor size in do_div()
>
> The second parameter of do_div() should be a 32-bit number.
> Enforce this using BUILD_BUG_ON().
>
> The first parameter of do_div() should be a 64-bit number,
> enforce this on 64-bit architectures, just like is done on 32-bit
> architectures.
>
> Signed-off-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> ---
>  include/asm-generic/div64.h |    4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/include/asm-generic/div64.h b/include/asm-generic/div64.h
> index 8f4e319..69c0307 100644
> --- a/include/asm-generic/div64.h
> +++ b/include/asm-generic/div64.h
> @@ -19,12 +19,15 @@
>
>  #include <linux/types.h>
>  #include <linux/compiler.h>
> +#include <linux/bug.h>
>
>  #if BITS_PER_LONG == 64
>
>  # define do_div(n,base) ({                                     \
>         uint32_t __base = (base);                               \
>         uint32_t __rem;                                         \
> +       (void)(((typeof((n)) *)0) == ((uint64_t *)0));          \
> +       BUILD_BUG_ON(sizeof(base) != 4);                        \
>         __rem = ((uint64_t)(n)) % __base;                       \
>         (n) = ((uint64_t)(n)) / __base;                         \
>         __rem;                                                  \
> @@ -41,6 +44,7 @@ extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
>         uint32_t __base = (base);                       \
>         uint32_t __rem;                                 \
>         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
> +       BUILD_BUG_ON(sizeof(base) != 4);                \
>         if (likely(((n) >> 32) == 0)) {                 \
>                 __rem = (uint32_t)(n) % __base;         \
>                 (n) = (uint32_t)(n) / __base;           \
> --
> 1.7.9.5

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/