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:   Thu, 14 Oct 2021 20:33:30 +0200
From:   Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To:     Nick Desaulniers <ndesaulniers@...gle.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, Rasmus Villemoes <linux@...musvillemoes.dk>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] compiler_types: mark __compiletime_assert failure as __noreturn

On Thu, Oct 14, 2021 at 7:49 PM Nick Desaulniers
<ndesaulniers@...gle.com> wrote:
>
> It's a good question; I'm pretty sure we had a thread with Rasmus on
> the idea a while ago, and IIRC the answer is no.

Yeah, I remember that too.

> Basically, we can't convert BUILD_BUG_ON to _Static_assert because
> _Static_assert requires integer constant expressions (ICE) while many
> expressions passed to BUILD_BUG_ON in the kernel require that
> optimizations such as inlining run (they are not ICEs); BUILD_BUG_ON
> is more flexible.  So you can't replace the guts of BUILD_BUG_ON
> wholesale with _Static_assert (without doing anything else); it would
> be preferable for kernel developers to use _Static_assert (I think we
> have a macro, static_assert, too) in cases where they have ICEs rather
> than BUILD_BUG_ON (though it flips the condition of the expression;
> _Static_assert errors if the expression evaluates to false;
> BUILD_BUG_ON when true), but I think there's too much muscle memory
> around just using BUILD_BUG_ON that if you introduced something new,
> folks wouldn't know to use that instead.

Indeed, `BUILD_BUG_ON` requires the optimizer to see through whatever
you are trying to do. Way more powerful, but finicky too.

Another difference is that `_Static_assert` can be used in more places
(file scope, inside `struct`s...) for tests about e.g. sizes, i.e.
`BUILD_BUG_ON` is not a complete replacement either.

> Probably a better demonstration would be to try it and observe some of
> the spooky failures at build time that result.  We may be able to
> separate the macro into two; BUILD_BUG_ON and BUILD_BUG_ON_OPT (or
> whatever color bikeshed), where the former uses _Static_assert under
> the hood, and the latter uses __attribute__((error)). Then we could go
> about converting cases that could not use _Static_assert to use the
> new macro, while the old macro is what folks still reach for first.

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.

> I'm not sure how worthwhile that yakshave would be, but at least the
> front end of the compiler would error sooner in the case of
> _Static_assert, FWIW (not much).  But I don't think we can ever
> eliminate __attribute__((error)) from the kernel unless we're ok
> outright removing asserts that aren't ICEs.  I would not recommend
> that.  I would like to see more usage of static_assert, but I'm not
> sure how best to promote that, and if it's worth discussing the subtle
> distinction between BUILD_BUG_ON vs _Static_assert again and again and
> again every time.

Perhaps we should add a comment in `BUILD_BUG*` about checking out
`static_assert` -- we have the comment in the latter, but those
reading the former will not realize the may be able to use the
latter...

Cheers,
Miguel