module_refcounting and anonymous inodes

PATCH Applied Christian Borntraeger 3 participants 12 messages Dec 2, 2008 – Dec 10, 2008

Subsystems (2)

VFS KVM

lore

Share & tools

lore

Christian Borntraeger

author

2008-12-02 10:15 UTC #0 lore raw

cover module_refcounting and anonymous inodes

Hello Avi,

here is the latest respin of my fixes for the kvm module unload problem:

[PATCH 1/2] anon_inodes: use fops->owner for module refcount [PATCH 2/2] kvm: set owner of cpu and vm file operations

Both patches fix module reference counting problems and only matter for module unload - nothing critical.

Tested on s390 and x86_32.

Christian

Christian Borntraeger

author

2008-12-02 10:17 UTC #1 lore raw

PATCH 1/2 anon_inodes: use fops->owner for module refcount

There is an imbalance for anonymous inodes. If the fops->owner field is set, the module reference count of owner is decreases on release. ("filp_close" --> "__fput" ---> "fops_put")

On the other hand, anon_inode_getfd does not increase the module reference count of owner. This causes two problems:

- if owner is set, the module refcount goes negative
- if owner is not set, the module can be unloaded while code is running

This patch changes anon_inode_getfd to be symmetric regarding fops->owner handling.

I have checked all existing users of anon_inode_getfd. Noone sets fops->owner, thats why nobody has seen the module refcount negative. The refcounting was tested with a patched and unpatched KVM module.(see patch 2/2) I also did an epoll_open/close test.

Signed-off-by Christian Borntraeger <[email protected]>

Reviewed-by Davide Libenzi <[email protected]>

---
 fs/anon_inodes.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

fs/anon_inodes.c +6 -1

Index: kvm/fs/anon_inodes.c
===================================================================
@@ -79,9 +79,12 @@ 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;
+		goto err_module;
 	fd = error;
 
 	/*

@@ -128,6 +131,8 @@ err_dput:
 	dput(dentry);
 err_put_unused_fd:
 	put_unused_fd(fd);
+err_module:
+	module_put(fops->owner);
 	return error;
 }
 EXPORT_SYMBOL_GPL(anon_inode_getfd);

Christian Borntraeger

author

2008-12-02 10:17 UTC #2 lore raw

PATCH 2/2 kvm: set owner of cpu and vm file operations

In reply to Christian Borntraeger

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:

Quoted 7 lines

virt/kvm/kvm_main.c +1 -0

@@ -648,10 +648,14 @@ void kvm_free_physmem(struct kvm *kvm)
               kvm_free_physmem_slot(&kvm->memslots[i], NULL);
 }
 
+#include <linux/delay.h>

virt/kvm/kvm_main.c void kvm_free_physmem(struct kvm *kvm) · line 661

Quoted 10 lines

virt/kvm/kvm_main.c +3 -0

 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 killing the userspace, followed by an rmmod.

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.

This patch requires the fix for anon_inodes (anon_inodes: use fops->owner for module refcount).
With this patch, we can set the owner of all anonymous KVM inodes file operations. The VFS will then control the KVM module refcount as long as there is an open file. kvm_destroy_vm will be called by the release function of the last closed file - before the VFS drops the module refcount.

Signed-off-by Christian Borntraeger <[email protected]>

---
 virt/kvm/kvm_main.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

virt/kvm/kvm_main.c +4 -2

Index: 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,

@@ -1697,7 +1697,7 @@ static int kvm_vm_mmap(struct file *file
 	return 0;
 }
 
-static const struct file_operations kvm_vm_fops = {
+static struct file_operations kvm_vm_fops = {
 	.release        = kvm_vm_release,
 	.unlocked_ioctl = kvm_vm_ioctl,
 	.compat_ioctl   = kvm_vm_ioctl,

@@ -2061,6 +2061,8 @@ int kvm_init(void *opaque, unsigned int 
 	}
 
 	kvm_chardev_ops.owner = module;
+	kvm_vm_fops.owner = module;
+	kvm_vcpu_fops.owner = module;
 
 	r = misc_register(&kvm_dev);
 	if (r) {

Christian Borntraeger

author

2008-12-08 11:51 UTC #3 lore raw

In reply to Christian Borntraeger

Am Dienstag, 2. Dezember 2008 schrieb Christian Borntraeger:

Quoted 6 lines

Hello Avi,
here is the latest respin of my fixes for the kvm module unload problem:
[PATCH 1/2] anon_inodes: use fops->owner for module refcount [PATCH 2/2] kvm: set owner of cpu and vm file operations

In the meantime patch 2 has a offset against the latest git. Should I resend the patch or will you apply it anyway?

Christian

Avi Kivity

Applied

2008-12-08 11:57 UTC #4 lore raw

Quoted 4 lines · Christian Borntraeger

Am Dienstag, 2. Dezember 2008 schrieb Christian Borntraeger:
Quoted 2 lines
Hello Avi,

Quoted 9 lines

Quoted 5 lines
here is the latest respin of my fixes for the kvm module unload problem:
[PATCH 1/2] anon_inodes: use fops->owner for module refcount [PATCH 2/2] kvm: set owner of cpu and vm file operations
In the meantime patch 2 has a offset against the latest git. Should I resend the patch or will you apply it anyway?

Applied the patches; thanks for the reminder.

Signature · 1 line

error compiling committee.c: too many arguments to function

Sheng Yang

2008-12-09 09:47 UTC #5 lore raw

Quoted 6 lines · On Monday 08 December 2008 19:57:14 Avi Kivity wrote:

Quoted 5 lines · Christian Borntraeger
Am Dienstag, 2. Dezember 2008 schrieb Christian Borntraeger:
Quoted 4 lines
Hello Avi,
here is the latest respin of my fixes for the kvm module unload problem:

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
[PATCH 1/2] anon_inodes: use fops->owner for module refcount [PATCH 2/2] kvm: set owner of cpu and vm file operations
In the meantime patch 2 has a offset against the latest git. Should I resend the patch or will you apply it anyway?
Applied the patches; thanks for the reminder.

Should we push the first patch to 2.6.28? I got some trouble with the separate 2nd patch, for I am using Linus' tree and make KVM as modules, so the reference count reduced to negative now... (Oh Avi, I know you suggest to use in kernel rather than modules, but module is indeed convenient. :) )

Signature · 2 lines

regards
Yang, Sheng

Avi Kivity

2008-12-09 09:52 UTC #6 lore raw

Quoted 1 line · Sheng Yang

Should we push the first patch to 2.6.28?

It's not a recent regression, so no.

Quoted 2 lines

I got some trouble with the separate 2nd patch, for I am using Linus' tree and make KVM as modules, so the reference count reduced to negative now... (Oh Avi, I know you suggest to use in kernel rather than modules, but module is indeed convenient. :) )

Right, that would affect everyone. What we need is to hack the second patch for external modules on <2.6.29.

Signature · 1 line

error compiling committee.c: too many arguments to function

Christian Borntraeger

author

2008-12-09 10:08 UTC #7 lore raw

Am Dienstag, 9. Dezember 2008 schrieb Avi Kivity:

Quoted 8 lines

Quoted 1 line · Sheng Yang
Should we push the first patch to 2.6.28?
It's not a recent regression, so no.
Quoted 3 lines
I got some trouble with the separate
2nd patch, for I am using Linus' tree and make KVM as modules, so the reference count reduced to negative now... (Oh Avi, I know you suggest to

use

Quoted 5 lines

Quoted 2 lines
in kernel rather than modules, but module is indeed convenient. :) )
Right, that would affect everyone. What we need is to hack the second patch for external modules on <2.6.29.

Oh this is tricky. Both patches belong together, patch 2 depends on patch 1. For base kernels which do not contain patch1, this additional (untested) patch would probably help:

---
 virt/kvm/kvm_main.c |   21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

virt/kvm/kvm_main.c +17 -4

Index: kvm/virt/kvm/kvm_main.c
===================================================================
@@ -1501,9 +1501,15 @@ static struct file_operations kvm_vcpu_f
  */
 static int create_vcpu_fd(struct kvm_vcpu *vcpu)
 {
-	int fd = anon_inode_getfd("kvm-vcpu", &kvm_vcpu_fops, vcpu, 0);
-	if (fd < 0)
+	int fd;
+
+	if (!try_module_get(kvm_vcpu_fops.owner))
+		return -ENOENT;
+	fd = anon_inode_getfd("kvm-vcpu", &kvm_vcpu_fops, vcpu, 0);
+	if (fd < 0) {
 		kvm_put_kvm(vcpu->kvm);
+		module_put(kvm_vcpu_fops.owner);
+	}
 	return fd;
 }

@@ -1895,12 +1901,19 @@ static int kvm_dev_ioctl_create_vm(void)
 	int fd;
 	struct kvm *kvm;
 
+	if (!try_module_get(kvm_vm_fops.owner))
+		return -ENOENT;
+
 	kvm = kvm_create_vm();
-	if (IS_ERR(kvm))
+	if (IS_ERR(kvm)) {
+		module_put(kvm_vm_fops.owner);
 		return PTR_ERR(kvm);
+	}
 	fd = anon_inode_getfd("kvm-vm", &kvm_vm_fops, kvm, 0);
-	if (fd < 0)
+	if (fd < 0) {
 		kvm_put_kvm(kvm);
+		module_put(kvm_vm_fops.owner);
+	}
 
 	return fd;
 }

The problem is, how do you detect if the base kernel has patch1 applied?

Christian

Avi Kivity

2008-12-09 10:16 UTC #8 lore raw

Quoted 2 lines · Christian Borntraeger

The problem is, how do you detect if the base kernel has patch1 applied?

In the external module compatibility kit, implement two versions of anon_inode_getfd(), and select the appropriate one according to kernel version (like we currently support 91 smp_call_function_single variants).

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2008-12-09 13:22 UTC #9 lore raw

Quoted 6 lines · Avi Kivity

Quoted 2 lines · Christian Borntraeger
The problem is, how do you detect if the base kernel has patch1 applied?
In the external module compatibility kit, implement two versions of anon_inode_getfd(), and select the appropriate one according to kernel version (like we currently support 91 smp_call_function_single variants).

I committed something simpler: if running on 2.6.28 or earlier, I hack out the two lines your second patch adds.\

Signature · 1 line

error compiling committee.c: too many arguments to function

Sheng Yang

2008-12-10 02:09 UTC #10 lore raw

Quoted 10 lines · On Tuesday 09 December 2008 21:22:42 Avi Kivity wrote:

Quoted 6 lines · Avi Kivity
Quoted 1 line · Christian Borntraeger
The problem is, how do you detect if the base kernel has patch1 applied?
In the external module compatibility kit, implement two versions of anon_inode_getfd(), and select the appropriate one according to kernel version (like we currently support 91 smp_call_function_single variants).
I committed something simpler: if running on 2.6.28 or earlier, I hack out the two lines your second patch adds.\

Good hack!

kernel/external-module-compat-comm.h static inline void cpumask_clear_cpu(int cpu,

Quoted 10 lines

(quoted diff) +0 -0
b/kernel/external-module-compat-comm.h index a089f62..d90522d 100644
kernel/external-module-compat-comm.h +1 -0
@@ -682,3 +682,13 @@ static inline void cpumask_clear_cpu(int cpu,
 cpumask_var_t mask)
 
 #endif
 
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,28)

But should it be 2,6,29?...

Signature · 2 lines

regards
Yang, Sheng

Quoted 9 lines

kernel/external-module-compat-comm.h +9 -0

+
+#define IF_ANON_INODES_DOES_REFCOUNTS(x)
+
+#else
+
+#define IF_ANON_INODES_DOES_REFCOUNTS(x) x
+
+#endif
+

Avi Kivity

2008-12-10 09:13 UTC #11 lore raw

Quoted 6 lines · Sheng Yang

Quoted 2 lines
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,28)
But should it be 2,6,29?...

Thanks, fixed.

Signature · 1 line

error compiling committee.c: too many arguments to function