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

Laurent Pinchart
2011-09-06T08:20:01Z

On Tuesday 06 September 2011 00:31:03 Sitsofe Wheeler wrote:
> 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

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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

--
Regards,

Laurent Pinchart