Messages in this thread | | | Date | Fri, 27 Oct 2023 23:45:17 +0800 | Subject | Re: [PATCH v6 06/25] x86/fpu/xstate: Opt-in kernel dynamic bits when calculate guest xstate size | From | "Yang, Weijiang" <> |
| |
On 10/27/2023 1:24 AM, Sean Christopherson wrote: > On Wed, Oct 25, 2023, Weijiang Yang wrote: >> On 10/25/2023 1:07 AM, Sean Christopherson wrote: >>> On Fri, Sep 15, 2023, Weijiang Yang wrote: >>> IIUC, the "dynamic" features contains CET_KERNEL, whereas xfeatures_mask_supervisor() >>> conatins PASID, CET_USER, and CET_KERNEL. PASID isn't virtualized by KVM, but >>> doesn't that mean CET_USER will get dropped/lost if userspace requests AMX/XTILE >>> enabling? >> Yes, __state_size is correct for guest enabled xfeatures, including CET_USER, >> and it gets removed from __state_perm. >> >> IIUC, from current qemu/kernel interaction for guest permission settings, >> __xstate_request_perm() is called only _ONCE_ to set AMX/XTILE for every vCPU >> thread, so the removal of guest supervisor xfeatures won't hurt guest! ;-/ > Huh? I don't follow. What does calling __xstate_request_perm() only once have > to do with anything? > > /me stares more > > OMG, hell no. First off, this code is a nightmare to follow. The existing comment > is useless. No shit the code is adding in supervisor states for the host. What's > not AT ALL clear is *why*. > > The commit says it's necessary because the "permission bitmap is only relevant > for user states": > > commit 781c64bfcb735960717d1cb45428047ff6a5030c > Author: Thomas Gleixner <tglx@linutronix.de> > Date: Thu Mar 24 14:47:14 2022 +0100 > > x86/fpu/xstate: Handle supervisor states in XSTATE permissions > > The size calculation in __xstate_request_perm() fails to take supervisor > states into account because the permission bitmap is only relevant for user > states. > > But @permitted comes from: > > permitted = xstate_get_group_perm(guest); > > which is either fpu->guest_perm.__state_perm or fpu->perm.__state_perm. And > __state_perm is initialized to: > > fpu->perm.__state_perm = fpu_kernel_cfg.default_features; > > where fpu_kernel_cfg.default_features contains everything except the dynamic > xfeatures, i.e. everything except XFEATURE_MASK_XTILE_DATA: > > fpu_kernel_cfg.default_features = fpu_kernel_cfg.max_features; > fpu_kernel_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC; > > So why on earth does this code to force back xfeatures_mask_supervisor()? Because > the code just below drops the supervisor bits to compute the user xstate size and > then clobbers __state_perm. > > /* Calculate the resulting user state size */ > mask &= XFEATURE_MASK_USER_SUPPORTED; > usize = xstate_calculate_size(mask, false); > > ... > > WRITE_ONCE(perm->__state_perm, mask); > > That is beyond asinine. IIUC, the intent is to apply the permission bitmap only > for user states, because the only dynamic states are user states. Bbut the above > creates an inconsistent mess. If userspace doesn't request XTILE_DATA, > __state_perm will contain supervisor states, but once userspace does request > XTILE_DATA, __state_perm will be lost.
Exactly, thanks for calling it out!
> And because that's not confusing enough, clobbering __state_perm would also drop > FPU_GUEST_PERM_LOCKED, except that __xstate_request_perm() can' be reached with > said LOCKED flag set. > > fpu_xstate_prctl() already strips out supervisor features: > > case ARCH_GET_XCOMP_PERM: > /* > * Lockless snapshot as it can also change right after the > * dropping the lock. > */ > permitted = xstate_get_host_group_perm(); > permitted &= XFEATURE_MASK_USER_SUPPORTED; > return put_user(permitted, uptr); > > case ARCH_GET_XCOMP_GUEST_PERM: > permitted = xstate_get_guest_group_perm(); > permitted &= XFEATURE_MASK_USER_SUPPORTED; > return put_user(permitted, uptr); > > and while KVM doesn't apply the __state_perm to supervisor states, if it did > there would be zero harm in doing so. > > case 0xd: { > u64 permitted_xcr0 = kvm_get_filtered_xcr0(); > u64 permitted_xss = kvm_caps.supported_xss; > > Second, the relying on QEMU to only trigger __xstate_request_perm() is not acceptable. > It "works" for the current code, but only because there's only a single dynamic > feature, i.e. this will short circuit and prevent computing a bad ksize. > > /* Check whether fully enabled */ > if ((permitted & requested) == requested) > return 0; > > I don't know how I can possibly make it any clearer: KVM absolutely must not assume > userspace behavior. > > So rather than continue with the current madness, which will break if/when the > next dynamic feature comes along, just preserve non-user xfeatures/flags in > __guest_perm.
Yes, it's time to rectify the confusion and make permission based settings clearer. Below patch looks good to me, thanks!
> If there are no objections, I'll test the below and write a proper changelog. > > -- > From: Sean Christopherson <seanjc@google.com> > Date: Thu, 26 Oct 2023 10:17:33 -0700 > Subject: [PATCH] x86/fpu/xstate: Always preserve non-user xfeatures/flags in > __state_perm > > Fixes: 781c64bfcb73 ("x86/fpu/xstate: Handle supervisor states in XSTATE permissions") > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kernel/fpu/xstate.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > index ef6906107c54..73f6bc00d178 100644 > --- a/arch/x86/kernel/fpu/xstate.c > +++ b/arch/x86/kernel/fpu/xstate.c > @@ -1601,16 +1601,20 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest) > if ((permitted & requested) == requested) > return 0; > > - /* Calculate the resulting kernel state size */ > + /* > + * Calculate the resulting kernel state size. Note, @permitted also > + * contains supervisor xfeatures even though supervisor are always > + * permitted for kernel and guest FPUs, and never permitted for user > + * FPUs. > + */ > mask = permitted | requested; > - /* Take supervisor states into account on the host */ > - if (!guest) > - mask |= xfeatures_mask_supervisor(); > ksize = xstate_calculate_size(mask, compacted); > > - /* Calculate the resulting user state size */ > - mask &= XFEATURE_MASK_USER_SUPPORTED; > - usize = xstate_calculate_size(mask, false); > + /* > + * Calculate the resulting user state size. Take care not to clobber > + * the supervisor xfeatures in the new mask! > + */ > + usize = xstate_calculate_size(mask & XFEATURE_MASK_USER_SUPPORTED, false); > > if (!guest) { > ret = validate_sigaltstack(usize); > > base-commit: c076acf10c78c0d7e1aa50670e9cc4c91e8d59b4
| |