Messages in this thread | | | From | Nick Desaulniers <> | Date | Mon, 26 Sep 2022 12:02:15 -0700 | Subject | Re: [PATCH] x86/fpu: use __alignof__ to avoid UB in TYPE_ALIGN |
| |
+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
| |