From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH/RFC] kvm: fix refcounting race release vs. module unload Date: Sun, 23 Nov 2008 15:49:33 +0200 Message-ID: <49295F6D.8010200@redhat.com> References: <200811202010.00125.borntraeger@de.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org To: Christian Borntraeger Return-path: Received: from mx2.redhat.com ([66.187.237.31]:59864 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758008AbYKWNtj (ORCPT ); Sun, 23 Nov 2008 08:49:39 -0500 In-Reply-To: <200811202010.00125.borntraeger@de.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: Christian Borntraeger wrote: > The problem is that kvm_destroy_vm can run while the module count > is 0. That means, you can remove the module while kvm_destroy_vm > is running. But kvm_destroy_vm is part of the module text. This > causes a kerneloops. The race exists without the msleep but is much > harder to trigger. > > Nevertheless, the right solution is to call kvm_destroy_vm only > with module_ref_count > 0. This can be done by calling kvm_destroy_vm > only via a release function, since the VFS will not allow module unload. > This patch sets kvm_vcpu_fops.owner to the module and manually > increases the module refcount after anon_inode_getfd, since > anon_inode_getfd does not do it. > > Signed-off-by: Christian Borntraeger > --- > virt/kvm/kvm_main.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > Index: kvm/virt/kvm/kvm_main.c > =================================================================== > --- kvm.orig/virt/kvm/kvm_main.c > +++ kvm/virt/kvm/kvm_main.c > @@ -1303,7 +1303,7 @@ static int kvm_vcpu_release(struct inode > return 0; > } > > -static const struct file_operations kvm_vcpu_fops = { > +static struct file_operations kvm_vcpu_fops = { > .release = kvm_vcpu_release, > .unlocked_ioctl = kvm_vcpu_ioctl, > .compat_ioctl = kvm_vcpu_ioctl, > @@ -1318,6 +1318,7 @@ static int create_vcpu_fd(struct kvm_vcp > int fd = anon_inode_getfd("kvm-vcpu", &kvm_vcpu_fops, vcpu, 0); > if (fd < 0) > kvm_put_kvm(vcpu->kvm); > + __module_get(kvm_vcpu_fops.owner); > return fd; > } > > @@ -2061,6 +2062,7 @@ int kvm_init(void *opaque, unsigned int > } > > kvm_chardev_ops.owner = module; > + kvm_vcpu_fops.owner = module; > > r = misc_register(&kvm_dev); > if (r) { > Messing with module counts is slightly ugly. How about having a vm fd fget() the /dev/kvm fd() instead? -- error compiling committee.c: too many arguments to function