RE: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[David Laight]

From: Kees Cook
> Sent: 13 March 2018 22:15
...
> I'll send a "const_max()" which will refuse to work on
> non-constant-values (so it doesn't get accidentally used on variables
> that could be exposed to double-evaluation), and will work for stack
> array declarations (to avoid the overly-sensitive -Wvla checks).

ISTR the definitions were of the form:
char foo[max(sizeof (struct bah), sizeof (struct baz))];
This doesn't generate a 'foo' with the required alignment.
It would be much better to use a union.

David

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Kees Cook]

On Tue, Mar 13, 2018 at 2:02 PM, Andrew Morton
 wrote:
> On Mon, 12 Mar 2018 21:28:57 -0700 Kees Cook  wrote:
>
>> On Mon, Mar 12, 2018 at 4:57 PM, Linus Torvalds
>>  wrote:
>> > On Mon, Mar 12, 2018 at 3:55 PM, Andrew Morton
>> >  wrote:
>> >>
>> >> Replacing the __builtin_choose_expr() with ?: works of course.
>> >
>> > Hmm. That sounds like the right thing to do. We were so myopically
>> > staring at the __builtin_choose_expr() problem that we overlooked the
>> > obvious solution.
>> >
>> > Using __builtin_constant_p() together with a ?: is in fact our common
>> > pattern, so that should be fine. The only real reason to use
>> > __builtin_choose_expr() is if you want to get the *type* to vary
>> > depending on which side you choose, but that's not an issue for
>> > min/max.
>>
>> This doesn't solve it for -Wvla, unfortunately. That was the point of
>> Josh's original suggestion of __builtin_choose_expr().
>>
>> Try building with KCFLAGS=-Wval and checking net/ipv6/proc.c:
>>
>> net/ipv6/proc.c: In function ‘snmp6_seq_show_item’:
>> net/ipv6/proc.c:198:2: warning: ISO C90 forbids array ‘buff’ whose
>> size can’t be evaluated [-Wvla]
>>   unsigned long buff[SNMP_MIB_MAX];
>>   ^~~~
>
> PITA.  Didn't we once have a different way of detecting VLAs?  Some
> post-compilation asm parser, iirc.
>
> I suppose the world wouldn't end if we had a gcc version ifdef in
> kernel.h.  We'll get to remove it in, oh, ten years.

For fixing only 6 VLAs, we don't need all this effort. When it looked
like we could get away with just a "better" max(), sure. ;)

I'll send a "const_max()" which will refuse to work on
non-constant-values (so it doesn't get accidentally used on variables
that could be exposed to double-evaluation), and will work for stack
array declarations (to avoid the overly-sensitive -Wvla checks).

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Andrew Morton]

On Mon, 12 Mar 2018 21:28:57 -0700 Kees Cook  wrote:

> On Mon, Mar 12, 2018 at 4:57 PM, Linus Torvalds
>  wrote:
> > On Mon, Mar 12, 2018 at 3:55 PM, Andrew Morton
> >  wrote:
> >>
> >> Replacing the __builtin_choose_expr() with ?: works of course.
> >
> > Hmm. That sounds like the right thing to do. We were so myopically
> > staring at the __builtin_choose_expr() problem that we overlooked the
> > obvious solution.
> >
> > Using __builtin_constant_p() together with a ?: is in fact our common
> > pattern, so that should be fine. The only real reason to use
> > __builtin_choose_expr() is if you want to get the *type* to vary
> > depending on which side you choose, but that's not an issue for
> > min/max.
> 
> This doesn't solve it for -Wvla, unfortunately. That was the point of
> Josh's original suggestion of __builtin_choose_expr().
> 
> Try building with KCFLAGS=-Wval and checking net/ipv6/proc.c:
> 
> net/ipv6/proc.c: In function ‘snmp6_seq_show_item’:
> net/ipv6/proc.c:198:2: warning: ISO C90 forbids array ‘buff’ whose
> size can’t be evaluated [-Wvla]
>   unsigned long buff[SNMP_MIB_MAX];
>   ^~~~

PITA.  Didn't we once have a different way of detecting VLAs?  Some
post-compilation asm parser, iirc.

I suppose the world wouldn't end if we had a gcc version ifdef in
kernel.h.  We'll get to remove it in, oh, ten years.

RE: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[David Laight]

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

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Kees Cook]

On Mon, Mar 12, 2018 at 4:57 PM, Linus Torvalds
 wrote:
> On Mon, Mar 12, 2018 at 3:55 PM, Andrew Morton
>  wrote:
>>
>> Replacing the __builtin_choose_expr() with ?: works of course.
>
> Hmm. That sounds like the right thing to do. We were so myopically
> staring at the __builtin_choose_expr() problem that we overlooked the
> obvious solution.
>
> Using __builtin_constant_p() together with a ?: is in fact our common
> pattern, so that should be fine. The only real reason to use
> __builtin_choose_expr() is if you want to get the *type* to vary
> depending on which side you choose, but that's not an issue for
> min/max.

This doesn't solve it for -Wvla, unfortunately. That was the point of
Josh's original suggestion of __builtin_choose_expr().

Try building with KCFLAGS=-Wval and checking net/ipv6/proc.c:

net/ipv6/proc.c: In function ‘snmp6_seq_show_item’:
net/ipv6/proc.c:198:2: warning: ISO C90 forbids array ‘buff’ whose
size can’t be evaluated [-Wvla]
  unsigned long buff[SNMP_MIB_MAX];
  ^~~~


-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Linus Torvalds]

On Mon, Mar 12, 2018 at 3:55 PM, Andrew Morton
 wrote:
>
> Replacing the __builtin_choose_expr() with ?: works of course.

Hmm. That sounds like the right thing to do. We were so myopically
staring at the __builtin_choose_expr() problem that we overlooked the
obvious solution.

Using __builtin_constant_p() together with a ?: is in fact our common
pattern, so that should be fine. The only real reason to use
__builtin_choose_expr() is if you want to get the *type* to vary
depending on which side you choose, but that's not an issue for
min/max.

> What will be the runtime effects?

There should be none. Gcc will turn the conditional for the ?: into a
constant, and DTRT.

  Linus

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Andrew Morton]

On Fri, 9 Mar 2018 17:30:15 -0800 Kees Cook  wrote:

> > 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)

Replacing the __builtin_choose_expr() with ?: works of course.  What
will be the runtime effects?

I tried replacing

__builtin_choose_expr(__builtin_constant_p(x) &&
  __builtin_constant_p(y),

with

__builtin_choose_expr(__builtin_constant_p(x + y),

but no success.

I'm not sure what else to try to trick gcc into working.

--- 
a/include/linux/kernel.h~kernelh-skip-single-eval-logic-on-literals-in-min-max-v3-fix
+++ a/include/linux/kernel.h
@@ -804,13 +804,10 @@ static inline void ftrace_dump(enum ftra
  * 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))
+   ((__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
@@ -826,13 +823,11 @@ static inline void ftrace_dump(enum ftra
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))
+   ((__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
_

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Tobin C. Harding]

On Fri, Mar 09, 2018 at 01:10:30PM -0800, Linus Torvalds wrote:
> On Fri, Mar 9, 2018 at 12:05 PM, Kees Cook  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.

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Linus Torvalds]

On Sun, Mar 11, 2018 at 4:05 AM, Ingo Molnar  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

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Ingo Molnar]

* Linus Torvalds  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

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Miguel Ojeda]

On Sat, Mar 10, 2018 at 6:51 PM, Linus Torvalds
 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

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Linus Torvalds]

On Sat, Mar 10, 2018 at 9:34 AM, Miguel Ojeda
 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

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Miguel Ojeda]

On Sat, Mar 10, 2018 at 5:30 PM, Linus Torvalds
 wrote:
> On Sat, Mar 10, 2018 at 7:33 AM, Kees Cook  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

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Linus Torvalds]

On Sat, Mar 10, 2018 at 7:33 AM, Kees Cook  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

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Linus Torvalds]

On Sat, Mar 10, 2018 at 7:33 AM, Kees Cook  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] 

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Linus Torvalds]

On Fri, Mar 9, 2018 at 11:03 PM, Miguel Ojeda
 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

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Kees Cook]

On Fri, Mar 9, 2018 at 10:10 PM, Miguel Ojeda
 wrote:
> On Sat, Mar 10, 2018 at 4:11 AM, Randy Dunlap  wrote:
>> On 03/09/2018 04:07 PM, Andrew Morton wrote:
>>> On Fri, 9 Mar 2018 12:05:36 -0800 Kees Cook  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] *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

-- 
Kees Cook
Pixel Security

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Miguel Ojeda]

On Sat, Mar 10, 2018 at 7:10 AM, Miguel Ojeda
 wrote:
> On Sat, Mar 10, 2018 at 4:11 AM, Randy Dunlap  wrote:
>> On 03/09/2018 04:07 PM, Andrew Morton wrote:
>>> On Fri, 9 Mar 2018 12:05:36 -0800 Kees Cook  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

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Miguel Ojeda]

On Sat, Mar 10, 2018 at 4:11 AM, Randy Dunlap  wrote:
> On 03/09/2018 04:07 PM, Andrew Morton wrote:
>> On Fri, 9 Mar 2018 12:05:36 -0800 Kees Cook  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

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Randy Dunlap]

On 03/09/2018 04:07 PM, Andrew Morton wrote:
> On Fri, 9 Mar 2018 12:05:36 -0800 Kees Cook  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?


-- 
~Randy

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Linus Torvalds]

On Fri, Mar 9, 2018 at 5:31 PM, Kees Cook  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

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Kees Cook]

On Fri, Mar 9, 2018 at 5:30 PM, Kees Cook  wrote:
> --
> Kees Cook
> Pixel SecurityOn
> [...]

WTF, gmail just blasted HTML into my explicitly plain-text email?! Apologies...

-- 
Kees Cook
Pixel SecurityOn
Fri, Mar 9, 2018 at 5:30 PM, Kees Cook mailto:keesc...@chromium.org;
target="_blank">keesc...@chromium.org
wrote:On
Fri, Mar 9, 2018 at 4:38 PM, Linus Torvalds
mailto:torva...@linux-foundation.org;>torva...@linux-foundation.org
wrote:
 On Fri, Mar 9, 2018 at 4:32 PM, Andrew Morton mailto:a...@linux-foundation.org;>a...@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

--
Kees Cook
Pixel Securitydiv class="gmail_extra"brdiv
class="gmail_quote"On
Fri, Mar 9, 2018 at 4:38 PM, Linus Torvalds span
dir="ltr"lt;a
href="mailto:mailto:torva...@linux-foundation.org;>torvalds@linux-foundation.org"
target="_blank"mailto:torva...@linux-foundation.org;>torvalds@linux-foundation.org/agt;/span
wrote:brblockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex"span
class=""On
Fri, Mar 9, 2018 at 4:32 PM, Andrew Morton lt;a
href="mailto:mailto:a...@linux-foundation.org;>akpm@linux-foundation.org"mailto:a...@linux-foundation.org;>akpm@linux-foundation.org/agt;
wrote:br
gt;br
gt; I wonder which gcc versions actually accept Kees's
addition.br
br
/spanNote that we already do have this pattern, as seen
by:br
br
nbsp; git grep -2nbsp; __builtin_choose_expr | grep -2
__builtin_constant_pbr
br
which show drivers/pinctrl/intel/pinctrl-wbrintel.h, so
the patternbr
already exists current kernels and _works_ - it apparently justbr
doesn't work in slightly more complicated cases.br
br
It's one reason why I wondered if simplifying the expression to
havebr
just that single __builtin_constant_p() might not end up working..br
span
class="HOEnZb"font color="#88"br
nbsp; nbsp; nbsp; nbsp; nbsp; nbsp;
nbsp; Linusbr
/font/span/blockquote/divbrbr
clear="all"divbr/div--
brdiv class="gmail_signature"
data-smartmail="gmail_signature"Kees
CookbrPixel Security/div
/div
--
Kees
CookPixel Security

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Kees Cook]

On Fri, Mar 9, 2018 at 4:38 PM, Linus Torvalds
 wrote:
> On Fri, Mar 9, 2018 at 4:32 PM, Andrew Morton  
> 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

-- 
Kees Cook
Pixel SecurityOn
Fri, Mar 9, 2018 at 4:38 PM, Linus Torvalds mailto:torva...@linux-foundation.org;
target="_blank">torva...@linux-foundation.org
wrote:On
Fri, Mar 9, 2018 at 4:32 PM, Andrew Morton mailto:a...@linux-foundation.org;>a...@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
--
Kees
CookPixel Security

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Linus Torvalds]

On Fri, Mar 9, 2018 at 4:32 PM, Andrew Morton  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

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Andrew Morton]

On Fri, 9 Mar 2018 16:28:51 -0800 Linus Torvalds 
 wrote:

> On Fri, Mar 9, 2018 at 4:07 PM, Andrew Morton  
> 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.

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Linus Torvalds]

On Fri, Mar 9, 2018 at 4:07 PM, Andrew Morton  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

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Andrew Morton]

On Fri, 9 Mar 2018 12:05:36 -0800 Kees Cook  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?

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Kees Cook]

On Fri, Mar 9, 2018 at 1:10 PM, Linus Torvalds
 wrote:
> On Fri, Mar 9, 2018 at 12:05 PM, Kees Cook  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

-- 
Kees Cook
Pixel SecurityOn
Fri, Mar 9, 2018 at 1:10 PM, Linus Torvalds mailto:torva...@linux-foundation.org;
target="_blank">torva...@linux-foundation.org
wrote:On
Fri, Mar 9, 2018 at 12:05 PM, Kees Cook mailto:keesc...@chromium.org;>keesc...@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 

Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

[Linus Torvalds]

On Fri, Mar 9, 2018 at 12:05 PM, Kees Cook  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