Re: [PATCH] x86/fpu: use __alignof__ to avoid UB in TYPE_ALIGN

[Nick Desaulniers <ndesaulniers@google.com>]

+Jiri

Hi YingChi,
Thank you very much for the patch and your consideration when
implementing this check in clang.

It looks like you sent a few different versions of this patch; please
use `-v2`, `-v3`, etc. when invoking `git format-patch` to include the
patch version in the subject line.

On Mon, Sep 26, 2022 at 6:19 AM YingChi Long <me@inclyc.cn> wrote:
>
> Seems GCC __alignof__ is not evaluated to the minimum alignment of some
> TYPE,
> and depends on fields of the struct.
>
>  > Notably I think 'long long' has 4 byte alignment on i386 and some other
>  > 32bit archs.
>
> C11 _Alignof matches in the case (see godbolt link below). How about
> switch to
> _Alignof?
>
>
> Link: https://godbolt.org/z/T749MfM9o
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10360
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52023
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69560

I think you should additionally include the following 2 link tags:

Link: https://reviews.llvm.org/D133574
Link: https://gcc.gnu.org/onlinedocs/gcc/Alignment.html

While Peter is right that there is a subtle distinction between GNU
__alignof__ and ISO C11 _Alignof, reading
commit 25ec02f2c144 ("x86/fpu: Properly align size in
CHECK_MEMBER_AT_END_OF() macro")
wasn't the intent of 25ec02f2c144 to account for alignment of members
within structs? Hence shouldn't we be using __alignof__ and not
_Alignof? (If I've understood all those GCC bug report comments
correctly; will reread them again after lunch).

$ ARCH=i386 make LLVM=1 -j$(nproc) defconfig all
$ ARCH=i386 make -j$(nproc) defconfig all
$ make LLVM=1 -j$(nproc) defconfig all
$ make -j$(nproc) defconfig all

will all build either way (with __alignof__ vs _Alignof).  The comment
in fpu__init_task_struct_size() in arch/x86/kernel/fpu/init.c alludes
to struct fpu being dynamically sized; perhaps on certain kernel
configs which would be needed to tease out potential build failures.

Also, commit messages on other versions state:

>> _alignof__() will in fact return the 'sane' result

Please use more descriptive language rather than 'sane.' That
statement tells readers nothing about the distinctions between
__alignof__ and _Alignof.

Finally, I wonder if it's possible to use static_assert (defined in
include/linux/build_bug.h) here rather than BUILD_BUG_ON?

>
> On 2022/9/26 17:01, Peter Zijlstra wrote:
> > On Sun, Sep 25, 2022 at 11:31:50PM +0800, YingChi Long wrote:
> >> WG14 N2350 made very clear that it is an UB having type definitions with
> >> in "offsetof". This patch change the implementation of macro
> >> "TYPE_ALIGN" to builtin "__alignof__" to avoid undefined behavior.
> >>
> >> I've grepped all source files to find any type definitions within
> >> "offsetof".
> >>
> >>      offsetof\(struct .*\{ .*,
> >>
> >> This implementation of macro "TYPE_ALIGN" seemes to be the only case of
> >> type definitions within offsetof in the kernel codebase.
> >>
> >> Signed-off-by: YingChi Long <me@inclyc.cn>
> >> Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
> >> ---
> >>   arch/x86/kernel/fpu/init.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
> >> index 621f4b6cac4a..41425ba0b6b1 100644
> >> --- a/arch/x86/kernel/fpu/init.c
> >> +++ b/arch/x86/kernel/fpu/init.c
> >> @@ -134,7 +134,7 @@ static void __init fpu__init_system_generic(void)
> >>   }
> >>
> >>   /* Get alignment of the TYPE. */
> >> -#define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
> >> +#define TYPE_ALIGN(TYPE) __alignof__(TYPE)
> > IIRC there's a problem with alignof() in that it will return the ABI
> > alignment instead of that preferred or natural alignment for some types.
> >
> > Notably I think 'long long' has 4 byte alignment on i386 and some other
> > 32bit archs.
> >
> > That said; please just replace the *one* instance of TYPE_ALIGN entirely
> > and get rid of the thing.
> >



-- 
Thanks,
~Nick Desaulniers