Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index

On Fri, Jan 15, 2021 at 08:54:18PM +0100, Arnd Bergmann wrote:

On Fri, Jan 15, 2021 at 8:18 PM Borislav Petkov [off-list ref] wrote:

quoted

On Fri, Jan 15, 2021 at 02:11:25PM -0500, Arvind Sankar wrote:

quoted

That's how build-time assertions work: they are _supposed_ to be
optimized away completely when the assertion is true. If they're
_not_ optimized away, the build will fail.

Yah, that I know, thanks.

If gcc really inlines p4d_index() and does a lot more aggressive
optimization to determine that the condition is false and thus optimize
everything away (and clang doesn't), then that would explain the
observation.

One difference is that gcc does not have
-fsanitize=unsigned-integer-overflow at all, and I don't see the
assertion without that on clang either, so it's possible that clang
behaves as designed here.

The description is:
    -fsanitize=unsigned-integer-overflow: Unsigned integer overflow, where
     the result of an unsigned integer computation cannot be represented in
     its type. Unlike signed integer overflow, this is not undefined behavior,
     but it is often unintentional. This sanitizer does not check for
lossy implicit
     conversions performed before such a computation (see
    -fsanitize=implicit-conversion).

The "-68 * ((1UL) << 30" computation does overflow an unsigned long
as intended, right? Maybe this is enough for the ubsan code in clang to
just disable some of the optimization steps that the assertion relies on.

        Arnd

That does seem to be an overflow, but that happens at compile-time.
Maybe
	AC(-68, UL) << 30
would be a better definition to avoid overflow.

The real issue might be (ptrs_per_p4d - 1) which can overflow at
run-time, and maybe the added ubsan code makes p4d_index() not worth
inlining according to clang?