Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Thu, Mar 22, 2018 at 8:01 AM, Kees Cook wrote: > > Seems like it doesn't like void * arguments: Yeah, that was discussed separately, I just didn't realize we had any such users. As David said, just adding a (long) cast to it should be fine, ie #define __is_constant(a) \ (sizeof(int) == sizeof(*(1 ? ((void*)((long)(a) * 0l)) : (int*)1))) and is probably a good idea even outside of pointers (because "long long" constants could cause warnings too due to the cast to (void *)). Linus
RE: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Kees Cook > Sent: 22 March 2018 15:01 ... > > /* Glory to Martin Uecker */ > > #define __is_constant(a) \ > > (sizeof(int) == sizeof(*(1 ? ((void*)((a) * 0l)) : (int*)1))) ... > So, this time it's not a catastrophic failure with gcc 4.4. Instead it > fails in 11 distinct places: ... > Seems like it doesn't like void * arguments: > > mm/percpu.c: > void *ptr; > ... > base = min(ptr, base); Try adding (unsigned long) before the (a). David
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Tue, Mar 20, 2018 at 4:23 PM, Linus Torvalds wrote: > On Sat, Mar 17, 2018 at 1:07 PM, Kees Cook wrote: >> >> No luck! :( gcc 4.4 refuses to play along. And, hilariously, not only >> does it not change the complaint about __builtin_choose_expr(), it >> also thinks that's a VLA now. > > Hmm. So thanks to the diseased mind of Martin Uecker, there's a better > test for "__is_constant()": > > /* Glory to Martin Uecker */ > #define __is_constant(a) \ > (sizeof(int) == sizeof(*(1 ? ((void*)((a) * 0l)) : (int*)1))) > > that is actually *specified* by the C standard to work, and doesn't > even depend on any gcc extensions. I feel we risk awakening Cthulhu with this. :) > The reason is some really subtle pointer conversion rules, where the > type of the ternary operator will depend on whether one of the > pointers is NULL or not. > > And the definition of NULL, in turn, very much depends on "integer > constant expression that has the value 0". > > Are you willing to do one final try on a generic min/max? Same as my > last patch, but using the above __is_constant() test instead of > __builtin_constant_p? So, this time it's not a catastrophic failure with gcc 4.4. Instead it fails in 11 distinct places: $ grep "first argument to ‘__builtin_choose_expr’ not a constant" log | cut -d: -f1-2 crypto/ablkcipher.c:71 crypto/blkcipher.c:70 crypto/skcipher.c:95 mm/percpu.c:2453 net/ceph/osdmap.c:1545 net/ceph/osdmap.c:1756 net/ceph/osdmap.c:1763 mm/kmemleak.c:1371 mm/kmemleak.c:1403 drivers/infiniband/hw/hfi1/pio_copy.c:421 drivers/infiniband/hw/hfi1/pio_copy.c:547 Seems like it doesn't like void * arguments: mm/percpu.c: void *ptr; ... base = min(ptr, base); mm/kmemleak.c: static void scan_large_block(void *start, void *end) ... next = min(start + MAX_SCAN_SIZE, end); I'll poke a bit more... -Kees -- Kees Cook Pixel Security
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Tue, Mar 20, 2018 at 04:26:52PM -0700, Linus Torvalds wrote: > On Tue, Mar 20, 2018 at 4:23 PM, Linus Torvalds > wrote: > > > > Hmm. So thanks to the diseased mind of Martin Uecker, there's a better > > test for "__is_constant()": > > > > /* Glory to Martin Uecker */ > > #define __is_constant(a) \ > > (sizeof(int) == sizeof(*(1 ? ((void*)((a) * 0l)) : (int*)1))) > > > > that is actually *specified* by the C standard to work, and doesn't > > even depend on any gcc extensions. > > Well, it does depend on 'sizeof(*(void *)X)' being 1 and the compiler > not complaining about it, and that sizeof(int) is not 1. > > But since we depend on those things in the kernel anyway, that's fine. It also depends upon "ICE for null pointer constant purposes" having the same extensions as "ICE for enum purposes", etc., which is not obvious. Back in 2007 or so we had a long thread regarding null pointer constants in sparse; I probably still have notes from back then, but that'll take some serious digging to find ;-/ What's more, gcc definitely has odd extensions. Example I remember from back then: extern unsigned n; struct { int x : 1 + n - n; } y; is accepted. Used to be quietly accepted with -Wpedantic -std=c99, even, but that got fixed - with -Wpedantic it does, at least, warn. What is and what is not recognized is fairly random - 1 + n - n + 1 + n - n is recognized as "constant", 1 + n + n + 1 - n - n is not. Of course, neither is an ICE.
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Tue, Mar 20, 2018 at 4:23 PM, Linus Torvalds wrote: > > Hmm. So thanks to the diseased mind of Martin Uecker, there's a better > test for "__is_constant()": > > /* Glory to Martin Uecker */ > #define __is_constant(a) \ > (sizeof(int) == sizeof(*(1 ? ((void*)((a) * 0l)) : (int*)1))) > > that is actually *specified* by the C standard to work, and doesn't > even depend on any gcc extensions. Well, it does depend on 'sizeof(*(void *)X)' being 1 and the compiler not complaining about it, and that sizeof(int) is not 1. But since we depend on those things in the kernel anyway, that's fine. Linus
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Sat, Mar 17, 2018 at 1:07 PM, Kees Cook wrote: > > No luck! :( gcc 4.4 refuses to play along. And, hilariously, not only > does it not change the complaint about __builtin_choose_expr(), it > also thinks that's a VLA now. Hmm. So thanks to the diseased mind of Martin Uecker, there's a better test for "__is_constant()": /* Glory to Martin Uecker */ #define __is_constant(a) \ (sizeof(int) == sizeof(*(1 ? ((void*)((a) * 0l)) : (int*)1))) that is actually *specified* by the C standard to work, and doesn't even depend on any gcc extensions. The reason is some really subtle pointer conversion rules, where the type of the ternary operator will depend on whether one of the pointers is NULL or not. And the definition of NULL, in turn, very much depends on "integer constant expression that has the value 0". Are you willing to do one final try on a generic min/max? Same as my last patch, but using the above __is_constant() test instead of __builtin_constant_p? Linus
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Tue, Mar 20, 2018 at 7:29 AM, Linus Torvalds wrote: > On Mon, Mar 19, 2018 at 2:43 AM, David Laight wrote: >> >> Is it necessary to have the full checks for old versions of gcc? >> >> Even -Wvla could be predicated on very recent gcc - since we aren't >> worried about whether gcc decides to generate a vla, but whether >> the source requests one. > > You are correct. We could just ignore the issue with old gcc versions, > and disable -Wvla rather than worry about it. This version might also be an option: diff --git a/Makefile b/Makefile index 37fc475a2b92..49dd9f0fb76c 100644 --- a/Makefile +++ b/Makefile @@ -687,7 +687,8 @@ KBUILD_CFLAGS += $(call cc-option,-fno-reorder-blocks,) \ endif ifneq ($(CONFIG_FRAME_WARN),0) -KBUILD_CFLAGS += $(call cc-option,-Wframe-larger-than=${CONFIG_FRAME_WARN}) +KBUILD_CFLAGS += $(call cc-option,-Wstack-usage=${CONFIG_FRAME_WARN}, \ + -$(call cc-option,-Wframe-larger-than=${CONFIG_FRAME_WARN})) endif # This selects the stack protector compiler flag. Testing it is delayed Wiht -Wstack-usage=, we should get a similar warning to -Wvla for frames that contain real VLAs, but not when there is a VLA that ends up being a compile-time constant size in the end. Wstack-usage was introduced in gcc-4.7, so on older versions it turns back into Wframe-larger-than=. An example output would be security/integrity/ima/ima_crypto.c: In function 'ima_calc_buffer_hash': security/integrity/ima/ima_crypto.c:616:5: error: stack usage might be unbounded [-Werror=stack-usage=] Arnd
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Mon, Mar 19, 2018 at 2:43 AM, David Laight wrote: > > Is it necessary to have the full checks for old versions of gcc? > > Even -Wvla could be predicated on very recent gcc - since we aren't > worried about whether gcc decides to generate a vla, but whether > the source requests one. You are correct. We could just ignore the issue with old gcc versions, and disable -Wvla rather than worry about it. Linus
RE: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: linus...@gmail.com [mailto:linus...@gmail.com] On Behalf Of Linus Torvalds > Sent: 18 March 2018 23:36 ... > > Yeah, and since we're in the situation that *new* gcc versions work > for us anyway, and we only have issues with older gcc's (that sadly > people still use), even if there was a new cool feature we couldn't > use it. Is it necessary to have the full checks for old versions of gcc? Even -Wvla could be predicated on very recent gcc - since we aren't worried about whether gcc decides to generate a vla, but whether the source requests one. David
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Sun, Mar 18, 2018 at 3:59 PM, Rasmus Villemoes wrote: > > OK, I missed where this was made about side effects of x and y We never made it explicit, since all we really cared about in the end is the constantness. But yes: > but I suppose the idea was to use > > no_side_effects(x) && no_side_effects(y) ? trivial_max(x, y) : > old_max(x, y) Exactly. Except with __builtin_choose_expr(), because we need the end result to be seen as a integer constant expression, so that we can then use it as an array size. So it needs that early parse-time resolution. >> We only really use __builtin_constant_p() as a (bad) approximation of >> that in this case, since it's the best we can do. > > I don't think you should parenthesize bad, rather capitalize it. ({x++; > 0;}) is constant in the eyes of __builtin_constant_p, but not > side-effect free. Hmm. Yeah, but probably don't much care for the kernel. For example, we do things like this: #define __swab64(x) \ (__builtin_constant_p((__u64)(x)) ? \ ___constant_swab64(x) : \ __fswab64(x)) where that "___constant_swab64()" very much uses the same argument over and over. And we do that for related reasons - we really want to do the constant folding at build time for some cases, and this was the only sane way to do it. Eg code like return (addr & htonl(0xff00)) == htonl(0x7f00); wants to do the right thing, and long ago gcc didn't have builtins for byte swapping, so we had to just do nasty horribly macros that DTRT for constants. But since the kernel is standalone, we don't need to really worry about the *generic* case, we just need to worry about our own macros, and if somebody does that example you show I guess we'll just have to shun them ;) Of course, our own macros are often macros from hell, exactly because they often contain a lot of type-checking and/or type-(size)-based polymorphism. But then we actually *want* gcc to simplify things, and avoid side effects, just have potentially very complex expressions. But we basically never have that kind of intentionally evil macros, so we are willing to live with a bad substitute. But yes, it would be better to have some more control over things, and actually have a way to say "if X is a constant integer expression, do transformation Y, otherwise call function y()". Actually sparse started out with the idea in the background that it could become not just a checker, but a "transformation tool". Partly because of long gone historical issues (ie gcc people used to be very anti-plugin due to licensing issues). Of course, a more integrated and powerful preprocessor language is almost always what we *really* wanted, but traditionally "powerful preprocessor" has always meant "completely incomprehensible and badly integrated preprocessor". "cpp" may be a horrid language, but it's there and it's fast (when integrated with the front-end, like everybody does now) But sadly, cpp is really bad for the above kind of "if argument is constant" kind of tricks. I suspect we'd use it a lot otherwise. >> So the real use would be to say "can we use the simple direct macro >> that just happens to also fold into a nice integer constant >> expression" for __builtin_choose_expr(). >> >> I tried to find something like that, but it really doesn't exist, even >> though I would actually have expected it to be a somewhat common >> concern for macro writers: write a macro that works in any arbitrary >> environment. > > Yeah, I think the closest is a five year old suggestion > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57612) to add a > __builtin_assert_no_side_effects, that could be used in macros that for > some reason cannot be implemented without evaluating some argument > multiple times. It would be more useful to have the predicate version, > which one could always turn into a build bug version. But we have > neither, unfortunately. Yeah, and since we're in the situation that *new* gcc versions work for us anyway, and we only have issues with older gcc's (that sadly people still use), even if there was a new cool feature we couldn't use it. Oh well. Thanks for the background. Linus
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On 2018-03-18 22:33, Linus Torvalds wrote: > On Sun, Mar 18, 2018 at 2:13 PM, Rasmus Villemoes > wrote: >> On 2018-03-17 19:52, Linus Torvalds wrote: >>> >>> Ok, so it really looks like that same "__builtin_constant_p() doesn't >>> return a constant". >>> >>> Which is really odd, but there you have it. >> >> Not really. We do rely on builtin_constant_p not being folded too >> quickly to a 0/1 answer, so that gcc still generates good code even if >> the argument is only known to be constant at a late(r) optimization >> stage (through inlining and all). > > Hmm. That makes sense. It just doesn't work for our case when we > really want to choose the expression based on side effects or not. > >> So unlike types_compatible_p, which >> can obviously be answered early during parsing, builtin_constant_p is >> most of the time a yes/no/maybe/it's complicated thing. > > The silly thing is, the thing we *really* want to know _is_ actually > easily accessible during the early parsing, exactly like > __builtin_compatible_p(): it's not really that we care about the > expressions being constant, as much as the "can this have side > effects" question. OK, I missed where this was made about side effects of x and y, but I suppose the idea was to use no_side_effects(x) && no_side_effects(y) ? trivial_max(x, y) : old_max(x, y) or the same thing spelled with b_c_e? Yes, I think that would work, if we indeed had that way of checking an expression. > We only really use __builtin_constant_p() as a (bad) approximation of > that in this case, since it's the best we can do. I don't think you should parenthesize bad, rather capitalize it. ({x++; 0;}) is constant in the eyes of __builtin_constant_p, but not side-effect free. Sure, that's very contrived, but I can easily imagine some max(f(foo), -1) call where f is sometimes an external function, but for other .configs it's a static inline that always returns 0, but still has some non-trivial side-effect before that. And this would all depend on which optimizations gcc applies before it decides to evaluate builtin_constant_p, so could be some fun debugging. Good thing that that didn't work out... > So the real use would be to say "can we use the simple direct macro > that just happens to also fold into a nice integer constant > expression" for __builtin_choose_expr(). > > I tried to find something like that, but it really doesn't exist, even > though I would actually have expected it to be a somewhat common > concern for macro writers: write a macro that works in any arbitrary > environment. Yeah, I think the closest is a five year old suggestion (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57612) to add a __builtin_assert_no_side_effects, that could be used in macros that for some reason cannot be implemented without evaluating some argument multiple times. It would be more useful to have the predicate version, which one could always turn into a build bug version. But we have neither, unfortunately. Rasmus
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Sun, Mar 18, 2018 at 2:13 PM, Rasmus Villemoes wrote: > On 2018-03-17 19:52, Linus Torvalds wrote: >> >> Ok, so it really looks like that same "__builtin_constant_p() doesn't >> return a constant". >> >> Which is really odd, but there you have it. > > Not really. We do rely on builtin_constant_p not being folded too > quickly to a 0/1 answer, so that gcc still generates good code even if > the argument is only known to be constant at a late(r) optimization > stage (through inlining and all). Hmm. That makes sense. It just doesn't work for our case when we really want to choose the expression based on side effects or not. > So unlike types_compatible_p, which > can obviously be answered early during parsing, builtin_constant_p is > most of the time a yes/no/maybe/it's complicated thing. The silly thing is, the thing we *really* want to know _is_ actually easily accessible during the early parsing, exactly like __builtin_compatible_p(): it's not really that we care about the expressions being constant, as much as the "can this have side effects" question. We only really use __builtin_constant_p() as a (bad) approximation of that in this case, since it's the best we can do. So the real use would be to say "can we use the simple direct macro that just happens to also fold into a nice integer constant expression" for __builtin_choose_expr(). I tried to find something like that, but it really doesn't exist, even though I would actually have expected it to be a somewhat common concern for macro writers: write a macro that works in any arbitrary environment. I guess array sizes are really the only true limiting environment (static initializers is another one). How annoying. Odd that newer gcc's seem to do so much better (ie gcc-5 seems to be fine). So _something_ must have changed. Linus
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On 2018-03-17 19:52, Linus Torvalds wrote: > On Sat, Mar 17, 2018 at 12:27 AM, Kees Cook wrote: >> >> Unfortunately my 4.4 test fails quickly: >> >> ./include/linux/jiffies.h: In function ‘jiffies_delta_to_clock_t’: >> ./include/linux/jiffies.h:444: error: first argument to >> ‘__builtin_choose_expr’ not a constant > > Ok, so it really looks like that same "__builtin_constant_p() doesn't > return a constant". > > Which is really odd, but there you have it. Not really. We do rely on builtin_constant_p not being folded too quickly to a 0/1 answer, so that gcc still generates good code even if the argument is only known to be constant at a late(r) optimization stage (through inlining and all). So unlike types_compatible_p, which can obviously be answered early during parsing, builtin_constant_p is most of the time a yes/no/maybe/it's complicated thing. Sure, when the argument is just a literal or perhaps even any kind of ICE, gcc can fold it to "yes", and I think it does (though the details of when and if gcc does that can obviously be very version-dependent, which may be some of what we've seen). But when it's not that obvious, gcc leaves it in the undetermined state. That's not good enough for builtin_choose_expr, because even the type of the resulting expression depends on that first argument, so that really must be resolved early. So to have some kind of builtin_constant_p control a builtin_choose_expr, it would need to be a "builtin_ice_p" or "builtin_obviously_constant_p" that would always be folded to 0/1 as part of evaluating ICEs. So I don't think there's any way around creating a separate macro for use with compile-time constants. Rasmus
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Sat, Mar 17, 2018 at 01:07:32PM -0700, Kees Cook wrote: > On Sat, Mar 17, 2018 at 11:52 AM, Linus Torvalds > wrote: > > So the above is completely insane, bit there is actually a chance that > > using that completely crazy "x -> sizeof(char[x])" conversion actually > > helps, because it really does have a (very odd) evaluation-time > > change. sizeof() has to be evaluated as part of the constant > > expression evaluation, in ways that "__builtin_constant_p()" isn't > > specified to be done. > > > > But it is also definitely me grasping at straws. If that doesn't work > > for 4.4, there's nothing else I can possibly see. > > No luck! :( gcc 4.4 refuses to play along. And, hilariously, not only > does it not change the complaint about __builtin_choose_expr(), it > also thinks that's a VLA now. > > ./include/linux/mm.h: In function ‘get_mm_hiwater_rss’: > ./include/linux/mm.h:1567: warning: variable length array is used > ./include/linux/mm.h:1567: error: first argument to > ‘__builtin_choose_expr’ not a constant > > 6.8 is happy with it (of course). > > I do think the earlier version (without the > sizeof-hiding-builting_constant_p) provides a template for a > const_max() that both you and Rasmus would be happy with, though! I thought we were dropping support for 4.4 (for other reasons). Isn't it 4.6 we should be looking at? -- Josh
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Sat, Mar 17, 2018 at 11:52 AM, Linus Torvalds wrote: > So the above is completely insane, bit there is actually a chance that > using that completely crazy "x -> sizeof(char[x])" conversion actually > helps, because it really does have a (very odd) evaluation-time > change. sizeof() has to be evaluated as part of the constant > expression evaluation, in ways that "__builtin_constant_p()" isn't > specified to be done. > > But it is also definitely me grasping at straws. If that doesn't work > for 4.4, there's nothing else I can possibly see. No luck! :( gcc 4.4 refuses to play along. And, hilariously, not only does it not change the complaint about __builtin_choose_expr(), it also thinks that's a VLA now. ./include/linux/mm.h: In function ‘get_mm_hiwater_rss’: ./include/linux/mm.h:1567: warning: variable length array is used ./include/linux/mm.h:1567: error: first argument to ‘__builtin_choose_expr’ not a constant 6.8 is happy with it (of course). I do think the earlier version (without the sizeof-hiding-builting_constant_p) provides a template for a const_max() that both you and Rasmus would be happy with, though! -Kees -- Kees Cook Pixel Security
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Sat, Mar 17, 2018 at 12:27 AM, Kees Cook wrote: > > Unfortunately my 4.4 test fails quickly: > > ./include/linux/jiffies.h: In function ‘jiffies_delta_to_clock_t’: > ./include/linux/jiffies.h:444: error: first argument to > ‘__builtin_choose_expr’ not a constant Ok, so it really looks like that same "__builtin_constant_p() doesn't return a constant". Which is really odd, but there you have it. I wonder if you can use that "sizeof()" to force evaluation of it, because sizeof() really does end up being magical when it comes to "integer constant expression". So instead of this: #define __no_side_effects(a,b) \ (__builtin_constant_p(a)&&__builtin_constant_p(b)) that just assumes that __builtin_constant_p() itself always counts as a constant expression, what happens if you do #define __is_constant(a) \ (sizeof(char[__builtin_constant_p(a)])) #define __no_side_effects(a,b) \ (__is_constant(a) && __is_constant(b)) I realize that the above looks completely insane: the whole point is to *not* have VLA's, and we know that __builtin_constant_p() isn't always evaliated as a constant. But hear me out: if the issue is that there's some evaluation ordering between the two builtins, and the problem is that the __builtin_choose_expr() part of the expression is expanded *before* the __builtin_constant_p() has been expanded, then just hiding it inside that bat-shit-crazy sizeof() will force that to be evaluated first (because a sizeof() is defined to be a integer constant expression. So the above is completely insane, bit there is actually a chance that using that completely crazy "x -> sizeof(char[x])" conversion actually helps, because it really does have a (very odd) evaluation-time change. sizeof() has to be evaluated as part of the constant expression evaluation, in ways that "__builtin_constant_p()" isn't specified to be done. But it is also definitely me grasping at straws. If that doesn't work for 4.4, there's nothing else I can possibly see. Linus
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Fri, Mar 16, 2018 at 12:27 PM, Linus Torvalds wrote: > Kees - is there some online "gcc-4.4 checker" somewhere? This does > seem to work with my gcc. I actually tested some of those files you > pointed at now. Unfortunately my 4.4 test fails quickly: ./include/linux/jiffies.h: In function ‘jiffies_delta_to_clock_t’: ./include/linux/jiffies.h:444: error: first argument to ‘__builtin_choose_expr’ not a constant static inline clock_t jiffies_delta_to_clock_t(long delta) { return jiffies_to_clock_t(max(0L, delta)); } I think this is the same problem of using __builtin_constant_p() in 4.4 that we hit earlier? :( -Kees -- Kees Cook Pixel Security
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Fri, Mar 16, 2018 at 9:14 PM, Linus Torvalds wrote: > On Fri, Mar 16, 2018 at 1:03 PM, Miguel Ojeda > wrote: >>> >>> Kees - is there some online "gcc-4.4 checker" somewhere? This does >>> seem to work with my gcc. I actually tested some of those files you >>> pointed at now. >> >> I use this one: >> >> https://godbolt.org/ > > Well, my *test* code works on that one and -Wvla -Werror. > > It does not work with gcc-4.1.x, but works with gcc-4.4.x. > > I can't seem to see the errors any way, I wonder if > __builtin_choose_expr() simply didn't exist back then. > > Odd that you can't view warnings/errors with it. > > But it's possible that it fails on more complex stuff in the kernel. > > I've done a "allmodconfig" build with that patch, and the only issue > it found was that (real) type issue in tpm_tis_core.h. Just tested your max() with a Python script I wrote yesterday to try a lot of combinations for this thing. Your version gives some warnings in some cases, like: warning: signed and unsigned type in conditional expression [-Wsign-compare] #define __cmp(a,b,op) ((a)op(b)?(a):(b)) warning: comparison between signed and unsigned integer expressions [-Wsign-compare] #define const_max(a,b) __careful_cmp(a,b,>) warning: comparison of distinct pointer types lacks a cast (!!(sizeof((typeof(a)*)1==(typeof(b)*)1))) But it fails on something like (with warnings): int a[const_max(-30, 60u)]; Sorry... :-( Has anyone taken a look at the last one I sent? Patch attached with the draft changes on the kernel. It compiles fine the cases Kees cleaned up in the other patch, but also works without a explicit type, for mixed types, and for both positive and negative values. Cheers, Miguel diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 3fd291503576..f83658a4f15d 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -819,6 +819,49 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } __UNIQUE_ID(max1_), __UNIQUE_ID(max2_), \ x, y) +size_t __error_not_const_arg(void) \ +__compiletime_error("const_max() used with non-compile-time constant arg"); +size_t __error_too_big(void) \ +__compiletime_error("const_max() used with an arg too big"); + +#define INTMAXT_MAX LLONG_MAX +typedef int64_t intmax_t; + +#define const_cmp(x, y, op) \ +__builtin_choose_expr(\ +!__builtin_constant_p(x) || !__builtin_constant_p(y), \ +__error_not_const_arg(), \ +__builtin_choose_expr(\ +(x) > INTMAXT_MAX || (y) > INTMAXT_MAX, \ +__error_too_big(),\ +__builtin_choose_expr(\ +(intmax_t)(x) op (intmax_t)(y), \ +(x), \ +(y) \ +) \ +) \ +) + +/** + * const_min - returns minimum of two compile-time constant values + * @x: first compile-time constant value + * @y: second compile-time constant value + * + * The result has the type of the winner of the comparison. Works for any + * value up to LLONG_MAX. + */ +#define const_min(x, y) const_cmp((x), (y), <=) + +/** + * const_max - returns maximum of two compile-time constant values + * @x: first compile-time constant value + * @y: second compile-time constant value + * + * The result has the type of the winner of the comparison. Works for any + * value up to LLONG_MAX. + */ +#define const_max(x, y) const_cmp((x), (y), >=) + /** * min3 - return minimum of three values * @x: first value
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Fri, Mar 16, 2018 at 9:14 PM, Linus Torvalds wrote: > On Fri, Mar 16, 2018 at 1:03 PM, Miguel Ojeda > wrote: >>> >>> Kees - is there some online "gcc-4.4 checker" somewhere? This does >>> seem to work with my gcc. I actually tested some of those files you >>> pointed at now. >> >> I use this one: >> >> https://godbolt.org/ > > Well, my *test* code works on that one and -Wvla -Werror. > > It does not work with gcc-4.1.x, but works with gcc-4.4.x. > > I can't seem to see the errors any way, I wonder if > __builtin_choose_expr() simply didn't exist back then. > > Odd that you can't view warnings/errors with it. Just click on the button left/bottom of the compilation window. Cheers, Miguel
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Fri, Mar 16, 2018 at 10:44 AM, David Laight wrote: > > I looked at the generated code for one of the constant sized VLA that > the compiler barfed at. > It seemed to subtract constants from %sp separately for the VLA. > So it looks like the compiler treats them as VLA even though it > knows the size. > That is probably missing optimisation. Looking at the code is definitely an option. In fact, instead of depending on -Wvla, we could just make 'objtool' warn about real variable-sized stack frames. That said, if that "sizeof()" trick of Al's actually works with older gcc versions too (it *should*, but it's not like __builtin_choose_expr() and __builtin_constant_p() have well-defined rules in the standard), that may just be the solution. And if gcc ends up generating bad code for those "constant sized vlas" anyway, then -Wvla would effectively warn about that code generation problem. Linus
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Fri, Mar 16, 2018 at 1:14 PM, Linus Torvalds wrote: > > It does not work with gcc-4.1.x, but works with gcc-4.4.x. > > I can't seem to see the errors any way, I wonder if > __builtin_choose_expr() simply didn't exist back then. No, that goes further back. It seems to be -Wvla itself that doesn't exist in 4.1, so the test build failed simply because I used that flag ;) Linus
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Fri, Mar 16, 2018 at 01:15:27PM -0700, Linus Torvalds wrote: > On Fri, Mar 16, 2018 at 1:12 PM, Al Viro wrote: > > > > That's C99, straight from N1256.pdf (C99-TC3)... > > I checked C90, since the error is > >ISO C90 forbids variable length array > > and I didn't see anything there. Well, yes - VLA are new in C99; C90 never had those...
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Fri, Mar 16, 2018 at 1:12 PM, Al Viro wrote: > > That's C99, straight from N1256.pdf (C99-TC3)... I checked C90, since the error is ISO C90 forbids variable length array and I didn't see anything there. Admittedly I only found a draft copy. Linus
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Fri, Mar 16, 2018 at 1:03 PM, Miguel Ojeda wrote: >> >> Kees - is there some online "gcc-4.4 checker" somewhere? This does >> seem to work with my gcc. I actually tested some of those files you >> pointed at now. > > I use this one: > > https://godbolt.org/ Well, my *test* code works on that one and -Wvla -Werror. It does not work with gcc-4.1.x, but works with gcc-4.4.x. I can't seem to see the errors any way, I wonder if __builtin_choose_expr() simply didn't exist back then. Odd that you can't view warnings/errors with it. But it's possible that it fails on more complex stuff in the kernel. I've done a "allmodconfig" build with that patch, and the only issue it found was that (real) type issue in tpm_tis_core.h. Linus
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Fri, Mar 16, 2018 at 12:27:23PM -0700, Linus Torvalds wrote: > But it sure isn't "variable" either as far as the standard is > concerned, because the standard doesn't even have that concept (it > uses "variable" for argument numbers and for variables). Huh? 6.7.5.2p4: If the size is not present, the array type is an incomplete type. If the size is * instead of being an expression, the array type is a variable length array type of unspecified size, which can only be used in declarations with function prototype scope [footnote]; such arrays are nonetheless complete types. If the size is an integer constant expression and the element type has a known constant size, the array type is not a variable length array type; otherwise, the array type is a variable length array type. footnote: Thus, * can be used only in function declarations that are not definitions (see 6.7.5.3). That's C99, straight from N1256.pdf (C99-TC3)...
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Fri, Mar 16, 2018 at 8:27 PM, Linus Torvalds wrote: > On Fri, Mar 16, 2018 at 10:55 AM, Al Viro wrote: >> >> That's not them, that's C standard regarding ICE. > > Yes. The C standard talks about "integer constant expression". I know. > It's come up in this very thread before. > > The C standard at no point talks about - or forbids - "variable length > arrays". That never comes up in the whole standard, I checked. > > So we are right now hindered by a _syntactic_ check, without any way > to have a _semantic_ check. > > That's my problem. The warnings are misleading and imply semantics. > > And apparently there is no way to actually check semantics. > >> 1,100 is *not* a constant expression as far as the standard is concerned, > > I very much know. > > But it sure isn't "variable" either as far as the standard is > concerned, because the standard doesn't even have that concept (it > uses "variable" for argument numbers and for variables). > > So being pedantic doesn't really change anything. > >> Would you argue that in >> void foo(char c) >> { >> int a[(c<<1) + 10 - c + 2 - c]; > > Yeah, I don't think that even counts as a constant value, even if it > can be optimized to one. I would not at all be unhppy to see > __builtin_constant_p() to return zero. > > But that is very much different from the syntax issue. > > So I would like to get some way to get both type-checking and constant > checking without the annoying syntax issue. > >> expr, constant_expression is not a constant_expression. And in >> this particular case the standard is not insane - the only reason >> for using that is typechecking and _that_ can be achieved without >> violating 6.6p6: >> sizeof(expr,0) * 0 + ICE >> *is* an integer constant expression, and it gives you exact same >> typechecking. So if somebody wants to play odd games, they can >> do that just fine, without complicating the logics for compilers... > > Now that actually looks like a good trick. Maybe we can use that > instead of the comma expression that causes problems. > > And using sizeof() to make sure that __builtin_choose_expression() > really gets an integer constant expression and that there should be no > ambiguity looks good. > > Hmm. > > This works for me, and I'm being *very* careful (those casts to > pointer types are inside that sizeof, because it's not an integral > type, and non-integral casts are not valid in an ICE either) but > somebody needs to check gcc-4.4: > > #define __typecheck(a,b) \ > (!!(sizeof((typeof(a)*)1==(typeof(b)*)1))) > > #define __no_side_effects(a,b) \ > (__builtin_constant_p(a)&&__builtin_constant_p(b)) > > #define __safe_cmp(a,b) \ > (__typecheck(a,b) && __no_side_effects(a,b)) > > #define __cmp(a,b,op) ((a)op(b)?(a):(b)) > > #define __cmp_once(a,b,op) ({ \ > typeof(a) __a = (a);\ > typeof(b) __b = (b);\ > __cmp(__a,__b,op); }) > > #define __careful_cmp(a,b,op) \ > __builtin_choose_expr(__safe_cmp(a,b), __cmp(a,b,op), > __cmp_once(a,b,op)) > > #define min(a,b) __careful_cmp(a,b,<) > #define max(a,b) __careful_cmp(a,b,>) > #define min_t(t,a,b) __careful_cmp((t)(a),(t)(b),<) > #define max_t(t,a,b) __careful_cmp((t)(a),(t)(b),>) > > and yes, it does cause new warnings for that > > comparison between ‘enum tis_defaults’ and ‘enum tpm2_const’ > > in drivers/char/tpm/tpm_tis_core.h due to > >#define TIS_TIMEOUT_A_MAX max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_A) > > but technically that warning is actually correct, I'm just confused > why gcc cares about the cast placement or something. > > That warning is easy to fix by turning it into a "max_t(int, enum1, > enum2)' and that is technically the right thing to do, it's just not > warned about for some odd reason with the current code. > > Kees - is there some online "gcc-4.4 checker" somewhere? This does > seem to work with my gcc. I actually tested some of those files you > pointed at now. I use this one: https://godbolt.org/ Cheers, Miguel
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Fri, Mar 16, 2018 at 10:55 AM, Al Viro wrote: > > That's not them, that's C standard regarding ICE. Yes. The C standard talks about "integer constant expression". I know. It's come up in this very thread before. The C standard at no point talks about - or forbids - "variable length arrays". That never comes up in the whole standard, I checked. So we are right now hindered by a _syntactic_ check, without any way to have a _semantic_ check. That's my problem. The warnings are misleading and imply semantics. And apparently there is no way to actually check semantics. > 1,100 is *not* a constant expression as far as the standard is concerned, I very much know. But it sure isn't "variable" either as far as the standard is concerned, because the standard doesn't even have that concept (it uses "variable" for argument numbers and for variables). So being pedantic doesn't really change anything. > Would you argue that in > void foo(char c) > { > int a[(c<<1) + 10 - c + 2 - c]; Yeah, I don't think that even counts as a constant value, even if it can be optimized to one. I would not at all be unhppy to see __builtin_constant_p() to return zero. But that is very much different from the syntax issue. So I would like to get some way to get both type-checking and constant checking without the annoying syntax issue. > expr, constant_expression is not a constant_expression. And in > this particular case the standard is not insane - the only reason > for using that is typechecking and _that_ can be achieved without > violating 6.6p6: > sizeof(expr,0) * 0 + ICE > *is* an integer constant expression, and it gives you exact same > typechecking. So if somebody wants to play odd games, they can > do that just fine, without complicating the logics for compilers... Now that actually looks like a good trick. Maybe we can use that instead of the comma expression that causes problems. And using sizeof() to make sure that __builtin_choose_expression() really gets an integer constant expression and that there should be no ambiguity looks good. Hmm. This works for me, and I'm being *very* careful (those casts to pointer types are inside that sizeof, because it's not an integral type, and non-integral casts are not valid in an ICE either) but somebody needs to check gcc-4.4: #define __typecheck(a,b) \ (!!(sizeof((typeof(a)*)1==(typeof(b)*)1))) #define __no_side_effects(a,b) \ (__builtin_constant_p(a)&&__builtin_constant_p(b)) #define __safe_cmp(a,b) \ (__typecheck(a,b) && __no_side_effects(a,b)) #define __cmp(a,b,op) ((a)op(b)?(a):(b)) #define __cmp_once(a,b,op) ({ \ typeof(a) __a = (a);\ typeof(b) __b = (b);\ __cmp(__a,__b,op); }) #define __careful_cmp(a,b,op) \ __builtin_choose_expr(__safe_cmp(a,b), __cmp(a,b,op), __cmp_once(a,b,op)) #define min(a,b) __careful_cmp(a,b,<) #define max(a,b) __careful_cmp(a,b,>) #define min_t(t,a,b) __careful_cmp((t)(a),(t)(b),<) #define max_t(t,a,b) __careful_cmp((t)(a),(t)(b),>) and yes, it does cause new warnings for that comparison between ‘enum tis_defaults’ and ‘enum tpm2_const’ in drivers/char/tpm/tpm_tis_core.h due to #define TIS_TIMEOUT_A_MAX max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_A) but technically that warning is actually correct, I'm just confused why gcc cares about the cast placement or something. That warning is easy to fix by turning it into a "max_t(int, enum1, enum2)' and that is technically the right thing to do, it's just not warned about for some odd reason with the current code. Kees - is there some online "gcc-4.4 checker" somewhere? This does seem to work with my gcc. I actually tested some of those files you pointed at now. Linus include/linux/kernel.h | 77 +- 1 file changed, 20 insertions(+), 57 deletions(-) diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 3fd291503576..23c31bf1d7fb 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -787,37 +787,29 @@ 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) ({ \ - t1 min1 = (x); \ - t2 min2 = (y); \ - (void) (&min1 == &min2); \ - min1 < min2 ? min1 : min2; }) +#define __typecheck(a,b) \ + (!!(sizeof((typeof(a)*)1==(typeof(b)*)1))) -/** - * 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 __no_side_effects(a,b) \ + (__builtin_constant_p(a)&&__builtin_constant_p(b)) -#define __max(t1, t2, max1, max2, x, y) ({ \ - t1 max1 = (x); \ - t2 max2 = (y); \ - (void) (&max1 == &max2); \ - max1 > max2
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Fri, Mar 16, 2018 at 05:55:02PM +, Al Viro wrote: > On Fri, Mar 16, 2018 at 10:29:16AM -0700, Linus Torvalds wrote: > >t.c: In function ‘test’: > >t.c:6:6: error: argument to variable-length array is too large > > [-Werror=vla-larger-than=] > > int array[(1,100)]; > > > > Gcc people are crazy. > > That's not them, that's C standard regarding ICE. 1,100 is *not* a > constant expression as far as the standard is concerned, and that > type is actually a VLA with the size that can be optimized into > a compiler-calculated value. > > Would you argue that in s/argue/agree/, sorry > void foo(char c) > { > int a[(c<<1) + 10 - c + 2 - c]; > > a is not a VLA? FWIW, 6.6 starts with constant-expression: conditional-expression for syntax, with 6.6p3 being "Constant expression shall not contain assignment, increment, decrement, function call or comma operators, except when they are contained in a subexpression that is not evaluated", with "The operand of sizeof operator is usually not evaluated (6.5.3.4)" as a footnote. 6.6p10 allows implementation to accept other forms of constant expressions, but arguing that such-and-such construct surely must be recognized as one, when there are perfectly portable ways to achieve the same... Realistically, code like that can come only from macros, and one can wrap the damn thing into 0 * sizeof(..., 0) + just fine there. Which will satisfy the conditions for sizeof argument not being evaluated...
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Fri, Mar 16, 2018 at 10:29:16AM -0700, Linus Torvalds wrote: >t.c: In function ‘test’: >t.c:6:6: error: argument to variable-length array is too large > [-Werror=vla-larger-than=] > int array[(1,100)]; > > Gcc people are crazy. That's not them, that's C standard regarding ICE. 1,100 is *not* a constant expression as far as the standard is concerned, and that type is actually a VLA with the size that can be optimized into a compiler-calculated value. Would you argue that in void foo(char c) { int a[(c<<1) + 10 - c + 2 - c]; a is not a VLA? Sure, compiler probably would be able to reduce that expression to 12, but demanding that to be recognized means that compiler must do a bunch of optimizations in the middle of typechecking. expr, constant_expression is not a constant_expression. And in this particular case the standard is not insane - the only reason for using that is typechecking and _that_ can be achieved without violating 6.6p6: sizeof(expr,0) * 0 + ICE *is* an integer constant expression, and it gives you exact same typechecking. So if somebody wants to play odd games, they can do that just fine, without complicating the logics for compilers...
RE: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Linus Torvalds > Sent: 16 March 2018 17:29 > On Fri, Mar 16, 2018 at 4:47 AM, Florian Weimer wrote: > > > > If you want to catch stack frames which have unbounded size, > > -Werror=stack-usage=1000 or -Werror=vla-larger-than=1000 (with the constant > > adjusted as needed) might be the better approach. > > No, we want to catch *variable* stack sizes. > > Does "-Werror=vla-larger-than=0" perhaps work for that? No, because > the stupid compiler says that is "meaningless". > > And no, using "-Werror=vla-larger-than=1" doesn't work either, because > the moronic compiler continues to think that "vla" is about the > _type_, not the code: > >t.c: In function ‘test’: >t.c:6:6: error: argument to variable-length array is too large > [-Werror=vla-larger-than=] > int array[(1,100)]; > > Gcc people are crazy. > > Is there really no way to just say "shut up about the stupid _syntax_ > issue that is entirely irrelevant, and give us the _code_ issue". I looked at the generated code for one of the constant sized VLA that the compiler barfed at. It seemed to subtract constants from %sp separately for the VLA. So it looks like the compiler treats them as VLA even though it knows the size. That is probably missing optimisation. David
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On 03/16/2018 06:29 PM, Linus Torvalds wrote: Gcc people are crazy. End of discussion from me. This is not acceptable. Florian
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Fri, Mar 16, 2018 at 4:47 AM, Florian Weimer wrote: > > If you want to catch stack frames which have unbounded size, > -Werror=stack-usage=1000 or -Werror=vla-larger-than=1000 (with the constant > adjusted as needed) might be the better approach. No, we want to catch *variable* stack sizes. Does "-Werror=vla-larger-than=0" perhaps work for that? No, because the stupid compiler says that is "meaningless". And no, using "-Werror=vla-larger-than=1" doesn't work either, because the moronic compiler continues to think that "vla" is about the _type_, not the code: t.c: In function ‘test’: t.c:6:6: error: argument to variable-length array is too large [-Werror=vla-larger-than=] int array[(1,100)]; Gcc people are crazy. Is there really no way to just say "shut up about the stupid _syntax_ issue that is entirely irrelevant, and give us the _code_ issue". Linus
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On 03/16/2018 05:25 AM, Kees Cook wrote: In the effort to remove all VLAs from the kernel[1], it is desirable to build with -Wvla. However, this warning is overly pessimistic, in that it is only happy with stack array sizes that are declared as constant expressions, and not constant values. One case of this is the evaluation of the max() macro which, due to its construction, ends up converting constant expression arguments into a constant value result. Attempts to adjust the behavior of max() ran afoul of version-dependent compiler behavior[2]. I find this commit message confusing. VLAs have precisely defined semantics which differ from other arrays, and these differences can be observable (maybe not in the kernel, but certainly for userspace), so the compiler has to treat a VLA as such even if the length is a constant known at compile time. (The original intent of the warning probably was a portability check anyway.) If you want to catch stack frames which have unbounded size, -Werror=stack-usage=1000 or -Werror=vla-larger-than=1000 (with the constant adjusted as needed) might be the better approach. Thanks, Florian
[PATCH v5 0/2] Remove false-positive VLAs when using max()
Patch 1 adds const_max_t(), patch 2 uses it in all the places max() was used for stack arrays. Commit log from patch 1: ---snip--- kernel.h: Introduce const_max_t() for VLA removal In the effort to remove all VLAs from the kernel[1], it is desirable to build with -Wvla. However, this warning is overly pessimistic, in that it is only happy with stack array sizes that are declared as constant expressions, and not constant values. One case of this is the evaluation of the max() macro which, due to its construction, ends up converting constant expression arguments into a constant value result. Attempts to adjust the behavior of max() ran afoul of version-dependent compiler behavior[2]. To work around this and still gain -Wvla coverage, this patch introduces a new macro, const_max_t(), for use in these cases of stack array size declaration, where the constant expressions are retained. Since this means losing the double-evaluation protections of the max() macro, this macro is designed to explicitly fail if used on non-constant arguments. Older compilers will fail with the unhelpful message: error: first argument to ‘__builtin_choose_expr’ not a constant Newer compilers will fail with a hopefully more helpful message: error: call to ‘__error_non_const_arg’ declared with attribute error: const_max_t() used with non-constant expression To gain the ability to compare differing types, the desired type must be explicitly declared, as with the existing max_t() macro. This is needed when comparing different enum types and to allow things like: int foo[const_max_t(size_t, 6, sizeof(something))]; [1] https://lkml.org/lkml/2018/3/7/621 [2] https://lkml.org/lkml/2018/3/10/170 ---eol--- Hopefully this reads well as a summary from all the things that got tried. I've tested this on allmodconfig builds with gcc 4.4.4 and 6.3.0, with and without -Wvla. -Kees v5: explicit type argument v4: forced size_t type