linux-kernel - Re: [PATCH] compiler_types: mark __compiletime_assert failure as _

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 15 Oct 2021 09:55:58 +0200
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Nick Desaulniers <ndesaulniers@...gle.com>,
        Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Miguel Ojeda <ojeda@...nel.org>,
        Nathan Chancellor <nathan@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        llvm@...ts.linux.dev,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Joe Perches <joe@...ches.com>
Subject: Re: [PATCH] compiler_types: mark __compiletime_assert failure as
 __noreturn

On 14/10/2021 20.55, Nick Desaulniers wrote:
> On Thu, Oct 14, 2021 at 11:41 AM Miguel Ojeda
> <miguel.ojeda.sandonis@...il.com> wrote:
>>
>> On Thu, Oct 14, 2021 at 8:33 PM Miguel Ojeda
>> <miguel.ojeda.sandonis@...il.com> wrote:
>>>
>>> That would be a nice to do, but I am not sure about introducing one
>>> more macro about this... I think it would be simpler to submit patches
>>> for moves into `static_assert` even if we have to "flip" the meaning.
> 
> $ grep -r BUILD_BUG_ON | wc -l
> 3405
> 
>> Actually, what would be ideal is a compiler-backed lint that checks
>> whether it could be an `static_assert`, perhaps in clang-tidy?
> 
> Oh, that is a good idea.  There is one already for recommending the
> use of static_assert instead of assert.  That's actually very nice.

So I did

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index b6ff83a714ca..e212220216e8 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -295,12 +295,17 @@ struct ftrace_likely_data {
 # define __compiletime_object_size(obj) -1
 #endif

+#include <linux/const.h>
+
 #ifdef __OPTIMIZE__
 # define __compiletime_assert(condition, msg, prefix, suffix)          \
        do {                                                            \
                extern void prefix ## suffix(void)
__compiletime_error(msg); \
+               extern void prefix ## suffix ## ice(void)
__compiletime_warning("Ice ice baby"); \
                if (!(condition))                                       \
                        prefix ## suffix();                             \
+               if (__is_constexpr(condition))                          \
+                       prefix ## suffix ## ice();                      \
        } while (0)
 #else
 # define __compiletime_assert(condition, msg, prefix, suffix) do { }
while (0)

and that throws a gazillion warnings. Picking one at random shows that
container_of() has a BUILD_BUG_ON. So we could do

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2776423a587e..0a1969b11619 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -492,9 +492,9 @@ static inline void ftrace_dump(enum ftrace_dump_mode
oops_dump_mode) { }
  */
 #define container_of(ptr, type, member) ({                             \
        void *__mptr = (void *)(ptr);                                   \
-       BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&   \
-                        !__same_type(*(ptr), void),                    \
-                        "pointer type mismatch in container_of()");    \
+       static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
+                     __same_type(*(ptr), void),                        \
+                     "pointer type mismatch in container_of()");       \
        ((type *)(__mptr - offsetof(type, member))); })

 /**
@@ -507,9 +507,9 @@ static inline void ftrace_dump(enum ftrace_dump_mode
oops_dump_mode) { }
  */
 #define container_of_safe(ptr, type, member) ({
        \
        void *__mptr = (void *)(ptr);                                   \
-       BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&   \
-                        !__same_type(*(ptr), void),                    \
-                        "pointer type mismatch in container_of()");    \
+       static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
+                     __same_type(*(ptr), void),                        \
+                     "pointer type mismatch in container_of_safe()");  \
        IS_ERR_OR_NULL(__mptr) ? ERR_CAST(__mptr) :                     \
                ((type *)(__mptr - offsetof(type, member))); })

[fixing the copy-pasto in container_of_safe while at it].

Basically, all BUILD_BUG_ONs like that that do type checking are very
obviously candidates for using static_assert instead (they very
naturally go at the end of all the locally declared variables, so one
won't hit the declaration-after-statement problem that can otherwise
prevent using static_assert).

> I was playing with trying to adapt clang-tidy's C++11 `auto` fixit to
> work on GNU C code to automate the replacement of:
> 
> __typeof(x) y = (x);
> 
> with:
> 
> __auto_type y = (x);
> 
> in macros.  That's perhaps interesting, too.  Given the volume of code
> in the kernel, I wouldn't waste time with one off patches;

Well, for the kind of macros that are used _everywhere_ a few one-off
patches might be in order. It's also interesting if one could measure
any speedup from switching those core macros to static_assert.

Rasmus