I'm calling this "v4" since the last effort at this was v3, even
if it's a different approach. Patch 1 adds const_max(), patch 2
uses it in all the places max() was used for stack arrays. Commit
log from patch 1:
---snip---
kernel.h: Introduce const_max() for VLA removal
In the effort to remove all VLAs from the kernel[1], it is desirable to
build with -Wvla. However, this warning is overly pessimistic, in that
it is only happy with stack array sizes that are declared as constant
expressions, and not constant values. One case of this is the evaluation
of the max() macro which, due to its construction, ends up converting
constant expression arguments into a constant value result. Attempts
to adjust the behavior of max() ran afoul of version-dependent compiler
behavior[2].
To work around this and still gain -Wvla coverage, this patch introduces
a new macro, const_max(), for use in these cases of stack array size
declaration, where the constant expressions are retained. Since this means
losing the double-evaluation protections of the max() macro, this macro is
designed to explicitly fail if used on non-constant arguments.
Older compilers will fail with the unhelpful message:
error: first argument to ‘__builtin_choose_expr’ not a constant
Newer compilers will fail with a hopefully more helpful message:
error: call to ‘__error_not_const_arg’ declared with attribute error: const_max() used with non-compile-time constant arg
To gain the ability to compare differing types, the arguments are
explicitly cast to size_t. Without this, some compiler versions will
fail when comparing different enum types or similar constant expression
cases. With the casting, it's possible to do things like:
int foo[const_max(6, sizeof(something))];
[1] https://lkml.org/lkml/2018/3/7/621
[2] https://lkml.org/lkml/2018/3/10/170
---eol---
Hopefully this reads well as a summary from all the things that got tried.
I've tested this on allmodconfig builds with gcc 4.4.4 and 6.3.0, with and
without -Wvla.
-Kees
In the effort to remove all VLAs from the kernel[1], it is desirable to
build with -Wvla. However, this warning is overly pessimistic, in that
it is only happy with stack array sizes that are declared as constant
expressions, and not constant values. One case of this is the evaluation
of the max() macro which, due to its construction, ends up converting
constant expression arguments into a constant value result. Attempts
to adjust the behavior of max() ran afoul of version-dependent compiler
behavior[2].
To work around this and still gain -Wvla coverage, this patch introduces
a new macro, const_max(), for use in these cases of stack array size
declaration, where the constant expressions are retained. Since this means
losing the double-evaluation protections of the max() macro, this macro is
designed to explicitly fail if used on non-constant arguments.
Older compilers will fail with the unhelpful message:
error: first argument to ‘__builtin_choose_expr’ not a constant
Newer compilers will fail with a hopefully more helpful message:
error: call to ‘__error_not_const_arg’ declared with attribute error: const_max() used with non-compile-time constant arg
To gain the ability to compare differing types, the arguments are
explicitly cast to size_t. Without this, some compiler versions will
fail when comparing different enum types or similar constant expression
cases. With the casting, it's possible to do things like:
int foo[const_max(6, sizeof(something))];
[1] https://lkml.org/lkml/2018/3/7/621
[2] https://lkml.org/lkml/2018/3/10/170
Signed-off-by: Kees Cook <redacted>
---
include/linux/kernel.h | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
As part of removing VLAs from the kernel[1], we want to build with -Wvla,
but it is overly pessimistic and only accepts constant expressions for
stack array sizes, instead of also constant values. The max() macro
triggers the warning, so this refactors these uses of max() to use the
new const_max() instead.
[1] https://lkml.org/lkml/2018/3/7/621
Signed-off-by: Kees Cook <redacted>
---
drivers/input/touchscreen/cyttsp4_core.c | 2 +-
fs/btrfs/tree-checker.c | 3 ++-
lib/vsprintf.c | 4 ++--
net/ipv4/proc.c | 8 ++++----
net/ipv6/proc.c | 10 ++++------
5 files changed, 13 insertions(+), 14 deletions(-)
@@ -432,7 +432,7 @@ static int snmp_seq_show_tcp_udp(struct seq_file *seq, void *v)for(i=0;snmp4_udp_list[i].name;i++)seq_printf(seq," %lu",buff[i]);-memset(buff,0,TCPUDP_MIB_MAX*sizeof(unsignedlong));+memset(buff,0,sizeof(buff));/* the UDP and UDP-Lite MIBs are the same */seq_puts(seq,"\nUdpLite:");
On Thu, Mar 15, 2018 at 12:47 PM, Kees Cook [off-list ref] 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
On Thu, Mar 15, 2018 at 2:42 PM, Linus Torvalds
[off-list ref] wrote:
On Thu, Mar 15, 2018 at 12:47 PM, Kees Cook [off-list ref] wrote:
quoted
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
<anonymous>’ and ‘enum <anonymous>’ [-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
On Thu, Mar 15, 2018 at 3:16 PM, Kees Cook [off-list ref] 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
On Thu, Mar 15, 2018 at 3:23 PM, Linus Torvalds
[off-list ref] wrote:
On Thu, Mar 15, 2018 at 3:16 PM, Kees Cook [off-list ref] wrote:
quoted
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
From: Miguel Ojeda <hidden> Date: 2018-03-15 22:58:10
On Thu, Mar 15, 2018 at 11:46 PM, Kees Cook [off-list ref] wrote:
On Thu, Mar 15, 2018 at 3:23 PM, Linus Torvalds
[off-list ref] wrote:
quoted
On Thu, Mar 15, 2018 at 3:16 PM, Kees Cook [off-list ref] wrote:
quoted
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.
quoted
- 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
From: Miguel Ojeda <hidden> Date: 2018-03-15 23:08:49
On Thu, Mar 15, 2018 at 11:58 PM, Miguel Ojeda
[off-list ref] wrote:
On Thu, Mar 15, 2018 at 11:46 PM, Kees Cook [off-list ref] wrote:
quoted
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
From: Miguel Ojeda <hidden> Date: 2018-03-15 23:17:50
On Fri, Mar 16, 2018 at 12:08 AM, Miguel Ojeda
[off-list ref] wrote:
On Thu, Mar 15, 2018 at 11:58 PM, Miguel Ojeda
[off-list ref] wrote:
quoted
On Thu, Mar 15, 2018 at 11:46 PM, Kees Cook [off-list ref] wrote:
quoted
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.
On Thu, Mar 15, 2018 at 3:46 PM, Kees Cook [off-list ref] 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
On Thu, Mar 15, 2018 at 4:34 PM, Linus Torvalds
[off-list ref] wrote:
On Thu, Mar 15, 2018 at 3:46 PM, Kees Cook [off-list ref] wrote:
quoted
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.
quoted
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
On Thu, Mar 15, 2018 at 4:41 PM, Kees Cook [off-list ref] 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
From: Miguel Ojeda <hidden> Date: 2018-03-16 03:05:55
On Fri, Mar 16, 2018 at 12:49 AM, Kees Cook [off-list ref] wrote:
On Thu, Mar 15, 2018 at 4:46 PM, Linus Torvalds
[off-list ref] wrote:
quoted
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
From: Nikolay Borisov <hidden> Date: 2018-03-16 07:52:33
On 15.03.2018 21:47, Kees Cook wrote:
As part of removing VLAs from the kernel[1], we want to build with -Wvla,
but it is overly pessimistic and only accepts constant expressions for
stack array sizes, instead of also constant values. The max() macro
triggers the warning, so this refactors these uses of max() to use the
new const_max() instead.
[1] https://lkml.org/lkml/2018/3/7/621
For the btrfs portion :
Reviewed-by: Nikolay Borisov <redacted>
@@ -432,7 +432,7 @@ static int snmp_seq_show_tcp_udp(struct seq_file *seq, void *v)for(i=0;snmp4_udp_list[i].name;i++)seq_printf(seq," %lu",buff[i]);-memset(buff,0,TCPUDP_MIB_MAX*sizeof(unsignedlong));+memset(buff,0,sizeof(buff));/* the UDP and UDP-Lite MIBs are the same */seq_puts(seq,"\nUdpLite:");
On Thu, Mar 15, 2018 at 4:41 PM, Kees Cook [off-list ref] wrote:
quoted
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.