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

[Eric Biggers <ebiggers@kernel.org>]

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.

Best I can come up with is to leave it to the callers to allocate the
state, and then use scoped_guard() similar to a regular lock:

        struct user_fpsimd_state __uninitialized fpsimd_state;                   
                                                                                 
        scoped_guard(ksimd, &fpsimd_state)                                       
                foo_neon(...)

Maybe wrap the state declaration with a macro:
DECLARE_FPSIMD_STATE_ONSTACK(fpsimd_state);

- Eric