Re: [PATCH] x86: Use __fpu_invalidate_fpregs_state() in exec

[Sohil Mehta <sohil.mehta@intel.com>]

On 8/18/2023 12:35 PM, Lijun Pan wrote:

> So, I am thinking if it is more rigorous to have both 
> (__cpu_invalidate_fpregs_state, __fpu_invalidate_fpregs_state) 
> invalidated, similarly as fpregs_state_valid checks both conditions.
> 
> code changes like below:
> diff --git a/arch/x86/kernel/fpu/context.h b/arch/x86/kernel/fpu/context.h
> index 958accf2ccf0..fd3304bed0a2 100644
> --- a/arch/x86/kernel/fpu/context.h
> +++ b/arch/x86/kernel/fpu/context.h
> @@ -19,8 +19,8 @@
>    * FPU state for a task MUST let the rest of the kernel know that the
>    * FPU registers are no longer valid for this task.
>    *
> - * Either one of these invalidation functions is enough. Invalidate
> - * a resource you control: CPU if using the CPU for something else
> + * To be more rigorous and to prevent from future corner case, Invalidate
> + * both resources you control: CPU if using the CPU for something else
>    * (with preemption disabled), FPU for the current task, or a task that
>    * is prevented from running by the current task.
>    */

This is a general comment in the header to guide multiple scenarios. I
am not sure if invalidating both would be feasible in all scenarios. For
example, in cases such as the fpu_idle_fpregs() there is no task fpu
available.

I think the current issue stems from the fact that the code in exec()
doesn't really control the CPU. It is mostly running with preemption
enabled which can cause the CPUs to change as described in Rick's
scenario. Thus clearing CPU1's FPU pointer in step 4 seems unnecessary.

An extra invalidation probably won't impact performance for such a rare
scenario. However, replacing it with __fpu_invalidate_fpregs_state() to
invalidate the task's fpu state looks correct to me.

Maybe, we can expand the comment above to specify exactly when a
particular invalidation is expected vs when both are expected.

> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 97a899bf98b9..08b9cef0e076 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -725,6 +725,7 @@ static void fpu_reset_fpregs(void)
> 
>          fpregs_lock();
>          fpu__drop(fpu);
> +       __fpu_invalidate_fpregs_state(fpu);
>          /*
>           * This does not change the actual hardware registers. It just
>           * resets the memory image and sets TIF_NEED_FPU_LOAD so a
> 
> Thanks,
> Lijun
>