On Tue, May 22, 2018 at 05:05:02PM +0100, Dave Martin wrote:
> fpsimd_last_state.st is set to NULL as a way of indicating that
> current's FPSIMD registers are no longer loaded in the cpu. In
> particular, this is done when the kernel temporarily uses or
> clobbers the FPSIMD registers for its own purposes, as in CPU PM or
> kernel-mode NEON, resulting in them being populated with garbage
> data not belonging to a task.
>
> Commit 17eed27b02da ("arm64/sve: KVM: Prevent guests from using
> SVE") factors this operation out as a new helper
> fpsimd_flush_cpu_state() to make it clearer what is being done
> here, and on SVE systems this helper is now used, via
> kvm_fpsimd_flush_cpu_state(), to invalidate the registers after KVM
> has run a vcpu. The reason for this is that KVM does not yet
> understand how to restore the full host SVE registers itself after
> loading the guest FPSIMD context into them.
>
> This exposes a particular problem: if fpsimd_last_state.st is set
> to NULL without also setting TIF_FOREIGN_FPSTATE, the kernel may
> continue to think that current's FPSIMD registers are live even
> though they have actually been clobbered.
>
> Prior to the aforementioned commit, the only path where
> fpsimd_last_state.st is set to NULL without setting
> TIF_FOREIGN_FPSTATE is when kernel_neon_begin() is called by a
> kernel thread (where current->mm can be NULL). This does not
> matter, because the only harm is that at context-switch time
> fpsimd_thread_switch() may unnecessarily save the FPSIMD registers
> back to current's thread_struct (even though kernel threads are not
> considered to have any FPSIMD context of their own and the
> registers will never be reloaded).
>
> Note that although CPU_PM_ENTER lacks the TIF_FOREIGN_FPSTATE
> setting, every CPU passing through that path must subsequently pass
> through CPU_PM_EXIT before it can re-enter the kernel proper.
> CPU_PM_EXIT sets the flag.
>
> The sve_flush_cpu_state() function added by commit 17eed27b02da
> also lacks the proper maintenance of TIF_FOREIGN_FPSTATE. This may
> cause the bits of a host task's SVE registers that do not alias the
> FPSIMD register file to spontaneously appear zeroed if a KVM vcpu
> runs in the same task in the meantime. Although this effect is
> hidden by the fact that the non-FPSIMD bits of the SVE registers
> are zeroed by a syscall anyway, it is doubtless a bad idea to rely
> on these different code paths interacting correctly under future
> maintenance.
>
> This patch makes TIF_FOREIGN_FPSTATE an unconditional side-effect
> of fpsimd_flush_cpu_state(), and removes the set_thread_flag()
> calls that become redunant as a result. This ensures that
nit: redundant
> TIF_FOREIGN_FPSTATE cannot remain clear if the FPSIMD state in the
> FPSIMD registers is invalid.
>
> Signed-off-by: Dave Martin <Dave.Martin@xxxxxxx>
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Will Deacon <will.deacon@xxxxxxx>
> Cc: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>
> ---
>
> Changes since v9:
>
> * New patch (bugfix to subsequent commits).
> ---
> arch/arm64/kernel/fpsimd.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 87a3536..12e1c96 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -1067,6 +1067,7 @@ void fpsimd_flush_task_state(struct task_struct *t)
> static inline void fpsimd_flush_cpu_state(void)
> {
> __this_cpu_write(fpsimd_last_state.st, NULL);
> + set_thread_flag(TIF_FOREIGN_FPSTATE);
> }
>
> /*
> @@ -1121,10 +1122,8 @@ void kernel_neon_begin(void)
> __this_cpu_write(kernel_neon_busy, true);
>
> /* Save unsaved task fpsimd state, if any: */
> - if (current->mm) {
> + if (current->mm)
> task_fpsimd_save();
> - set_thread_flag(TIF_FOREIGN_FPSTATE);
> - }
>
> /* Invalidate any task state remaining in the fpsimd regs: */
> fpsimd_flush_cpu_state();
> @@ -1251,8 +1250,6 @@ static int fpsimd_cpu_pm_notifier(struct notifier_block *self,
> fpsimd_flush_cpu_state();
> break;
> case CPU_PM_EXIT:
> - if (current->mm)
> - set_thread_flag(TIF_FOREIGN_FPSTATE);
> break;
> case CPU_PM_ENTER_FAILED:
> default:
> --
> 2.1.4
>
Reviewed-by: Christoffer Dall <christoffer.dall@xxxxxxx>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
