| Message ID | 20240220080628.13141-1-benjamin.gaignard@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: usbtv: Remove useless locks in usbtv_video_free() | expand |
On Tue, Feb 20, 2024 at 5:06 PM Benjamin Gaignard <benjamin.gaignard@collabora.com> 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 <benjamin.gaignard@collabora.com> > 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 <tfiga@chromium.org> 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 <benjamin.gaignard@collabora.com> > 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 <benjamin.gaignard@collabora.com> >> 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 <hverkuil-cisco@xs4all.nl> 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); >> } > >
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); }
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 <benjamin.gaignard@collabora.com> 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(-)