On Fri, May 28, 2021, at 9:11 AM, Dave Hansen wrote: > On 5/28/21 8:32 AM, Thomas Gleixner wrote: > >> > >> This series: > >> * Moves the PKRU manipulation to a more appropriate location, > >> away from the page table code > >> * Wraps get_xsave_addr() with more structured, less error-prone > >> interfaces. > >> * Conditionally hides a pkey debugfs file, eliminating the need > >> for new runtime checks to work with the new interface. > >> * Add a selftest to make it more likely to catch bugs like this > >> in the future. This improved selftest catches this issue on > >> Intel CPUs. Without the improvement, it only triggers on AMD. > > I think all of this is fundamentaly wrong. > > > > Contrary to FPU state, PKRU has to be updated at context switch > > time. There is absolutely no point in having PKRU XSAVES managed. > > > > It's broken in several ways. Anything which clears and loads the FPU > > will load the wrong PKRU value. Go figure... > > > > So the right thing is to disable PKRU in XCR0 and on sched out simply do > > > > task->thread.pkru = read_pkru(); > > > > and on sched in > > > > write_pkru(task->thread.pkru); > > > > Simple, trivial and not going to be wreckaged by anything which fiddles > > with xstates. We all know by now that xstates is a trainwreck and not > > having stuff like that in there is making the fixes I'm doing way > > simpler. > > As for the general sentiment that PKRU is not suitable for management > with XSAVE, I'm with you. > > I have a few concerns about moving away from XSAVE management, though. > I'm not nixing the whole idea, but there are some things we need to resolve. > > First is that there _may_ be ABI concerns. I tend to think that, for -stable, we should fix the bug without an ABI change.
