Messages in this thread | | | Subject | Re: [PATCH 15/23] x86/fpu: Add sanity checks for XFD | From | Dave Hansen <> | Date | Mon, 25 Oct 2021 12:57:50 -0700 |
| |
On 10/25/21 11:13 AM, Thomas Gleixner wrote: > On Mon, Oct 25 2021 at 11:33, Mika Penttilä wrote: >> On 22.10.2021 1.55, Chang S. Bae wrote: >>> +#ifdef CONFIG_X86_DEBUG_FPU >>> +/* >>> + * Ensure that a subsequent XSAVE* or XRSTOR* instruction with RFBM=@mask >>> + * can safely operate on the @fpstate buffer. >>> + */ >>> +static bool xstate_op_valid(struct fpstate *fpstate, u64 mask, bool rstor) >>> +{ >>> + u64 xfd = __this_cpu_read(xfd_state); >>> + >>> + if (fpstate->xfd == xfd) >>> + return true; >>> + >>> + /* For current's fpstate the XFD state must be correct. */ >>> + if (fpstate->xfd == current->thread.fpu.fpstate->xfd) >>> + return false; >>> + >> Should this return true or is the comment confusing? > Comment might be confusing. The logic here is: > > If fpstate->xfd equal xfd then it's valid. > > So the next check is whether fpstate is the same as current's > fpstate. If that's the case then the result is invalid because for > current's fpstate the first condition should be true. But if it is not > true then the state is not valid.
Maybe I'm just a dummy today, but I spent way too much time looking at that line of code. Would a slightly longer comment help. Perhaps:
/* * The XFD MSR doesn't match the passed-in fpstate. * Make sure it at least matches current's fpstate. * If not, XFD was set up wrong and is invalid. */
Below is an even more exhaustive look at it. Feel free to ignore.
--
In most cases, we are doing a normal old context switch. XFD is already set up by this point and the @fpstate is pointed to by current->thread.fpu. fpstate->xfd==xfd, life is good and we skip the rest of the checks.
If they don't match, we have some more checks to do. Here's a bit-by-bit walk through it. Just like the hardware MSR at this point, assume that only single bit can be set in any of the xfd's.
MSR | fpstate | cur->fpstate | valid ------------------------------------- 0 | 0 | x | 1 // MSR matches @fpstate 0 | 1 | 0 | 1 // MSR matches cur->fpstate 0 | 1 | 1 | 0 <- *** MSR matches nothing! 1 | 0 | 0 | 0 <- *** MSR matches nothing! 1 | 0 | 1 | 1 // MSR matches cur->fpstate 1 | 1 | x | 1 // MSR matches @fpstate
(BTW, "valid" here means either return true from earlier, or falling through to the later xstate_op_valid() checks)
If a bit is *CLEAR* in the MSR, then it better be *CLEAR* in either fpstate->xfd or current->...fpstate->xfd.
If a bit is *SET* in the MSR, then it better be *SET* in either fpstate->xfd or current->...fpstate->xfd.
It's bad if the MSR doesn't match *either* fpstate->xfd or current->...fpstate->xfd. Because, if it does not match, how did we get here? What random fpstate was the MSR set up to work with?
| |