Re: [PATCH 0/3] KVM: VMX: Fix for kexec VMCLEAR and VMXON cleanup

[Paolo Bonzini]

On 27/02/20 23:30, Sean Christopherson wrote:
> Patch 1 fixes a a theoretical bug where a crashdump NMI that arrives
> while KVM is messing with the percpu VMCS list would result in one or more
> VMCSes not being cleared, potentially causing memory corruption in the new
> kexec'd kernel.
> 
> Patch 2 isn't directly related, but it conflicts with the crash cleanup
> changes, both from a code and a semantics perspective.  Without the crash
> cleanup, IMO hardware_enable() should do crash_disable_local_vmclear()
> if VMXON fails, i.e. clean up after itself.  But hardware_disable()
> doesn't even do crash_disable_local_vmclear() (which is what got me
> looking at that code in the first place).  Basing the VMXON change on top
> of the crash cleanup avoids the debate entirely.
> 
> Patch 3 is a simple clean up (no functional change intended ;-) ).
> 
> I verified my analysis of the NMI bug by simulating what would happen if
> an NMI arrived in the middle of list_add() and list_del().  The below
> output matches expectations, e.g. nothing hangs, the entry being added
> doesn't show up, and the entry being deleted _does_ show up.
> 
> [    8.205898] KVM: testing NMI in list_add()
> [    8.205898] KVM: testing NMI in list_del()
> [    8.205899] KVM: found e3
> [    8.205899] KVM: found e2
> [    8.205899] KVM: found e1
> [    8.205900] KVM: found e3
> [    8.205900] KVM: found e1
> 
> static void vmx_test_list(struct list_head *list, struct list_head *e1,
> 			  struct list_head *e2, struct list_head *e3)
> {
> 	struct list_head *tmp;
> 
> 	list_for_each(tmp, list) {
> 		if (tmp == e1)
> 			pr_warn("KVM: found e1\n");
> 		else if (tmp == e2)
> 			pr_warn("KVM: found e2\n");
> 		else if (tmp == e3)
> 			pr_warn("KVM: found e3\n");
> 		else
> 			pr_warn("KVM: kaboom\n");
> 	}
> }
> 
> static int __init vmx_init(void)
> {
> 	LIST_HEAD(list);
> 	LIST_HEAD(e1);
> 	LIST_HEAD(e2);
> 	LIST_HEAD(e3);
> 
> 	pr_warn("KVM: testing NMI in list_add()\n");
> 
> 	list.next->prev = &e1;
> 	vmx_test_list(&list, &e1, &e2, &e3);
> 
> 	e1.next = list.next;
> 	vmx_test_list(&list, &e1, &e2, &e3);
> 
> 	e1.prev = &list;
> 	vmx_test_list(&list, &e1, &e2, &e3);
> 
> 	INIT_LIST_HEAD(&list);
> 	INIT_LIST_HEAD(&e1);
> 
> 	list_add(&e1, &list);
> 	list_add(&e2, &list);
> 	list_add(&e3, &list);
> 
> 	pr_warn("KVM: testing NMI in list_del()\n");
> 
> 	e3.prev = &e1;
> 	vmx_test_list(&list, &e1, &e2, &e3);
> 
> 	list_del(&e2);
> 	list.prev = &e1;
> 	vmx_test_list(&list, &e1, &e2, &e3);
> }
> 
> Sean Christopherson (3):
>   KVM: VMX: Always VMCLEAR in-use VMCSes during crash with kexec support
>   KVM: VMX: Gracefully handle faults on VMXON
>   KVM: VMX: Make loaded_vmcs_init() a static function
> 
>  arch/x86/kvm/vmx/vmx.c | 101 +++++++++++++++++------------------------
>  arch/x86/kvm/vmx/vmx.h |   1 -
>  2 files changed, 41 insertions(+), 61 deletions(-)
> 

Queued, thanks (but still awaiting "counter-review" on the patch 1
suggestions).

Paolo