[PATCH/RFC] kvm: fix refcounting race release vs. module unload

[PATCH/RFC] kvm: fix refcounting race release vs. module unload

[Christian Borntraeger]

Hello Avi,

while fixing a module reference counting problem for s390 I spotted 
another problem in the kvm main code. Any comments on the patch?

From: Christian Borntraeger <borntraeger@de.ibm.com>

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 qemu ; rmmod kvm_intel, rmmod kvm
also crashes my system)

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 <borntraeger@de.ibm.com>
---
 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) {

Re: [PATCH/RFC] kvm: fix refcounting race release vs. module unload

[Avi Kivity]

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 <borntraeger@de.ibm.com>
> ---
>  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

Re: [PATCH/RFC] kvm: fix refcounting race release vs. module unload

[Christian Borntraeger]

> > --- 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?

I personally find fget (and fput) slightly more ugly than handling the module 
reference count. Especially if the problem is module unloading...the module 
refcount looks so natural.

I am also a bit worried by fget/fput, since we would call fput in the release 
function - which is part of the module. Wouldnt that open another very small 
race?

In addition, we would need variables containing the fd and the file pointer 
for /dev/kvm, since fget/fput need some parameters, no? (Is there an easy way 
to get the fd from the struct file *filp? Searching current->files, seems to
be the only method I know)

To me, the fget approach looks more complicated and less safe.

Christian

Re: [PATCH/RFC 2] kvm: fix module refcount issues with anon_inodegetfd

[Christian Borntraeger]

> 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@de.ibm.com>

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@de.ibm.com>

---
 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) {

Re: [PATCH/RFC 2] kvm: fix module refcount issues with anon_inodegetfd

[Avi Kivity]

Christian Borntraeger wrote:
>> 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 agree with your analysis, and also that the anon_inodes change is 
useful.  If it's acceptable to the vfs/anon_inode maintainers, I'll 
apply the patch.

-- 
error compiling committee.c: too many arguments to function

[PATCH/Request for review]: check for fops->owner in anon_inode_getfd

[Christian Borntraeger]

Am Dienstag, 25. November 2008 schrieb Avi Kivity:
> I agree with your analysis, and also that the anon_inodes change is 
> useful.  If it's acceptable to the vfs/anon_inode maintainers, I'll 
> apply the patch.

I think it is a good idea to strip the fs specific changes into a separate
patch for easier review:

From: Christian Borntraeger <borntraeger@de.ibm.com>

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.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

--- 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;

Re: [PATCH/Request for review]: check for fops->owner in anon_inode_getfd

[Davide Libenzi]

On Thu, 27 Nov 2008, Christian Borntraeger wrote:

> Am Dienstag, 25. November 2008 schrieb Avi Kivity:
> > I agree with your analysis, and also that the anon_inodes change is 
> > useful.  If it's acceptable to the vfs/anon_inode maintainers, I'll 
> > apply the patch.
> 
> I think it is a good idea to strip the fs specific changes into a separate
> patch for easier review:
> 
> From: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> 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.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> --- 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;

What if get_unused_fd_flags() (or the following error-returing ops) fails 
after a successful try_module_get()?


- Davide

[PATCH v2]: check for fops->owner in anon_inode_getfd

[Christian Borntraeger]

> > +	if (fops->owner && !try_module_get(fops->owner))
> > +		return -ENOENT;
> > +
> >  	error = get_unused_fd_flags(flags);
> >  	if (error < 0)
> >  		return error;
> 
> What if get_unused_fd_flags() (or the following error-returing ops) fails 
> after a successful try_module_get()?

Right, well spotted. I have added a fixup label.

From: Christian Borntraeger <borntraeger@de.ibm.com>

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.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>


From: Christian Borntraeger <borntraeger@de.ibm.com>

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.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 fs/anon_inodes.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Index: kvm/fs/anon_inodes.c
===================================================================
--- kvm.orig/fs/anon_inodes.c
+++ 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);

Re: [PATCH v2]: check for fops->owner in anon_inode_getfd

[Davide Libenzi]

On Thu, 27 Nov 2008, Christian Borntraeger wrote:

> > > +	if (fops->owner && !try_module_get(fops->owner))
> > > +		return -ENOENT;
> > > +
> > >  	error = get_unused_fd_flags(flags);
> > >  	if (error < 0)
> > >  		return error;
> > 
> > What if get_unused_fd_flags() (or the following error-returing ops) fails 
> > after a successful try_module_get()?
> 
> Right, well spotted. I have added a fixup label.
> 
> From: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> 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.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> 
> From: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> 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.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  fs/anon_inodes.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> Index: kvm/fs/anon_inodes.c
> ===================================================================
> --- kvm.orig/fs/anon_inodes.c
> +++ 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);

Looks OK to me.



- Davide

Re: [PATCH v2]: check for fops->owner in anon_inode_getfd

[Christian Borntraeger]

Am Donnerstag, 27. November 2008 schrieb Davide Libenzi:
> > ===================================================================
> > --- kvm.orig/fs/anon_inodes.c
> > +++ 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);
> 
> Looks OK to me.


Ok. Thanks. I will push this to Avi. 
Can I add a
Reviewed-by: Davide Libenzi <davidel@xmailserver.org>
to the patch?

Christian