Remove locks calls in usbtv_video_free() because
are useless and may led to a deadlock as reported here:
https://syzkaller.appspot.com/x/bisect.txt?x=166dc872180000
Signed-off-by: Benjamin Gaignard <[email protected]>
Fixes: c838530d230b ("media: media videobuf2: Be more flexible on the number of queue stored buffers")
---
drivers/media/usb/usbtv/usbtv-video.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
index 62a583040cd4..96276358d116 100644
--- a/drivers/media/usb/usbtv/usbtv-video.c
+++ b/drivers/media/usb/usbtv/usbtv-video.c
@@ -963,15 +963,9 @@ int usbtv_video_init(struct usbtv *usbtv)
void usbtv_video_free(struct usbtv *usbtv)
{
- mutex_lock(&usbtv->vb2q_lock);
- mutex_lock(&usbtv->v4l2_lock);
-
usbtv_stop(usbtv);
vb2_video_unregister_device(&usbtv->vdev);
v4l2_device_disconnect(&usbtv->v4l2_dev);
- mutex_unlock(&usbtv->v4l2_lock);
- mutex_unlock(&usbtv->vb2q_lock);
-
v4l2_device_put(&usbtv->v4l2_dev);
}
--
2.40.1
On Tue, Feb 20, 2024 at 5:06 PM Benjamin Gaignard
<[email protected]> wrote:
>
> Remove locks calls in usbtv_video_free() because
> are useless and may led to a deadlock as reported here:
> https://syzkaller.appspot.com/x/bisect.txt?x=166dc872180000
>
> Signed-off-by: Benjamin Gaignard <[email protected]>
> Fixes: c838530d230b ("media: media videobuf2: Be more flexible on the number of queue stored buffers")
> ---
> drivers/media/usb/usbtv/usbtv-video.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
> index 62a583040cd4..96276358d116 100644
> --- a/drivers/media/usb/usbtv/usbtv-video.c
> +++ b/drivers/media/usb/usbtv/usbtv-video.c
> @@ -963,15 +963,9 @@ int usbtv_video_init(struct usbtv *usbtv)
>
> void usbtv_video_free(struct usbtv *usbtv)
> {
> - mutex_lock(&usbtv->vb2q_lock);
> - mutex_lock(&usbtv->v4l2_lock);
> -
> usbtv_stop(usbtv);
> vb2_video_unregister_device(&usbtv->vdev);
> v4l2_device_disconnect(&usbtv->v4l2_dev);
>
> - mutex_unlock(&usbtv->v4l2_lock);
> - mutex_unlock(&usbtv->vb2q_lock);
> -
> v4l2_device_put(&usbtv->v4l2_dev);
> }
> --
> 2.40.1
>
From looking at the code in related vb2 functions and how the uvc
driver handles the cleanup, these locks shouldn't be necessary indeed,
but it would be good if someone could test this.
Reviewed-by: Tomasz Figa <[email protected]>
Best regards,
Tomasz
On 20/02/2024 09:06, Benjamin Gaignard wrote:
> Remove locks calls in usbtv_video_free() because
> are useless and may led to a deadlock as reported here:
> https://syzkaller.appspot.com/x/bisect.txt?x=166dc872180000
>
> Signed-off-by: Benjamin Gaignard <[email protected]>
> Fixes: c838530d230b ("media: media videobuf2: Be more flexible on the number of queue stored buffers")
Hmm, the code was really always wrong, it is just that before this commit
it would only fail if you did a disconnect while streaming, and now it also
fails if you disconnect when you are not streaming, so it is much more
noticeable now.
That should probably be explained better in the commit log. You could add a
second Fixes line:
Fixes: f3d27f34fdd7 ("[media] usbtv: Add driver for Fushicai USBTV007 video frame grabber")
but I am not sure how useful that is.
> ---
> drivers/media/usb/usbtv/usbtv-video.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
> index 62a583040cd4..96276358d116 100644
> --- a/drivers/media/usb/usbtv/usbtv-video.c
> +++ b/drivers/media/usb/usbtv/usbtv-video.c
> @@ -963,15 +963,9 @@ int usbtv_video_init(struct usbtv *usbtv)
>
> void usbtv_video_free(struct usbtv *usbtv)
> {
> - mutex_lock(&usbtv->vb2q_lock);
> - mutex_lock(&usbtv->v4l2_lock);
> -
> usbtv_stop(usbtv);
This call should also be dropped...
> vb2_video_unregister_device(&usbtv->vdev);
..since this function will call stop_streaming if streaming is in progress,
which will call usbtv_stop with all the correct locks held.
Regards,
Hans
> v4l2_device_disconnect(&usbtv->v4l2_dev);
>
> - mutex_unlock(&usbtv->v4l2_lock);
> - mutex_unlock(&usbtv->vb2q_lock);
> -
> v4l2_device_put(&usbtv->v4l2_dev);
> }
On 28/02/2024 09:45, Hans Verkuil wrote:
> On 20/02/2024 09:06, Benjamin Gaignard wrote:
>> Remove locks calls in usbtv_video_free() because
>> are useless and may led to a deadlock as reported here:
>> https://syzkaller.appspot.com/x/bisect.txt?x=166dc872180000
>>
>> Signed-off-by: Benjamin Gaignard <[email protected]>
>> Fixes: c838530d230b ("media: media videobuf2: Be more flexible on the number of queue stored buffers")
>
> Hmm, the code was really always wrong, it is just that before this commit
> it would only fail if you did a disconnect while streaming, and now it also
> fails if you disconnect when you are not streaming, so it is much more
> noticeable now.
>
> That should probably be explained better in the commit log. You could add a
> second Fixes line:
>
> Fixes: f3d27f34fdd7 ("[media] usbtv: Add driver for Fushicai USBTV007 video frame grabber")
>
> but I am not sure how useful that is.
>
>> ---
>> drivers/media/usb/usbtv/usbtv-video.c | 6 ------
>> 1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
>> index 62a583040cd4..96276358d116 100644
>> --- a/drivers/media/usb/usbtv/usbtv-video.c
>> +++ b/drivers/media/usb/usbtv/usbtv-video.c
>> @@ -963,15 +963,9 @@ int usbtv_video_init(struct usbtv *usbtv)
>>
>> void usbtv_video_free(struct usbtv *usbtv)
>> {
>> - mutex_lock(&usbtv->vb2q_lock);
>> - mutex_lock(&usbtv->v4l2_lock);
>> -
>> usbtv_stop(usbtv);
>
> This call should also be dropped...
>
>> vb2_video_unregister_device(&usbtv->vdev);
>
> ...since this function will call stop_streaming if streaming is in progress,
> which will call usbtv_stop with all the correct locks held.
With that change you can add my:
Tested-by: Hans Verkuil <[email protected]>
Regards,
Hans
>
> Regards,
>
> Hans
>
>> v4l2_device_disconnect(&usbtv->v4l2_dev);
>>
>> - mutex_unlock(&usbtv->v4l2_lock);
>> - mutex_unlock(&usbtv->vb2q_lock);
>> -
>> v4l2_device_put(&usbtv->v4l2_dev);
>> }
>
>