Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220925153151.2467884-1-me@inclyc.cn> <20221006141442.2475978-1-me@inclyc.cn>
In-Reply-To: <20221006141442.2475978-1-me@inclyc.cn>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Thu, 6 Oct 2022 10:34:31 -0700
Message-ID: <CAKwvOd=sS3zMj4LxyM_FeMDCtyirgawcv-ek2ApY5fjaYL6MyQ@mail.gmail.com>
Subject: Re: [PATCH v3] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN
To:     YingChi Long <me@inclyc.cn>
Cc:     bp@alien8.de, chang.seok.bae@intel.com,
        dave.hansen@linux.intel.com, hpa@zytor.com,
        linux-kernel@vger.kernel.org, mingo@redhat.com,
        pbonzini@redhat.com, tglx@linutronix.de, x86@kernel.org,
        david.laight@aculab.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Oct 6, 2022 at 7:15 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

Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
> v3:
> - commit message changes suggested by Nick and David
>
> v2: https://lore.kernel.org/all/20220927153338.4177854-1-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 621f4b6cac4a..de96c11e1fe9 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.35.1
>


-- 
Thanks,
~Nick Desaulniers