| Message ID | 20180309200536.GA5670@beast (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 9, 2018 at 12:05 PM, 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: Ok, looks good. I just have a couple of questions about applying it. In particular, if this will help people working on getting rid of VLA's in the short term, I can apply it directly. But if people who are looking at it (anybody else than Kees?) don't much care, then this might be a 4.17 thing or at least "random -mm queue"? The other unrelated reaction I had to this was that "we're passing those types down very deep, even though nobody _cares_ about them all that much at that deep level". Honestly, the only case that really cares is the very top level, and the rest could just take the properly cast versions. For example, "max_t/min_t" really don't care at all, since they - by definition - just take the single specified type. So I'm wondering if we should just drop the types from __max/__min (and everything they call) entirely, and instead do #define __check_type(x,y) ((void)((typeof(x)*)1==(typeof(y)*)1)) #define min(x,y) (__check_type(x,y),__min(x,y)) #define max(x,y) (__check_type(x,y),__max(x,y)) #define min_t(t,x,y) __min((t)(x),(t)(y)) #define max_t(t,x,y) __max((t)(x),(t)(y)) and then __min/__max and friends are much simpler (and can just assume that the type is already fine, and the casting has been done). This is technically entirely independent of this VLA cleanup thing, but the "passing the types around unnecessarily" just becomes more obvious when there's now another level of macros, _and_ it's a more complex expression too. Yes, yes, the __single_eval_xyz() functions still end up wanting the types for the declaration of the new single-evaluation variables, but the 'typeof' pattern is the standard pattern, so #define __single_eval_max(max1, max2, x, y) ({ \ typeof (x) max1 = (x); \ typeof (y) max2 = (y); \ max1 > max2 ? max1 : max2; }) actually looks more natural to me than passing the two types in as arguments to the macro. (That form also is amenable to things like "__auto_type" etc simplifications). Side note: do we *really* need the unique variable names? That's what makes those things _really_ illegible. I thgink it's done just for a sparse warning that we should probably ignore. It's off by default anyway exactly because it doesn't work that well due to nested macro expansions like this. There is very real value to keeping our odd macros legible, I feel. Even when they are complicated by issues like this, it would be good to keep them as simple as possible. Comments? Linus
On Fri, Mar 9, 2018 at 1:10 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Fri, Mar 9, 2018 at 12:05 PM, 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: > > Ok, looks good. > > I just have a couple of questions about applying it. > > In particular, if this will help people working on getting rid of > VLA's in the short term, I can apply it directly. AFAIK, all the VLAs effected by max() are fixed with this patch. i.e. no other VLA work I'm aware of depends on this (famous last words). > But if people who are looking at it (anybody else than Kees?) FWIW, I've seen at least 6 other people helping out now with VLA clean up patches. Whee. :) > don't much care, then this > might be a 4.17 thing or at least "random -mm queue"? Andrew has this (v2) queued in -mm for 4.17. I didn't view VLA clean up (or this macro change) as urgent by any means. > #define __single_eval_max(max1, max2, x, y) ({ \ > typeof (x) max1 = (x); \ > typeof (y) max2 = (y); \ > max1 > max2 ? max1 : max2; }) > > actually looks more natural to me than passing the two types in as > arguments to the macro. I (obviously) didn't design this macro originally, but my take on this was that it's safer to keep the type check together with the comparison so that it is never possible for someone to run off and use __single_eval_max() directly and accidentally bypass the type check. While it does look silly from the max_t()/min_t() perspective, it just seems more defensive. > (That form also is amenable to things like "__auto_type" etc simplifications). Agreed, I think that's the biggest reason to lift the types. However, since we're still not actually doing anything with the types (i.e. this change doesn't weaken the type checking), I would think it would be orthogonal. While it would be nice to resolve differing types sanely, there doesn't appear to be a driving reason to make this change. The example case of max(5, sizeof(thing)) doesn't currently exist in the code and I don't see how making min()/max() _less_ strict would be generically useful (in fact, it has proven useful to have strict type checking). > Side note: do we *really* need the unique variable names? That's what > makes those things _really_ illegible. I thgink it's done just for a > sparse warning that we should probably ignore. It's off by default > anyway exactly because it doesn't work that well due to nested macro > expansions like this. That I'm not sure about. I'd be fine to remove them; I left them in place because it seemed quite intentional. :) > There is very real value to keeping our odd macros legible, I feel. > Even when they are complicated by issues like this, it would be good > to keep them as simple as possible. > > Comments? I'm open to whatever! I'm just glad this gets to kill a handful of "accidental" stack VLAs. :) -Kees
On Fri, 9 Mar 2018 12:05:36 -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. v1, v2 and v3 of this patch all fail with gcc-4.4.4: ./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 That's with #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), x, y) A brief poke failed to reveal a workaround - gcc-4.4.4 doesn't appear to know that __builtin_constant_p(x) is a constant. Or something. Sigh. Wasn't there some talk about modernizing our toolchain requirements?
On Fri, Mar 9, 2018 at 4:07 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > > A brief poke failed to reveal a workaround - gcc-4.4.4 doesn't appear > to know that __builtin_constant_p(x) is a constant. Or something. LOL. I suspect it might be that it wants to evaluate __builtin_choose_expr() at an earlier stage than it evaluates __builtin_constant_p(), so it's not that it doesn't know that __builtin_constant_p() is a constant, it just might not know it *yet*. Maybe. Side note, if it's not that, but just the "complex" expression that has the logical 'and' etc, maybe the code could just use __builtin_constant_p((x)+(y)) or something. But yeah: > Sigh. Wasn't there some talk about modernizing our toolchain > requirements? Maybe it's just time to give up on 4.4. We wanted 4.5 for "asm goto", and once we upgrade to 4.5 I think Arnd said that no distro actually ships it, so we might as well go to 4.6. So maybe this is just the excuse to finally make that official, if there is no clever workaround any more. Linus
On Fri, 9 Mar 2018 16:28:51 -0800 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Fri, Mar 9, 2018 at 4:07 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > > > > A brief poke failed to reveal a workaround - gcc-4.4.4 doesn't appear > > to know that __builtin_constant_p(x) is a constant. Or something. > > LOL. > > I suspect it might be that it wants to evaluate > __builtin_choose_expr() at an earlier stage than it evaluates > __builtin_constant_p(), so it's not that it doesn't know that > __builtin_constant_p() is a constant, it just might not know it *yet*. > > Maybe. > > Side note, if it's not that, but just the "complex" expression that > has the logical 'and' etc, maybe the code could just use > > __builtin_constant_p((x)+(y)) > > or something. I'll do a bit more poking at it. > But yeah: > > > Sigh. Wasn't there some talk about modernizing our toolchain > > requirements? > > Maybe it's just time to give up on 4.4. We wanted 4.5 for "asm goto", > and once we upgrade to 4.5 I think Arnd said that no distro actually > ships it, so we might as well go to 4.6. > > So maybe this is just the excuse to finally make that official, if > there is no clever workaround any more. I wonder which gcc versions actually accept Kees's addition.
On Fri, Mar 9, 2018 at 4:32 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > > I wonder which gcc versions actually accept Kees's addition. Note that we already do have this pattern, as seen by: git grep -2 __builtin_choose_expr | grep -2 __builtin_constant_p which show drivers/pinctrl/intel/pinctrl-intel.h, so the pattern already exists current kernels and _works_ - it apparently just doesn't work in slightly more complicated cases. It's one reason why I wondered if simplifying the expression to have just that single __builtin_constant_p() might not end up working.. Linus
On Fri, Mar 9, 2018 at 4:38 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Fri, Mar 9, 2018 at 4:32 PM, Andrew Morton <akpm@linux-foundation.org> wrote: >> >> I wonder which gcc versions actually accept Kees's addition. Ah, my old nemesis, gcc 4.4.4. *sob* > Note that we already do have this pattern, as seen by: > > git grep -2 __builtin_choose_expr | grep -2 __builtin_constant_p > > which show drivers/pinctrl/intel/pinctrl-intel.h, so the pattern > already exists current kernels and _works_ - it apparently just > doesn't work in slightly more complicated cases. Fun. Yeah, so all the PIN_GROUP() callers have either a literal or an array name for that argument, so the argument to __builtin_constant_p() isn't complex. git grep '\bPIN_GROUP\b' | egrep -v '(1|2|3|true|false)\)' > It's one reason why I wondered if simplifying the expression to have > just that single __builtin_constant_p() might not end up working.. Yeah, it seems like it doesn't bail out as "false" for complex expressions given to __builtin_constant_p(). If no magic solution, then which of these? - go back to my original "multi-eval max only for constants" macro (meh) - add gcc version checks around this and similarly for -Wvla in the future (eww) - raise gcc version (yikes) -Kees
On Fri, Mar 9, 2018 at 5:30 PM, Kees Cook <keescook@chromium.org> wrote: > -- > Kees Cook > Pixel Security<div class="gmail_extra"><br><div class="gmail_quote">On > [...] WTF, gmail just blasted HTML into my explicitly plain-text email?! Apologies...
On Fri, Mar 9, 2018 at 5:31 PM, Kees Cook <keescook@chromium.org> wrote: > > WTF, gmail just blasted HTML into my explicitly plain-text email?! Apologies... There's more now in your email, I think maybe it's triggered by your signature file and some gmail web interface bug. Or it just tries to force quote using html. There's some html email disease inside google, where some parts of the company seems to think that html email is _good_. The official gmail Android app is a big pain, int hat it doesn't *have* a plain-text mode, even though it knows how to format the plain-text part of the email. You might want to be on the lookout for some bad drugs at the office. Because the world would thank you if you took them away from the gmail app people. Linus
On Fri, 09 Mar 2018 21:34:45 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> 2 files changed, 1050 insertions(+), 1273 deletions(-)
BTW, it's a bit bigger impact than 223 deletions. As I added a lot of
comments to explain the logic better. Removing comments and white space
from the modifications we have:
649 insertions(+), 1035 deletions(-)
That's 386 lines of code removed.
-- Steve
On 03/09/2018 04:07 PM, Andrew Morton wrote: > On Fri, 9 Mar 2018 12:05:36 -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. > > v1, v2 and v3 of this patch all fail with gcc-4.4.4: > > ./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 I'm seeing that problem with > gcc --version gcc (SUSE Linux) 4.8.5 in mmotm. > That's with > > #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), x, y) > > > A brief poke failed to reveal a workaround - gcc-4.4.4 doesn't appear > to know that __builtin_constant_p(x) is a constant. Or something. > > Sigh. Wasn't there some talk about modernizing our toolchain > requirements?
I don't know what the hell happened, but claws mail just inserted a ton of people into the Cc (I didn't add you). I noticed it just after I hit send. The added Cc looks like it came from the email right after the email I was replying to "Subject: Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()". Sorry for the spam. -- Steve On Fri, 9 Mar 2018 22:10:19 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > On Fri, 09 Mar 2018 21:34:45 -0500 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > 2 files changed, 1050 insertions(+), 1273 deletions(-) > > BTW, it's a bit bigger impact than 223 deletions. As I added a lot of > comments to explain the logic better. Removing comments and white space > from the modifications we have: > > 649 insertions(+), 1035 deletions(-) > > That's 386 lines of code removed. > > -- Steve '
On Fri, 9 Mar 2018 22:15:23 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> Sorry for the spam.
A little more spam ;-)
I know what happened, as I'm able to recreate it. For those that use
claws-mail, be careful.
I clicked on the email I wanted to reply to. Then I must have hit the
down arrow key, as it moved the selected email to the next email. I hit
"Reply All", and it showed the email I clicked on (as well as the
subject), but the Cc list belonged to the email that was selected by the
down arrow key.
That's a nasty subtle bug. I think I'll go report this to the claws
folks.
-- Steve
On Sat, Mar 10, 2018 at 4:11 AM, Randy Dunlap <rdunlap@infradead.org> wrote: > On 03/09/2018 04:07 PM, Andrew Morton wrote: >> On Fri, 9 Mar 2018 12:05:36 -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. >> >> v1, v2 and v3 of this patch all fail with gcc-4.4.4: >> >> ./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 > > > I'm seeing that problem with >> gcc --version > gcc (SUSE Linux) 4.8.5 Same here, 4.8.5 fails. gcc 5.4.1 seems to work. I compiled a minimal 5.1.0 and it seems to work as well. Cheers, Miguel
On Sat, Mar 10, 2018 at 7:10 AM, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > On Sat, Mar 10, 2018 at 4:11 AM, Randy Dunlap <rdunlap@infradead.org> wrote: >> On 03/09/2018 04:07 PM, Andrew Morton wrote: >>> On Fri, 9 Mar 2018 12:05:36 -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. >>> >>> v1, v2 and v3 of this patch all fail with gcc-4.4.4: >>> >>> ./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 >> >> >> I'm seeing that problem with >>> gcc --version >> gcc (SUSE Linux) 4.8.5 > > Same here, 4.8.5 fails. gcc 5.4.1 seems to work. I compiled a minimal > 5.1.0 and it seems to work as well. > Just compiled 4.9.0 and it seems to work -- so that would be the minimum required. Sigh... Some enterprise distros are either already shipping gcc >= 5 or will probably be shipping it soon (e.g. RHEL 8), so how much does it hurt to ask for a newer gcc? Are there many users/companies out there using enterprise distributions' gcc to compile and run the very latest kernels? Miguel
On Fri, Mar 9, 2018 at 10:10 PM, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > On Sat, Mar 10, 2018 at 4:11 AM, Randy Dunlap <rdunlap@infradead.org> wrote: >> On 03/09/2018 04:07 PM, Andrew Morton wrote: >>> On Fri, 9 Mar 2018 12:05:36 -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. >>> >>> v1, v2 and v3 of this patch all fail with gcc-4.4.4: >>> >>> ./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 >> >> >> I'm seeing that problem with >>> gcc --version >> gcc (SUSE Linux) 4.8.5 > > Same here, 4.8.5 fails. gcc 5.4.1 seems to work. I compiled a minimal > 5.1.0 and it seems to work as well. And sparse freaks out too: drivers/net/ethernet/via/via-velocity.c:97:26: sparse: incorrect type in initializer (different address spaces) @@ expected void *addr @@ got struct mac_regs [noderef] <avoid *addr @@ drivers/net/ethernet/via/via-velocity.c:97:26: expected void *addr drivers/net/ethernet/via/via-velocity.c:97:26: got struct mac_regs [noderef] <asn:2>*mac_regs drivers/net/ethernet/via/via-velocity.c:100:49: sparse: incorrect type in argument 2 (different base types) @@ expected restricted pci_power_t [usertype] state @@ got _t [usertype] state @@ Alright, I'm giving up on fixing max(). I'll go back to STACK_MAX() or some other name for the simple macro. Bleh. -Kees
On Fri, Mar 9, 2018 at 11:03 PM, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > Just compiled 4.9.0 and it seems to work -- so that would be the > minimum required. > > Sigh... > > Some enterprise distros are either already shipping gcc >= 5 or will > probably be shipping it soon (e.g. RHEL 8), so how much does it hurt > to ask for a newer gcc? Are there many users/companies out there using > enterprise distributions' gcc to compile and run the very latest > kernels? I wouldn't mind upping the compiler requirements, and we have other reasons to go to 4.6. But _this_ particular issue doesn't seem worth it to then go even further. Annoying. Linus
On Sat, Mar 10, 2018 at 7:33 AM, Kees Cook <keescook@chromium.org> wrote: > > And sparse freaks out too: > > drivers/net/ethernet/via/via-velocity.c:97:26: sparse: incorrect > type in initializer (different address spaces) @@ expected void > *addr @@ got struct mac_regs [noderef] <avoid *addr @@ Actually, this seems a valid warning - it's assigning a __iomem pointer to a regular void, and dropping the iomem in the process. Sparse does *not* like VLA's, so what may be going on is that fixing something VLA-related in your tree just makes sparse (correctly) check something that it had given up on before. So don't worry about the sparse ones, if they are new. They seem correct. Linus
On Sat, Mar 10, 2018 at 7:33 AM, Kees Cook <keescook@chromium.org> wrote: > > Alright, I'm giving up on fixing max(). I'll go back to STACK_MAX() or > some other name for the simple macro. Bleh. Oh, and I'm starting to see the real problem. It's not that our current "min/max()" are broiken. It's that "-Wvla" is garbage. Lookie here: int array[(1,2)]; results in gcc saying warning: ISO C90 forbids variable length array ‘array’ [-Wvla] int array[(1,2)]; ^~~ and that error message - and the name of the flag - is obviously pure garbage. What is *actually* going on is that ISO C90 requires an array size to be not a constant value, but a constant *expression*. Those are two different things. A constant expression has little to do with "compile-time constant". It's a more restricted form of it, and has actual syntax requirements. A comma expression is not a constant expression, for example, which was why I tested this. So "-Wvla" is garbage, with a misleading name, and a misleading warning string. It has nothing to do with "variable length" and whether the compiler can figure it out at build time, and everything to do with a _syntax_ rule. Linus
On Sat, Mar 10, 2018 at 5:30 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sat, Mar 10, 2018 at 7:33 AM, Kees Cook <keescook@chromium.org> wrote: >> >> Alright, I'm giving up on fixing max(). I'll go back to STACK_MAX() or >> some other name for the simple macro. Bleh. > > Oh, and I'm starting to see the real problem. > > It's not that our current "min/max()" are broiken. It's that "-Wvla" is garbage. > > Lookie here: > > int array[(1,2)]; > > results in gcc saying > > warning: ISO C90 forbids variable length array ‘array’ [-Wvla] > int array[(1,2)]; > ^~~ > > and that error message - and the name of the flag - is obviously pure garbage. > > What is *actually* going on is that ISO C90 requires an array size to > be not a constant value, but a constant *expression*. Those are two > different things. > > A constant expression has little to do with "compile-time constant". > It's a more restricted form of it, and has actual syntax requirements. > A comma expression is not a constant expression, for example, which > was why I tested this. > > So "-Wvla" is garbage, with a misleading name, and a misleading > warning string. It has nothing to do with "variable length" and > whether the compiler can figure it out at build time, and everything > to do with a _syntax_ rule. The warning string is basically the same to the one used for C++, i.e.: int size2 = 2; constexpr int size3 = 2; int array1[(2,2)]; int array2[(size2, size2)]; int array3[(size3, size3)]; only warns for array2 with: warning: ISO C++ forbids variable length array 'array2' [-Wvla] int array2[(size2, size2)]; So the warning is probably implemented to just trigger whenever VLAs are used but the given standard does not allow them, for all languages. The problem is why the ISO C90 frontend is not giving an error for using invalid syntax for array sizes to begin with? Miguel
On Sat, Mar 10, 2018 at 9:34 AM, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > So the warning is probably implemented to just trigger whenever VLAs > are used but the given standard does not allow them, for all > languages. The problem is why the ISO C90 frontend is not giving an > error for using invalid syntax for array sizes to begin with? So in *historical* context - when a compiler didn't do variable length arrays at all - the original semantics of C "constant expressions" actually make a ton of sense. You can basically think of a constant expression as something that can be (historically) handled by the front-end without any real complexity, and no optimization phases - just evaluating a simple parse tree with purely compile-time constants. So there's a good and perfectly valid reason for why C limits certain expressions to just a very particular subset. It's not just array sizes, it's case statements etc too. And those are very much part of the C standard. So an error message like warning: ISO C90 requires array sizes to be constant-expressions would be technically correct and useful from a portability angle. It tells you when you're doing something non-portable, and should be automatically enabled with "-ansi -pedantic", for example. So what's misleading is actually the name of the warning and the message, not that it happens. The warning isn't about "variable length", it's literally about the rules for what a "constant-expression" is. And in C, the expression (2,3) has a constant _value_ (namely 3), but it isn't a constant-expression as specified by the language. Now, the thing is that once you actually do variable length arrays, those old front-end rules make no sense any more (outside of the "give portability warnings" thing). Because once you do variable length arrays, you obviously _parse_ everything just fine, and you're doing to evaluate much more complex expressions than some limited constant-expression rule. And at that point it would make a whole lot more sense to add a *new* warning that basically says "I have to generate code for a variable-sized array", if you actually talk about VLA's. But that's clearly not what gcc actually did. So the problem really is that -Wvla doesn't actually warn about VLA's, but about something technically completely different. And that's why those stupid syntactic issues with min/max matter. It's not whether the end result is a compile-time constant or not, it's about completely different issues, like whether there is a comma-expression in it. Linus
On Sat, Mar 10, 2018 at 6:51 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > So in *historical* context - when a compiler didn't do variable length > arrays at all - the original semantics of C "constant expressions" > actually make a ton of sense. > > You can basically think of a constant expression as something that can > be (historically) handled by the front-end without any real > complexity, and no optimization phases - just evaluating a simple > parse tree with purely compile-time constants. > > So there's a good and perfectly valid reason for why C limits certain > expressions to just a very particular subset. It's not just array > sizes, it's case statements etc too. And those are very much part of > the C standard. > > So an error message like > > warning: ISO C90 requires array sizes to be constant-expressions > > would be technically correct and useful from a portability angle. It > tells you when you're doing something non-portable, and should be > automatically enabled with "-ansi -pedantic", for example. > > So what's misleading is actually the name of the warning and the > message, not that it happens. The warning isn't about "variable > length", it's literally about the rules for what a > "constant-expression" is. > > And in C, the expression (2,3) has a constant _value_ (namely 3), but > it isn't a constant-expression as specified by the language. > > Now, the thing is that once you actually do variable length arrays, > those old front-end rules make no sense any more (outside of the "give > portability warnings" thing). > > Because once you do variable length arrays, you obviously _parse_ > everything just fine, and you're doing to evaluate much more complex > expressions than some limited constant-expression rule. > > And at that point it would make a whole lot more sense to add a *new* > warning that basically says "I have to generate code for a > variable-sized array", if you actually talk about VLA's. > > But that's clearly not what gcc actually did. > > So the problem really is that -Wvla doesn't actually warn about VLA's, > but about something technically completely different. > I *think* I followed your reasoning. For gcc, -Wvla is the "I have to generate code for a variable-sized array" one; but in this case, the array size is the actual issue that you would have liked to be warned about; since people writing: int a[(2,3)]; did not really mean to declare a VLA. Therefore, you say warning them about the "warning: ISO C90 requires array sizes to be constant-expressions" (let's call it -Wpedantic-array-sizes) would be more helpful here instead of saying stuff about VLAs. In my case, I was just expecting gcc to give us both warnings and that's it, instead of trying to be smart and give only the -Wpedantic-array-sizes one (which is the one I was wondering in my previous email about why it was missing). I think it would be clear enough if both warnings are shown are the same time. And it makes sense, since if you write that line in ISO C90 it means there really are 2 things going wrong in the end (fishy syntax while in ISO C90 mode and, due to that, VLA code generated as well), no? Thanks for taking the time to write about the historical context, by the way! Miguel > And that's why those stupid syntactic issues with min/max matter. It's > not whether the end result is a compile-time constant or not, it's > about completely different issues, like whether there is a > comma-expression in it. > > Linus
* Linus Torvalds <torvalds@linux-foundation.org> wrote: > So an error message like > > warning: ISO C90 requires array sizes to be constant-expressions > > would be technically correct and useful from a portability angle. It > tells you when you're doing something non-portable, and should be > automatically enabled with "-ansi -pedantic", for example. > > So what's misleading is actually the name of the warning and the > message, not that it happens. The warning isn't about "variable > length", it's literally about the rules for what a > "constant-expression" is. > > And in C, the expression (2,3) has a constant _value_ (namely 3), but > it isn't a constant-expression as specified by the language. > > Now, the thing is that once you actually do variable length arrays, > those old front-end rules make no sense any more (outside of the "give > portability warnings" thing). > > Because once you do variable length arrays, you obviously _parse_ > everything just fine, and you're doing to evaluate much more complex > expressions than some limited constant-expression rule. BTW., while I fully agree with everything you said, it's not entirely correct to claim that if a C compiler can generate VLA code it is necessarily able to parse and evaluate constant array sizes "just fine". Constant expressions are typically parsed very early on, at the preprocessing stage. They can be used with some preprocessor directives as well, such as '#if' (with some further limitations on their syntax). If VLA support is implemented in a later stage, and results in heavy-handed code generation that will technically work for constant value expressions as well but results in suboptimal code, then a warning should probably be emitted - and it wouldn't be pedantic. The existing warning is still very misleading: warning: ISO C90 forbids variable length array ‘array’ [-Wvla] ... and if my above theory is correct then I think a better warning would be something like: warning: Array declaration is not a C90 constant expression, resulting in VLA code generation ... and note that in this specific case it's not misleading to talk about VLAs in the warning text, because the array size, even if it's constant value, results in VLA code generation. I don't know whether GCC has such a limitation, but a quick experiment with GCC 7.2 suggests that a (2,3) array size expression results in a lot more code being generated than with a constant expression. Thanks, Ingo
On Sun, Mar 11, 2018 at 4:05 AM, Ingo Molnar <mingo@kernel.org> wrote: > > BTW., while I fully agree with everything you said, it's not entirely correct to > claim that if a C compiler can generate VLA code it is necessarily able to parse > and evaluate constant array sizes "just fine". > > Constant expressions are typically parsed very early on, at the preprocessing > stage. They can be used with some preprocessor directives as well, such as '#if' > (with some further limitations on their syntax). Yes. But constant simplification and CSE etc is just a very fundamental part of a compiler, and anybody who actually implements VLA's would have to do it anyway. So no, a message like warning: Array declaration is not a C90 constant expression, resulting in VLA code generation would be moronic. Only some completely mindless broken shit would do "oh, it's not a parse-time constant, so it will be variable". The two just do not follow AT ALL. So the message might be about _possibly_ resulting in VLA code generation, but honestly, at that point you should just add the warning when you actually generate the code to do the stack allocation. Because at that point, you know whether it's variable or not. And trust me, it won't be variable for things like (2,3), or even for our "max()" thing with other odd builtins. Not unless the compiler doesn't really support VLA at all (maybe some bolted-on crazy thing that just turns a VLA at the front-end time into just an alloca), or the user has explicitly asked to disable some fundamental optimization phase. Linus
On Fri, Mar 09, 2018 at 01:10:30PM -0800, Linus Torvalds wrote: > On Fri, Mar 9, 2018 at 12:05 PM, 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: > > Ok, looks good. > > I just have a couple of questions about applying it. > > In particular, if this will help people working on getting rid of > VLA's in the short term, I can apply it directly. But if people who > are looking at it (anybody else than Kees?) don't much care, then this > might be a 4.17 thing or at least "random -mm queue"? It's easy enough to work on the other VLA removals without basing on this, no rush. thanks, Tobin.
The amount of replicated defined could also be reduced by passing > or < to a min_max() macro. So you start off with something like: #define min(x, y) __min_max(x, <, y) #define max(x, y) __min_max(x, >, y) then have: #define __min_max(x, cond, y) ((x) cond (y) ? (x) : (y)) in all its associated flavours. David
diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 3fd291503576..a0fca4deb3ab 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -787,37 +787,55 @@ 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 compile-time 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), \ + (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y), \ + __single_eval_min(t1, t2, \ + __UNIQUE_ID(min1_), \ + __UNIQUE_ID(min2_), \ + 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), \ + (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 @@ -869,10 +887,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } * @x: first value * @y: second value */ -#define min_t(type, x, y) \ - __min(type, type, \ - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ - x, y) +#define min_t(type, x, y) __min(type, type, x, y) /** * max_t - return maximum of two values, using the specified type @@ -880,10 +895,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } * @x: first value * @y: second value */ -#define max_t(type, x, y) \ - __max(type, type, \ - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ - x, y) +#define max_t(type, x, y) __max(type, type, x, y) /** * clamp_t - return a value clamped to a given range using a given type
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> --- v3: - drop __builtin_types_compatible_p() (Rasmus, Linus) v2: - fix copy/paste-o max1_/max2_ (ijc) - clarify "compile-time" constant in comment (Rasmus) - clean up formatting on min_t()/max_t() --- include/linux/kernel.h | 48 ++++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 18 deletions(-)