Re: BUG: unable to handle kernel paging request at 6b6b6bcb (v4l2_device_disconnect+0x11/0x30)

Sitsofe Wheeler
2011-09-05T22:31:08Z

On Mon, Sep 05, 2011 at 12:16:42PM +0200, Hans Verkuil wrote:
> On Monday, September 05, 2011 12:13:26 Hans Verkuil wrote:
> >
> > The original order is correct, but what I missed is that for drivers
> > that release (free) everything in the videodev release callback the
> > v4l2_device struct is also freed and v4l2_device_put will fail.
> >
> > To fix this, add this code just before the vdev->release call:
> >
> > /* Do not call v4l2_device_put if there is no release callback set. */
> > if (v4l2_dev->release == NULL)
> > v4l2_dev = NULL;
> >
> > If there is no release callback, then the refcounting is pointless
> > anyway.
> >
> > This should work.
>
> Note that in the long run using the v4l2_device release callback
> instead of the videodev release is better. But it's a lot of work to
> convert everything so that's long term. I'm quite surprised BTW that
> this bug wasn't found much earlier.

This inline patch fixes the second "poison overwritten" problem so:
Tested-by: Sitsofe Wheeler <sitsofe@yahoo.com>

However, it does not prevent the original oops that was reported in the
original message. Yang Ruirui's patch in
https://lkml.org/lkml/2011/9/1/74 seems to be required to resolve
that initial problem - can it be ACK'd? Yang's patch is reproduced
inline below:

For uvc device, dev->vdev.dev is the &intf->dev,
uvc_delete code is as below:
usb_put_intf(dev->intf);
usb_put_dev(dev->udev);

uvc_status_cleanup(dev);
uvc_ctrl_cleanup_device(dev);

## the intf dev is released above, so below code will oops.

if (dev->vdev.dev)
v4l2_device_unregister(&dev->vdev);
Fix it by get_device in v4l2_device_register and put_device in v4l2_device_disconnect
---
drivers/media/video/v4l2-device.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c
index c72856c..e6a2c3b 100644
--- a/drivers/media/video/v4l2-device.c
+++ b/drivers/media/video/v4l2-device.c
@@ -38,6 +38,7 @@ int v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev)
mutex_init(&v4l2_dev->ioctl_lock);
v4l2_prio_init(&v4l2_dev->prio);
kref_init(&v4l2_dev->ref);
+ get_device(dev);
v4l2_dev->dev = dev;
if (dev == NULL) {
/* If dev == NULL, then name must be filled in by the caller */
@@ -93,6 +94,7 @@ void v4l2_device_disconnect(struct v4l2_device *v4l2_dev)

if (dev_get_drvdata(v4l2_dev->dev) == v4l2_dev)
dev_set_drvdata(v4l2_dev->dev, NULL);
+ put_device(v4l2_dev->dev);
v4l2_dev->dev = NULL;
}
EXPORT_SYMBOL_GPL(v4l2_device_disconnect);

--
Sitsofe | http://sucs.org/~sits/