| Message ID | 20180308214045.GA6787@beast (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2018-03-08 at 13:40 -0800, Kees Cook wrote: > > +#define __min(t1, t2, x, y) \ > + __builtin_choose_expr(__builtin_constant_p(x) && \ > + __builtin_constant_p(y) && \ > + __builtin_types_compatible_p(t1, t2), \ > + (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y), \ > + __single_eval_min(t1, t2, \ > + __UNIQUE_ID(max1_), \ > + __UNIQUE_ID(max2_), \ min1_ etc? Ian. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 8 Mar 2018 13:40:45 -0800 Kees Cook <keescook@chromium.org> wrote: > When max() is used in stack array size calculations from literal values > (e.g. "char foo[max(sizeof(struct1), sizeof(struct2))]", the compiler > thinks this is a dynamic calculation due to the single-eval logic, which > is not needed in the literal case. This change removes several accidental > stack VLAs from an x86 allmodconfig build: > > $ diff -u before.txt after.txt | grep ^- > -drivers/input/touchscreen/cyttsp4_core.c:871:2: warning: ISO C90 forbids variable length array ‘ids’ [-Wvla] > -fs/btrfs/tree-checker.c:344:4: warning: ISO C90 forbids variable length array ‘namebuf’ [-Wvla] > -lib/vsprintf.c:747:2: warning: ISO C90 forbids variable length array ‘sym’ [-Wvla] > -net/ipv4/proc.c:403:2: warning: ISO C90 forbids variable length array ‘buff’ [-Wvla] > -net/ipv6/proc.c:198:2: warning: ISO C90 forbids variable length array ‘buff’ [-Wvla] > -net/ipv6/proc.c:218:2: warning: ISO C90 forbids variable length array ‘buff64’ [-Wvla] > > Based on an earlier patch from Josh Poimboeuf. > > ... > > --- a/include/linux/kernel.h > +++ b/include/linux/kernel.h > @@ -787,37 +787,57 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } > * strict type-checking.. See the > * "unnecessary" pointer comparison. > */ > -#define __min(t1, t2, min1, min2, x, y) ({ \ > +#define __single_eval_min(t1, t2, min1, min2, x, y) ({ \ > t1 min1 = (x); \ > t2 min2 = (y); \ > (void) (&min1 == &min2); \ > min1 < min2 ? min1 : min2; }) > > +/* > + * In the case of builtin constant values, there is no need to do the > + * double-evaluation protection, so the raw comparison can be made. > + * This allows min()/max() to be used in stack array allocations and > + * avoid the compiler thinking it is a dynamic value leading to an > + * accidental VLA. > + */ > +#define __min(t1, t2, x, y) \ > + __builtin_choose_expr(__builtin_constant_p(x) && \ > + __builtin_constant_p(y) && \ > + __builtin_types_compatible_p(t1, t2), \ > + (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y), \ > + __single_eval_min(t1, t2, \ > + __UNIQUE_ID(max1_), \ > + __UNIQUE_ID(max2_), \ > + x, y)) > + Holy crap. I suppose gcc will one day be fixed and we won't need this. Is there a good reason to convert min()? Surely nobody will be using min to dimension an array - always max? Just for symmetry, I guess. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 8, 2018 at 2:18 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Thu, 8 Mar 2018 13:40:45 -0800 Kees Cook <keescook@chromium.org> wrote: > >> When max() is used in stack array size calculations from literal values >> (e.g. "char foo[max(sizeof(struct1), sizeof(struct2))]", the compiler >> thinks this is a dynamic calculation due to the single-eval logic, which >> is not needed in the literal case. This change removes several accidental >> stack VLAs from an x86 allmodconfig build: >> >> $ diff -u before.txt after.txt | grep ^- >> -drivers/input/touchscreen/cyttsp4_core.c:871:2: warning: ISO C90 forbids variable length array ‘ids’ [-Wvla] >> -fs/btrfs/tree-checker.c:344:4: warning: ISO C90 forbids variable length array ‘namebuf’ [-Wvla] >> -lib/vsprintf.c:747:2: warning: ISO C90 forbids variable length array ‘sym’ [-Wvla] >> -net/ipv4/proc.c:403:2: warning: ISO C90 forbids variable length array ‘buff’ [-Wvla] >> -net/ipv6/proc.c:198:2: warning: ISO C90 forbids variable length array ‘buff’ [-Wvla] >> -net/ipv6/proc.c:218:2: warning: ISO C90 forbids variable length array ‘buff64’ [-Wvla] >> >> Based on an earlier patch from Josh Poimboeuf. >> >> ... >> >> --- a/include/linux/kernel.h >> +++ b/include/linux/kernel.h >> @@ -787,37 +787,57 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } >> * strict type-checking.. See the >> * "unnecessary" pointer comparison. >> */ >> -#define __min(t1, t2, min1, min2, x, y) ({ \ >> +#define __single_eval_min(t1, t2, min1, min2, x, y) ({ \ >> t1 min1 = (x); \ >> t2 min2 = (y); \ >> (void) (&min1 == &min2); \ >> min1 < min2 ? min1 : min2; }) >> >> +/* >> + * In the case of builtin constant values, there is no need to do the >> + * double-evaluation protection, so the raw comparison can be made. >> + * This allows min()/max() to be used in stack array allocations and >> + * avoid the compiler thinking it is a dynamic value leading to an >> + * accidental VLA. >> + */ >> +#define __min(t1, t2, x, y) \ >> + __builtin_choose_expr(__builtin_constant_p(x) && \ >> + __builtin_constant_p(y) && \ >> + __builtin_types_compatible_p(t1, t2), \ >> + (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y), \ >> + __single_eval_min(t1, t2, \ >> + __UNIQUE_ID(max1_), \ >> + __UNIQUE_ID(max2_), \ >> + x, y)) >> + > > Holy crap. > > I suppose gcc will one day be fixed and we won't need this. > > Is there a good reason to convert min()? Surely nobody will be using > min to dimension an array - always max? Just for symmetry, I guess. I just went with symmetry. It seems like an ugly risk to implement min and mix differently. :) In theory it may produce smaller code for rare min() uses, but I haven't actually verified that. I will send a v2 with the two nits mentioned... -Kees
On Thu, Mar 8, 2018 at 1:40 PM, Kees Cook <keescook@chromium.org> wrote: > +#define __min(t1, t2, x, y) \ > + __builtin_choose_expr(__builtin_constant_p(x) && \ > + __builtin_constant_p(y) && \ > + __builtin_types_compatible_p(t1, t2), \ > + (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y), \ I understand why you use __builtin_types_compatible_p(), but please don't. It will mean that trivial constants like "5" and "sizeof(x)" won't simplify, because they have different types. The ?: will give the right combined type anyway, and if you want the type comparison warning, just add a comma-expression with something like like (t1 *)1 == (t2 *)1 to get the type compatibility warning. Yeah, yeah, maybe none of the VLA cases triggered that, but it seems silly to not just get that obvious constant case right. Hmm? Linus -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 8, 2018 at 3:48 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Thu, Mar 8, 2018 at 1:40 PM, Kees Cook <keescook@chromium.org> wrote: >> +#define __min(t1, t2, x, y) \ >> + __builtin_choose_expr(__builtin_constant_p(x) && \ >> + __builtin_constant_p(y) && \ >> + __builtin_types_compatible_p(t1, t2), \ >> + (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y), \ > > I understand why you use __builtin_types_compatible_p(), but please don't. > > It will mean that trivial constants like "5" and "sizeof(x)" won't > simplify, because they have different types. Rasmus mentioned this too. What I said there was that I was shy to make that change, since we already can't mix that kind of thing with the existing min()/max() implementation. The existing min()/max() is already extremely strict, so there are no instances of this in the tree. If I explicitly add one, I see this with or without the patch: In file included from drivers/misc/lkdtm.h:7:0, from drivers/misc/lkdtm_core.c:33: drivers/misc/lkdtm_core.c: In function ‘lkdtm_module_exit’: ./include/linux/kernel.h:809:16: warning: comparison of distinct pointer types lacks a cast (void) (&max1 == &max2); \ ^ ./include/linux/kernel.h:818:2: note: in expansion of macro ‘__max’ __max(typeof(x), typeof(y), \ ^~~~~ ./include/linux/printk.h:308:34: note: in expansion of macro ‘max’ printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__) ^~~~~~~~~~~ drivers/misc/lkdtm_core.c:500:2: note: in expansion of macro ‘pr_info’ pr_info("%lu\n", max(16, sizeof(unsigned long))); ^~~~~~~ > The ?: will give the right combined type anyway, and if you want the > type comparison warning, just add a comma-expression with something > like like > > (t1 *)1 == (t2 *)1 > > to get the type compatibility warning. When I tried removing __builtin_types_compatible_p(), I still got the type-check warning because I think the preprocessor still sees the "(void) (&min1 == &min2)" before optimizing? So, I technically _can_ drop the __builtin_types_compatible_p(), and still keep the type warning. :P > Yeah, yeah, maybe none of the VLA cases triggered that, but it seems > silly to not just get that obvious constant case right. > > Hmm? So are you saying you _want_ the type enforcement weakened here, or that I should just not use __builtin_types_compatible_p()? Thanks! -Kees
On Thu, Mar 8, 2018 at 4:45 PM, Kees Cook <keescook@chromium.org> wrote: > > Rasmus mentioned this too. What I said there was that I was shy to > make that change, since we already can't mix that kind of thing with > the existing min()/max() implementation. The existing min()/max() is > already extremely strict, so there are no instances of this in the > tree. Yes, but I also didn't want to add any new cases in case people add new min/max() users. But: > If I explicitly add one, I see this with or without the patch: > > In file included from drivers/misc/lkdtm.h:7:0, > from drivers/misc/lkdtm_core.c:33: > drivers/misc/lkdtm_core.c: In function ‘lkdtm_module_exit’: > ./include/linux/kernel.h:809:16: warning: comparison of distinct > pointer types lacks a cast Oh, ok, in that case, just drop the __builtin_types_compatible_p() entirely. It's not adding anything. I was expecting the non-chosen expression in __builtin_choose_expr() to not cause type warnings. I'm actually surprised it does. Type games is why __builtin_choose_expr() tends to exist in the first place. > So are you saying you _want_ the type enforcement weakened here, or > that I should just not use __builtin_types_compatible_p()? I don't want to weaken the type enforcement, and I _thought_ you had done that __builtin_types_compatible_p() to keep it in place. But if that's not why you did it, then why was it there at all? If the type warning shows through even if it's in the other expression, then just a #define __min(t1, t2, x, y) \ __builtin_choose_expr( \ __builtin_constant_p(x) & \ __builtin_constant_p(y), \ (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y), \ __single_eval_min(t1, t2, \ ... would seem to be sufficient? Because logically, the only thing that matters is that x and y don't have any side effects and can be evaluated twice, and "__builtin_constant_p()" is already a much stronger version of that. Hmm? The __builtin_types_compatible_p() just doesn't seem to matter for the only thing I thought it was there for. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 8, 2018 at 5:35 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > I don't want to weaken the type enforcement, and I _thought_ you had > done that __builtin_types_compatible_p() to keep it in place. I thought so too (that originally came from Josh), but on removal, I was surprised that the checking was retained. :) > But if that's not why you did it, then why was it there at all? If the > type warning shows through even if it's in the other expression, then > just a > > > #define __min(t1, t2, x, y) \ > __builtin_choose_expr( \ > __builtin_constant_p(x) & \ > __builtin_constant_p(y), \ > (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y), \ > __single_eval_min(t1, t2, \ > ... > > would seem to be sufficient? > > Because logically, the only thing that matters is that x and y don't > have any side effects and can be evaluated twice, and > "__builtin_constant_p()" is already a much stronger version of that. > > Hmm? The __builtin_types_compatible_p() just doesn't seem to matter > for the only thing I thought it was there for. Yup, agreed. I'll drop it. -Kees
diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 3fd291503576..e0b39d461582 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -787,37 +787,57 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } * strict type-checking.. See the * "unnecessary" pointer comparison. */ -#define __min(t1, t2, min1, min2, x, y) ({ \ +#define __single_eval_min(t1, t2, min1, min2, x, y) ({ \ t1 min1 = (x); \ t2 min2 = (y); \ (void) (&min1 == &min2); \ min1 < min2 ? min1 : min2; }) +/* + * In the case of builtin constant values, there is no need to do the + * double-evaluation protection, so the raw comparison can be made. + * This allows min()/max() to be used in stack array allocations and + * avoid the compiler thinking it is a dynamic value leading to an + * accidental VLA. + */ +#define __min(t1, t2, x, y) \ + __builtin_choose_expr(__builtin_constant_p(x) && \ + __builtin_constant_p(y) && \ + __builtin_types_compatible_p(t1, t2), \ + (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y), \ + __single_eval_min(t1, t2, \ + __UNIQUE_ID(max1_), \ + __UNIQUE_ID(max2_), \ + x, y)) + /** * min - return minimum of two values of the same or compatible types * @x: first value * @y: second value */ -#define min(x, y) \ - __min(typeof(x), typeof(y), \ - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ - x, y) +#define min(x, y) __min(typeof(x), typeof(y), x, y) -#define __max(t1, t2, max1, max2, x, y) ({ \ +#define __single_eval_max(t1, t2, max1, max2, x, y) ({ \ t1 max1 = (x); \ t2 max2 = (y); \ (void) (&max1 == &max2); \ max1 > max2 ? max1 : max2; }) +#define __max(t1, t2, x, y) \ + __builtin_choose_expr(__builtin_constant_p(x) && \ + __builtin_constant_p(y) && \ + __builtin_types_compatible_p(t1, t2), \ + (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y), \ + __single_eval_max(t1, t2, \ + __UNIQUE_ID(max1_), \ + __UNIQUE_ID(max2_), \ + x, y)) /** * max - return maximum of two values of the same or compatible types * @x: first value * @y: second value */ -#define max(x, y) \ - __max(typeof(x), typeof(y), \ - __UNIQUE_ID(max1_), __UNIQUE_ID(max2_), \ - x, y) +#define max(x, y) __max(typeof(x), typeof(y), x, y) /** * min3 - return minimum of three values @@ -871,7 +891,6 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } */ #define min_t(type, x, y) \ __min(type, type, \ - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ x, y) /** @@ -882,7 +901,6 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } */ #define max_t(type, x, y) \ __max(type, type, \ - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ x, y) /**
When max() is used in stack array size calculations from literal values (e.g. "char foo[max(sizeof(struct1), sizeof(struct2))]", the compiler thinks this is a dynamic calculation due to the single-eval logic, which is not needed in the literal case. This change removes several accidental stack VLAs from an x86 allmodconfig build: $ diff -u before.txt after.txt | grep ^- -drivers/input/touchscreen/cyttsp4_core.c:871:2: warning: ISO C90 forbids variable length array ‘ids’ [-Wvla] -fs/btrfs/tree-checker.c:344:4: warning: ISO C90 forbids variable length array ‘namebuf’ [-Wvla] -lib/vsprintf.c:747:2: warning: ISO C90 forbids variable length array ‘sym’ [-Wvla] -net/ipv4/proc.c:403:2: warning: ISO C90 forbids variable length array ‘buff’ [-Wvla] -net/ipv6/proc.c:198:2: warning: ISO C90 forbids variable length array ‘buff’ [-Wvla] -net/ipv6/proc.c:218:2: warning: ISO C90 forbids variable length array ‘buff64’ [-Wvla] Based on an earlier patch from Josh Poimboeuf. Signed-off-by: Kees Cook <keescook@chromium.org> --- include/linux/kernel.h | 42 ++++++++++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 12 deletions(-)