Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal

[Rasmus Villemoes]

On 2018-03-16 00:46, Linus Torvalds wrote:
> On Thu, Mar 15, 2018 at 4:41 PM, Kees Cook  wrote:
>>
>> I much prefer explicit typing, but both you and Rasmus mentioned
>> wanting the int/sizeof_t mixing.
> 
> Well, the explicit typing allows that mixing, in that you can just
> have "const_max_t(5,sizeof(x))"
> 
> So I'm ok with that.
> 
> What I'm *not* so much ok with is "const_max(5,sizeof(x))" erroring
> out, or silently causing insane behavior due to hidden subtle type
> casts..

I don't like const_max_t, at least not as the "primary" interface -
forcing the user to pass in a type, or equivalently passing in cast
expressions to a const_max(), can hide errors, e.g. if the -1 is really
SOME_MACRO or some complicated expression that is usually positive, but
that expression always gets cast to size_t because the user was forced to do

  const_max_t(size_t, SOME_MACRO, sizeof(foo))

to make the code compile. Not to mention that it's both easier to read
and write if one could just do

  const_max(SOME_MACRO, sizeof(foo))

Can we instead do one of the following:

(1) Effectively do the comparison in an infinitely wide signed integer,
i.e. implement

  x < 0 && y >= 0  -->  y
  x >= 0 && y < 0  -->  x
  otherwise, if both have the same sign (but not necessarily the same
signedness of their types), the type promotions do not alter either's
value, so __builtin_choose_expr(x > y, x, y) will do the right thing

with the resulting thing having the same type as the chosen one of x and
y. [Or having type typeof(x+y), which would just be a cast in the
macro.] This would allow const_max(-1, sizeof(foo)) and give
sizeof(foo), but perhaps that's too magic.

(2) Allow mixed types, but ensure the build fails if one of the values
is not representable in typeof(x+y) (i.e., one value is negative but the
common type is unsigned). That allows the const_max(SOME_MACRO,
sizeof()), but prevents silent failure in case some weird combination of
CONFIG options make SOME_MACRO evaluate to something negative.

The user can always pass in (size_t)-1 explicitly if needed, or cast the
sizeof() to int if that's what makes sense, but that's a case-by-case
thing. I'd really like that the simple case

  const_max(16, sizeof(foo))

Just Works. Then if a lot users turn up that do need some casting,
const_max_t can be implemented as a trivial const_max wrapper.

Rasmus

(1) something like __builtin_choose_expr((x >= 0 && y < 0) || \
(x >= 0 && y >= 0 && x > y) || \
(x < 0 && y < 0 && x > y), x, y)

(2) something like

// 1 or build error
#define __check_promotion(t, x) ( 1/(((t)(x) < 0) == ((x) < 0)) )

__builtin_choose_expr(__check_promotion(typeof((x)+(y)), x) && \
__check_promotion(typeof((x)+(y)), y) && \
(x) > (y), x, y)

Not sure how to get a more sensible error message, I'd like this to also
work outside functions.

Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal

[Miguel Ojeda]

On Fri, Mar 16, 2018 at 12:49 AM, Kees Cook  wrote:
> On Thu, Mar 15, 2018 at 4:46 PM, Linus Torvalds
>  wrote:
>> What I'm *not* so much ok with is "const_max(5,sizeof(x))" erroring
>> out, or silently causing insane behavior due to hidden subtle type
>> casts..
>
> Yup! I like it as an explicit argument. Thanks!
>

What about something like this?

#define INTMAXT_MAX LLONG_MAX
typedef int64_t intmax_t;

#define const_max(x, y)   \
__builtin_choose_expr(\
!__builtin_constant_p(x) || !__builtin_constant_p(y), \
__error_not_const_arg(),  \
__builtin_choose_expr(\
(x) > INTMAXT_MAX || (y) > INTMAXT_MAX,   \
__error_too_big(),\
__builtin_choose_expr(\
(intmax_t)(x) >= (intmax_t)(y),   \
(x),  \
(y)   \
) \
) \
)

Works for different types, allows to mix negatives and positives and
returns the original type, e.g.:

  const_max(-1, sizeof(char));

is of type 'long unsigned int', but:

  const_max(2, sizeof(char));

is of type 'int'. While I am not a fan that the return type depends on
the arguments, it is useful if you are going to use the expression in
something that needs expects a precise (a printk() for instance?).

The check against the INTMAXT_MAX is there to avoid complexity (if we
do not handle those cases, it is safe to use intmax_t for the
comparison; otherwise you have to have another compile time branch for
the case positive-positive using uintmax_t) and also avoids odd
warnings for some cases above LLONG_MAX about comparisons with 0 for
unsigned expressions being always true. On the positive side, it
prevents using the macro for thing like "(size_t)-1".

Cheers,
Miguel

Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal

[Kees Cook]

On Thu, Mar 15, 2018 at 4:46 PM, Linus Torvalds
 wrote:
> What I'm *not* so much ok with is "const_max(5,sizeof(x))" erroring
> out, or silently causing insane behavior due to hidden subtle type
> casts..

Yup! I like it as an explicit argument. Thanks!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal

[Linus Torvalds]

On Thu, Mar 15, 2018 at 4:46 PM, Linus Torvalds
 wrote:
>
> Well, the explicit typing allows that mixing, in that you can just
> have "const_max_t(5,sizeof(x))"

I obviously meant "const_max_t(size_t,5,sizeof(x))". Heh.

Linus

Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal

[Linus Torvalds]

On Thu, Mar 15, 2018 at 4:41 PM, Kees Cook  wrote:
>
> I much prefer explicit typing, but both you and Rasmus mentioned
> wanting the int/sizeof_t mixing.

Well, the explicit typing allows that mixing, in that you can just
have "const_max_t(5,sizeof(x))"

So I'm ok with that.

What I'm *not* so much ok with is "const_max(5,sizeof(x))" erroring
out, or silently causing insane behavior due to hidden subtle type
casts..

Linus

Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal

[Kees Cook]

On Thu, Mar 15, 2018 at 4:34 PM, Linus Torvalds
 wrote:
> On Thu, Mar 15, 2018 at 3:46 PM, Kees Cook  wrote:
>>
>> So, AIUI, I can either get strict type checking, in which case, this
>> is rejected (which I assume there is still a desire to have):
>>
>> int foo[const_max(6, sizeof(whatever))];
>
> Ehh, yes, that looks fairly sane, and erroring out would be annoying.
>
> But maybe we should just make the type explicit, and make it "const_max_t()"?
>
> I think all the existing users are of type "max_t()" anyway due to the
> very same issue, no?

All but one are using max()[1]. One case uses max_t() to get u32.

> At least if there's an explicit type like 'size_t', then passing in
> "-1" becoming a large unsigned integer is understandable and clear,
> not just some odd silent behavior.
>
> Put another way: I think it's unacceptable that
>
>  const_max(-1,6)
>
> magically becomes a huge positive number like in that patch of yours, but
>
>  const_max_t(size_t, -1, 6)
>
> *obviously* is a huge positive number.
>
> The two things would *do* the same thing, but in the second case the
> type is explicit and visible.
>
>> due to __builtin_types_compatible_p() rejecting it, or I can construct
>> a "positive arguments only" test, in which the above is accepted, but
>> this is rejected:
>
> That sounds acceptable too, although the "const_max_t()" thing is
> presumably going to be simpler?

I much prefer explicit typing, but both you and Rasmus mentioned
wanting the int/sizeof_t mixing. I'm totally happy with const_max_t()
-- even if it makes my line-wrapping harder due to the longer name. ;)

I'll resend in a moment...

-Kees

[1] https://patchwork.kernel.org/patch/10285709/

-- 
Kees Cook
Pixel Security

Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal

[Linus Torvalds]

On Thu, Mar 15, 2018 at 3:46 PM, Kees Cook  wrote:
>
> So, AIUI, I can either get strict type checking, in which case, this
> is rejected (which I assume there is still a desire to have):
>
> int foo[const_max(6, sizeof(whatever))];

Ehh, yes, that looks fairly sane, and erroring out would be annoying.

But maybe we should just make the type explicit, and make it "const_max_t()"?

I think all the existing users are of type "max_t()" anyway due to the
very same issue, no?

At least if there's an explicit type like 'size_t', then passing in
"-1" becoming a large unsigned integer is understandable and clear,
not just some odd silent behavior.

Put another way: I think it's unacceptable that

 const_max(-1,6)

magically becomes a huge positive number like in that patch of yours, but

 const_max_t(size_t, -1, 6)

*obviously* is a huge positive number.

The two things would *do* the same thing, but in the second case the
type is explicit and visible.

> due to __builtin_types_compatible_p() rejecting it, or I can construct
> a "positive arguments only" test, in which the above is accepted, but
> this is rejected:

That sounds acceptable too, although the "const_max_t()" thing is
presumably going to be simpler?

 Linus

Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal

[Kees Cook]

On Thu, Mar 15, 2018 at 4:17 PM, Miguel Ojeda
 wrote:
>> The full one, using your naming convention:
>>
>> #define const_max(x, y)  \
>> ({   \
>> if (!__builtin_constant_p(x))\
>> __error_not_const_arg(); \
>> if (!__builtin_constant_p(y))\
>> __error_not_const_arg(); \
>> if (!__builtin_types_compatible_p(typeof(x), typeof(y))) \
>> __error_incompatible_types();\
>> if ((x) < 0) \
>> __error_not_positive_arg();  \
>> if ((y) < 0) \
>> __error_not_positive_arg();  \
>> __builtin_choose_expr((x) > (y), (x), (y));  \
>> })
>>
>
> Nevermind... gcc doesn't take that as a constant expr, even if it
> compiles as one at -O0.

Yeah, unfortunately. :(

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal

[Miguel Ojeda]

On Fri, Mar 16, 2018 at 12:08 AM, Miguel Ojeda
 wrote:
> On Thu, Mar 15, 2018 at 11:58 PM, Miguel Ojeda
>  wrote:
>> On Thu, Mar 15, 2018 at 11:46 PM, Kees Cook  wrote:
>>>
>>> By using this eye-bleed:
>>>
>>> size_t __error_not_const_arg(void) \
>>> __compiletime_error("const_max() used with non-compile-time constant arg");
>>> size_t __error_not_positive_arg(void) \
>>> __compiletime_error("const_max() used with negative arg");
>>> #define const_max(x, y) \
>>> __builtin_choose_expr(__builtin_constant_p(x) &&\
>>>   __builtin_constant_p(y),  \
>>> __builtin_choose_expr((x) >= 0 && (y) >= 0, \
>>>   (typeof(x))(x) > (typeof(y))(y) ? \
>>> (x) : (y),  \
>>>   __error_not_positive_arg()),  \
>>> __error_not_const_arg())
>>>
>>
>> I was writing it like this:
>>
>> #define const_max(a, b) \
>> ({ \
>> if ((a) < 0) \
>> __const_max_called_with_negative_value(); \
>> if ((b) < 0) \
>> __const_max_called_with_negative_value(); \
>> if (!__builtin_types_compatible_p(typeof(a), typeof(b))) \
>> __const_max_called_with_incompatible_types(); \
>> __builtin_choose_expr((a) > (b), (a), (b)); \
>> })
>
> The full one, using your naming convention:
>
> #define const_max(x, y)  \
> ({   \
> if (!__builtin_constant_p(x))\
> __error_not_const_arg(); \
> if (!__builtin_constant_p(y))\
> __error_not_const_arg(); \
> if (!__builtin_types_compatible_p(typeof(x), typeof(y))) \
> __error_incompatible_types();\
> if ((x) < 0) \
> __error_not_positive_arg();  \
> if ((y) < 0) \
> __error_not_positive_arg();  \
> __builtin_choose_expr((x) > (y), (x), (y));  \
> })
>

Nevermind... gcc doesn't take that as a constant expr, even if it
compiles as one at -O0.

Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal

[Miguel Ojeda]

On Thu, Mar 15, 2018 at 11:58 PM, Miguel Ojeda
 wrote:
> On Thu, Mar 15, 2018 at 11:46 PM, Kees Cook  wrote:
>>
>> By using this eye-bleed:
>>
>> size_t __error_not_const_arg(void) \
>> __compiletime_error("const_max() used with non-compile-time constant arg");
>> size_t __error_not_positive_arg(void) \
>> __compiletime_error("const_max() used with negative arg");
>> #define const_max(x, y) \
>> __builtin_choose_expr(__builtin_constant_p(x) &&\
>>   __builtin_constant_p(y),  \
>> __builtin_choose_expr((x) >= 0 && (y) >= 0, \
>>   (typeof(x))(x) > (typeof(y))(y) ? \
>> (x) : (y),  \
>>   __error_not_positive_arg()),  \
>> __error_not_const_arg())
>>
>
> I was writing it like this:
>
> #define const_max(a, b) \
> ({ \
> if ((a) < 0) \
> __const_max_called_with_negative_value(); \
> if ((b) < 0) \
> __const_max_called_with_negative_value(); \
> if (!__builtin_types_compatible_p(typeof(a), typeof(b))) \
> __const_max_called_with_incompatible_types(); \
> __builtin_choose_expr((a) > (b), (a), (b)); \
> })

The full one, using your naming convention:

#define const_max(x, y)  \
({   \
if (!__builtin_constant_p(x))\
__error_not_const_arg(); \
if (!__builtin_constant_p(y))\
__error_not_const_arg(); \
if (!__builtin_types_compatible_p(typeof(x), typeof(y))) \
__error_incompatible_types();\
if ((x) < 0) \
__error_not_positive_arg();  \
if ((y) < 0) \
__error_not_positive_arg();  \
__builtin_choose_expr((x) > (y), (x), (y));  \
})

Miguel

Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal

[Miguel Ojeda]

On Thu, Mar 15, 2018 at 11:46 PM, Kees Cook  wrote:
> On Thu, Mar 15, 2018 at 3:23 PM, Linus Torvalds
>  wrote:
>> On Thu, Mar 15, 2018 at 3:16 PM, Kees Cook  wrote:
>>>
>>> size_t __error_not_const_arg(void) \
>>> __compiletime_error("const_max() used with non-compile-time constant arg");
>>> #define const_max(x, y) \
>>> __builtin_choose_expr(__builtin_constant_p(x) &&\
>>>   __builtin_constant_p(y),  \
>>>   (typeof(x))(x) > (typeof(y))(y) ? \
>>> (x) : (y),  \
>>>   __error_not_const_arg())
>>>
>>> Is typeof() forcing enums to int? Regardless, I'll put this through
>>> larger testing. How does that look?
>>
>> Ok, that alleviates my worry about one class of insane behavior, but
>> it does raise a few other questions:
>>
>>  - what drugs is gcc on where (typeof(x)(x)) makes a difference? Funky.
>
> Yeah, that's why I didn't even try that originally. But in looking
> back at max() again, it seemed to be the only thing missing that would
> handle the enum evaluation, which turned out to be true.
>
>>  - this does have the usual "what happen if you do
>>
>>  const_max(-1,sizeof(x))
>>
>> where the comparison will now be done in 'size_t', and -1 ends up
>> being a very very big unsigned integer.
>>
>> Is there no way to get that type checking inserted? Maybe now is a
>> good point for that __builtin_types_compatible(), and add it to the
>> constness checking (and change the name of that error case function)?
>
> So, AIUI, I can either get strict type checking, in which case, this
> is rejected (which I assume there is still a desire to have):
>
> int foo[const_max(6, sizeof(whatever))];

Is it that bad to just call it with (size_t)6?

>
> due to __builtin_types_compatible_p() rejecting it, or I can construct
> a "positive arguments only" test, in which the above is accepted, but
> this is rejected:
>
> int foo[const_max(-1, sizeof(whatever))];

Do we need this case?

>
> By using this eye-bleed:
>
> size_t __error_not_const_arg(void) \
> __compiletime_error("const_max() used with non-compile-time constant arg");
> size_t __error_not_positive_arg(void) \
> __compiletime_error("const_max() used with negative arg");
> #define const_max(x, y) \
> __builtin_choose_expr(__builtin_constant_p(x) &&\
>   __builtin_constant_p(y),  \
> __builtin_choose_expr((x) >= 0 && (y) >= 0, \
>   (typeof(x))(x) > (typeof(y))(y) ? \
> (x) : (y),  \
>   __error_not_positive_arg()),  \
> __error_not_const_arg())
>

I was writing it like this:

#define const_max(a, b) \
({ \
if ((a) < 0) \
__const_max_called_with_negative_value(); \
if ((b) < 0) \
__const_max_called_with_negative_value(); \
if (!__builtin_types_compatible_p(typeof(a), typeof(b))) \
__const_max_called_with_incompatible_types(); \
__builtin_choose_expr((a) > (b), (a), (b)); \
})

Cheers,
Miguel


> -Kees
>
> --
> Kees Cook
> Pixel Security

Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal

[Kees Cook]

On Thu, Mar 15, 2018 at 3:23 PM, Linus Torvalds
 wrote:
> On Thu, Mar 15, 2018 at 3:16 PM, Kees Cook  wrote:
>>
>> size_t __error_not_const_arg(void) \
>> __compiletime_error("const_max() used with non-compile-time constant arg");
>> #define const_max(x, y) \
>> __builtin_choose_expr(__builtin_constant_p(x) &&\
>>   __builtin_constant_p(y),  \
>>   (typeof(x))(x) > (typeof(y))(y) ? \
>> (x) : (y),  \
>>   __error_not_const_arg())
>>
>> Is typeof() forcing enums to int? Regardless, I'll put this through
>> larger testing. How does that look?
>
> Ok, that alleviates my worry about one class of insane behavior, but
> it does raise a few other questions:
>
>  - what drugs is gcc on where (typeof(x)(x)) makes a difference? Funky.

Yeah, that's why I didn't even try that originally. But in looking
back at max() again, it seemed to be the only thing missing that would
handle the enum evaluation, which turned out to be true.

>  - this does have the usual "what happen if you do
>
>  const_max(-1,sizeof(x))
>
> where the comparison will now be done in 'size_t', and -1 ends up
> being a very very big unsigned integer.
>
> Is there no way to get that type checking inserted? Maybe now is a
> good point for that __builtin_types_compatible(), and add it to the
> constness checking (and change the name of that error case function)?

So, AIUI, I can either get strict type checking, in which case, this
is rejected (which I assume there is still a desire to have):

int foo[const_max(6, sizeof(whatever))];

due to __builtin_types_compatible_p() rejecting it, or I can construct
a "positive arguments only" test, in which the above is accepted, but
this is rejected:

int foo[const_max(-1, sizeof(whatever))];

By using this eye-bleed:

size_t __error_not_const_arg(void) \
__compiletime_error("const_max() used with non-compile-time constant arg");
size_t __error_not_positive_arg(void) \
__compiletime_error("const_max() used with negative arg");
#define const_max(x, y) \
__builtin_choose_expr(__builtin_constant_p(x) &&\
  __builtin_constant_p(y),  \
__builtin_choose_expr((x) >= 0 && (y) >= 0, \
  (typeof(x))(x) > (typeof(y))(y) ? \
(x) : (y),  \
  __error_not_positive_arg()),  \
__error_not_const_arg())

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal

[Linus Torvalds]

On Thu, Mar 15, 2018 at 3:16 PM, Kees Cook  wrote:
>
> size_t __error_not_const_arg(void) \
> __compiletime_error("const_max() used with non-compile-time constant arg");
> #define const_max(x, y) \
> __builtin_choose_expr(__builtin_constant_p(x) &&\
>   __builtin_constant_p(y),  \
>   (typeof(x))(x) > (typeof(y))(y) ? \
> (x) : (y),  \
>   __error_not_const_arg())
>
> Is typeof() forcing enums to int? Regardless, I'll put this through
> larger testing. How does that look?

Ok, that alleviates my worry about one class of insane behavior, but
it does raise a few other questions:

 - what drugs is gcc on where (typeof(x)(x)) makes a difference? Funky.

 - this does have the usual "what happen if you do

 const_max(-1,sizeof(x))

where the comparison will now be done in 'size_t', and -1 ends up
being a very very big unsigned integer.

Is there no way to get that type checking inserted? Maybe now is a
good point for that __builtin_types_compatible(), and add it to the
constness checking (and change the name of that error case function)?

  Linus

Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal

[Kees Cook]

On Thu, Mar 15, 2018 at 2:42 PM, Linus Torvalds
 wrote:
> On Thu, Mar 15, 2018 at 12:47 PM, Kees Cook  wrote:
>>
>> To gain the ability to compare differing types, the arguments are
>> explicitly cast to size_t.
>
> Ugh, I really hate this.
>
> It silently does insane things if you do
>
>const_max(-1,6)
>
> and there is nothing in the name that implies that you can't use
> negative constants.

Yeah, I didn't like that effect either. I was seeing this:

./include/linux/kernel.h:836:14: warning: comparison between ‘enum
’ and ‘enum ’ [-Wenum-compare]
  (x) > (y) ? \
  ^
./include/linux/kernel.h:838:7: note: in definition of macro ‘const_max’
  (y),  \
   ^
net/ipv6/proc.c:34:11: note: in expansion of macro ‘const_max’
   const_max(IPSTATS_MIB_MAX, ICMP_MIB_MAX))
   ^

But it turns out that just doing a typeof() fixes this, and there's no
need for the hard cast to size_t:

size_t __error_not_const_arg(void) \
__compiletime_error("const_max() used with non-compile-time constant arg");
#define const_max(x, y) \
__builtin_choose_expr(__builtin_constant_p(x) &&\
  __builtin_constant_p(y),  \
  (typeof(x))(x) > (typeof(y))(y) ? \
(x) : (y),  \
  __error_not_const_arg())

Is typeof() forcing enums to int? Regardless, I'll put this through
larger testing. How does that look?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal

[Linus Torvalds]

On Thu, Mar 15, 2018 at 12:47 PM, Kees Cook  wrote:
>
> To gain the ability to compare differing types, the arguments are
> explicitly cast to size_t.

Ugh, I really hate this.

It silently does insane things if you do

   const_max(-1,6)

and there is nothing in the name that implies that you can't use
negative constants.

   Linus