[PATCH 0/2] arm64/fp: Fix missing invalidation when working with in memory FP state

[PATCH 0/2] arm64/fp: Fix missing invalidation when working with in memory FP state

[Mark Brown]

Mark Rutland identified a repeated pattern where we update the in memory
floating point state for tasks but do not invalidate the tracking of the
last CPU that the task's state was loaded on, meaning that we can
incorrectly fail to load the state from memory due to the checking in
fpsimd_thread_switch().  When we change the in-memory state we need to
also invalidate the last CPU information so that the state is corretly
identified as needing to be reloaded from memory.

This series adds the missing invalidations.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
Mark Brown (2):
      arm64/sve: Flush foreign register state in sve_init_regs()
      arm64/sme: Flush foreign register state in do_sme_acc()

 arch/arm64/kernel/fpsimd.c | 3 +++
 1 file changed, 3 insertions(+)
---
base-commit: 8e929cb546ee42c9a61d24fae60605e9e3192354
change-id: 20241030-arm64-fpsimd-foreign-flush-6913aa24cd9b

Best regards,
-- 
Mark Brown <broonie@kernel.org>

[PATCH 1/2] arm64/sve: Flush foreign register state in sve_init_regs()

[Mark Brown]

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@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: stable@vger.kernel.org
---
 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

Re: [PATCH 1/2] arm64/sve: Flush foreign register state in sve_init_regs()

[Mark Rutland]

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@arm.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Cc: stable@vger.kernel.org

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@mail.gmail.com/
|   https://lore.kernel.org/linux-arm-kernel/000000000000511e9a060ce5a45c@google.com/
| 
| 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@arm.com>
| Signed-off-by: Mark Brown <broonie@kernel.org>
| Cc: stable@vger.kernel.org

Note: I fixed the typo (s/Rutlamd/Rutland)

With that:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

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
> 
> 

[PATCH 2/2] arm64/sme: Flush foreign register state in do_sme_acc()

[Mark Brown]

When do_sme_acc() runs with foreign FP state it does not do any updates of
the task structure, relying on the next return to userspace to reload the
register state appropriately, but leaves the task's last loaded CPU
untouched. This means that if the task returns to userspace on the last
CPU it ran on then the checks in fpsimd_bind_task_to_cpu() will incorrectly
determine that the register state on the CPU is current and suppress reload
of the floating point register state before returning to userspace. This
will result in spurious warnings due to SME access traps occuring for the
task after TIF_SME is set.

Call fpsimd_flush_task_state() to invalidate the last loaded CPU
recorded in the task, forcing detection of the task as foreign.

Fixes: 8bd7f91c03d8 ("arm64/sme: Implement traps and syscall handling for SME")Reported-by: Mark Rutlamd <mark.rutland@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/kernel/fpsimd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 6d21971ae5594f32947480cfa168db400a69a283..1eaa670cbffa448c1aced8c8b37040492e18a21f 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -1460,6 +1460,8 @@ void do_sme_acc(unsigned long esr, struct pt_regs *regs)
 		sme_set_vq(vq_minus_one);
 
 		fpsimd_bind_task_to_cpu();
+	} else {
+		fpsimd_flush_task_state(current);
 	}
 
 	put_cpu_fpsimd_context();

-- 
2.39.2

Re: [PATCH 0/2] arm64/fp: Fix missing invalidation when working with in memory FP state

[Will Deacon]

On Wed, 30 Oct 2024 20:23:49 +0000, Mark Brown wrote:
> Mark Rutland identified a repeated pattern where we update the in memory
> floating point state for tasks but do not invalidate the tracking of the
> last CPU that the task's state was loaded on, meaning that we can
> incorrectly fail to load the state from memory due to the checking in
> fpsimd_thread_switch().  When we change the in-memory state we need to
> also invalidate the last CPU information so that the state is corretly
> identified as needing to be reloaded from memory.
> 
> [...]

Applied SVE patch (with updated commit message) to arm64 (for-next/fixes),
thanks!

[1/2] arm64/sve: Flush foreign register state in sve_init_regs()
      https://git.kernel.org/arm64/c/751ecf6afd65

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev