Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751522AbbEXWcW (ORCPT ); Sun, 24 May 2015 18:32:22 -0400 Received: from galahad.ideasonboard.com ([185.26.127.97]:38704 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750980AbbEXWcU (ORCPT ); Sun, 24 May 2015 18:32:20 -0400 From: Laurent Pinchart To: Eugene Shatokhin Cc: Mauro Carvalho Chehab , LKML Subject: Re: uvcvideo: Race on dev->state between uvc_disconnect() and uvc_v4l2_open() Date: Mon, 25 May 2015 01:32:39 +0300 Message-ID: <3959395.nkzPWg6atK@avalon> User-Agent: KMail/4.14.3 (Linux/3.18.11-gentoo; KDE/4.14.3; x86_64; ; ) In-Reply-To: <555C9EC9.40303@rosalab.ru> References: <555C9EC9.40303@rosalab.ru> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2952 Lines: 80 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 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 Signed-off-by: Mauro Carvalho Chehab 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 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 Signed-off-by: Mauro Carvalho Chehab 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/