On 11/10/22 16:03, Kyle Huey wrote:
> On Tue, Nov 8, 2022 at 10:23 AM Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
...
>> At a high level, this patch does a *LOT*. Generally, it's nice when
>> bugfixes can be encapsulted in one patch, but I think there's too much
>> going on here for one patch.
>
> Ok. How about I break the first part into two pieces, one that changes the
> signatures of copy_uabi_from_kernel_to_xstate() and
> copy_sigframe_from_user_to_xstate(), and one that moves the relevant
> KVM code from fpu_copy_uabi_to_guest_fpstate() to copy_uabi_to_xstate()
> and deals with the edge case behavior of the mask?
Sounds like a good start. My gut says there's another patch or two that
could be broken out, but that sounds like a reasonable next step.
>>> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
>>> index 3b28c5b25e12..c273669e8a00 100644
>>> --- a/arch/x86/kernel/fpu/core.c
>>> +++ b/arch/x86/kernel/fpu/core.c
>>> @@ -391,8 +391,6 @@ int fpu_copy_uabi_to_guest_fpstate(struct fpu_guest *gfpu, const void *buf,
>>> {
>>> struct fpstate *kstate = gfpu->fpstate;
>>> const union fpregs_state *ustate = buf;
>>> - struct pkru_state *xpkru;
>>> - int ret;
>>>
>>> if (!cpu_feature_enabled(X86_FEATURE_XSAVE)) {
>>> if (ustate->xsave.header.xfeatures & ~XFEATURE_MASK_FPSSE)
>>> @@ -406,16 +404,16 @@ int fpu_copy_uabi_to_guest_fpstate(struct fpu_guest *gfpu, const void *buf,
>>> if (ustate->xsave.header.xfeatures & ~xcr0)
>>> return -EINVAL;
>>>
>>> - ret = copy_uabi_from_kernel_to_xstate(kstate, ustate);
>>> - if (ret)
>>> - return ret;
>>> + /*
>>> + * Nullify @vpkru to preserve its current value if PKRU's bit isn't set
>>> + * in the header. KVM's odd ABI is to leave PKRU untouched in this
>>> + * case (all other components are eventually re-initialized).
>>> + * (Not clear that this is actually necessary for compat).
>>> + */
>>> + if (!(ustate->xsave.header.xfeatures & XFEATURE_MASK_PKRU))
>>> + vpkru = NULL;
>>
>> I'm not a big fan of hunks that are part of bugfixes where it is not
>> clear that the hunk is necessary.
>
> This is necessary to avoid changing KVM's behavior at the same time
> that we change
> ptrace, since KVM doesn't want the same behavior as ptrace.
Your "This is necessary" doesn't really match with "Not clear that this
is actually necessary" from the comment, right?
Rather than claim whether it is necessary or not, maybe just say why
it's there: it's there to preserve wonky KVM behavior.
BTW, I'd love to know if KVM *REALLY* depends on this. It'd be nice to
kill if not.
>> Would something like this be more clear?
>>
>> if (hdr.xfeatures & XFEATURE_MASK_PKRU) {
>> struct pkru_state *xpkru;
>>
>> xpkru = __raw_xsave_addr(xsave, XFEATURE_PKRU);
>> *pkru = xpkru->pkru;
>> } else {
>> /*
>> * KVM may pass a NULL 'pkru' to indicate
>> * that it does not need PKRU updated.
>> */
>> if (pkru)
>> *pkru = 0;
>> }
>
> Yeah, Sean Christopherson suggested this (with the else and if
> collapsed into a single level) when I submitted this previously.
I generally agree with Sean, but he's also been guilty of an atrocity or
two over the years. :) While I generally like low levels of
indentation I also think my version is much more clear in this case.
