[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200707095651.422f0b22@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date: Tue, 7 Jul 2020 09:56:51 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Sami Tolvanen <samitolvanen@...gle.com>
Cc: Masahiro Yamada <masahiroy@...nel.org>, Will Deacon <will@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Paul E. McKenney"
<paulmck@...nel.org>, Kees Cook <keescook@...omium.org>, Nick Desaulniers
<ndesaulniers@...gle.com>, clang-built-linux
<clang-built-linux@...glegroups.com>, Kernel Hardening
<kernel-hardening@...ts.openwall.com>, linux-arch
<linux-arch@...r.kernel.org>, linux-arm-kernel
<linux-arm-kernel@...ts.infradead.org>, Linux Kbuild mailing list
<linux-kbuild@...r.kernel.org>, Linux Kernel Mailing List
<linux-kernel@...r.kernel.org>, linux-pci@...r.kernel.org, X86 ML
<x86@...nel.org>
Subject: Re: [PATCH 00/22] add support for Clang LTO
On Tue, 7 Jul 2020 09:05:28 -0700 Sami Tolvanen wrote:
> On Tue, Jul 07, 2020 at 08:51:07AM -0700, Sami Tolvanen wrote:
> > After spending some time debugging this with Nick, it looks like the
> > error is caused by a recent optimization change in LLVM, which together
> > with the inlining of ur_load_imm_any into jeq_imm, changes a runtime
> > check in FIELD_FIT that would always fail, to a compile-time check that
> > breaks the build. In jeq_imm, we have:
> >
> > /* struct bpf_insn: _s32 imm */
> > u64 imm = insn->imm; /* sign extend */
> > ...
> > if (imm >> 32) { /* non-zero only if insn->imm is negative */
> > /* inlined from ur_load_imm_any */
> > u32 __imm = imm >> 32; /* therefore, always 0xffffffff */
> >
> > /*
> > * __imm has a value known at compile-time, which means
> > * __builtin_constant_p(__imm) is true and we end up with
> > * essentially this in __BF_FIELD_CHECK:
> > */
> > if (__builtin_constant_p(__imm) && __imm <= 255)
>
> Should be __imm > 255, of course, which means the compiler will generate
> a call to __compiletime_assert.
I think FIELD_FIT() should not pass the value into __BF_FIELD_CHECK().
So:
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 48ea093ff04c..4e035aca6f7e 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -77,7 +77,7 @@
*/
#define FIELD_FIT(_mask, _val) \
({ \
- __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_FIT: "); \
+ __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: "); \
!((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \
})
It's perfectly legal to pass a constant which does not fit, in which
case FIELD_FIT() should just return false not break the build.
Right?
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
