diff mbox

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

Message ID 1521143266-31350-2-git-send-email-keescook@chromium.org (mailing list archive)
State New, archived
Headers show

Commit Message

Kees Cook March 15, 2018, 7:47 p.m. UTC
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 <keescook@chromium.org>
---
 include/linux/kernel.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Linus Torvalds March 15, 2018, 9:42 p.m. UTC | #1
On Thu, Mar 15, 2018 at 12:47 PM, Kees Cook <keescook@chromium.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kees Cook March 15, 2018, 10:16 p.m. UTC | #2
On Thu, Mar 15, 2018 at 2:42 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Mar 15, 2018 at 12:47 PM, Kees Cook <keescook@chromium.org> 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
<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
Linus Torvalds March 15, 2018, 10:23 p.m. UTC | #3
On Thu, Mar 15, 2018 at 3:16 PM, Kees Cook <keescook@chromium.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kees Cook March 15, 2018, 10:46 p.m. UTC | #4
On Thu, Mar 15, 2018 at 3:23 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Mar 15, 2018 at 3:16 PM, Kees Cook <keescook@chromium.org> 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
Miguel Ojeda March 15, 2018, 10:58 p.m. UTC | #5
On Thu, Mar 15, 2018 at 11:46 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Mar 15, 2018 at 3:23 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Thu, Mar 15, 2018 at 3:16 PM, Kees Cook <keescook@chromium.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Miguel Ojeda March 15, 2018, 11:08 p.m. UTC | #6
On Thu, Mar 15, 2018 at 11:58 PM, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> On Thu, Mar 15, 2018 at 11:46 PM, Kees Cook <keescook@chromium.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Miguel Ojeda March 15, 2018, 11:17 p.m. UTC | #7
On Fri, Mar 16, 2018 at 12:08 AM, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> On Thu, Mar 15, 2018 at 11:58 PM, Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>> On Thu, Mar 15, 2018 at 11:46 PM, Kees Cook <keescook@chromium.org> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kees Cook March 15, 2018, 11:31 p.m. UTC | #8
On Thu, Mar 15, 2018 at 4:17 PM, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> 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
Linus Torvalds March 15, 2018, 11:34 p.m. UTC | #9
On Thu, Mar 15, 2018 at 3:46 PM, Kees Cook <keescook@chromium.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kees Cook March 15, 2018, 11:41 p.m. UTC | #10
On Thu, Mar 15, 2018 at 4:34 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Mar 15, 2018 at 3:46 PM, Kees Cook <keescook@chromium.org> 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/
Linus Torvalds March 15, 2018, 11:46 p.m. UTC | #11
On Thu, Mar 15, 2018 at 4:41 PM, Kees Cook <keescook@chromium.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds March 15, 2018, 11:47 p.m. UTC | #12
On Thu, Mar 15, 2018 at 4:46 PM, Linus Torvalds
<torvalds@linux-foundation.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kees Cook March 15, 2018, 11:49 p.m. UTC | #13
On Thu, Mar 15, 2018 at 4:46 PM, Linus Torvalds
<torvalds@linux-foundation.org> 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
Miguel Ojeda March 16, 2018, 3:05 a.m. UTC | #14
On Fri, Mar 16, 2018 at 12:49 AM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Mar 15, 2018 at 4:46 PM, Linus Torvalds
> <torvalds@linux-foundation.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rasmus Villemoes March 16, 2018, 2:15 p.m. UTC | #15
On 2018-03-16 00:46, Linus Torvalds wrote:
> On Thu, Mar 15, 2018 at 4:41 PM, Kees Cook <keescook@chromium.org> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3fd291503576..012f588b5a25 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -820,6 +820,25 @@  static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
 	      x, y)
 
 /**
+ * const_max - return maximum of two positive compile-time constant values
+ * @x: first compile-time constant value
+ * @y: second compile-time constant value
+ *
+ * This has no type checking nor multi-evaluation defenses, and must
+ * only ever be used with positive compile-time constant values (for
+ * example when calculating a stack array size).
+ */
+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),		\
+			      (size_t)(x) > (size_t)(y) ?	\
+					(size_t)(x) :		\
+					(size_t)(y),		\
+			      __error_not_const_arg())
+
+/**
  * min3 - return minimum of three values
  * @x: first value
  * @y: second value