* uvcvideo: Race on dev->state between uvc_disconnect() and uvc_v4l2_open()
@ 2015-05-20 14:48 Eugene Shatokhin
2015-05-24 22:32 ` Laurent Pinchart
0 siblings, 1 reply; 3+ messages in thread
From: Eugene Shatokhin @ 2015-05-20 14:48 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Mauro Carvalho Chehab, LKML
Hi,
There is a race in uvcvideo module between uvc_disconnect() and
uvc_v4l2_open() on dev->state. Checked and reproduced that with kernel
4.1-rc1.
drivers/media/usb/uvc/uvc_driver.c, uvc_disconnect():
dev->state |= UVC_DEV_DISCONNECTED;
drivers/media/usb/uvc/uvc_v4l2.c, uvc_v4l2_open():
if (stream->dev->state & UVC_DEV_DISCONNECTED)
return -ENODEV;
I checked that the race does happen by introducing a delay in
uvc_disconnect() right before that assignment and armed a hardware
breakpoint to detect the access to stream->dev->state from
uvc_v4l2_open(). When I disconnected the webcam while Google Hangout was
running, the hardware breakpoint triggered several times for that read
in uvc_v4l2_open (uvc_v4l2.c:484). uvc_v4l2_open() was called in the
context of GoogleTalkPlugin processes.
Not sure if the race is intentional but I guess, better to report it
anyway. Nothing has crashed during my (brief) testing yet, but still.
Regards,
Eugene
--
Eugene Shatokhin, ROSA
www.rosalab.com
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: uvcvideo: Race on dev->state between uvc_disconnect() and uvc_v4l2_open()
2015-05-20 14:48 uvcvideo: Race on dev->state between uvc_disconnect() and uvc_v4l2_open() Eugene Shatokhin
@ 2015-05-24 22:32 ` Laurent Pinchart
2015-05-25 6:31 ` Eugene Shatokhin
0 siblings, 1 reply; 3+ messages in thread
From: Laurent Pinchart @ 2015-05-24 22:32 UTC (permalink / raw)
To: Eugene Shatokhin; +Cc: Mauro Carvalho Chehab, LKML
Hi Eugene,
On Wednesday 20 May 2015 17:48:41 Eugene Shatokhin wrote:
> Hi,
>
> There is a race in uvcvideo module between uvc_disconnect() and
> uvc_v4l2_open() on dev->state. Checked and reproduced that with kernel
> 4.1-rc1.
>
> drivers/media/usb/uvc/uvc_driver.c, uvc_disconnect():
>
> dev->state |= UVC_DEV_DISCONNECTED;
>
> drivers/media/usb/uvc/uvc_v4l2.c, uvc_v4l2_open():
>
> if (stream->dev->state & UVC_DEV_DISCONNECTED)
> return -ENODEV;
>
> I checked that the race does happen by introducing a delay in
> uvc_disconnect() right before that assignment and armed a hardware
> breakpoint to detect the access to stream->dev->state from
> uvc_v4l2_open(). When I disconnected the webcam while Google Hangout was
> running, the hardware breakpoint triggered several times for that read
> in uvc_v4l2_open (uvc_v4l2.c:484). uvc_v4l2_open() was called in the
> context of GoogleTalkPlugin processes.
>
> Not sure if the race is intentional but I guess, better to report it
> anyway. Nothing has crashed during my (brief) testing yet, but still.
The race condition between disconnect and open is unavoidable. What is
avoidable, though, is the crashes and other ill side-effects that could result
from it. The following commit handles that.
commit ca9afe6f87b569cdf8e797395381f18ae23a2905
Author: Hans Verkuil <hverkuil@xs4all.nl>
Date: Fri Nov 26 06:54:53 2010 -0300
[media] v4l2-dev: fix race condition
The unregister function had a race condition with the v4l2_open
function. Ensure that both functions test and clear the REGISTER
flag from within a critical section.
Thanks to Laurent Pinchart for finding this race.
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
The race was previously handled by the uvcvideo driver, and the code got
removed in the following commit after the above commit got merged.
commit 716fdee110ceb816cca8c46c0890d08c5a1addb9
Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date: Tue Sep 29 21:07:19 2009 -0300
V4L/DVB (13152): uvcvideo: Rely on videodev to reference-count the device
The uvcvideo driver has a driver-wide lock and a reference count to
protect
against a disconnect/open race. Now that videodev handles the race itself,
reference-counting in the driver can be removed.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
Setting the UVC_DEV_DISCONNECTED flag seems unneeded nowadays. I'll have to
carefully think about it though, and it's too late right now to do so :-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: uvcvideo: Race on dev->state between uvc_disconnect() and uvc_v4l2_open()
2015-05-24 22:32 ` Laurent Pinchart
@ 2015-05-25 6:31 ` Eugene Shatokhin
0 siblings, 0 replies; 3+ messages in thread
From: Eugene Shatokhin @ 2015-05-25 6:31 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Mauro Carvalho Chehab, LKML
25.05.2015 01:32, Laurent Pinchart пишет:
> Hi Eugene,
>
> On Wednesday 20 May 2015 17:48:41 Eugene Shatokhin wrote:
>> Hi,
>>
>> There is a race in uvcvideo module between uvc_disconnect() and
>> uvc_v4l2_open() on dev->state. Checked and reproduced that with kernel
>> 4.1-rc1.
>>
>> drivers/media/usb/uvc/uvc_driver.c, uvc_disconnect():
>>
>> dev->state |= UVC_DEV_DISCONNECTED;
>>
>> drivers/media/usb/uvc/uvc_v4l2.c, uvc_v4l2_open():
>>
>> if (stream->dev->state & UVC_DEV_DISCONNECTED)
>> return -ENODEV;
>>
>> I checked that the race does happen by introducing a delay in
>> uvc_disconnect() right before that assignment and armed a hardware
>> breakpoint to detect the access to stream->dev->state from
>> uvc_v4l2_open(). When I disconnected the webcam while Google Hangout was
>> running, the hardware breakpoint triggered several times for that read
>> in uvc_v4l2_open (uvc_v4l2.c:484). uvc_v4l2_open() was called in the
>> context of GoogleTalkPlugin processes.
>>
>> Not sure if the race is intentional but I guess, better to report it
>> anyway. Nothing has crashed during my (brief) testing yet, but still.
>
> The race condition between disconnect and open is unavoidable. What is
> avoidable, though, is the crashes and other ill side-effects that could result
> from it. The following commit handles that.
>
> commit ca9afe6f87b569cdf8e797395381f18ae23a2905
> Author: Hans Verkuil <hverkuil@xs4all.nl>
> Date: Fri Nov 26 06:54:53 2010 -0300
>
> [media] v4l2-dev: fix race condition
>
> The unregister function had a race condition with the v4l2_open
> function. Ensure that both functions test and clear the REGISTER
> flag from within a critical section.
>
> Thanks to Laurent Pinchart for finding this race.
>
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>
>
> The race was previously handled by the uvcvideo driver, and the code got
> removed in the following commit after the above commit got merged.
>
> commit 716fdee110ceb816cca8c46c0890d08c5a1addb9
> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Date: Tue Sep 29 21:07:19 2009 -0300
>
> V4L/DVB (13152): uvcvideo: Rely on videodev to reference-count the device
>
> The uvcvideo driver has a driver-wide lock and a reference count to
> protect
> against a disconnect/open race. Now that videodev handles the race itself,
> reference-counting in the driver can be removed.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>
>
> Setting the UVC_DEV_DISCONNECTED flag seems unneeded nowadays. I'll have to
> carefully think about it though, and it's too late right now to do so :-)
>
I see. Thanks for the explanation!
Regards,
Eugene
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-05-25 6:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-20 14:48 uvcvideo: Race on dev->state between uvc_disconnect() and uvc_v4l2_open() Eugene Shatokhin
2015-05-24 22:32 ` Laurent Pinchart
2015-05-25 6:31 ` Eugene Shatokhin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.