> Messing with module counts is slightly ugly.
I changed my patch to be more generic. Instead of messing with the
module refcount in the kvm module, I decided to change
anon_inode_getfd instead. If the owner field is set, we will
try an module_get in anon_inode_getfd, since the VFS release
function will do a module_put. This makes the open/release
symmetric. What do you think?
I also CCed fs-devel, since fs/* is affected
From: Christian Borntraeger <borntraeger@xxxxxxxxxx>
There is a race between a "close of the file descriptors" and module
unload in the kvm module.
You can easily trigger this problem by applying this debug patch:
>--- kvm.orig/virt/kvm/kvm_main.c
>+++ kvm/virt/kvm/kvm_main.c
>@@ -648,10 +648,14 @@ void kvm_free_physmem(struct kvm *kvm)
> kvm_free_physmem_slot(&kvm->memslots[i], NULL);
> }
>
>+#include <linux/delay.h>
> static void kvm_destroy_vm(struct kvm *kvm)
> {
> struct mm_struct *mm = kvm->mm;
>
>+ printk("off1\n");
>+ msleep(5000);
>+ printk("off2\n");
> spin_lock(&kvm_lock);
> list_del(&kvm->vm_list);
> spin_unlock(&kvm_lock);
and changing userspace to closing the vcpu file descriptors after
the vm file descriptors. (killall sometimes also seems to work)
kvm_destroy_vm is called, after all kvm file descriptors have entered
the release function and kvm_put_kvm was called.
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 seems to be a change to anon_inode_getfd. The
VFS release function will drop the module ref count if fops->owner
is set. Therefore, it makes a lot of sense, that anon_inode_getfd
increases the refcount.
I have check all existing users of anon_inode_getfs. None of them,
sets the owner field. anon inodes should continue to work for them
as expected.
In addition, the kvm module must set the owner field accordingly.
Signed-off-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>
---
fs/anon_inodes.c | 3 +++
virt/kvm/kvm_main.c | 3 ++-
2 files changed, 5 insertions(+), 1 deletion(-)
Index: kvm/fs/anon_inodes.c
===================================================================
--- kvm.orig/fs/anon_inodes.c
+++ kvm/fs/anon_inodes.c
@@ -79,6 +79,9 @@ int anon_inode_getfd(const char *name, c
if (IS_ERR(anon_inode_inode))
return -ENODEV;
+ if (fops->owner && !try_module_get(fops->owner))
+ return -ENOENT;
+
error = get_unused_fd_flags(flags);
if (error < 0)
return error;
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,
@@ -2061,6 +2061,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) {
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
