lkml.org 
[lkml]   [2020]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: general protection fault in start_creating
From
Date
On 05/06/20 15:38, Vitaly Kuznetsov wrote:
>
> I think what happens here is one thread does kvm_vm_ioctl_create_vcpu()
> and another tries to delete the VM. The problem was probably present
> even before the commit as both kvm_create_vcpu_debugfs() and
> kvm_destroy_vm_debugfs() happen outside of kvm_lock but maybe it was
> harder to trigger.

I think it wasn't, because:

- you cannot reach kvm_vcpu_release before you have created the file
descriptor, and the patch moved debugfs creation after creation of
the file descriptor

- on the other hand you cannot reach kvm_vm_release during KVM_CREATE_VCPU,
so you cannot reach kvm_destroy_vm either (because the VM file descriptor
holds a reference). So the bug can only occur for things that are touched
by kvm_vcpu_release, and that is only the debugfs dentry.

I have a patch for this already, which is simply removing the
debugfs_remove_recursive call in kvm_vcpu_release, but I have forgotten to
send it. What you have below could work but i don't see a good reason to
put things under kvm->lock.

Paolo

> I think what happens here is one thread does kvm_vm_ioctl_create_vcpu()
> and another tries to delete the VM. The problem was probably present
> even before the commit as both kvm_create_vcpu_debugfs() and
> kvm_destroy_vm_debugfs() happen outside of kvm_lock but maybe it was
> harder to trigger. Is there a reason to not put all this under kvm_lock?
> I.e. the following:
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 7fa1e38e1659..d53784cb920f 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -793,9 +793,9 @@ static void kvm_destroy_vm(struct kvm *kvm)
> struct mm_struct *mm = kvm->mm;
>
> kvm_uevent_notify_change(KVM_EVENT_DESTROY_VM, kvm);
> - kvm_destroy_vm_debugfs(kvm);
> kvm_arch_sync_events(kvm);
> mutex_lock(&kvm_lock);
> + kvm_destroy_vm_debugfs(kvm);
> list_del(&kvm->vm_list);
> mutex_unlock(&kvm_lock);
> kvm_arch_pre_destroy_vm(kvm);
> @@ -3084,9 +3084,9 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
> smp_wmb();
> atomic_inc(&kvm->online_vcpus);
>
> + kvm_create_vcpu_debugfs(vcpu);
> mutex_unlock(&kvm->lock);
> kvm_arch_vcpu_postcreate(vcpu);
> - kvm_create_vcpu_debugfs(vcpu);
> return r;
>
> unlock_vcpu_destroy:
>
> should probably do. The reproducer doesn't work for me (or just takes
> too long so I gave up), unfortunately. Or I may have misunderstood
> everything

\
 
 \ /
  Last update: 2020-06-05 15:53    [W:0.091 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site