On Tue, Nov 05, 2024 at 04:36:38PM +0100, Ricardo Ribalda wrote:
> On Tue, 5 Nov 2024 at 15:43, Laurent Pinchart wrote:
> > On Tue, Nov 05, 2024 at 02:32:39PM +0000, Ricardo Ribalda wrote:
> > > After commit c9ec6f173636 ("media: uvcvideo: Stop stream during unregister")
> > > we have some guarantee that userspace will not be able to access any of
> > > our internal structures after disconnect().
> > >
> > > This means that we can do the cleanup at the end of disconnect and make
> > > the code more resilient to races.
> > >
> > > This change will also enable the use of devres functions in more parts
> > > of the code.
> >
> > That's the wrong direction, let's not go there, especially given that
> > this doesn't fix anything. Strong nack on my side, especially given how
> > many of your previous patches introduced race conditions. It's not
>
> They have also fixed some race conditions... keep the discussion
> professional please.
>
> I think this only proves that uvc code is quite complicated. It also
> lacks a lot of consistency with the rest of the drivers in media (and
> in the kernel in general)
> that is exactly what this patch tries to fix.
>
> > broken, don't touch it. A better use of your time would be to fix the
> > unplug race issue at the subsystem level.
>
> Now memory is allocated during uvc_probe(), but it is not freed until
> after disconnect() if userspace has a videodevice open.
> Luckily userspace now cannot interfere with the driver after
> disconnect(), so lets make use of that property to simplify the code.
>
> As the commit message says, with this change we can start using devm_
> functions and we will have less chances to make mistakes
> Eg: no more
> - this cleanout belongs to uvc_register_video_device vs uvc_delete
> - devm_ function fails because it is called too late
>
> This patch fixes a class of bugs. I would really appreciate it if you
> can review it.
> I have moved it to its own patchset:
> https://patchwork.linuxtv.org/project/linux-media/patch/20241105-uvc-rmrefcount-v1-1-123f56b01731@xxxxxxxxxxxx/
> I am planning to send more patches on top of it making use of devres
I currently lack confidence in your patches when it comes to race
conditions and use-after-free. For the ones that fix bugs, or introduce
new features, I bite the bullet and review them. The most recent example
(https://lore.kernel.org/r/20241105-uvc-crashrmmod-v5-1-8623fa51a74f@xxxxxxxxxxxx)
is a good example of why I lack that confidence. I will prioritize my
time to get important uvcvideo patches in as quickly as possible, not on
reviewing patches that do not address any issue and have a high risk of
introducing problems.
If you would like to get this kind of rework in, I recommend rebuilding
bridges first and creating trust. I would request you to drop this patch
in the meantime.
> > > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > > ---
> > > drivers/media/usb/uvc/uvc_driver.c | 24 +++++-------------------
> > > drivers/media/usb/uvc/uvcvideo.h | 1 -
> > > 2 files changed, 5 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index a96f6ca0889f..2735fccdf454 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -1868,16 +1868,12 @@ static int uvc_scan_device(struct uvc_device *dev)
> > > /*
> > > * Delete the UVC device.
> > > *
> > > - * Called by the kernel when the last reference to the uvc_device structure
> > > - * is released.
> > > - *
> > > - * As this function is called after or during disconnect(), all URBs have
> > > + * As this function is called during disconnect(), all URBs have
> > > * already been cancelled by the USB core. There is no need to kill the
> > > * interrupt URB manually.
> > > */
> > > -static void uvc_delete(struct kref *kref)
> > > +static void uvc_delete(struct uvc_device *dev)
> > > {
> > > - struct uvc_device *dev = container_of(kref, struct uvc_device, ref);
> > > struct list_head *p, *n;
> > >
> > > uvc_status_cleanup(dev);
> > > @@ -1919,14 +1915,6 @@ static void uvc_delete(struct kref *kref)
> > > kfree(dev);
> > > }
> > >
> > > -static void uvc_release(struct video_device *vdev)
> > > -{
> > > - struct uvc_streaming *stream = video_get_drvdata(vdev);
> > > - struct uvc_device *dev = stream->dev;
> > > -
> > > - kref_put(&dev->ref, uvc_delete);
> > > -}
> > > -
> > > /*
> > > * Unregister the video devices.
> > > */
> > > @@ -2009,7 +1997,7 @@ int uvc_register_video_device(struct uvc_device *dev,
> > > vdev->v4l2_dev = &dev->vdev;
> > > vdev->fops = fops;
> > > vdev->ioctl_ops = ioctl_ops;
> > > - vdev->release = uvc_release;
> > > + vdev->release = video_device_release_empty;
> > > vdev->prio = &stream->chain->prio;
> > > if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > > vdev->vfl_dir = VFL_DIR_TX;
> > > @@ -2045,7 +2033,6 @@ int uvc_register_video_device(struct uvc_device *dev,
> > > return ret;
> > > }
> > >
> > > - kref_get(&dev->ref);
> > > return 0;
> > > }
> > >
> > > @@ -2160,7 +2147,6 @@ static int uvc_probe(struct usb_interface *intf,
> > > INIT_LIST_HEAD(&dev->entities);
> > > INIT_LIST_HEAD(&dev->chains);
> > > INIT_LIST_HEAD(&dev->streams);
> > > - kref_init(&dev->ref);
> > > atomic_set(&dev->nmappings, 0);
> > >
> > > dev->udev = usb_get_dev(udev);
> > > @@ -2300,7 +2286,7 @@ static int uvc_probe(struct usb_interface *intf,
> > >
> > > error:
> > > uvc_unregister_video(dev);
> > > - kref_put(&dev->ref, uvc_delete);
> > > + uvc_delete(dev);
> > > return -ENODEV;
> > > }
> > >
> > > @@ -2319,7 +2305,7 @@ static void uvc_disconnect(struct usb_interface *intf)
> > > return;
> > >
> > > uvc_unregister_video(dev);
> > > - kref_put(&dev->ref, uvc_delete);
> > > + uvc_delete(dev);
> > > }
> > >
> > > static int uvc_suspend(struct usb_interface *intf, pm_message_t message)
> > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > index 07f9921d83f2..feb8de640a26 100644
> > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -578,7 +578,6 @@ struct uvc_device {
> > >
> > > /* Video Streaming interfaces */
> > > struct list_head streams;
> > > - struct kref ref;
> > >
> > > /* Status Interrupt Endpoint */
> > > struct usb_host_endpoint *int_ep;
> > >
> > > ---
> > > base-commit: c7ccf3683ac9746b263b0502255f5ce47f64fe0a
> > > change-id: 20241105-uvc-rmrefcount-010d98d496c5
--
Regards,
Laurent Pinchart
