Re: Are *all* 2.4.x v4l drivers now broken?

Duncan Haldane wrote:
>  Hi,
>   
>  continuing my investigation of the effects of the lock added to
>  video_unregister_device() in 2.4.19, I got reports of yet more
>  drivers that are broken in a way that is fatal to the system if the
>  camera is disconnected while open.
>  
>  I was hoping that usbvideo.c would turn out to be a example that
>  shows how to close gracefully, but I now see that even that is
>  probably broken (at best, it might accidentally  be OK if
>  a race gets lost)
> 
>  In all cases, if the usb cable is disconnected while the camera is open,
>  the  call to video_unregister_device() is deferred till
>  the final v4l_close() call (which is called during a v4l lock
>  by video_release() ).

Hmm.  The only _sane_ way to fix that is using video_device->fops (like
it must be done in 2.5.x).  That allows to unregister the device at
unplug time, you don't have to deferred that until close() is called to
avoid video_close() barf.  The unplug races are fixed up nicely this
way.

>  Schematically, what happens is:
>  
>  video_release()
>  {
>  down(&lock);
>  v4l_close();
>  up(&lock);
>  }
>  
>  void v4l_close()
>  {
>  video_unregister_device(); 
>  }
>  
>  void video_unregister_device()
>  {
>  down(&lock);
>  AARGH!
>  up(&lock);
>  }

And simply fixing that deadlock by removing the lock in video_release()
or video_unregister_device() creates an ugly race:  video_release()
might reference memory just kfree()ed by the driver.  Thats why the lock
was added in the first place.  It is unlikely that you actually hit that
race, thats why it probably never caused major trouble in older
versions.

The patch below hopefully fixes that without creating new holes.
Comments?

  Gerd

==============================[ cut here ]==============================
--- videodev.c.lock	2002-12-10 11:06:04.000000000 +0100
+++ videodev.c	2002-12-10 11:21:44.000000000 +0100
@@ -187,13 +187,19 @@
 {
 	struct video_device *vfl;
 
-	down(&videodev_lock);
 	vfl = video_devdata(file);
 	if(vfl->close)
 		vfl->close(vfl);
-	vfl->users--;
-	if(vfl->owner)
-		__MOD_DEC_USE_COUNT(vfl->owner);
+
+	down(&videodev_lock);
+	/* ->close() might have called video_device_unregister()
+           in case of a hot unplug, thus we have to get vfl again */
+	vfl = video_devdata(file);
+	if (NULL != vfl) {
+		vfl->users--;
+		if(vfl->owner)
+			__MOD_DEC_USE_COUNT(vfl->owner);
+	}
 	up(&videodev_lock);
 	return 0;
 }