Re: [PATCH v6 1/2] x86/fpu: Allow PKRU to be (once again) written by ptrace.

On Thu, Sep 1, 2022 at 8:29 AM Sean Christopherson seanjc@google.com wrote:

On Mon, Aug 29, 2022, Kyle Huey wrote:

...

@@ -1246,6 +1246,21 @@ static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf, } }

• /*
  

•  * Update the user protection key storage. Allow KVM to
  

•  * pass in a NULL pkru pointer if the mask bit is unset
  

•  * for its legacy ABI behavior.
  

•  */
  

• if (pkru)
  

•         *pkru = 0;
  

• if (hdr.xfeatures & XFEATURE_MASK_PKRU) {
  

•         struct pkru_state *xpkru;
  

•         xpkru = __raw_xsave_addr(xsave, XFEATURE_PKRU);
  

•         *pkru = xpkru->pkru;
  

• }
  

What about writing this as:

    if (hdr.xfeatures & XFEATURE_MASK_PKRU) {
            ...

            *pkru = xpkru->pkru;
    } else if (pkru) {
            *pkru = 0;
    }

to make it slightly more obvious that @pkru must be non-NULL if the feature flag is enabled?

tglx didn't seem to like the branchiness before but maybe he'll change his mind since we have to have the `if (pkru)` now anyways.

Or we could be paranoid, though I'm not sure this is worthwhile.

    if ((hdr.xfeatures & XFEATURE_MASK_PKRU) &&
        !WARN_ON_ONCE(!pkru)) {
            ...

            *pkru = xpkru->pkru;
    } else if (pkru) {
            *pkru = 0;
    }

I don't feel strongly about this.

Otherwise, looks good from a KVM perspective. Thanks!

Great.

- Kyle