mbox series

[v4,0/2] x86/kvm/fpu: Fix guest migration bugs that can crash guest

Message ID 20220217053028.96432-1-leobras@redhat.com (mailing list archive)
Headers show
Series x86/kvm/fpu: Fix guest migration bugs that can crash guest | expand

Message

Leonardo Bras Feb. 17, 2022, 5:30 a.m. UTC
This patchset comes from a bug I found during qemu guest migration from a
host with newer CPU to a host with an older version of this CPU, and thus 
having less FPU features.

When the guests were created, the one with less features is used as 
config, so migration is possible.

Patch 1 fix a bug that always happens during this migration, and is
related to the fact that xsave saves all feature flags, but xrstor does
not touch the PKRU flag. It also changes how fpstate->user_xfeatures
is set, going from kvm_check_cpuid() to the later called
kvm_vcpu_after_set_cpuid().

Patch 2 removes kvm_vcpu_arch.guest_supported_xcr0 since it now 
duplicates guest_fpu.fpstate->user_xfeatures. Some wrappers were
introduced in order to make it easier to read the replaced version.

Patches were compile-tested, and could fix the bug found.

Please let me know of anything to improve!

Best regards,
Leo

--
Changes since v3:
- Add new patch to remove the use of kvm_vcpu_arch.guest_supported_xcr0,
  since it is now duplicating guest_fpu.fpstate->user_xfeatures.
- On patch 1, also avoid setting user_xfeatures on kvm_check_cpuid(),
  since it is already set in kvm_vcpu_after_set_cpuid() now.
Changes since v2:
- Fix building error because I forgot to EXPORT_SYMBOL(fpu_user_cfg)
Changes since v1:
- Instead of masking xfeatures, mask user_xfeatures instead. This will
  only change the value sent to user, instead of the one saved in buf.
- Above change removed the need of the patch 2/2
- Instead of masking the current value of user_xfeatures, save on it
  fpu_user_cfg.default_features & vcpu->arch.guest_supported_xcr0 

Leonardo Bras (2):
  x86/kvm/fpu: Mask guest fpstate->xfeatures with guest_supported_xcr0
  x86/kvm/fpu: Remove kvm_vcpu_arch.guest_supported_xcr0

 arch/x86/include/asm/kvm_host.h |  1 -
 arch/x86/kernel/fpu/xstate.c    |  5 ++++-
 arch/x86/kvm/cpuid.c            |  5 ++++-
 arch/x86/kvm/x86.c              | 20 +++++++++++++++-----
 4 files changed, 23 insertions(+), 8 deletions(-)

Comments

Paolo Bonzini Feb. 17, 2022, 2:52 p.m. UTC | #1
On 2/17/22 06:30, Leonardo Bras wrote:
> This patchset comes from a bug I found during qemu guest migration from a
> host with newer CPU to a host with an older version of this CPU, and thus
> having less FPU features.
> 
> When the guests were created, the one with less features is used as
> config, so migration is possible.
> 
> Patch 1 fix a bug that always happens during this migration, and is
> related to the fact that xsave saves all feature flags, but xrstor does
> not touch the PKRU flag. It also changes how fpstate->user_xfeatures
> is set, going from kvm_check_cpuid() to the later called
> kvm_vcpu_after_set_cpuid().
> 
> Patch 2 removes kvm_vcpu_arch.guest_supported_xcr0 since it now
> duplicates guest_fpu.fpstate->user_xfeatures. Some wrappers were
> introduced in order to make it easier to read the replaced version.
> 
> Patches were compile-tested, and could fix the bug found.

Queued, thanks (for 5.17 of course)!  For patch 2, I renamed the 
function to kvm_guest_supported_xcr0.

Paolo

> Please let me know of anything to improve!
> 
> Best regards,
> Leo
> 
> --
> Changes since v3:
> - Add new patch to remove the use of kvm_vcpu_arch.guest_supported_xcr0,
>    since it is now duplicating guest_fpu.fpstate->user_xfeatures.
> - On patch 1, also avoid setting user_xfeatures on kvm_check_cpuid(),
>    since it is already set in kvm_vcpu_after_set_cpuid() now.
> Changes since v2:
> - Fix building error because I forgot to EXPORT_SYMBOL(fpu_user_cfg)
> Changes since v1:
> - Instead of masking xfeatures, mask user_xfeatures instead. This will
>    only change the value sent to user, instead of the one saved in buf.
> - Above change removed the need of the patch 2/2
> - Instead of masking the current value of user_xfeatures, save on it
>    fpu_user_cfg.default_features & vcpu->arch.guest_supported_xcr0
> 
> Leonardo Bras (2):
>    x86/kvm/fpu: Mask guest fpstate->xfeatures with guest_supported_xcr0
>    x86/kvm/fpu: Remove kvm_vcpu_arch.guest_supported_xcr0
> 
>   arch/x86/include/asm/kvm_host.h |  1 -
>   arch/x86/kernel/fpu/xstate.c    |  5 ++++-
>   arch/x86/kvm/cpuid.c            |  5 ++++-
>   arch/x86/kvm/x86.c              | 20 +++++++++++++++-----
>   4 files changed, 23 insertions(+), 8 deletions(-)
>
Leonardo Bras Feb. 17, 2022, 6:08 p.m. UTC | #2
On Thu, Feb 17, 2022 at 11:52 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 2/17/22 06:30, Leonardo Bras wrote:
> > This patchset comes from a bug I found during qemu guest migration from a
> > host with newer CPU to a host with an older version of this CPU, and thus
> > having less FPU features.
> >
> > When the guests were created, the one with less features is used as
> > config, so migration is possible.
> >
> > Patch 1 fix a bug that always happens during this migration, and is
> > related to the fact that xsave saves all feature flags, but xrstor does
> > not touch the PKRU flag. It also changes how fpstate->user_xfeatures
> > is set, going from kvm_check_cpuid() to the later called
> > kvm_vcpu_after_set_cpuid().
> >
> > Patch 2 removes kvm_vcpu_arch.guest_supported_xcr0 since it now
> > duplicates guest_fpu.fpstate->user_xfeatures. Some wrappers were
> > introduced in order to make it easier to read the replaced version.
> >
> > Patches were compile-tested, and could fix the bug found.
>
> Queued, thanks (for 5.17 of course)!  For patch 2, I renamed the
> function to kvm_guest_supported_xcr0.
>
> Paolo
>

That's great!
Thanks Paolo!