On Wed, Jun 02, 2021 at 11:55:46AM +0200, Thomas Gleixner wrote:
> From: Andy Lutomirski <luto@xxxxxxxxxx>
>
> If XRSTOR fails due to sufficiently complicated paging errors (e.g.
> concurrent TLB invalidation),
I can't connect "concurrent TLB invalidation" to "sufficiently
complicated paging errors". Can you elaborate pls?
> it may fault with #PF but still modify
> portions of the user extended state.
Yikes, leaky leaky insn.
> If this happens in __fpu_restore_sig() with a victim task's FPU registers
> loaded and the task is preempted by the victim task,
This is probably meaning another task but the only task mentioned here
is a "victim task"?
> the victim task
> resumes with partially corrupt extended state.
>
> Invalidate the FPU registers when XRSTOR fails to prevent this scenario.
>
> Fixes: 1d731e731c4c ("x86/fpu: Add a fastpath to __fpu__restore_sig()")
> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
> arch/x86/kernel/fpu/signal.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -369,6 +369,27 @@ static int __fpu__restore_sig(void __use
> fpregs_unlock();
> return 0;
> }
> +
> + if (test_thread_flag(TIF_NEED_FPU_LOAD)) {
> + /*
> + * The FPU registers do not belong to current, and
> + * we just did an FPU restore operation, restricted
Please get rid of the "we"-personal pronouns formulations.
> + * to the user portion of the register file, and
"register file"? That sounds like comment which belongs in microcode but
not in software. :-)
> + * failed. In the event that the ucode was
> + * unfriendly and modified the registers at all, we
> + * need to make sure that we aren't corrupting an
> + * innocent non-current task's registers.
> + */
> + __cpu_invalidate_fpregs_state();
> + } else {
> + /*
> + * As above, we may have just clobbered current's
> + * user FPU state. We will either successfully
> + * load it or clear it below, so no action is
> + * required here.
> + */
> + }
I'm wondering if that comment can simply be above the TIF_NEED_FPU_LOAD
testing, standalone, instead of having it in an empty else? And then get
rid of that else.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
