@@ -76,7 +76,6 @@ 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_signal_preserve_current_state(void);
extern void fpsimd_preserve_current_state(void);
extern void fpsimd_restore_current_state(void);
extern void fpsimd_update_current_state(struct user_fpsimd_state const *state);
@@ -1662,18 +1662,6 @@ void fpsimd_preserve_current_state(void)
put_cpu_fpsimd_context();
}
-/*
- * Like fpsimd_preserve_current_state(), but ensure that
- * current->thread.uw.fpsimd_state is updated so that it can be copied to
- * the signal frame.
- */
-void fpsimd_signal_preserve_current_state(void)
-{
- fpsimd_preserve_current_state();
- if (current->thread.fp_type == FP_STATE_SVE)
- sve_to_fpsimd(current);
-}
-
/*
* Associate current's FPSIMD context with this cpu
* The caller must have ownership of the cpu FPSIMD context before calling
@@ -1766,30 +1754,14 @@ void fpsimd_restore_current_state(void)
put_cpu_fpsimd_context();
}
-/*
- * Load an updated userland FPSIMD state for 'current' from memory and set the
- * flag that indicates that the FPSIMD register contents are the most recent
- * FPSIMD state of 'current'. This is used by the signal code to restore the
- * register state when returning from a signal handler in FPSIMD only cases,
- * any SVE context will be discarded.
- */
void fpsimd_update_current_state(struct user_fpsimd_state const *state)
{
if (WARN_ON(!system_supports_fpsimd()))
return;
- get_cpu_fpsimd_context();
-
current->thread.uw.fpsimd_state = *state;
if (current->thread.fp_type == FP_STATE_SVE)
fpsimd_to_sve(current);
-
- task_fpsimd_load();
- fpsimd_bind_task_to_cpu();
-
- clear_thread_flag(TIF_FOREIGN_FPSTATE);
-
- put_cpu_fpsimd_context();
}
/*
@@ -250,6 +250,8 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
¤t->thread.uw.fpsimd_state;
int err;
+ sve_sync_to_fpsimd(current);
+
/* copy the FP and status/control registers */
err = __copy_to_user(ctx->vregs, fpsimd->vregs, sizeof(fpsimd->vregs));
__put_user_error(fpsimd->fpsr, &ctx->fpsr, err);
@@ -291,8 +293,6 @@ static int preserve_fpmr_context(struct fpmr_context __user *ctx)
{
int err = 0;
- current->thread.uw.fpmr = read_sysreg_s(SYS_FPMR);
-
__put_user_error(FPMR_MAGIC, &ctx->head.magic, err);
__put_user_error(sizeof(*ctx), &ctx->head.size, err);
__put_user_error(current->thread.uw.fpmr, &ctx->fpmr, err);
@@ -310,7 +310,7 @@ static int restore_fpmr_context(struct user_ctxs *user)
__get_user_error(fpmr, &user->fpmr->fpmr, err);
if (!err)
- write_sysreg_s(fpmr, SYS_FPMR);
+ current->thread.uw.fpmr = fpmr;
return err;
}
@@ -372,11 +372,6 @@ static int preserve_sve_context(struct sve_context __user *ctx)
err |= __copy_to_user(&ctx->__reserved, reserved, sizeof(reserved));
if (vq) {
- /*
- * This assumes that the SVE state has already been saved to
- * the task struct by calling the function
- * fpsimd_signal_preserve_current_state().
- */
err |= __copy_to_user((char __user *)ctx + SVE_SIG_REGS_OFFSET,
current->thread.sve_state,
SVE_SIG_REGS_SIZE(vq));
@@ -432,16 +427,6 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
if (user->sve_size < SVE_SIG_CONTEXT_SIZE(vq))
return -EINVAL;
- /*
- * Careful: we are about __copy_from_user() directly into
- * thread.sve_state with preemption enabled, so protection is
- * needed to prevent a racing context switch from writing stale
- * registers back over the new data.
- */
-
- fpsimd_flush_task_state(current);
- /* From now, fpsimd_thread_switch() won't touch thread.sve_state */
-
sve_alloc(current, true);
if (!current->thread.sve_state) {
clear_thread_flag(TIF_SVE);
@@ -541,11 +526,6 @@ static int preserve_za_context(struct za_context __user *ctx)
err |= __copy_to_user(&ctx->__reserved, reserved, sizeof(reserved));
if (vq) {
- /*
- * This assumes that the ZA state has already been saved to
- * the task struct by calling the function
- * fpsimd_signal_preserve_current_state().
- */
err |= __copy_to_user((char __user *)ctx + ZA_SIG_REGS_OFFSET,
current->thread.sme_state,
ZA_SIG_REGS_SIZE(vq));
@@ -580,16 +560,6 @@ static int restore_za_context(struct user_ctxs *user)
if (user->za_size < ZA_SIG_CONTEXT_SIZE(vq))
return -EINVAL;
- /*
- * Careful: we are about __copy_from_user() directly into
- * thread.sme_state with preemption enabled, so protection is
- * needed to prevent a racing context switch from writing stale
- * registers back over the new data.
- */
-
- fpsimd_flush_task_state(current);
- /* From now, fpsimd_thread_switch() won't touch thread.sve_state */
-
sme_alloc(current, true);
if (!current->thread.sme_state) {
current->thread.svcr &= ~SVCR_ZA_MASK;
@@ -627,11 +597,6 @@ static int preserve_zt_context(struct zt_context __user *ctx)
BUILD_BUG_ON(sizeof(ctx->__reserved) != sizeof(reserved));
err |= __copy_to_user(&ctx->__reserved, reserved, sizeof(reserved));
- /*
- * This assumes that the ZT state has already been saved to
- * the task struct by calling the function
- * fpsimd_signal_preserve_current_state().
- */
err |= __copy_to_user((char __user *)ctx + ZT_SIG_REGS_OFFSET,
thread_zt_state(¤t->thread),
ZT_SIG_REGS_SIZE(1));
@@ -657,16 +622,6 @@ static int restore_zt_context(struct user_ctxs *user)
if (nregs != 1)
return -EINVAL;
- /*
- * Careful: we are about __copy_from_user() directly into
- * thread.zt_state with preemption enabled, so protection is
- * needed to prevent a racing context switch from writing stale
- * registers back over the new data.
- */
-
- fpsimd_flush_task_state(current);
- /* From now, fpsimd_thread_switch() won't touch ZT in thread state */
-
err = __copy_from_user(thread_zt_state(¤t->thread),
(char __user const *)user->zt +
ZT_SIG_REGS_OFFSET,
@@ -1017,6 +972,8 @@ static int restore_sigframe(struct pt_regs *regs,
*/
forget_syscall(regs);
+ fpsimd_save_and_flush_current_state();
+
err |= !valid_user_regs(®s->user_regs, current);
if (err == 0)
err = parse_user_sigframe(&user, sf);
@@ -1510,8 +1467,11 @@ static int setup_return(struct pt_regs *regs, struct ksignal *ksig,
/*
* If we were in streaming mode the saved register
* state was SVE but we will exit SM and use the
- * FPSIMD register state - flush the saved FPSIMD
- * register state in case it gets loaded.
+ * FPSIMD register state.
+ *
+ * TODO: decide if this should behave as SMSTOP (e.g. reset
+ * FPSR + FPMR), or whether this should only clear the scalable
+ * registers + ZA state.
*/
if (current->thread.svcr & SVCR_SM_MASK) {
memset(¤t->thread.uw.fpsimd_state, 0,
@@ -1519,9 +1479,7 @@ static int setup_return(struct pt_regs *regs, struct ksignal *ksig,
current->thread.fp_type = FP_STATE_FPSIMD;
}
- current->thread.svcr &= ~(SVCR_ZA_MASK |
- SVCR_SM_MASK);
- sme_smstop();
+ current->thread.svcr &= ~(SVCR_ZA_MASK | SVCR_SM_MASK);
}
return 0;
@@ -1535,7 +1493,7 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,
struct user_access_state ua_state;
int err = 0;
- fpsimd_signal_preserve_current_state();
+ fpsimd_save_and_flush_current_state();
if (get_sigframe(&user, ksig, regs))
return 1;
There are several issues with the way the native signal handling code manipulates FPSIMD/SVE/SME state, described in detail below. These issues largely result from races with preemption and inconsistent handling of live state vs saved state. Known issues with native FPSIMD/SVE/SME state management include: * On systems with FPMR, the code to save/restore the FPMR accesses the register while it is not owned by the current task. Consequently, this may corrupt the FPMR of the current task and/or may corrupt the FPMR of an unrelated task. The FPMR save/restore has been broken since it was introduced in commit: 8c46def44409fc91 ("arm64/signal: Add FPMR signal handling") * On systems with SME, setup_return() modifies both the live register state and the saved state register state regardless of whether the task's state is live, and without holding the cpu fpsimd context. Consequently: - This may corrupt the state an unrelated task which has PSTATE.SM set and/or PSTATE.ZA set. - The task may enter the signal handler in streaming mode, and or with ZA storage enabled unexpectedly. - The task may enter the signal handler in non-streaming SVE mode with stale SVE register state, which may have been inherited from streaming SVE mode unexpectedly. Where the streaming and non-streaming vector lengths differ, this may be packed into registers arbitrarily. This logic has been broken since it was introduced in commit: 40a8e87bb32855b3 ("arm64/sme: Disable ZA and streaming mode when handling signals") Further incorrect manipulation of state was added in commits: ea64baacbc36a0d5 ("arm64/signal: Flush FPSIMD register state when disabling streaming mode") baa8515281b30861 ("arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE") * Several restoration functions use fpsimd_flush_task_state() to discard the live FPSIMD/SVE/SME while the in-memory copy is stale. When a subset of the FPSIMD/SVE/SME state is restored, the remainder may be non-deterministically reset to a stale snapshot from some arbitrary point in the past. This non-deterministic discarding was introduced in commit: 8cd969d28fd2848d ("arm64/sve: Signal handling support") As of that commit, when TIF_SVE was initially clear, failure to restore the SVE signal frame could reset the FPSIMD registers to a stale snapshot. The pattern of discarding unsaved state was subsequently copied into restoration functions for some new state in commits: 39782210eb7e8763 ("arm64/sme: Implement ZA signal handling") ee072cf708048c0d ("arm64/sme: Implement signal handling for ZT") * On systems with SME/SME2, the entire FPSIMD/SVE/SME state may be loaded onto the CPU redundantly. Either restore_fpsimd_context() or restore_sve_fpsimd_context() will load the entire FPSIMD/SVE/SME state via fpsimd_update_current_state() before restore_za_context() and restore_zt_context() each discard the state via fpsimd_flush_task_state(). This is purely redundant work, and not a functional bug. To fix these issues, rework the native signal handling code to always save+flush the current task's FPSIMD/SVE/SME state before manipulating that state. This avoids races with preemption and ensures that state is manipulated consistently regardless of whether it happened to be live prior to manipulation. This largely involes: * Using fpsimd_save_and_flush_current_state() to save+flush the state for both signal delivery and signal return, before the state is manipulated in any way. * Removing fpsimd_signal_preserve_current_state() and updating preserve_fpsimd_context() to explicitly ensure that the FPSIMD state is up-to-date, as preserve_fpsimd_context() is the only consumer of the FPSIMD state during signal delivery. * Modifying fpsimd_update_current_state() to not reload the FPSIMD state onto the CPU. Ideally we'd remove fpsimd_update_current_state() entirely, but I've left that for subsequent patches as there are a number of of other problems with the FPSIMD<->SVE conversion helpers that should be addressed at the same time. For now I've removed the misleading comment. For setup_return(), we need to decide (for ABI reasons) whether signal delivery should have all the side-effects of an SMSTOP. For now I've left a TODO comment, as there are other questions in this area that I'll address with subsequent patches. Fixes: 8c46def44409fc91 ("arm64/signal: Add FPMR signal handling") Fixes: 40a8e87bb32855b3 ("arm64/sme: Disable ZA and streaming mode when handling signals") Fixes: ea64baacbc36a0d5 ("arm64/signal: Flush FPSIMD register state when disabling streaming mode") Fixes: baa8515281b30861 ("arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE") Fixes: 8cd969d28fd2848d ("arm64/sve: Signal handling support") Fixes: 39782210eb7e8763 ("arm64/sme: Implement ZA signal handling") Fixes: ee072cf708048c0d ("arm64/sme: Implement signal handling for ZT") Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Marc Zyngier <maz@kernel.org> Cc: Mark Brown <broonie@kernel.org> Cc: Will Deacon <will@kernel.org> Reviewed-by: Mark Brown <broonie@kernel.org> --- arch/arm64/include/asm/fpsimd.h | 1 - arch/arm64/kernel/fpsimd.c | 28 -------------- arch/arm64/kernel/signal.c | 66 ++++++--------------------------- 3 files changed, 12 insertions(+), 83 deletions(-)