On Tue, Dec 03, 2024 at 12:45:57PM +0000, Mark Brown wrote:
> We intend that signal handlers are entered with PSTATE.{SM,ZA}={0,0}.
> The logic for this in setup_return() manipulates the saved state and
> live CPU state in an unsafe manner, and consequently, when a task enters
> a signal handler:
>
> * The task entering the signal handler might not have its PSTATE.{SM,ZA}
> bits cleared, and other register state that is affected by changes to
> PSTATE.{SM,ZA} might not be zeroed as expected.
>
> * An unrelated task might have its PSTATE.{SM,ZA} bits cleared
> unexpectedly, potentially zeroing other register state that is
> affected by changes to PSTATE.{SM,ZA}.
>
> Tasks which do not set PSTATE.{SM,ZA} (i.e. those only using plain
> FPSIMD or non-streaming SVE) are not affected, as there is no
> resulting change to PSTATE.{SM,ZA}.
>
> Consider for example two tasks on one CPU:
>
> A: Begins signal entry in kernel mode, is preempted prior to SMSTOP.
> B: Using SM and/or ZA in userspace with register state current on the
> CPU, is preempted.
> A: Scheduled in, no register state changes made as in kernel mode.
> A: Executes SMSTOP, modifying live register state.
> A: Scheduled out.
> B: Scheduled in, fpsimd_thread_switch() sees the register state on the
> CPU is tracked as being that for task B so the state is not reloaded
> prior to returning to userspace.
>
> Task B is now running with SM and ZA incorrectly cleared.
>
> Fix this by:
>
> * Checking TIF_FOREIGN_FPSTATE, and only updating the saved or live
> state as appropriate.
>
> * Using {get,put}_cpu_fpsimd_context() to ensure mutual exclusion
> against other code which manipulates this state. To allow their use,
> the logic is moved into a new fpsimd_enter_sighandler() helper in
> fpsimd.c.
>
> This race has been observed intermittently with fp-stress, especially
> with preempt disabled, commonly but not exclusively reporting "Bad SVCR: 0".
>
> While we're at it also fix a discrepancy between in register and in memory
> entries. When operating on the register state we issue a SMSTOP, exiting
> streaming mode if we were in it. This clears the V/Z and P register and
> FPMR but nothing else. The in memory version clears all the user FPSIMD
> state including FPCR and FPSR but does not clear FPMR. Add the clear of
> FPMR and limit the existing memset() to only cover the vregs, preserving
> the state of FPCR and FPSR like SMSTOP does.
>
> Fixes: 40a8e87bb3285 ("arm64/sme: Disable ZA and streaming mode when handling signals")
> Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
> arch/arm64/include/asm/fpsimd.h | 1 +
> arch/arm64/kernel/fpsimd.c | 39 +++++++++++++++++++++++++++++++++++++++
> arch/arm64/kernel/signal.c | 19 +------------------
> 3 files changed, 41 insertions(+), 18 deletions(-)
>
> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> index f2a84efc361858d4deda99faf1967cc7cac386c1..09af7cfd9f6c2cec26332caa4c254976e117b1bf 100644
> --- a/arch/arm64/include/asm/fpsimd.h
> +++ b/arch/arm64/include/asm/fpsimd.h
> @@ -76,6 +76,7 @@ extern void fpsimd_load_state(struct user_fpsimd_state *state);
> extern void fpsimd_thread_switch(struct task_struct *next);
> extern void fpsimd_flush_thread(void);
>
> +extern void fpsimd_enter_sighandler(void);
> extern void fpsimd_signal_preserve_current_state(void);
> extern void fpsimd_preserve_current_state(void);
> extern void fpsimd_restore_current_state(void);
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index f02762762dbcf954e9add6dfd3575ae7055b6b0e..c5465c8ec467cb1ab8bd211dc5370f91aa2bcf35 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -1696,6 +1696,45 @@ void fpsimd_signal_preserve_current_state(void)
> sve_to_fpsimd(current);
> }
>
> +/*
> + * Called by the signal handling code when preparing current to enter
> + * a signal handler. Currently this only needs to take care of exiting
> + * streaming mode and clearing ZA on SME systems.
> + */
> +void fpsimd_enter_sighandler(void)
> +{
> + if (!system_supports_sme())
> + return;
> +
> + get_cpu_fpsimd_context();
> +
> + if (test_thread_flag(TIF_FOREIGN_FPSTATE)) {
> + /*
> + * Exiting streaming mode zeros the V/Z and P
> + * registers and FPMR. Zero FPMR and the V registers,
> + * marking the state as FPSIMD only to force a clear
> + * of the remaining bits during reload if needed.
> + */
> + if (current->thread.svcr & SVCR_SM_MASK) {
> + memset(¤t->thread.uw.fpsimd_state.vregs, 0,
> + sizeof(current->thread.uw.fpsimd_state.vregs));
Do we need to hold the CPU fpsimd context across this memset?
IIRC, TIF_FOREIGN_FPSTATE can be spontaneously cleared along with
dumping of the regs into thread_struct (from current's PoV), but never
spontaneously set again. So ... -> [*]
> + current->thread.uw.fpmr = 0;
> + current->thread.fp_type = FP_STATE_FPSIMD;
> + }
> +
> + current->thread.svcr &= ~(SVCR_ZA_MASK |
> + SVCR_SM_MASK);
> +
> + /* Ensure any copies on other CPUs aren't reused */
> + fpsimd_flush_task_state(current);
(This is very similar to fpsimd_flush_thread(); can they be unified?)
> + } else {
> + /* The register state is current, just update it. */
> + sme_smstop();
... [*] the critical thing seems to be that the CPU fpsimd context is
held from the test on TIF_FOREIGN_FPSTATE, across this else clause.
(Whether or not this is a worthwhile optimisation is another matter.
But if the behaviour of TIF_FOREIGN_FPSTATE is still the same, then it
may be a good idea to avoid sending mixed messages about this in the
code.)
(A similar argument applies in fpsimd_flush_thread().)
[...]
Cheers
---Dave
