[PATCH] videodev: fix kobj ref count

[PATCH] videodev: fix kobj ref count

[David Ellingsworth]

Mauro,

If Laurent approves, please apply the following patch to the devel
branch. I've been using it locally for the past five days or so
without issue.

This patch increments the kobject reference count during video_open
and decrements it during video_close. Doing so allows
video_unregister_device to be called during the disconnect callback of
usb and pci devices. It also ensures that the video_device struct is
not freed while it is still in use and that the kobject release
callback occurs at the appropriate time. With this patch, the
following sequence is now possible and no longer results in a crash.

video_open
  disconnect
    video_unregister_device
      video_ioctl2 (crash was here)
        video_close
          video_release

Regards,

David Ellingsworth

Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
---
 drivers/media/video/videodev.c |   50 +++++++++++++++++++++++++++------------
 include/media/v4l2-dev.h       |    1 +
 2 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/drivers/media/video/videodev.c b/drivers/media/video/videodev.c
index 0d52819..9922cd6 100644
--- a/drivers/media/video/videodev.c
+++ b/drivers/media/video/videodev.c
@@ -406,17 +406,22 @@ void video_device_release(struct video_device *vfd)
 }
 EXPORT_SYMBOL(video_device_release);

+/*
+ *	Active devicesSigned-off-by: David Ellingsworth <david@identd.dyndns.org>
---
 drivers/media/video/videodev.c |   50 +++++++++++++++++++++++++++------------
 include/media/v4l2-dev.h       |    1 +
 2 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/drivers/media/video/videodev.c b/drivers/media/video/videodev.c
index 0d52819..9922cd6 100644
--- a/drivers/media/video/videodev.c
+++ b/drivers/media/video/videodev.c
@@ -406,17 +406,22 @@ void video_device_release(struct video_device *vfd)
 }
 EXPORT_SYMBOL(video_device_release);

+/*
+ *	Active devices
+ */
+
+static struct video_device *video_device[VIDEO_NUM_DEVICES];
+static DEFINE_MUTEX(videodev_lock);
+
+/* must be called with videodev_lock held */
 static void video_release(struct device *cd)
 {
 	struct video_device *vfd = container_of(cd, struct video_device,
 								class_dev);

-#if 1
-	/* needed until all drivers are fixed */
-	if (!vfd->release)
-		return;
-#endif
-	vfd->release(vfd);
+	if (vfd->release)
+		vfd->release(vfd);
+	video_device[vfd->minor] = NULL;
 }

 static struct device_attribute video_device_attrs[] = {
@@ -431,19 +436,30 @@ static struct class video_class = {
 	.dev_release = video_release,
 };

-/*
- *	Active devices
- */
-
-static struct video_device *video_device[VIDEO_NUM_DEVICES];
-static DEFINE_MUTEX(videodev_lock);
-
 struct video_device* video_devdata(struct file *file)
 {
 	return video_device[iminor(file->f_path.dentry->d_inode)];
 }
 EXPORT_SYMBOL(video_devdata);

+static int video_close(struct inode *inode, struct file *file)
+{
+	unsigned int minor = iminor(inode);
+	int err = 0;
+	struct video_device *vfl;
+
+	vfl = video_device[minor];
+
+	if (vfl->fops && vfl->fops->release)
+		err = vfl->fops->release(inode, file);
+
+	mutex_lock(&videodev_lock);
+	kobject_put(&vfl->class_dev.kobj);
+	mutex_unlock(&videodev_lock);
+
+	return err;
+}
+
 /*
  *	Open a video device - FIXME: Obsoleted
  */
@@ -469,10 +485,11 @@ static int video_open(struct inode *inode,
struct file *file)
 		}
 	}
 	old_fops = file->f_op;
-	file->f_op = fops_get(vfl->fops);
-	if(file->f_op->open)
+	file->f_op = fops_get(&vfl->priv_fops);
+	if (file->f_op->open && kobject_get(&vfl->class_dev.kobj))
 		err = file->f_op->open(inode,file);
 	if (err) {
+		kobject_put(&vfl->class_dev.kobj);
 		fops_put(file->f_op);
 		file->f_op = fops_get(old_fops);
 	}
@@ -2175,6 +2192,8 @@ int video_register_device_index(struct
video_device *vfd, int type, int nr,
 	}

 	vfd->index = ret;
+	vfd->priv_fops = *vfd->fops;
+	vfd->priv_fops.release = video_close;

 	mutex_unlock(&videodev_lock);
 	mutex_init(&vfd->lock);
@@ -2225,7 +2244,6 @@ void video_unregister_device(struct video_device *vfd)
 	if(video_device[vfd->minor]!=vfd)
 		panic("videodev: bad unregister");

-	video_device[vfd->minor]=NULL;
 	device_unregister(&vfd->class_dev);
 	mutex_unlock(&videodev_lock);
 }
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 3c93414..d4fe617 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -342,6 +342,7 @@ void *priv;
 	/* for videodev.c intenal usage -- please don't touch */
 	int users;                     /* video_exclusive_{open|close} ... */
 	struct mutex lock;             /* ... helper function uses these   */
+	struct file_operations priv_fops; /* video_close */
 };

 /* Class-dev to video-device */
-- 
1.5.5.1


+ */
+
+static struct video_device *video_device[VIDEO_NUM_DEVICES];
+static DEFINE_MUTEX(videodev_lock);
+
+/* must be called with videodev_lock held */
 static void video_release(struct device *cd)
 {
 	struct video_device *vfd = container_of(cd, struct video_device,
 								class_dev);

-#if 1
-	/* needed until all drivers are fixed */
-	if (!vfd->release)
-		return;
-#endif
-	vfd->release(vfd);
+	if (vfd->release)
+		vfd->release(vfd);
+	video_device[vfd->minor] = NULL;
 }

 static struct device_attribute video_device_attrs[] = {
@@ -431,19 +436,30 @@ static struct class video_class = {
 	.dev_release = video_release,
 };

-/*
- *	Active devices
- */
-
-static struct video_device *video_device[VIDEO_NUM_DEVICES];
-static DEFINE_MUTEX(videodev_lock);
-
 struct video_device* video_devdata(struct file *file)
 {
 	return video_device[iminor(file->f_path.dentry->d_inode)];
 }
 EXPORT_SYMBOL(video_devdata);

+static int video_close(struct inode *inode, struct file *file)
+{
+	unsigned int minor = iminor(inode);
+	int err = 0;
+	struct video_device *vfl;
+
+	vfl = video_device[minor];
+
+	if (vfl->fops && vfl->fops->release)
+		err = vfl->fops->release(inode, file);
+
+	mutex_lock(&videodev_lock);
+	kobject_put(&vfl->class_dev.kobj);
+	mutex_unlock(&videodev_lock);
+
+	return err;
+}
+
 /*
  *	Open a video device - FIXME: Obsoleted
  */
@@ -469,10 +485,11 @@ static int video_open(struct inode *inode,
struct file *file)
 		}
 	}
 	old_fops = file->f_op;
-	file->f_op = fops_get(vfl->fops);
-	if(file->f_op->open)
+	file->f_op = fops_get(&vfl->priv_fops);
+	if (file->f_op->open && kobject_get(&vfl->class_dev.kobj))
 		err = file->f_op->open(inode,file);
 	if (err) {
+		kobject_put(&vfl->class_dev.kobj);
 		fops_put(file->f_op);
 		file->f_op = fops_get(old_fops);
 	}
@@ -2175,6 +2192,8 @@ int video_register_device_index(struct
video_device *vfd, int type, int nr,
 	}

 	vfd->index = ret;
+	vfd->priv_fops = *vfd->fops;
+	vfd->priv_fops.release = video_close;

 	mutex_unlock(&videodev_lock);
 	mutex_init(&vfd->lock);
@@ -2225,7 +2244,6 @@ void video_unregister_device(struct video_device *vfd)
 	if(video_device[vfd->minor]!=vfd)
 		panic("videodev: bad unregister");

-	video_device[vfd->minor]=NULL;
 	device_unregister(&vfd->class_dev);
 	mutex_unlock(&videodev_lock);
 }
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 3c93414..d4fe617 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -342,6 +342,7 @@ void *priv;
 	/* for videodev.c intenal usage -- please don't touch */
 	int users;                     /* video_exclusive_{open|close} ... */
 	struct mutex lock;             /* ... helper function uses these   */
+	struct file_operations priv_fops; /* video_close */
 };

 /* Class-dev to video-device */
-- 
1.5.5.1

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] videodev: fix kobj ref count

[Hans de Goede]

David Ellingsworth wrote:
> Mauro,
> 
> If Laurent approves, please apply the following patch to the devel
> branch. I've been using it locally for the past five days or so
> without issue.
> 
> This patch increments the kobject reference count during video_open
> and decrements it during video_close. Doing so allows
> video_unregister_device to be called during the disconnect callback of
> usb and pci devices. It also ensures that the video_device struct is
> not freed while it is still in use and that the kobject release
> callback occurs at the appropriate time. With this patch, the
> following sequence is now possible and no longer results in a crash.
> 
> video_open
>   disconnect
>     video_unregister_device
>       video_ioctl2 (crash was here)
>         video_close
>           video_release
> 

I like this patch, I really do, as currently this refcounting needs to 
be done in all v4l devices seperately, and an video_ioctl2 wrapper needs 
to be used to stop the crash (check if device was disconnected and thus 
video_unregister_device was called and ifso don't call video_ioctl2).

So I've taken a good look and this, and I cannot find any issues.

Regards,

Hans

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] videodev: fix kobj ref count

[Laurent Pinchart]

Hi Hans,

On Wednesday 09 July 2008, Hans de Goede wrote:
> David Ellingsworth wrote:
> > Mauro,
> >
> > If Laurent approves, please apply the following patch to the devel
> > branch. I've been using it locally for the past five days or so
> > without issue.
> >
> > This patch increments the kobject reference count during video_open
> > and decrements it during video_close. Doing so allows
> > video_unregister_device to be called during the disconnect callback of
> > usb and pci devices. It also ensures that the video_device struct is
> > not freed while it is still in use and that the kobject release
> > callback occurs at the appropriate time. With this patch, the
> > following sequence is now possible and no longer results in a crash.
> >
> > video_open
> >   disconnect
> >     video_unregister_device
> >       video_ioctl2 (crash was here)
> >         video_close
> >           video_release
>
> I like this patch, I really do, as currently this refcounting needs to
> be done in all v4l devices seperately, and an video_ioctl2 wrapper needs
> to be used to stop the crash (check if device was disconnected and thus
> video_unregister_device was called and ifso don't call video_ioctl2).

I like it as well, as there is currently no way for video drivers to avoid 
both race conditions and deadlocks. As you stated, refcounting currently 
needs to be done in the device drivers. To avoid race conditions between 
open() and disconnect(), device drivers need a global lock that will prevent 
open() to reference driver-specific structures that just got freed through 
disconnect(). That lock must be taken in the driver open() handler, already 
protected by the videodev_lock, and in the driver disconnect() handler, that 
calls video_unregister_device() that will in turn take the videodev_lock. 
This results in a AB-BA deadlock.

In short, reference counting in the videodev layer is required.

> So I've taken a good look and this, and I cannot find any issues.

I'm currently reviewing David's patch (sorry for the delay). I'll post a go/no 
go (hopefully the former :-)) very soon.

Best regards,

Laurent Pinchart

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] videodev: fix kobj ref count

[Laurent Pinchart]

On Wednesday 09 July 2008, Laurent Pinchart wrote:
> Hi Hans,
>
> On Wednesday 09 July 2008, Hans de Goede wrote:
> > David Ellingsworth wrote:
> > > Mauro,
> > >
> > > If Laurent approves, please apply the following patch to the devel
> > > branch. I've been using it locally for the past five days or so
> > > without issue.
> > >
> > > This patch increments the kobject reference count during video_open
> > > and decrements it during video_close. Doing so allows
> > > video_unregister_device to be called during the disconnect callback of
> > > usb and pci devices. It also ensures that the video_device struct is
> > > not freed while it is still in use and that the kobject release
> > > callback occurs at the appropriate time. With this patch, the
> > > following sequence is now possible and no longer results in a crash.
> > >
> > > video_open
> > >   disconnect
> > >     video_unregister_device
> > >       video_ioctl2 (crash was here)
> > >         video_close
> > >           video_release
> >
> > I like this patch, I really do, as currently this refcounting needs to
> > be done in all v4l devices seperately, and an video_ioctl2 wrapper needs
> > to be used to stop the crash (check if device was disconnected and thus
> > video_unregister_device was called and ifso don't call video_ioctl2).
>
> I like it as well, as there is currently no way for video drivers to avoid
> both race conditions and deadlocks. As you stated, refcounting currently
> needs to be done in the device drivers. To avoid race conditions between
> open() and disconnect(), device drivers need a global lock that will
> prevent open() to reference driver-specific structures that just got freed
> through disconnect(). That lock must be taken in the driver open() handler,
> already protected by the videodev_lock, and in the driver disconnect()
> handler, that calls video_unregister_device() that will in turn take the
> videodev_lock. This results in a AB-BA deadlock.

I spoke too soon. Device drivers don't need to protect 
video_unregister_device() with the driver lock. video_unregister_device() 
will wait until video_open() releases the videodev_lock

All a driver has to do is ensure open() will not complete successfully if 
video_unregister_device() is about to be called or to proceed (waiting on 
videodev_lock). This can easily be achieved by setting a flag in the 
disconnect() handler and checking that flag in the open() handler.

To avoid race conditions, setting the flag must be protected by the device 
lock.

disconnect()                           | open()
                                       |
lock driver                            |
set disconnected flag                  |
unlock driver                          |
                                       | lock driver
                                       | return error if disconnected flag set
                                       | initialise
                                       | increment the reference count
                                       | unlock driver
decrement reference count              |
if (reference count reached 0)         |
        uvc_unregister_device()        |
                                       |

The disconnected flag will either be set before open() proceeds, in which case 
it will return an error, or after open() is done, in which case the reference 
count will not reach 0 in disconnect().

> In short, reference counting in the videodev layer is required.

Maybe not required, but still desirable. Getting complex locking right is 
tricky and very error-prone, so let's put all locking bugs in videodev :-)

> > So I've taken a good look and this, and I cannot find any issues.
>
> I'm currently reviewing David's patch (sorry for the delay). I'll post a
> go/no go (hopefully the former :-)) very soon.


Best regards,

Laurent Pinchart

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] videodev: fix kobj ref count

[Laurent Pinchart]

Hi David,

On Wednesday 09 July 2008, David Ellingsworth wrote:
> Mauro,
>
> If Laurent approves, please apply the following patch to the devel
> branch. I've been using it locally for the past five days or so
> without issue.
>
> This patch increments the kobject reference count during video_open
> and decrements it during video_close. Doing so allows
> video_unregister_device to be called during the disconnect callback of
> usb and pci devices. It also ensures that the video_device struct is
> not freed while it is still in use and that the kobject release
> callback occurs at the appropriate time. With this patch, the
> following sequence is now possible and no longer results in a crash.
>
> video_open
>   disconnect
>     video_unregister_device
>       video_ioctl2 (crash was here)
>         video_close
>           video_release
>
> Regards,
>
> David Ellingsworth
>
> Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
> ---
>  drivers/media/video/videodev.c |   50
> +++++++++++++++++++++++++++------------ include/media/v4l2-dev.h       |   
> 1 +
>  2 files changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/video/videodev.c
> b/drivers/media/video/videodev.c index 0d52819..9922cd6 100644
> --- a/drivers/media/video/videodev.c
> +++ b/drivers/media/video/videodev.c
> @@ -406,17 +406,22 @@ void video_device_release(struct video_device *vfd)
>  }
>  EXPORT_SYMBOL(video_device_release);
>
> +/*
> + *	Active devicesSigned-off-by: David Ellingsworth
> <david@identd.dyndns.org> ---
>  drivers/media/video/videodev.c |   50
> +++++++++++++++++++++++++++------------ include/media/v4l2-dev.h       |   
> 1 +
>  2 files changed, 35 insertions(+), 16 deletions(-)

Copy & paste issue ?

> diff --git a/drivers/media/video/videodev.c
> b/drivers/media/video/videodev.c index 0d52819..9922cd6 100644
> --- a/drivers/media/video/videodev.c
> +++ b/drivers/media/video/videodev.c
> @@ -406,17 +406,22 @@ void video_device_release(struct video_device *vfd)
>  }
>  EXPORT_SYMBOL(video_device_release);
>
> +/*
> + *	Active devices
> + */
> +
> +static struct video_device *video_device[VIDEO_NUM_DEVICES];
> +static DEFINE_MUTEX(videodev_lock);
> +
> +/* must be called with videodev_lock held */
>  static void video_release(struct device *cd)
>  {
>  	struct video_device *vfd = container_of(cd, struct video_device,
>  								class_dev);
>
> -#if 1
> -	/* needed until all drivers are fixed */
> -	if (!vfd->release)
> -		return;
> -#endif
> -	vfd->release(vfd);
> +	if (vfd->release)
> +		vfd->release(vfd);
> +	video_device[vfd->minor] = NULL;
>  }
>
>  static struct device_attribute video_device_attrs[] = {
> @@ -431,19 +436,30 @@ static struct class video_class = {
>  	.dev_release = video_release,
>  };
>
> -/*
> - *	Active devices
> - */
> -
> -static struct video_device *video_device[VIDEO_NUM_DEVICES];
> -static DEFINE_MUTEX(videodev_lock);
> -
>  struct video_device* video_devdata(struct file *file)
>  {
>  	return video_device[iminor(file->f_path.dentry->d_inode)];
>  }
>  EXPORT_SYMBOL(video_devdata);
>
> +static int video_close(struct inode *inode, struct file *file)
> +{
> +	unsigned int minor = iminor(inode);
> +	int err = 0;
> +	struct video_device *vfl;
> +
> +	vfl = video_device[minor];
> +
> +	if (vfl->fops && vfl->fops->release)
> +		err = vfl->fops->release(inode, file);
> +
> +	mutex_lock(&videodev_lock);
> +	kobject_put(&vfl->class_dev.kobj);
> +	mutex_unlock(&videodev_lock);
> +
> +	return err;
> +}
> +
>  /*
>   *	Open a video device - FIXME: Obsoleted
>   */
> @@ -469,10 +485,11 @@ static int video_open(struct inode *inode,
> struct file *file)
>  		}
>  	}
>  	old_fops = file->f_op;
> -	file->f_op = fops_get(vfl->fops);
> -	if(file->f_op->open)
> +	file->f_op = fops_get(&vfl->priv_fops);
> +	if (file->f_op->open && kobject_get(&vfl->class_dev.kobj))

Shouldn't kobject_get be called even if file->f_op->open is NULL ?

>  		err = file->f_op->open(inode,file);
>  	if (err) {
> +		kobject_put(&vfl->class_dev.kobj);
>  		fops_put(file->f_op);
>  		file->f_op = fops_get(old_fops);
>  	}
> @@ -2175,6 +2192,8 @@ int video_register_device_index(struct
> video_device *vfd, int type, int nr,
>  	}
>
>  	vfd->index = ret;
> +	vfd->priv_fops = *vfd->fops;
> +	vfd->priv_fops.release = video_close;
>
>  	mutex_unlock(&videodev_lock);
>  	mutex_init(&vfd->lock);
> @@ -2225,7 +2244,6 @@ void video_unregister_device(struct video_device
> *vfd) if(video_device[vfd->minor]!=vfd)
>  		panic("videodev: bad unregister");
>
> -	video_device[vfd->minor]=NULL;
>  	device_unregister(&vfd->class_dev);
>  	mutex_unlock(&videodev_lock);
>  }
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index 3c93414..d4fe617 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -342,6 +342,7 @@ void *priv;
>  	/* for videodev.c intenal usage -- please don't touch */
>  	int users;                     /* video_exclusive_{open|close} ... */
>  	struct mutex lock;             /* ... helper function uses these   */
> +	struct file_operations priv_fops; /* video_close */
>  };
>
>  /* Class-dev to video-device */

The rest looks ok to me. Just one last question, in a previous e-mail you 
stated

On Wednesday 02 July 2008, David Ellingsworth wrote:
> I think I found a solution to the above issue. I removed the lock from
> video_release and the main portion of video_close and wrapped the two
> calls to kobject_put by the videodev_lock. Since video_close is called
> when the BKL is held the lock is not required around the main portion
> of video_close. Acquiring the lock around the calls to kobject_put
> insures video_release is always called while the lock is being held.
> This should fix the above race condition between device_unregister and
> video_open as well. Here is the corrected patch.

Could you elaborate as to why the lock would be required around the main 
portion of video_close if it wasn't called with the BKL held ? I want to be 
sure there is no race condition on SMP systems, where the BKL will only 
prevent open() and close() from running concurrently but will not protect 
from any other race condition.

As we are dealing with tricky race conditions (the amount of mails exchanged 
to prepare this patch proves the issue is not trivial) I think you should 
include a comment in the code to explain the rationale behind the patch and 
the design decisions/requirements to help other developers not to introduce 
race conditions or dead locks when hacking on videodev.c.

You also mentioned that videodev should be converted to char_dev. Would you 
volunteer ? ;-)

Best regards,

Laurent Pinchart

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] videodev: fix kobj ref count

[David Ellingsworth]

On Wed, Jul 9, 2008 at 4:21 PM, Laurent Pinchart
<laurent.pinchart@skynet.be> wrote:
> On Wednesday 09 July 2008, Laurent Pinchart wrote:
>> Hi Hans,
>>
>> On Wednesday 09 July 2008, Hans de Goede wrote:
>> > David Ellingsworth wrote:
>> > > Mauro,
>> > >
>> > > If Laurent approves, please apply the following patch to the devel
>> > > branch. I've been using it locally for the past five days or so
>> > > without issue.
>> > >
>> > > This patch increments the kobject reference count during video_open
>> > > and decrements it during video_close. Doing so allows
>> > > video_unregister_device to be called during the disconnect callback of
>> > > usb and pci devices. It also ensures that the video_device struct is
>> > > not freed while it is still in use and that the kobject release
>> > > callback occurs at the appropriate time. With this patch, the
>> > > following sequence is now possible and no longer results in a crash.
>> > >
>> > > video_open
>> > >   disconnect
>> > >     video_unregister_device
>> > >       video_ioctl2 (crash was here)
>> > >         video_close
>> > >           video_release
>> >
>> > I like this patch, I really do, as currently this refcounting needs to
>> > be done in all v4l devices seperately, and an video_ioctl2 wrapper needs
>> > to be used to stop the crash (check if device was disconnected and thus
>> > video_unregister_device was called and ifso don't call video_ioctl2).
>>
>> I like it as well, as there is currently no way for video drivers to avoid
>> both race conditions and deadlocks. As you stated, refcounting currently
>> needs to be done in the device drivers. To avoid race conditions between
>> open() and disconnect(), device drivers need a global lock that will
>> prevent open() to reference driver-specific structures that just got freed
>> through disconnect(). That lock must be taken in the driver open() handler,
>> already protected by the videodev_lock, and in the driver disconnect()
>> handler, that calls video_unregister_device() that will in turn take the
>> videodev_lock. This results in a AB-BA deadlock.
>
> I spoke too soon. Device drivers don't need to protect
> video_unregister_device() with the driver lock. video_unregister_device()
> will wait until video_open() releases the videodev_lock
>
Correct, but the current implementation of video_unregister_device
when called always causes the kobject release callback to occur which
frees the video_device struct and makes video_ioctl2 unusable. Thus
drivers either must avoid calling video_unregister device while the
device is still open or use a wrapper around video_ioctl2 to prevent
it from being called after video_device_unregister has been called. In
my opinion, video_unregister_device should always be called during
disconnect handler to prevent future calls to open. In doing so it
should not cause the video_device struct to be freed unless it is no
longer in use.

> All a driver has to do is ensure open() will not complete successfully if
> video_unregister_device() is about to be called or to proceed (waiting on
> videodev_lock). This can easily be achieved by setting a flag in the
> disconnect() handler and checking that flag in the open() handler.
>
Agreed, a flag is still necessary as a call to open() may still occur
after the disconnect() handler. However, in my opinion drivers should
not have to delay the call to video_unregister_device nor maintain a
reference count in order to determine when it's safe for it to be
called (like the example below). Always being able to call
video_device_unregister during the disconnect() handler reduces the
number of failed calls to open while the device is waiting for the
final close to occur.

> To avoid race conditions, setting the flag must be protected by the device
> lock.
>
> disconnect()                           | open()
>                                       |
> lock driver                            |
> set disconnected flag                  |
> unlock driver                          |
>                                       | lock driver
>                                       | return error if disconnected flag set
>                                       | initialise
>                                       | increment the reference count
>                                       | unlock driver
> decrement reference count              |
> if (reference count reached 0)         |
>        uvc_unregister_device()        |
>                                       |
>
> The disconnected flag will either be set before open() proceeds, in which case
> it will return an error, or after open() is done, in which case the reference
> count will not reach 0 in disconnect().
>

With this patch the above sequence could be simplified as follows:

disconnect()			| open()
				|
lock driver			|
set disconnect flag		|
unlock driver			|
video_unregister_device()	|
				| lock driver
				| return error if disconnect flag set
				| initialize
				| unlock driver

Note, no reference count is required. Freeing of all internal objects
can occur during the kobject release handler. In the example above, if
video_unregister_device() obtains the videodev_lock before open() and
decrements the kobject reference count to 0, the call to open will
fail long before it reaches the the driver specified implementation of
open() and the bottom half of the above will not even occur. The only
conditions under which the above would occur is if the device is
already open (decrementing the kobject reference count in
video_unregister_device did not cause it to go to 0), or open()
obtained the videodev_lock before video_unregister_device(). In either
case, video_unregister_device has been called and no more opens may
occur thereafter.

>> In short, reference counting in the videodev layer is required.
>
> Maybe not required, but still desirable. Getting complex locking right is
> tricky and very error-prone, so let's put all locking bugs in videodev :-)
>
>> > So I've taken a good look and this, and I cannot find any issues.
>>
>> I'm currently reviewing David's patch (sorry for the delay). I'll post a
>> go/no go (hopefully the former :-)) very soon.
>

Regards,

David Ellingsworth

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] videodev: fix kobj ref count

[David Ellingsworth]

On Wed, Jul 9, 2008 at 5:42 PM, Laurent Pinchart
<laurent.pinchart@skynet.be> wrote:
> Hi David,
>
> On Wednesday 09 July 2008, David Ellingsworth wrote:
>> Mauro,
>>
>> If Laurent approves, please apply the following patch to the devel
>> branch. I've been using it locally for the past five days or so
>> without issue.
>>
>> This patch increments the kobject reference count during video_open
>> and decrements it during video_close. Doing so allows
>> video_unregister_device to be called during the disconnect callback of
>> usb and pci devices. It also ensures that the video_device struct is
>> not freed while it is still in use and that the kobject release
>> callback occurs at the appropriate time. With this patch, the
>> following sequence is now possible and no longer results in a crash.
>>
>> video_open
>>   disconnect
>>     video_unregister_device
>>       video_ioctl2 (crash was here)
>>         video_close
>>           video_release
>>
>> Regards,
>>
>> David Ellingsworth
>>
>> Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
>> ---
>>  drivers/media/video/videodev.c |   50
>> +++++++++++++++++++++++++++------------ include/media/v4l2-dev.h       |
>> 1 +
>>  2 files changed, 35 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/media/video/videodev.c
>> b/drivers/media/video/videodev.c index 0d52819..9922cd6 100644
>> --- a/drivers/media/video/videodev.c
>> +++ b/drivers/media/video/videodev.c
>> @@ -406,17 +406,22 @@ void video_device_release(struct video_device *vfd)
>>  }
>>  EXPORT_SYMBOL(video_device_release);
>>
>> +/*
>> + *   Active devicesSigned-off-by: David Ellingsworth
>> <david@identd.dyndns.org> ---
>>  drivers/media/video/videodev.c |   50
>> +++++++++++++++++++++++++++------------ include/media/v4l2-dev.h       |
>> 1 +
>>  2 files changed, 35 insertions(+), 16 deletions(-)
>
> Copy & paste issue ?
>
>> diff --git a/drivers/media/video/videodev.c
>> b/drivers/media/video/videodev.c index 0d52819..9922cd6 100644
>> --- a/drivers/media/video/videodev.c
>> +++ b/drivers/media/video/videodev.c
>> @@ -406,17 +406,22 @@ void video_device_release(struct video_device *vfd)
>>  }
>>  EXPORT_SYMBOL(video_device_release);
>>
>> +/*
>> + *   Active devices
>> + */
>> +
>> +static struct video_device *video_device[VIDEO_NUM_DEVICES];
>> +static DEFINE_MUTEX(videodev_lock);
>> +
>> +/* must be called with videodev_lock held */
>>  static void video_release(struct device *cd)
>>  {
>>       struct video_device *vfd = container_of(cd, struct video_device,
>>                                                               class_dev);
>>
>> -#if 1
>> -     /* needed until all drivers are fixed */
>> -     if (!vfd->release)
>> -             return;
>> -#endif
>> -     vfd->release(vfd);
>> +     if (vfd->release)
>> +             vfd->release(vfd);
>> +     video_device[vfd->minor] = NULL;
>>  }
>>
>>  static struct device_attribute video_device_attrs[] = {
>> @@ -431,19 +436,30 @@ static struct class video_class = {
>>       .dev_release = video_release,
>>  };
>>
>> -/*
>> - *   Active devices
>> - */
>> -
>> -static struct video_device *video_device[VIDEO_NUM_DEVICES];
>> -static DEFINE_MUTEX(videodev_lock);
>> -
>>  struct video_device* video_devdata(struct file *file)
>>  {
>>       return video_device[iminor(file->f_path.dentry->d_inode)];
>>  }
>>  EXPORT_SYMBOL(video_devdata);
>>
>> +static int video_close(struct inode *inode, struct file *file)
>> +{
>> +     unsigned int minor = iminor(inode);
>> +     int err = 0;
>> +     struct video_device *vfl;
>> +
>> +     vfl = video_device[minor];
>> +
>> +     if (vfl->fops && vfl->fops->release)
>> +             err = vfl->fops->release(inode, file);
>> +
>> +     mutex_lock(&videodev_lock);
>> +     kobject_put(&vfl->class_dev.kobj);
>> +     mutex_unlock(&videodev_lock);
>> +
>> +     return err;
>> +}
>> +
>>  /*
>>   *   Open a video device - FIXME: Obsoleted
>>   */
>> @@ -469,10 +485,11 @@ static int video_open(struct inode *inode,
>> struct file *file)
>>               }
>>       }
>>       old_fops = file->f_op;
>> -     file->f_op = fops_get(vfl->fops);
>> -     if(file->f_op->open)
>> +     file->f_op = fops_get(&vfl->priv_fops);
>> +     if (file->f_op->open && kobject_get(&vfl->class_dev.kobj))
>
> Shouldn't kobject_get be called even if file->f_op->open is NULL ?
Now that I've looked at it again, it seems you are right, kobject_get
should be called even if file->f_op->open is null since we return err
and it will remain 0. This means a subsequent call to video_close
would occur. Good catch.

There are two ways to handle this, (1) initialize err to something
like -ENODEV so the open fails if file->f_op->open is not defined or
(2) always call kobject_get and wait for the call to video_close to do
the kobject_put.

Personally I prefer the first approach since it ensures no call to
video_close happens and thus no call to file->f_op->release occurs.
What do you think?
>
>>               err = file->f_op->open(inode,file);
>>       if (err) {
>> +             kobject_put(&vfl->class_dev.kobj);
>>               fops_put(file->f_op);
>>               file->f_op = fops_get(old_fops);
>>       }
>> @@ -2175,6 +2192,8 @@ int video_register_device_index(struct
>> video_device *vfd, int type, int nr,
>>       }
>>
>>       vfd->index = ret;
>> +     vfd->priv_fops = *vfd->fops;
>> +     vfd->priv_fops.release = video_close;
>>
>>       mutex_unlock(&videodev_lock);
>>       mutex_init(&vfd->lock);
>> @@ -2225,7 +2244,6 @@ void video_unregister_device(struct video_device
>> *vfd) if(video_device[vfd->minor]!=vfd)
>>               panic("videodev: bad unregister");
>>
>> -     video_device[vfd->minor]=NULL;
>>       device_unregister(&vfd->class_dev);
>>       mutex_unlock(&videodev_lock);
>>  }
>> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
>> index 3c93414..d4fe617 100644
>> --- a/include/media/v4l2-dev.h
>> +++ b/include/media/v4l2-dev.h
>> @@ -342,6 +342,7 @@ void *priv;
>>       /* for videodev.c intenal usage -- please don't touch */
>>       int users;                     /* video_exclusive_{open|close} ... */
>>       struct mutex lock;             /* ... helper function uses these   */
>> +     struct file_operations priv_fops; /* video_close */
>>  };
>>
>>  /* Class-dev to video-device */
>
> The rest looks ok to me. Just one last question, in a previous e-mail you
> stated
>
> On Wednesday 02 July 2008, David Ellingsworth wrote:
>> I think I found a solution to the above issue. I removed the lock from
>> video_release and the main portion of video_close and wrapped the two
>> calls to kobject_put by the videodev_lock. Since video_close is called
>> when the BKL is held the lock is not required around the main portion
>> of video_close. Acquiring the lock around the calls to kobject_put
>> insures video_release is always called while the lock is being held.
>> This should fix the above race condition between device_unregister and
>> video_open as well. Here is the corrected patch.
>
> Could you elaborate as to why the lock would be required around the main
> portion of video_close if it wasn't called with the BKL held ? I want to be
> sure there is no race condition on SMP systems, where the BKL will only
> prevent open() and close() from running concurrently but will not protect
> from any other race condition.
>
Initially I had hoped to use the videodev lock to prevent open() and
close from racing once the BKL is removed, but it's currently serving
a different purpose. If we maintain this code after the BLK is
removed, another lock will undoubtedly be needed to prevent them from
racing. Two locks are needed is because a call to video_close() may
call video_unregister_device() and therefore and both can not take the
same lock. I didn't want to introduce another lock at this time, and
the BKL is sufficient for the time being.

> As we are dealing with tricky race conditions (the amount of mails exchanged
> to prepare this patch proves the issue is not trivial) I think you should
> include a comment in the code to explain the rationale behind the patch and
> the design decisions/requirements to help other developers not to introduce
> race conditions or dead locks when hacking on videodev.c.
>
Sure, no problem. Should comments be displaced throughout the code? or
should I just place one large comment near the top somewhere?

> You also mentioned that videodev should be converted to char_dev. Would you
> volunteer ? ;-)
>
Sure, I'll volunteer to do the work once we have something stable and
behaves like char_dev.

Regards,

David Ellingsworth

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] videodev: fix kobj ref count

[David Ellingsworth]

[snip]
>>>  /*
>>>   *   Open a video device - FIXME: Obsoleted
>>>   */
>>> @@ -469,10 +485,11 @@ static int video_open(struct inode *inode,
>>> struct file *file)
>>>               }
>>>       }
>>>       old_fops = file->f_op;
>>> -     file->f_op = fops_get(vfl->fops);
>>> -     if(file->f_op->open)
>>> +     file->f_op = fops_get(&vfl->priv_fops);
>>> +     if (file->f_op->open && kobject_get(&vfl->class_dev.kobj))
>>
>> Shouldn't kobject_get be called even if file->f_op->open is NULL ?
> Now that I've looked at it again, it seems you are right, kobject_get
> should be called even if file->f_op->open is null since we return err
> and it will remain 0. This means a subsequent call to video_close
> would occur. Good catch.
>
> There are two ways to handle this, (1) initialize err to something
> like -ENODEV so the open fails if file->f_op->open is not defined or
> (2) always call kobject_get and wait for the call to video_close to do
> the kobject_put.
>
Keeping with the behavior of char_dev, the correct thing to do is to
always increment the kobject reference and allow the call to open to
succeed.

The following patch implements this behavior. I've also thrown in
quite a few comments to help document the new behavior.

Regards,

David Ellingsworth

[PATCH] videodev: fix kobj ref count


Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
---
 drivers/media/video/videodev.c |   91 +++++++++++++++++++++++++++++++--------
 include/media/v4l2-dev.h       |    1 +
 2 files changed, 73 insertions(+), 19 deletions(-)

diff --git a/drivers/media/video/videodev.c b/drivers/media/video/videodev.c
index 0d52819..fd6aec1 100644
--- a/drivers/media/video/videodev.c
+++ b/drivers/media/video/videodev.c
@@ -406,17 +406,31 @@ void video_device_release(struct video_device *vfd)
 }
 EXPORT_SYMBOL(video_device_release);

+/*
+ *	Active devices
+ */
+
+static struct video_device *video_device[VIDEO_NUM_DEVICES];
+
+/*
+ * videodev_lock is used to ensure exclusive access to the
+ * global video_device array and the kobject.
+ */
+static DEFINE_MUTEX(videodev_lock);
+
+/*
+ * video_release handles the kobject release callback.
+ * It must be called with the videodev_lock held
+ * since it modifies the global video_device array.
+ */
 static void video_release(struct device *cd)
 {
 	struct video_device *vfd = container_of(cd, struct video_device,
 								class_dev);

-#if 1
-	/* needed until all drivers are fixed */
-	if (!vfd->release)
-		return;
-#endif
-	vfd->release(vfd);
+	if (vfd->release)
+		vfd->release(vfd);
+	video_device[vfd->minor] = NULL;
 }

 static struct device_attribute video_device_attrs[] = {
@@ -431,13 +445,6 @@ static struct class video_class = {
 	.dev_release = video_release,
 };

-/*
- *	Active devices
- */
-
-static struct video_device *video_device[VIDEO_NUM_DEVICES];
-static DEFINE_MUTEX(videodev_lock);
-
 struct video_device* video_devdata(struct file *file)
 {
 	return video_device[iminor(file->f_path.dentry->d_inode)];
@@ -445,7 +452,49 @@ struct video_device* video_devdata(struct file *file)
 EXPORT_SYMBOL(video_devdata);

 /*
+ * video_close intercepts the file_operations release callback in
+ * order to decrement the kobject reference count and counteract
+ * the increment in video_open.
+ *
+ * This function is obsolete and will be removed in future releases.
+ * Races between video_open and video_close are currently prevented
+ * by the BKL.
+ */
+static int video_close(struct inode *inode, struct file *file)
+{
+	unsigned int minor = iminor(inode);
+	int err = 0;
+	struct video_device *vfl;
+
+	/*
+	 * videodev_lock is not taken here to prevent a deadlock that
+	 * would occur if vfl->fops->release calls video_unregister_device.
+	 * video_device[minor] is guaranteed not to be null in this case
+	 * since the kobject release callback has not yet been called.
+	 */
+	vfl = video_device[minor];
+
+	if (vfl->fops && vfl->fops->release)
+		err = vfl->fops->release(inode, file);
+
+	mutex_lock(&videodev_lock);
+	/*
+	 * The following kobject_put will cause video_release to be called
+	 * if the reference count is decremented to 0. This will only happen
+	 * if video_unregister_device has already been called and this is the
+	 * last call to video_close.
+	 */
+	kobject_put(&vfl->class_dev.kobj);
+	mutex_unlock(&videodev_lock);
+
+	return err;
+}
+
+/*
  *	Open a video device - FIXME: Obsoleted
+ *
+ *	A successful call to video_open increments the kobject
+ *	reference count.
  */
 static int video_open(struct inode *inode, struct file *file)
 {
@@ -469,10 +518,12 @@ static int video_open(struct inode *inode,
struct file *file)
 		}
 	}
 	old_fops = file->f_op;
-	file->f_op = fops_get(vfl->fops);
-	if(file->f_op->open)
+	file->f_op = fops_get(&vfl->priv_fops);
+	kobject_get(&vfl->class_dev.kobj);
+	if (file->f_op->open)
 		err = file->f_op->open(inode,file);
 	if (err) {
+		kobject_put(&vfl->class_dev.kobj);
 		fops_put(file->f_op);
 		file->f_op = fops_get(old_fops);
 	}
@@ -2175,6 +2226,8 @@ int video_register_device_index(struct
video_device *vfd, int type, int nr,
 	}

 	vfd->index = ret;
+	vfd->priv_fops = *vfd->fops;
+	vfd->priv_fops.release = video_close;

 	mutex_unlock(&videodev_lock);
 	mutex_init(&vfd->lock);
@@ -2215,17 +2268,17 @@ EXPORT_SYMBOL(video_register_device_index);
  *	video_unregister_device - unregister a video4linux device
  *	@vfd: the device to unregister
  *
- *	This unregisters the passed device and deassigns the minor
- *	number. Future open calls will be met with errors.
+ * 	This unregisters the passed device and decrements the kobject
+ * 	reference count. If the kobject reference count is decremented to
+ * 	0 video_release will be called to unassign the minor number.
+ * 	Calls to video_open will not occur after this function completes.
  */
-
 void video_unregister_device(struct video_device *vfd)
 {
 	mutex_lock(&videodev_lock);
 	if(video_device[vfd->minor]!=vfd)
 		panic("videodev: bad unregister");

-	video_device[vfd->minor]=NULL;
 	device_unregister(&vfd->class_dev);
 	mutex_unlock(&videodev_lock);
 }
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 3c93414..d4fe617 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -342,6 +342,7 @@ void *priv;
 	/* for videodev.c intenal usage -- please don't touch */
 	int users;                     /* video_exclusive_{open|close} ... */
 	struct mutex lock;             /* ... helper function uses these   */
+	struct file_operations priv_fops; /* video_close */
 };

 /* Class-dev to video-device */
-- 
1.5.6

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list