[RESEND,v3] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

  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.

I've made a clang patch that rejects any definitions within
__builtin_offsetof (usually #defined with "offsetof"), and tested
compiling with this patch, there are no error if this patch applied.

ISO C11 _Alignof is subtly different from the GNU C extension
__alignof__. __alignof__ is the preferred alignment and _Alignof the
minimal alignment. For 'long long' on x86 these are 8 and 4
respectively.

The macro TYPE_ALIGN we're replacing has behavior that matches
_Alignof rather than __alignof__.

Signed-off-by: YingChi Long <me@inclyc.cn>
Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
Link: https://godbolt.org/z/sPs1GEhbT
Link: https://gcc.gnu.org/onlinedocs/gcc/Alignment.html
Link: https://reviews.llvm.org/D133574
---
v3:
- commit message changes suggested by Nick and David

v2: https://lore.kernel.org/all/20220927153338.4177854-1-me@inclyc.cn/
Signed-off-by: YingChi Long <me@inclyc.cn>
---
 arch/x86/kernel/fpu/init.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

--
2.37.4
  

On Sat, Oct 29, 2022 at 5:27 AM YingChi Long <me@inclyc.cn> 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.
>
> I've made a clang patch that rejects any definitions within
> __builtin_offsetof (usually #defined with "offsetof"), and tested
> compiling with this patch, there are no error if this patch applied.
>
> ISO C11 _Alignof is subtly different from the GNU C extension
> __alignof__. __alignof__ is the preferred alignment and _Alignof the
> minimal alignment. For 'long long' on x86 these are 8 and 4
> respectively.
>
> The macro TYPE_ALIGN we're replacing has behavior that matches
> _Alignof rather than __alignof__.
>
> Signed-off-by: YingChi Long <me@inclyc.cn>
> Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
> Link: https://godbolt.org/z/sPs1GEhbT
> Link: https://gcc.gnu.org/onlinedocs/gcc/Alignment.html
> Link: https://reviews.llvm.org/D133574

YingChi,
You may retain my reviewed by tag when resending a rebased patch that
hasn't changed significantly.

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

That reminds me, I need to resend one of my own patches; the x86
maintainers must be very busy.

> ---
> v3:
> - commit message changes suggested by Nick and David
>
> v2: https://lore.kernel.org/all/20220927153338.4177854-1-me@inclyc.cn/
> Signed-off-by: YingChi Long <me@inclyc.cn>
> ---
>  arch/x86/kernel/fpu/init.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
> index 8946f89761cc..851eb13edc01 100644
> --- a/arch/x86/kernel/fpu/init.c
> +++ b/arch/x86/kernel/fpu/init.c
> @@ -133,9 +133,6 @@ static void __init fpu__init_system_generic(void)
>         fpu__init_system_mxcsr();
>  }
>
> -/* Get alignment of the TYPE. */
> -#define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
> -
>  /*
>   * Enforce that 'MEMBER' is the last field of 'TYPE'.
>   *
> @@ -143,8 +140,8 @@ static void __init fpu__init_system_generic(void)
>   * because that's how C aligns structs.
>   */
>  #define CHECK_MEMBER_AT_END_OF(TYPE, MEMBER) \
> -       BUILD_BUG_ON(sizeof(TYPE) != ALIGN(offsetofend(TYPE, MEMBER), \
> -                                          TYPE_ALIGN(TYPE)))
> +       BUILD_BUG_ON(sizeof(TYPE) !=         \
> +                    ALIGN(offsetofend(TYPE, MEMBER), _Alignof(TYPE)))
>
>  /*
>   * We append the 'struct fpu' to the task_struct:
> --
> 2.37.4
>
  

On Sat, Oct 29, 2022 at 08:25:52PM +0800, YingChi Long wrote:
> WG14 N2350 made very clear that it is an UB having type definitions
> with in "offsetof".

Pls put the working group URL note here, not below in a Link tag.

Also, write out "UB" pls.

> This patch change the implementation of macro

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

> "TYPE_ALIGN" to builtin "_Alignof" to avoid undefined behavior.

But you don't change the implementation of TYPE_ALIGN - you replace it
with _Alignof().

> 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.
>
> I've made a clang patch that rejects any definitions within
> __builtin_offsetof (usually #defined with "offsetof"), and tested
> compiling with this patch, there are no error if this patch applied.

What does that paragraph have to do with fixing the kernel?

Is this patch going to be part of clang? If so, which version?

Does gcc need it too?

If so, should a gcc bug be opened too?

> ISO C11 _Alignof is subtly different from the GNU C extension
> __alignof__. __alignof__ is the preferred alignment and _Alignof the
> minimal alignment. For 'long long' on x86 these are 8 and 4
> respectively.
> 
> The macro TYPE_ALIGN we're replacing has behavior that matches

Who's "we"?

> _Alignof rather than __alignof__.
> 
> Signed-off-by: YingChi Long <me@inclyc.cn>
> Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
> Link: https://godbolt.org/z/sPs1GEhbT

Put that link in the text above where you talk about the *align*
differences.

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

Aha, that's the clang patch. Put it in the text above too pls.

Thx.
  

> What does that paragraph have to do with fixing the kernel?

The clang patch D133574 has been made to satisfy the requirements of WG14
N2350. Compiling the kernel with this patched clang allows me to test where
type definitions are used in the kernel in the first argument of offsetof.

> Is this patch going to be part of clang? If so, which version?

Yes. Probably clang-16 because this patch is not landed to LLVM codebase
currently. The kernel needs this patch to be successfully compiled, in order
not to break the ability of LLVM mainline to compile the kernel, I am happy to
not landing D133574 for now.

> Does gcc need it too?

Since WG14 N2350 is generally applied to the C standard, I feel that GCC should
reject/fire a warning pointing out type definitions within offsetof.

> If so, should a gcc bug be opened too?

I'm not very familiar with the GCC community, but I thought it should be good
to file a bug.

Link: https://reviews.llvm.org/D133574
  

From: Borislav Petkov
> Sent: 02 November 2022 16:56
> 
> On Sat, Oct 29, 2022 at 08:25:52PM +0800, YingChi Long wrote:
> > WG14 N2350 made very clear that it is an UB having type definitions
> > with in "offsetof".
> 
> Pls put the working group URL note here, not below in a Link tag.
> 
> Also, write out "UB" pls.

I'm also pretty sure it is only undefined because a compiler
is allowed to add padding between structure members.
So the type definition inside offsetof() could be laid out
differently from any other similar definition.

However the kernel requires that the compiler only adds the
minimum padding required to align members.
So in the kernel it is actually fine.

OTOH using Alignof() (etc) is cleaner.

This is all similar to (probably) why clang objects to arithmetic
on NULL (ie implementing offsetof by looking at the address of
a field when NULL is cast to the struct type).
This is only undefined because the NULL pointer might not
have the all-zero bit pattern.
But pretty much every C ABI uses the zero bit pattern for NULL.
If it used any other value then most of the memset() of structures
would need changing. So for portability they ought to generate
warnings as well.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)