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
