Re: [PATCH] arm64/simd: Avoid pointless clearing of FP/SIMD buffer

[Eric Biggers <ebiggers@kernel.org>]

On Fri, Dec 05, 2025 at 09:13:46AM +0100, Ard Biesheuvel wrote:
> On Fri, 5 Dec 2025 at 07:50, Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Thu, Dec 04, 2025 at 05:28:15PM +0100, Ard Biesheuvel wrote:
> > > The buffer provided to kernel_neon_begin() is only used if the task is
> > > scheduled out while the FP/SIMD is in use by the kernel, or when such a
> > > section is interrupted by a softirq that also uses the FP/SIMD.
> > >
> > > IOW, this happens rarely, and even if it happened often, there is still
> > > no reason for this buffer to be cleared beforehand, which happens by
> > > default when using a compiler that supports -ftrivial-auto-var-init.
> > >
> > > So mark the buffer as __uninitialized. Given that this is a variable
> > > attribute not a type attribute, this requires that the expression is
> > > tweaked a bit.
> > >
> > > Cc: Will Deacon <will@kernel.org>,
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>,
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Eric Biggers <ebiggers@kernel.org>
> > > Cc: Justin Stitt <justinstitt@google.com>
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >  arch/arm64/include/asm/simd.h | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > The issue here is that returning a pointer to an automatic variable as
> > > it goes out of scope is slightly dodgy, especially in the context of
> > > __attribute__((cleanup())), on which the scoped guard API relies
> > > heavily. However, in this case it should be safe, given that this
> > > expression is the input to the guarded variable type's constructor.
> > >
> > > It is definitely not pretty, though, so hopefully here is a better way
> > > to attach this.
> > >
> > > diff --git a/arch/arm64/include/asm/simd.h b/arch/arm64/include/asm/simd.h
> > > index 0941f6f58a14..825b7fe94003 100644
> > > --- a/arch/arm64/include/asm/simd.h
> > > +++ b/arch/arm64/include/asm/simd.h
> > > @@ -48,6 +48,7 @@ DEFINE_LOCK_GUARD_1(ksimd,
> > >                   kernel_neon_begin(_T->lock),
> > >                   kernel_neon_end(_T->lock))
> > >
> > > -#define scoped_ksimd()       scoped_guard(ksimd, &(struct user_fpsimd_state){})
> > > +#define scoped_ksimd()       \
> > > +     scoped_guard(ksimd, ({ struct user_fpsimd_state __uninitialized s; &s; }))
> >
> > Ick.  I should have looked at the generated code more closely.
> >
> > It's actually worse than you describe, because the zeroing is there even
> > without CONFIG_INIT_STACK_ALL_ZERO=y, simply because the
> > user_fpsimd_state struct is declared using a compound literal.
> >
> > I'm afraid that this patch probably isn't a good idea, as it relies on
> > undefined behavior.  Before this patch, the user_fpsimd_state is
> > declared using a compound literal, which takes on its enclosing scope,
> > i.e. the 'for' statement generated by scoped_guard().  After this patch,
> > it's in a new inner scope, and the pointer to it escapes from it.
> >
> > Unfortunately I don't think there's any way to solve this while keeping
> > the scoped_ksimd() API as-is.
> >
> 
> How about
> 
> --- a/arch/arm64/include/asm/simd.h
> +++ b/arch/arm64/include/asm/simd.h
> @@ -48,6 +48,8 @@ DEFINE_LOCK_GUARD_1(ksimd,
>                     kernel_neon_begin(_T->lock),
>                     kernel_neon_end(_T->lock))
> 
> -#define scoped_ksimd() scoped_guard(ksimd, &(struct user_fpsimd_state){})
> +#define scoped_ksimd()         __scoped_ksimd(__UNIQUE_ID(fpsimd_state))
> +#define __scoped_ksimd(id)     struct user_fpsimd_state __uninitialized id; \
> +                               scoped_guard(ksimd, &id)

I guess that will work.  It's not great that it will make scoped_ksimd()
expand into more than one statement, which is error-prone and not
normally allowed in macros.  But it looks okay for all the current users
of it.

- Eric