On 13/09/2021 22.32, Nick Desaulniers wrote:
> commit fad7cd3310db ("nbd: add the check to prevent overflow in
> __nbd_ioctl()") raised an issue from the fallback helpers added in
> commit f0907827a8a9 ("compiler.h: enable builtin overflow checkers and
> add fallback code")
>
> ERROR: modpost: "__divdi3" [drivers/block/nbd.ko] undefined!
>
> As Stephen Rothwell notes:
> The added check_mul_overflow() call is being passed 64 bit values.
> COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW is not set for this build (see
> include/linux/overflow.h).
>
> Specifically, the helpers for checking whether the results of a
> multiplication overflowed (__unsigned_mul_overflow,
> __signed_add_overflow) use the division operator when
> !COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW. This is problematic for 64b
> operands on 32b hosts.
>
> This was fixed upstream by
> commit 76ae847497bc ("Documentation: raise minimum supported version of
> GCC to 5.1")
> which is not suitable to be backported to stable; I didn't have this
> patch ready in time.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Arnd Bergmann <arnd@xxxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Cc: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> Reported-by: Naresh Kamboju <naresh.kamboju@xxxxxxxxxx>
> Reported-by: Nathan Chancellor <nathan@xxxxxxxxxx>
> Reported-by: Stephen Rothwell <sfr@xxxxxxxxxxxxxxxx>
> Suggested-by: Pavel Machek <pavel@xxxxxx>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1438
> Link: https://lore.kernel.org/all/20210909182525.372ee687@xxxxxxxxxxxxxxxx/
> Link: https://lore.kernel.org/lkml/20210910234047.1019925-1-ndesaulniers@xxxxxxxxxx/
> Fixes: f0907827a8a9 ("compiler.h: enable builtin overflow checkers and
> add fallback code")
> Signed-off-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> ---
> This kind of generic meta-programming in C and my lack of experience in
> doing so makes me very uncomfortable (and slightly ashamed) to send
> this. I would appreciate careful review and feedback. I would appreciate
> if Naresh can test this with GCC 4.9, which I don't have handy.
>
> Linus also suggested I look into the use of _Generic; I haven't
> evaluated that approach yet, but I felt like posting this early for
> feedback.
>
> include/linux/math64.h | 35 +++++++++++++++++++++++++++++++++++
> include/linux/overflow.h | 8 ++++----
> 2 files changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/math64.h b/include/linux/math64.h
> index 66deb1fdc2ef..bc9c12c168d0 100644
> --- a/include/linux/math64.h
> +++ b/include/linux/math64.h
> @@ -10,6 +10,9 @@
>
> #define div64_long(x, y) div64_s64((x), (y))
> #define div64_ul(x, y) div64_u64((x), (y))
> +#ifndef is_signed_type
> +#define is_signed_type(type) (((type)(-1)) < (type)1)
> +#endif
>
> /**
> * div_u64_rem - unsigned 64bit divide with 32bit divisor with remainder
> @@ -111,6 +114,15 @@ extern s64 div64_s64(s64 dividend, s64 divisor);
>
> #endif /* BITS_PER_LONG */
Some comments on when and how to use this would be nice (not just build
bugs when used wrong).
> +#define div64_x64(dividend, divisor) ({ \
> + BUILD_BUG_ON_MSG(sizeof(dividend) < sizeof(u64),\
> + "prefer div_x64"); \
> + __builtin_choose_expr( \
> + is_signed_type(typeof(dividend)), \
> + div64_s64(dividend, divisor), \
> + div64_u64(dividend, divisor)); \
> +})
> +
> /**
> * div_u64 - unsigned 64bit divide with 32bit divisor
> * @dividend: unsigned 64bit dividend
> @@ -141,6 +153,29 @@ static inline s64 div_s64(s64 dividend, s32 divisor)
> }
> #endif
>
> +#define div_x64(dividend, divisor) ({ \
> + BUILD_BUG_ON_MSG(sizeof(dividend) > sizeof(u32),\
> + "prefer div64_x64"); \
> + __builtin_choose_expr( \
> + is_signed_type(typeof(dividend)), \
> + div_s64(dividend, divisor), \
> + div_u64(dividend, divisor)); \
> +})
> +
> +#define div_64(dividend, divisor) ({ \
> + BUILD_BUG_ON_MSG(sizeof(dividend) > sizeof(u64), \
> + "128b div unsupported"); \
> + __builtin_choose_expr( \
> + __builtin_types_compatible_p(typeof(dividend), s64) || \
> + __builtin_types_compatible_p(typeof(dividend), u64), \
You can save a bit of typing using __same_type(dividend, s64) - it's a
nice property of typeof() that it's idempotent when applied to a type
name. _Generic would probably also do, but I don't think it would save
that much, if anything, here.
> u32 iter_div_u64_rem(u64 dividend, u32 divisor, u64 *remainder);
>
> #ifndef mul_u32_u32
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index ef74051d5cfe..2ebdf220c184 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -123,8 +123,8 @@ static inline bool __must_check __must_check_overflow(bool overflow)
> (void) (&__a == __d); \
> *__d = __a * __b; \
> __builtin_constant_p(__b) ? \
> - __b > 0 && __a > type_max(typeof(__a)) / __b : \
> - __a > 0 && __b > type_max(typeof(__b)) / __a; \
> + __b > 0 && __a > div_64(type_max(typeof(__a)), __b) : \
> + __a > 0 && __b > div_64(type_max(typeof(__b)), __a); \
> })
>
> /*
> @@ -195,8 +195,8 @@ static inline bool __must_check __must_check_overflow(bool overflow)
> (void) (&__a == &__b); \
> (void) (&__a == __d); \
> *__d = (u64)__a * (u64)__b; \
> - (__b > 0 && (__a > __tmax/__b || __a < __tmin/__b)) || \
> - (__b < (typeof(__b))-1 && (__a > __tmin/__b || __a < __tmax/__b)) || \
> + (__b > 0 && (__a > div_64(__tmax, __b) || __a < div_64(__tmin, __b))) || \
> + (__b < (typeof(__b))-1 && (__a > div_64(__tmin, __b) || __a < div_64(__tmax, __b))) || \
> (__b == (typeof(__b))-1 && __a == __tmin); \
> })
I had actually forgotten what horrors lay hidden in these fallback
macros, I just knew they needed a wooden stake sooner or later. Now you
made me look at this right before bedtime :(
So, I'd sleep a little better if we got the 64 bit tests commented back
in in test_overflow.c, and [assuming that the above would actually make
that file build with gcc 4.9] that patch also backported to 5.10, so we
had some confidence that the whole house of cards is actually solid.
Rasmus
