Re: [PATCH 18/22] x86/fpu: Update xstate's PKRU value on write_pkru()

Dave Hansen
2019-01-23T17:28:24Z

On 1/9/19 3:47 AM, Sebastian Andrzej Siewior wrote:
> + pk = get_xsave_addr(&current->thread.fpu.state.xsave, XFEATURE_PKRU);
> + /*
> + * The PKRU value in xstate needs to be in sync with the value that is
> + * written to the CPU. The FPU restore on return to userland would
> + * otherwise load the previous value again.
> + */
> + __fpregs_changes_begin();
> + if (pk)
> + pk->pkru = pkru;
> + __write_pkru(pkru);
> + __fpregs_changes_end();
> }

I'm not sure this is right.

The "if (pk)" check is basically to see if there was a location for
XFEATURE_PKRU in the XSAVE buffer. The only way this can be false in
the current code is if the "init optimization" is in play and
XFEATURE_PKRU was in the init state (all 0's for PKRU).

If it were in the init state, we need to take it *out* of the init
state, both in the buffer and in the registers. The __write_pkru()
obviously does this for the registers, but "pk->pkru = pkru" is not
enough for the XSAVE buffer. xsave->header.xfeatures (aka. XSTATE_BV)
also needs to have XFEATURE_PKRU set. Otherwise, two calls to this
function in succession would break.

pk = get_xsave_addr(...xsave, XFEATURE_PKRU);
pk->pkru = pkru;
__write_pkru(pkru);

pk = get_xsave_addr(...xsave, XFEATURE_PKRU);
/* 'pk' is still NULL, won't see 'pkru' set */

I *think* just setting

xsave->header.xfeatures |= XFEATURE_MASK_PKRU;

will fix this. I thought we did that whole dance somewhere else in the
code, but I don't see it right now. Might have been in some other patch.