| Message ID | 202008151007.EF679DF@keescook (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] overflow: Add __must_check attribute to check_*() helpers | expand |
On Sat, Aug 15, 2020 at 10:09:24AM -0700, Kees Cook wrote: > Since the destination variable of the check_*_overflow() helpers will > contain a wrapped value on failure, it would be best to make sure callers > really did check the return result of the helper. Adjust the macros to use > a bool-wrapping static inline that is marked with __must_check. This means > the macros can continue to have their type-agnostic behavior while gaining > the function attribute (that cannot be applied directly to macros). > > Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > v2: > - de-generalized __must_check_overflow() from being named "bool" (willy) > - fix comment typos (rasmus) > v1: https://lore.kernel.org/lkml/202008121450.405E4A3@keescook > --- > include/linux/overflow.h | 39 ++++++++++++++++++++++++--------------- > 1 file changed, 24 insertions(+), 15 deletions(-) > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > index 93fcef105061..f1c4e7b56bd9 100644 > --- a/include/linux/overflow.h > +++ b/include/linux/overflow.h > @@ -43,6 +43,16 @@ > #define is_non_negative(a) ((a) > 0 || (a) == 0) > #define is_negative(a) (!(is_non_negative(a))) > > +/* > + * Allows for effectively applying __must_check to a macro so we can have > + * both the type-agnostic benefits of the macros while also being able to > + * enforce that the return value is, in fact, checked. > + */ > +static inline bool __must_check __must_check_overflow(bool overflow) > +{ > + return unlikely(overflow); How does the 'unlikely' hint propagate through return? It is in a static inline so compiler has complete information in order to use it, but I'm curious if it actually does. > +} > + > #ifdef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW > /* > * For simplicity and code hygiene, the fallback code below insists on > @@ -52,32 +62,32 @@ > * alias for __builtin_add_overflow, but add type checks similar to > * below. > */ > -#define check_add_overflow(a, b, d) ({ \ > +#define check_add_overflow(a, b, d) __must_check_overflow(({ \ > typeof(a) __a = (a); \ > typeof(b) __b = (b); \ > typeof(d) __d = (d); \ > (void) (&__a == &__b); \ > (void) (&__a == __d); \ > __builtin_add_overflow(__a, __b, __d); \ > -}) > +})) In case the hint gets dropped, the fix would probably be #define check_add_overflow(a, b, d) unlikely(__must_check_overflow(({ \ typeof(a) __a = (a); \ typeof(b) __b = (b); \ typeof(d) __d = (d); \ (void) (&__a == &__b); \ (void) (&__a == __d); \ __builtin_add_overflow(__a, __b, __d); \ })))
On 17/08/2020 11.08, David Sterba wrote: > On Sat, Aug 15, 2020 at 10:09:24AM -0700, Kees Cook wrote: >> >> +/* >> + * Allows for effectively applying __must_check to a macro so we can have >> + * both the type-agnostic benefits of the macros while also being able to >> + * enforce that the return value is, in fact, checked. >> + */ >> +static inline bool __must_check __must_check_overflow(bool overflow) >> +{ >> + return unlikely(overflow); > > How does the 'unlikely' hint propagate through return? It is in a static > inline so compiler has complete information in order to use it, but I'm > curious if it actually does. I wondered the same thing, but as I noted in a reply in the v1 thread, that pattern is used in kernel/sched/, and the scheduler is a far more critical path than anywhere these might be used, so if it's good enough for kernel/sched/, it should be good enough here. I have no idea how one could write a piece of non-trivial code to see if the hint actually makes a difference. > > In case the hint gets dropped, the fix would probably be > > #define check_add_overflow(a, b, d) unlikely(__must_check_overflow(({ \ > typeof(a) __a = (a); \ > typeof(b) __b = (b); \ > typeof(d) __d = (d); \ > (void) (&__a == &__b); \ > (void) (&__a == __d); \ > __builtin_add_overflow(__a, __b, __d); \ > }))) > Well, maybe, but I'd be a little worried that the !! that unlikely() slabs on its argument may count as a use of that argument, hence nullifying the __must_check which is the main point - the unlikely just being something we can add for free while touching this code. Haven't tested it, though. Rasmus
On Mon, Aug 17, 2020 at 11:08:54AM +0200, David Sterba wrote: > On Sat, Aug 15, 2020 at 10:09:24AM -0700, Kees Cook wrote: > > +static inline bool __must_check __must_check_overflow(bool overflow) > > +{ > > + return unlikely(overflow); > > How does the 'unlikely' hint propagate through return? It is in a static > inline so compiler has complete information in order to use it, but I'm > curious if it actually does. It may not -- it depends on how the compiler decides to deal with it. :) > In case the hint gets dropped, the fix would probably be > > #define check_add_overflow(a, b, d) unlikely(__must_check_overflow(({ \ > typeof(a) __a = (a); \ > typeof(b) __b = (b); \ > typeof(d) __d = (d); \ > (void) (&__a == &__b); \ > (void) (&__a == __d); \ > __builtin_add_overflow(__a, __b, __d); \ > }))) Unfortunately not, as the unlikely() ends up eating the __must_check attribute. :(
On Mon, Aug 17, 2020 at 12:36:51PM -0700, Kees Cook wrote: > On Mon, Aug 17, 2020 at 11:08:54AM +0200, David Sterba wrote: > > On Sat, Aug 15, 2020 at 10:09:24AM -0700, Kees Cook wrote: > > > +static inline bool __must_check __must_check_overflow(bool overflow) > > > +{ > > > + return unlikely(overflow); > > > > How does the 'unlikely' hint propagate through return? It is in a static > > inline so compiler has complete information in order to use it, but I'm > > curious if it actually does. > > It may not -- it depends on how the compiler decides to deal with it. :) In theory yes, in practice, the compilers will ignore this macro. And if you success to force compiler to use this macro, it won't give any real performance advantage. We (RDMA) tried very hard to see any performance gain by instrumenting code with likely/unlikely in our performance critical data path both in user space and kernel. The performance results were statistically equal. If you are interested, we had a very intense discussion about it when likely/unlikely can still be usable (hint random input). https://lore.kernel.org/linux-rdma/20200807160956.GO4432@unreal Thanks > > > In case the hint gets dropped, the fix would probably be > > > > #define check_add_overflow(a, b, d) unlikely(__must_check_overflow(({ \ > > typeof(a) __a = (a); \ > > typeof(b) __b = (b); \ > > typeof(d) __d = (d); \ > > (void) (&__a == &__b); \ > > (void) (&__a == __d); \ > > __builtin_add_overflow(__a, __b, __d); \ > > }))) > > Unfortunately not, as the unlikely() ends up eating the __must_check > attribute. :( > > -- > Kees Cook
diff --git a/include/linux/overflow.h b/include/linux/overflow.h index 93fcef105061..f1c4e7b56bd9 100644 --- a/include/linux/overflow.h +++ b/include/linux/overflow.h @@ -43,6 +43,16 @@ #define is_non_negative(a) ((a) > 0 || (a) == 0) #define is_negative(a) (!(is_non_negative(a))) +/* + * Allows for effectively applying __must_check to a macro so we can have + * both the type-agnostic benefits of the macros while also being able to + * enforce that the return value is, in fact, checked. + */ +static inline bool __must_check __must_check_overflow(bool overflow) +{ + return unlikely(overflow); +} + #ifdef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW /* * For simplicity and code hygiene, the fallback code below insists on @@ -52,32 +62,32 @@ * alias for __builtin_add_overflow, but add type checks similar to * below. */ -#define check_add_overflow(a, b, d) ({ \ +#define check_add_overflow(a, b, d) __must_check_overflow(({ \ typeof(a) __a = (a); \ typeof(b) __b = (b); \ typeof(d) __d = (d); \ (void) (&__a == &__b); \ (void) (&__a == __d); \ __builtin_add_overflow(__a, __b, __d); \ -}) +})) -#define check_sub_overflow(a, b, d) ({ \ +#define check_sub_overflow(a, b, d) __must_check_overflow(({ \ typeof(a) __a = (a); \ typeof(b) __b = (b); \ typeof(d) __d = (d); \ (void) (&__a == &__b); \ (void) (&__a == __d); \ __builtin_sub_overflow(__a, __b, __d); \ -}) +})) -#define check_mul_overflow(a, b, d) ({ \ +#define check_mul_overflow(a, b, d) __must_check_overflow(({ \ typeof(a) __a = (a); \ typeof(b) __b = (b); \ typeof(d) __d = (d); \ (void) (&__a == &__b); \ (void) (&__a == __d); \ __builtin_mul_overflow(__a, __b, __d); \ -}) +})) #else @@ -190,21 +200,20 @@ }) -#define check_add_overflow(a, b, d) \ +#define check_add_overflow(a, b, d) __must_check_overflow( \ __builtin_choose_expr(is_signed_type(typeof(a)), \ __signed_add_overflow(a, b, d), \ - __unsigned_add_overflow(a, b, d)) + __unsigned_add_overflow(a, b, d))) -#define check_sub_overflow(a, b, d) \ +#define check_sub_overflow(a, b, d) __must_check_overflow( \ __builtin_choose_expr(is_signed_type(typeof(a)), \ __signed_sub_overflow(a, b, d), \ - __unsigned_sub_overflow(a, b, d)) + __unsigned_sub_overflow(a, b, d))) -#define check_mul_overflow(a, b, d) \ +#define check_mul_overflow(a, b, d) __must_check_overflow( \ __builtin_choose_expr(is_signed_type(typeof(a)), \ __signed_mul_overflow(a, b, d), \ - __unsigned_mul_overflow(a, b, d)) - + __unsigned_mul_overflow(a, b, d))) #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */ @@ -227,7 +236,7 @@ * '*d' will hold the results of the attempted shift, but is not * considered "safe for use" if false is returned. */ -#define check_shl_overflow(a, s, d) ({ \ +#define check_shl_overflow(a, s, d) __must_check_overflow(({ \ typeof(a) _a = a; \ typeof(s) _s = s; \ typeof(d) _d = d; \ @@ -237,7 +246,7 @@ *_d = (_a_full << _to_shift); \ (_to_shift != _s || is_negative(*_d) || is_negative(_a) || \ (*_d >> _to_shift) != _a); \ -}) +})) /** * array_size() - Calculate size of 2-dimensional array.
Since the destination variable of the check_*_overflow() helpers will contain a wrapped value on failure, it would be best to make sure callers really did check the return result of the helper. Adjust the macros to use a bool-wrapping static inline that is marked with __must_check. This means the macros can continue to have their type-agnostic behavior while gaining the function attribute (that cannot be applied directly to macros). Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> Signed-off-by: Kees Cook <keescook@chromium.org> --- v2: - de-generalized __must_check_overflow() from being named "bool" (willy) - fix comment typos (rasmus) v1: https://lore.kernel.org/lkml/202008121450.405E4A3@keescook --- include/linux/overflow.h | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-)