Re: [BUG] cdev_put() race condition

[Hans Verkuil <hverkuil@xs4all.nl>]

On Wednesday 17 December 2008 19:16:45 Greg KH wrote:
> On Wed, Dec 17, 2008 at 02:37:33PM +0100, Hans Verkuil wrote:
> > > Again, don't use cdev's reference counting for your own object
> > > lifecycle, it is different and will cause problems, like you have
> > > found out.
> >
> > Sigh. It has nothing to do with how v4l uses it. And to demonstrate
> > this, here is how you reproduce it with the sg module (tested it with
> > my USB harddisk).
> >
> > 1) apply this patch to char_dev.c:
>
> <snip>
>
> Ok, since I can't convince you that using a cdev for your reference
> counting is incorrect, I'll have to go change the cdev code to prevent
> you from doing this :(

Erm, you haven't told me yet why it's a bad idea. You just said "don't do 
it", but I haven't seen the reason for it. There doesn't seem to be any 
documentation on how to properly use cdev besides the Kernel Device Drivers 
book, which (if memory serves) doesn't mention anything on this topic.

I really don't mind implementing refcounting in the v4l framework, I just 
want to understand why it should be done like that!

It seems to me that I will just be shadowing the refcounting of cdev if I 
implement refcounting in v4l: init to 1 on creation, increase on open, 
decrease on close, decrease on deletion. It's all terribly familiar...

> Anyway, do you have a patch for the cdev code to propose how to fix this
> issue you are having?

Sure, here it is:

--- fs/char_dev.c.orig	2008-12-17 20:28:40.000000000 +0100
+++ fs/char_dev.c	2008-12-17 20:28:49.000000000 +0100
@@ -345,7 +345,9 @@
 {
 	if (p) {
 		struct module *owner = p->owner;
+		spin_lock(&cdev_lock);
 		kobject_put(&p->kobj);
+		spin_unlock(&cdev_lock);
 		module_put(owner);
 	}
 }
@@ -415,14 +417,12 @@
 
 static void cdev_purge(struct cdev *cdev)
 {
-	spin_lock(&cdev_lock);
 	while (!list_empty(&cdev->list)) {
 		struct inode *inode;
 		inode = container_of(cdev->list.next, struct inode, i_devices);
 		list_del_init(&inode->i_devices);
 		inode->i_cdev = NULL;
 	}
-	spin_unlock(&cdev_lock);
 }
 
 /*
@@ -478,7 +478,9 @@
 void cdev_del(struct cdev *p)
 {
 	cdev_unmap(p->dev, p->count);
+	spin_lock(&cdev_lock);
 	kobject_put(&p->kobj);
+	spin_unlock(&cdev_lock);
 }

This solves this particular problem. But this will certainly break v4l as it 
is right now, since the spin_lock means that the kref's release cannot do 
any sleeps, which is possible in v4l. If we want to allow that in cdev, 
then the spinlock has to be replaced by a mutex. But I have the strong 
feeling that that's not going to happen :-)

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG