On Wed, Oct 30, 2024 at 08:23:50PM +0000, Mark Brown wrote:
> When we update the in memory register state in sve_init_regs() we neglect
> to flush the task's CPU binding, meaning if the task is rescheduled to
> the last CPU it ran on it is possible for the check for current state in
> fpsimd_thread_switch() to falsely determine that up to date register
> state is present on the CPU. This results in it incorrectly clearing
> TIF_FOREIGN_FPSTATE and suppress reloading.
>
> This will also suppress the sve_user_enable() done in
> fpsimd_bind_task_to_cpu() as part of return to userspace, causing
> spurious SVE access traps.
>
> Call fpsimd_flush_task_state() to invalidate the last loaded CPU
> recorded in the task.
>
> Fixes: cccb78ce89c4 ("arm64/sve: Rework SVE access trap to convert state in registers")
> Reported-by: Mark Rutlamd <mark.rutland@xxxxxxx>
> Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
How about the following:
| arm64/sve: Discard stale CPU state when handling SVE traps
|
| The logic for handling SVE traps manipulates saved FPSIMD/SVE state
| incorrectly, and a race with preemption can result in a task having
| TIF_SVE set and TIF_FOREIGN_FPSTATE clear even though the live CPU state
| is stale (e.g. with SVE traps enabled). This has been observed to result
| in warnings from do_sve_acc() where SVE traps are not expected while
| TIF_SVE is set:
|
| | if (test_and_set_thread_flag(TIF_SVE))
| | WARN_ON(1); /* SVE access shouldn't have trapped */
|
| Warnings of this form have been reported intermittently, e.g.
|
| https://lore.kernel.org/linux-arm-kernel/CA+G9fYtEGe_DhY2Ms7+L7NKsLYUomGsgqpdBj+QwDLeSg=JhGg@xxxxxxxxxxxxxx/
| https://lore.kernel.org/linux-arm-kernel/000000000000511e9a060ce5a45c@xxxxxxxxxx/
|
| The race can occur when the SVE trap handler is preempted before and
| after manipulating the saved FPSIMD/SVE state, starting and ending on
| the same CPU, e.g.
|
| | void do_sve_acc(unsigned long esr, struct pt_regs *regs)
| | {
| | // Trap on CPU 0 with TIF_SVE clear, SVE traps enabled
| | // task->fpsimd_cpu is 0.
| | // per_cpu_ptr(&fpsimd_last_state, 0) is task.
| |
| | ...
| |
| | // Preempted; migrated from CPU 0 to CPU 1.
| | // TIF_FOREIGN_FPSTATE is set.
| |
| | get_cpu_fpsimd_context();
| |
| | if (test_and_set_thread_flag(TIF_SVE))
| | WARN_ON(1); /* SVE access shouldn't have trapped */
| |
| | sve_init_regs() {
| | if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
| | ...
| | fpsimd_bind_task_to_cpu();
| | } else {
| | fpsimd_to_sve(current);
| | current->thread.fp_type = FP_STATE_SVE;
| | }
| | }
| |
| | put_cpu_fpsimd_context();
| |
| | // Preempted; migrated from CPU 1 to CPU 0.
| | // task->fpsimd_cpu is still 0
| | // If per_cpu_ptr(&fpsimd_last_state, 0) is still task then:
| | // - Stale HW state is reused (with SVE traps enabled)
| | // - TIF_FOREIGN_FPSTATE is cleared
| | // - A return to userspace skips HW state restore
| | }
|
| In the case where sve_init_regs() is called while the state is live and
| TIF_FOREIGN_FPSTATE is clear, the state is correctly modified and the
| call to fpsimd_bind_task_to_cpu() disables the SVE trap.
|
| Fix the case where the state is not live and TIF_FOREIGN_FPSTATE is set
| by calling fpsimd_flush_task_state() to detach from the saved CPU
| state. This ensures that a subsequent context switch will not reuse the
| stale CPU state, and will instead set TIF_FOREIGN_FPSTATE, forcing the
| new state to be reloaded from memory prior to a return to userspace.
|
| Fixes: cccb78ce89c4 ("arm64/sve: Rework SVE access trap to convert state in registers")
| Reported-by: Mark Rutland <mark.rutland@xxxxxxx>
| Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>
| Cc: stable@xxxxxxxxxxxxxxx
Note: I fixed the typo (s/Rutlamd/Rutland)
With that:
Reviewed-by: Mark Rutland <mark.rutland@xxxxxxx>
Mark.
> ---
> arch/arm64/kernel/fpsimd.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 77006df20a75aee7c991cf116b6d06bfe953d1a4..6d21971ae5594f32947480cfa168db400a69a283 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -1367,6 +1367,7 @@ static void sve_init_regs(void)
> } else {
> fpsimd_to_sve(current);
> current->thread.fp_type = FP_STATE_SVE;
> + fpsimd_flush_task_state(current);
> }
> }
>
>
> --
> 2.39.2
>
>
