[v4,1/4] media: uvcvideo: stop stream during unregister

Message ID 20240327-guenter-mini-v4-1-49955c198eae@chromium.org (mailing list archive)
State Superseded
Delegated to: Laurent Pinchart
Headers
Series uvcvideo: Attempt N to land UVC race conditions fixes |

Commit Message

Ricardo Ribalda March 27, 2024, 8:24 a.m. UTC
  uvc_unregister_video() can be called asynchronously from
uvc_disconnect(). If the device is still streaming when that happens, a
plethora of race conditions can happen.

Make sure that the device has stopped streaming before exiting this
function.

If the user still holds handles to the driver's file descriptors, any
ioctl will return -ENODEV from the v4l2 core.

This change make uvc more consistent with the rest of the v4l2 drivers
using the vb2_fop_* and vb2_ioctl_* helpers.

Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
  

Comments

Hans Verkuil May 28, 2024, 7:55 a.m. UTC | #1
On 27/03/2024 09:24, Ricardo Ribalda wrote:
> uvc_unregister_video() can be called asynchronously from
> uvc_disconnect(). If the device is still streaming when that happens, a
> plethora of race conditions can happen.
> 
> Make sure that the device has stopped streaming before exiting this
> function.
> 
> If the user still holds handles to the driver's file descriptors, any
> ioctl will return -ENODEV from the v4l2 core.
> 
> This change make uvc more consistent with the rest of the v4l2 drivers
> using the vb2_fop_* and vb2_ioctl_* helpers.
> 
> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index bbd90123a4e76..17fc945c8deb6 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev)
>  		if (!video_is_registered(&stream->vdev))
>  			continue;
>  
> +		/*
> +		 * Serialize other access to the stream.
> +		 */
> +		mutex_lock(&stream->mutex);
> +		uvc_queue_streamoff(&stream->queue, stream->type);
>  		video_unregister_device(&stream->vdev);
>  		video_unregister_device(&stream->meta.vdev);
> +		mutex_unlock(&stream->mutex);

This sequence isn't fool proof. You have to follow the same sequence as
vb2_video_unregister_device(): take a reference to the video device,
then unregister, then take the stream mutex and call vb2_queue_release
for each queue. Finally unlock the mutex and call put_device.

Doing the video_unregister_device first ensures no new ioctls can be
called on that device node. Taking the mutex ensures that any still
running ioctls will finish (since it will sleep until they release the
mutex), and then you can release the queue.

This does require that you call get_device before calling video_unregister_device,
otherwise everything might be released at that point.

Instead of vb2_queue_release() you might have to call uvc_queue_streamoff,
but note that there are two queues here: video and meta. The code above
just calls streamoff for the video device.

For the meta device I think you can just use vb2_video_unregister_device()
directly, since that sets vdev->queue and vdev->queue.lock to the correct
values. That would just leave the video device where you need to do this
manually.

Regards,

	Hans

> +
> +		/*
> +		 * Now the vdev is not streaming and all the ioctls will
> +		 * return -ENODEV
> +		 */
>  
>  		uvc_debugfs_cleanup_stream(stream);
>  	}
>
  
Tomasz Figa June 6, 2024, 9:57 a.m. UTC | #2
Hi Ricardo,

On Wed, Mar 27, 2024 at 5:24 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> uvc_unregister_video() can be called asynchronously from
> uvc_disconnect(). If the device is still streaming when that happens, a
> plethora of race conditions can happen.
>
> Make sure that the device has stopped streaming before exiting this
> function.
>
> If the user still holds handles to the driver's file descriptors, any
> ioctl will return -ENODEV from the v4l2 core.
>
> This change make uvc more consistent with the rest of the v4l2 drivers
> using the vb2_fop_* and vb2_ioctl_* helpers.
>
> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>

First of all, thanks for the patch. I have a question about the
problem being fixed here.

Could you point out a specific race condition example that could
happen without this change?
From what I see in __video_do_ioctl((), no ioctls would be executed
anymore after the video node is unregistered.
Since the device is not present either, what asynchronous code paths
could be still triggered?

[1] https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-ioctl.c#L3023

Best regards,
Tomasz

> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index bbd90123a4e76..17fc945c8deb6 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev)
>                 if (!video_is_registered(&stream->vdev))
>                         continue;
>
> +               /*
> +                * Serialize other access to the stream.
> +                */
> +               mutex_lock(&stream->mutex);
> +               uvc_queue_streamoff(&stream->queue, stream->type);
>                 video_unregister_device(&stream->vdev);
>                 video_unregister_device(&stream->meta.vdev);
> +               mutex_unlock(&stream->mutex);
> +
> +               /*
> +                * Now the vdev is not streaming and all the ioctls will
> +                * return -ENODEV
> +                */
>
>                 uvc_debugfs_cleanup_stream(stream);
>         }
>
> --
> 2.44.0.396.g6e790dbe36-goog
>
  
Tomasz Figa June 6, 2024, 10:04 a.m. UTC | #3
Hi Hans,

On Tue, May 28, 2024 at 4:55 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 27/03/2024 09:24, Ricardo Ribalda wrote:
> > uvc_unregister_video() can be called asynchronously from
> > uvc_disconnect(). If the device is still streaming when that happens, a
> > plethora of race conditions can happen.
> >
> > Make sure that the device has stopped streaming before exiting this
> > function.
> >
> > If the user still holds handles to the driver's file descriptors, any
> > ioctl will return -ENODEV from the v4l2 core.
> >
> > This change make uvc more consistent with the rest of the v4l2 drivers
> > using the vb2_fop_* and vb2_ioctl_* helpers.
> >
> > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index bbd90123a4e76..17fc945c8deb6 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev)
> >               if (!video_is_registered(&stream->vdev))
> >                       continue;
> >
> > +             /*
> > +              * Serialize other access to the stream.
> > +              */
> > +             mutex_lock(&stream->mutex);
> > +             uvc_queue_streamoff(&stream->queue, stream->type);
> >               video_unregister_device(&stream->vdev);
> >               video_unregister_device(&stream->meta.vdev);
> > +             mutex_unlock(&stream->mutex);
>
> This sequence isn't fool proof. You have to follow the same sequence as
> vb2_video_unregister_device(): take a reference to the video device,
> then unregister, then take the stream mutex and call vb2_queue_release
> for each queue. Finally unlock the mutex and call put_device.

vb2_queue_release() will run when the userspace releases the file
handle that owns the vb2 queue [1], so we shouldn't really need to
call it here.

[1] https://elixir.bootlin.com/linux/v6.9.3/source/drivers/media/usb/uvc/uvc_v4l2.c#L655

>
> Doing the video_unregister_device first ensures no new ioctls can be
> called on that device node. Taking the mutex ensures that any still
> running ioctls will finish (since it will sleep until they release the
> mutex), and then you can release the queue.

Actually isn't the only missing thing in the code basically ensuring
that any ioctl currently being executed finishes? Why is the streamoff
or queue release needed?

Best regards,
Tomasz

>
> This does require that you call get_device before calling video_unregister_device,
> otherwise everything might be released at that point.
>
> Instead of vb2_queue_release() you might have to call uvc_queue_streamoff,
> but note that there are two queues here: video and meta. The code above
> just calls streamoff for the video device.
>
> For the meta device I think you can just use vb2_video_unregister_device()
> directly, since that sets vdev->queue and vdev->queue.lock to the correct
> values. That would just leave the video device where you need to do this
> manually.
>
> Regards,
>
>         Hans
>
> > +
> > +             /*
> > +              * Now the vdev is not streaming and all the ioctls will
> > +              * return -ENODEV
> > +              */
> >
> >               uvc_debugfs_cleanup_stream(stream);
> >       }
> >
>
  
Hans Verkuil June 6, 2024, 11:57 a.m. UTC | #4
On 06/06/2024 12:04, Tomasz Figa wrote:
> Hi Hans,
> 
> On Tue, May 28, 2024 at 4:55 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>
>> On 27/03/2024 09:24, Ricardo Ribalda wrote:
>>> uvc_unregister_video() can be called asynchronously from
>>> uvc_disconnect(). If the device is still streaming when that happens, a
>>> plethora of race conditions can happen.
>>>
>>> Make sure that the device has stopped streaming before exiting this
>>> function.
>>>
>>> If the user still holds handles to the driver's file descriptors, any
>>> ioctl will return -ENODEV from the v4l2 core.
>>>
>>> This change make uvc more consistent with the rest of the v4l2 drivers
>>> using the vb2_fop_* and vb2_ioctl_* helpers.
>>>
>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>> ---
>>>  drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>>> index bbd90123a4e76..17fc945c8deb6 100644
>>> --- a/drivers/media/usb/uvc/uvc_driver.c
>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>>> @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev)
>>>               if (!video_is_registered(&stream->vdev))
>>>                       continue;
>>>
>>> +             /*
>>> +              * Serialize other access to the stream.
>>> +              */
>>> +             mutex_lock(&stream->mutex);
>>> +             uvc_queue_streamoff(&stream->queue, stream->type);
>>>               video_unregister_device(&stream->vdev);
>>>               video_unregister_device(&stream->meta.vdev);
>>> +             mutex_unlock(&stream->mutex);
>>
>> This sequence isn't fool proof. You have to follow the same sequence as
>> vb2_video_unregister_device(): take a reference to the video device,
>> then unregister, then take the stream mutex and call vb2_queue_release
>> for each queue. Finally unlock the mutex and call put_device.
> 
> vb2_queue_release() will run when the userspace releases the file
> handle that owns the vb2 queue [1], so we shouldn't really need to
> call it here.
> 
> [1] https://elixir.bootlin.com/linux/v6.9.3/source/drivers/media/usb/uvc/uvc_v4l2.c#L655
> 
>>
>> Doing the video_unregister_device first ensures no new ioctls can be
>> called on that device node. Taking the mutex ensures that any still
>> running ioctls will finish (since it will sleep until they release the
>> mutex), and then you can release the queue.
> 
> Actually isn't the only missing thing in the code basically ensuring
> that any ioctl currently being executed finishes? Why is the streamoff
> or queue release needed?

See below...

> 
> Best regards,
> Tomasz
> 
>>
>> This does require that you call get_device before calling video_unregister_device,
>> otherwise everything might be released at that point.
>>
>> Instead of vb2_queue_release() you might have to call uvc_queue_streamoff,
>> but note that there are two queues here: video and meta. The code above
>> just calls streamoff for the video device.
>>
>> For the meta device I think you can just use vb2_video_unregister_device()
>> directly, since that sets vdev->queue and vdev->queue.lock to the correct
>> values. That would just leave the video device where you need to do this
>> manually.

Ideally uvc should just use centralized locking (i.e. set vdev->queue.lock)
for the video node, just like it does for the meta device.

If that was the case, then it can just call vb2_video_unregister_device().
The vb2_video_unregister_device helper was added to ensure that no ioctls
are running, and then streaming is stopped and the queue is released.

While it is true that the queue is released automatically when the last user
closes the filehandle, it can get complicated if the application has a file
handle open when the device is unregistered (usually due to it being unplugged).
Without that call to vb2_video_unregister_device() the queue is still active,
and especially if you also have subdevices that are still in streaming mode,
it is hard to make sure nothing is still using the hardware.

vb2_video_unregister_device() provides a clean way of ensuring that when the
device is unregistered all streaming is stopped and the vb2 queue is released.

After that the only file operation that can be used without returning an error
is close().

Since uvc uses its own locking for the video device, it has to do this manually.
It is probably enough to ensure no ioctls are running since uvc doesn't have
subdevices, but I think it makes sense to stop streaming and release the queue
when unregistering the device, it is weird to postpone that until the last fh
is closed.

Regards,

	Hans


>>
>> Regards,
>>
>>         Hans
>>
>>> +
>>> +             /*
>>> +              * Now the vdev is not streaming and all the ioctls will
>>> +              * return -ENODEV
>>> +              */
>>>
>>>               uvc_debugfs_cleanup_stream(stream);
>>>       }
>>>
>>
  
Tomasz Figa June 12, 2024, 3:25 a.m. UTC | #5
On Thu, Jun 6, 2024 at 8:58 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 06/06/2024 12:04, Tomasz Figa wrote:
> > Hi Hans,
> >
> > On Tue, May 28, 2024 at 4:55 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >>
> >> On 27/03/2024 09:24, Ricardo Ribalda wrote:
> >>> uvc_unregister_video() can be called asynchronously from
> >>> uvc_disconnect(). If the device is still streaming when that happens, a
> >>> plethora of race conditions can happen.
> >>>
> >>> Make sure that the device has stopped streaming before exiting this
> >>> function.
> >>>
> >>> If the user still holds handles to the driver's file descriptors, any
> >>> ioctl will return -ENODEV from the v4l2 core.
> >>>
> >>> This change make uvc more consistent with the rest of the v4l2 drivers
> >>> using the vb2_fop_* and vb2_ioctl_* helpers.
> >>>
> >>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>> ---
> >>>  drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> >>> index bbd90123a4e76..17fc945c8deb6 100644
> >>> --- a/drivers/media/usb/uvc/uvc_driver.c
> >>> +++ b/drivers/media/usb/uvc/uvc_driver.c
> >>> @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev)
> >>>               if (!video_is_registered(&stream->vdev))
> >>>                       continue;
> >>>
> >>> +             /*
> >>> +              * Serialize other access to the stream.
> >>> +              */
> >>> +             mutex_lock(&stream->mutex);
> >>> +             uvc_queue_streamoff(&stream->queue, stream->type);
> >>>               video_unregister_device(&stream->vdev);
> >>>               video_unregister_device(&stream->meta.vdev);
> >>> +             mutex_unlock(&stream->mutex);
> >>
> >> This sequence isn't fool proof. You have to follow the same sequence as
> >> vb2_video_unregister_device(): take a reference to the video device,
> >> then unregister, then take the stream mutex and call vb2_queue_release
> >> for each queue. Finally unlock the mutex and call put_device.
> >
> > vb2_queue_release() will run when the userspace releases the file
> > handle that owns the vb2 queue [1], so we shouldn't really need to
> > call it here.
> >
> > [1] https://elixir.bootlin.com/linux/v6.9.3/source/drivers/media/usb/uvc/uvc_v4l2.c#L655
> >
> >>
> >> Doing the video_unregister_device first ensures no new ioctls can be
> >> called on that device node. Taking the mutex ensures that any still
> >> running ioctls will finish (since it will sleep until they release the
> >> mutex), and then you can release the queue.
> >
> > Actually isn't the only missing thing in the code basically ensuring
> > that any ioctl currently being executed finishes? Why is the streamoff
> > or queue release needed?
>
> See below...
>
> >
> > Best regards,
> > Tomasz
> >
> >>
> >> This does require that you call get_device before calling video_unregister_device,
> >> otherwise everything might be released at that point.
> >>
> >> Instead of vb2_queue_release() you might have to call uvc_queue_streamoff,
> >> but note that there are two queues here: video and meta. The code above
> >> just calls streamoff for the video device.
> >>
> >> For the meta device I think you can just use vb2_video_unregister_device()
> >> directly, since that sets vdev->queue and vdev->queue.lock to the correct
> >> values. That would just leave the video device where you need to do this
> >> manually.
>
> Ideally uvc should just use centralized locking (i.e. set vdev->queue.lock)
> for the video node, just like it does for the meta device.
>
> If that was the case, then it can just call vb2_video_unregister_device().
> The vb2_video_unregister_device helper was added to ensure that no ioctls
> are running, and then streaming is stopped and the queue is released.

Yes, it would be as simple as that if it used standard locking, but
since it does its own stuff, I'm not very confident that the same
logic as in vb2_video_unregister_device() would work fine for it as
well.

>
> While it is true that the queue is released automatically when the last user
> closes the filehandle, it can get complicated if the application has a file
> handle open when the device is unregistered (usually due to it being unplugged).
> Without that call to vb2_video_unregister_device() the queue is still active,
> and especially if you also have subdevices that are still in streaming mode,
> it is hard to make sure nothing is still using the hardware.
>
> vb2_video_unregister_device() provides a clean way of ensuring that when the
> device is unregistered all streaming is stopped and the vb2 queue is released.
>
> After that the only file operation that can be used without returning an error
> is close().
>
> Since uvc uses its own locking for the video device, it has to do this manually.
> It is probably enough to ensure no ioctls are running since uvc doesn't have
> subdevices, but I think it makes sense to stop streaming and release the queue
> when unregistering the device, it is weird to postpone that until the last fh
> is closed.

My comment comes from a concern that releasing objects and changing
internal state in the release callback may break some assumptions in
the operations that will follow once the lock is released, e.g.
close(), leading to some kind of double frees or dereferencing freed
objects. Therefore just preventing new opens and setting the
video_device as unregistered could be potentially a safer option until
the driver is converted to standard locking. (Is there even a plan to
do that?)

Best regards,
Tomasz

>
> Regards,
>
>         Hans
>
>
> >>
> >> Regards,
> >>
> >>         Hans
> >>
> >>> +
> >>> +             /*
> >>> +              * Now the vdev is not streaming and all the ioctls will
> >>> +              * return -ENODEV
> >>> +              */
> >>>
> >>>               uvc_debugfs_cleanup_stream(stream);
> >>>       }
> >>>
> >>
>
>
  

Patch

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index bbd90123a4e76..17fc945c8deb6 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1911,8 +1911,19 @@  static void uvc_unregister_video(struct uvc_device *dev)
 		if (!video_is_registered(&stream->vdev))
 			continue;
 
+		/*
+		 * Serialize other access to the stream.
+		 */
+		mutex_lock(&stream->mutex);
+		uvc_queue_streamoff(&stream->queue, stream->type);
 		video_unregister_device(&stream->vdev);
 		video_unregister_device(&stream->meta.vdev);
+		mutex_unlock(&stream->mutex);
+
+		/*
+		 * Now the vdev is not streaming and all the ioctls will
+		 * return -ENODEV
+		 */
 
 		uvc_debugfs_cleanup_stream(stream);
 	}