Hi David,
On Saturday, 26 May 2018 19:37:18 EEST David Fries wrote:
> Laurent and Mauro,
>
> I've reviewed Philipp Zabel's 49770 patch, it looks like they
> equivalent, but take a different path. My version does the unref in
> uvc_unregister_video, Zabel's does the unref in each caller of
> uvc_unregister_video. I think I would prefer it to be in fewer
> places.
While I agree that avoiding code duplication is a good idea, we also need to
ensure that the unref call is the very last operation we perform, otherwise
we'll risk a use-after-free. The uvc_unregister_video() function is called
last so that's guaranteed with your patch. However, by hiding the unref there
from the callers, I wonder whether it couldn't result in issues in the future
if someone decides to add code after the uvc_unregister_video() calls without
noticing the unref issue.
> Any ideas on the sysfs group 'power' not found bug on removal while on
> use?
I haven't had time to investigate them yet, please feel free to give it a go
without waiting for me :-)
> Thanks, it will be great to get this fixed.
>
> On Sat, May 26, 2018 at 07:21:11PM +0300, Laurent Pinchart wrote:
> > Hi David,
> >
> > Thank you for the patch.
> >
> > On Saturday, 26 May 2018 18:50:46 EEST David Fries wrote:
> > > Commit 9d15cd958c17 ("media: uvcvideo: Convert from using an atomic
> > > variable to a reference count")
> > > didn't take into account that while the old counter was initialized to
> > > 0 (no stream open), kref_init starts with a reference of 1. The
> > > reference count on unplug no longer reaches 0, uvc_delete isn't
> > > called, and evdev doesn't release /dev/input/event*. Plug and unplug
> > > enough times and it runs out of device minors preventing any new input
> > > device and the use of newly plugged in USB video cameras until the
> > > system is rebooted.
> > >
> > > Signed-off-by: David Fries <da...@fries.net>
> > > Cc: Guennadi Liakhovetski <g.liakhovet...@gmx.de>
> > > Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> > > Cc: Mauro Carvalho Chehab <mche...@s-opensource.com>
> > > Cc: sta...@vger.kernel.org
> >
> > Philipp Zabel has already posted a similar patch, see https://
> > patchwork.linuxtv.org/patch/49770/
> >
> > Mauro,
> >
> > This is a serious issue so I'd like to get the patch merged in v4.18, but
> > it could be argued that it's getting late for that, especially given that
> > the bug has been there since v4.14. Would you be OK merging this in v4.18
> > ? If so could you pick https://patchwork.linuxtv.org/patch/49770/ up, or
> > would you like me to send a pull request ?
> >
> > > ---
> > >
> > > drivers/media/usb/uvc/uvc_driver.c | 11 ++++-------
> > > 1 file changed, 4 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > > b/drivers/media/usb/uvc/uvc_driver.c index 2469b49..3cbdc87 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -1871,13 +1871,6 @@ static void uvc_unregister_video(struct
> > > uvc_device
> > > *dev) {
> > >
> > > struct uvc_streaming *stream;
> > >
> > > - /* Unregistering all video devices might result in uvc_delete()
being
> > > - * called from inside the loop if there's no open file handle. To
> > > avoid
> > > - * that, increment the refcount before iterating over the streams
and
> > > - * decrement it when done.
> > > - */
> > > - kref_get(&dev->ref);
> > > -
> > >
> > > list_for_each_entry(stream, &dev->streams, list) {
> > >
> > > if (!video_is_registered(&stream->vdev))
> > >
> > > continue;
> > >
> > > @@ -1888,6 +1881,10 @@ static void uvc_unregister_video(struct
> > > uvc_device
> > > *dev) uvc_debugfs_cleanup_stream(stream);
> > >
> > > }
> > >
> > > + /* Release the reference implicit in kref_init from uvc_probe,
> > > + * the UVC device won't be deleted until the last file descriptor
> > > + * is also closed.
> > > + */
> > >
> > > kref_put(&dev->ref, uvc_delete);
> > >
> > > }
--
Regards,
Laurent Pinchart
