Message ID | 526256F5.1060404@gmail.com (mailing list archive) |
---|---|
State | RFC, archived |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1VXTG0-0002wJ-5h; Sat, 19 Oct 2013 11:55:40 +0200 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.72/mailfrontend-6) with esmtp id 1VXTFx-0003oL-5o; Sat, 19 Oct 2013 11:55:40 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751084Ab3JSJze (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Sat, 19 Oct 2013 05:55:34 -0400 Received: from mail-wi0-f171.google.com ([209.85.212.171]:39236 "EHLO mail-wi0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750843Ab3JSJzG (ORCPT <rfc822;linux-media@vger.kernel.org>); Sat, 19 Oct 2013 05:55:06 -0400 Received: by mail-wi0-f171.google.com with SMTP id h11so2021879wiv.10 for <multiple recipients>; Sat, 19 Oct 2013 02:55:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=dosy+hyjKNGvfRdj9wrusrQfOR8oBTnUe6ng22wHcnY=; b=mQPqh+sC+KRjLD+gEHPYQDdopZmk4ShwTdqR0iG9if0HVgfx6aC4ZtAc3lTAQlroqA CedOrvEiWN6wmi47TMAsEvZimpaH0rRKMJiKmIO2dx7YA9DoCqEFMlIetgXHIVHqAohx E7UpgoKVAgm++MDbBPKTXs5hEBsh2kMN7DJ1wAHggYe8RW1Ua/8zEEiSOjInrIr6y1JT cJMVPkHwYFG3MGVG0E5yCXalqssX1y7vmQ+ovGviuJxZ3Y8/Ul8fWR/HpV89EPIukRQx DOdDfOB2BTp1rHMN4yY8I4IpZhWyQEnlrNGruRWcGm2szJN38ZOXoPpPvTOPXNfQ5Vbs 6cEg== X-Received: by 10.180.89.225 with SMTP id br1mr2670732wib.50.1382176505259; Sat, 19 Oct 2013 02:55:05 -0700 (PDT) Received: from [192.168.1.110] (093105185086.warszawa.vectranet.pl. [93.105.185.86]) by mx.google.com with ESMTPSA id ev4sm35137192wib.7.2013.10.19.02.55.03 for <multiple recipients> (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Sat, 19 Oct 2013 02:55:04 -0700 (PDT) Message-ID: <526256F5.1060404@gmail.com> Date: Sat, 19 Oct 2013 11:55:01 +0200 From: Sylwester Nawrocki <sylvester.nawrocki@gmail.com> User-Agent: Mozilla/5.0 (X11; Linux i686; rv:11.0) Gecko/20120412 Thunderbird/11.0.1 MIME-Version: 1.0 To: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> CC: Pawel Osciak <pawel@osciak.com>, Marek Szyprowski <m.szyprowski@samsung.com>, Kyungmin Park <kyungmin.park@samsung.com>, Mauro Carvalho Chehab <m.chehab@samsung.com>, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, Hans Verkuil <hverkuil@xs4all.nl> Subject: Re: [PATCH] videobuf2: Add missing lock held on vb2_fop_relase References: <Hans Verkuil <hverkuil@xs4all.nl> <1381736489-27852-1-git-send-email-ricardo.ribalda@gmail.com> In-Reply-To: <1381736489-27852-1-git-send-email-ricardo.ribalda@gmail.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2013.10.19.94814 X-PMX-Spam: Gauge=IIIIIIIII, Probability=9%, Report=' FORGED_FROM_GMAIL 0.1, MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODY_SIZE_4000_4999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, DKIM_SIGNATURE 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __BOUNCE_CHALLENGE_SUBJ 0, __BOUNCE_NDR_SUBJ_EXEMPT 0, __CP_MEDIA_BODY 0, __CP_URI_IN_BODY 0, __CT 0, __CTE 0, __CT_TEXT_PLAIN 0, __FORWARDED_MSG 0, __FRAUD_WEBMAIL 0, __FRAUD_WEBMAIL_FROM 0, __FROM_GMAIL 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILING_LIST 0, __IN_REP_TO 0, __MIME_TEXT_ONLY 0, __MIME_VERSION 0, __MOZILLA_MSGID 0, __MOZILLA_USER_AGENT 0, __MULTIPLE_RCPTS_CC_X2 0, __PHISH_SPEAR_STRUCTURE_1 0, __SANE_MSGID 0, __SUBJ_ALPHA_NEGATE 0, __TO_MALFORMED_2 0, __URI_NO_WWW 0, __URI_NS , __USER_AGENT 0, __YOUTUBE_RCVD 0' |
Commit Message
Sylwester Nawrocki
Oct. 19, 2013, 9:55 a.m. UTC
Hi Ricardo, On 10/14/2013 09:41 AM, Ricardo Ribalda Delgado wrote: > vb2_fop_relase does not held the lock although it is modifying the > queue->owner field. [...] > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > index 9fc4bab..3a961ee 100644 > --- a/drivers/media/v4l2-core/videobuf2-core.c > +++ b/drivers/media/v4l2-core/videobuf2-core.c > @@ -2588,8 +2588,15 @@ int vb2_fop_release(struct file *file) > struct video_device *vdev = video_devdata(file); > > if (file->private_data == vdev->queue->owner) { > + struct mutex *lock; > + > + lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock; > + if (lock) > + mutex_lock(lock); > vb2_queue_release(vdev->queue); > vdev->queue->owner = NULL; > + if (lock) > + mutex_unlock(lock); > } > return v4l2_fh_release(file); > } It seems you didn't inspect all users of vb2_fop_release(). There are 3 drivers that don't assign vb2_fop_release() to struct v4l2_file_operations directly but instead call it from within its own release() handler. Two of them do call vb2_fop_release() with the video queue lock already held. $ git grep -n vb2_fop_rel -- drivers/media/ drivers/media/platform/exynos4-is/fimc-capture.c:552: ret = vb2_fop_release(file); drivers/media/platform/exynos4-is/fimc-lite.c:549: vb2_fop_release(file); A rather ugly solution would be to open code the vb2_fop_release() function in those driver, like in below patch (untested). Unless there are better proposals I would queue the patch as below together with the $subject patch upstream. From 3617684d759bb021e3cf1d862a91cb6e18d12052 Mon Sep 17 00:00:00 2001 From: Sylwester Nawrocki <s.nawrocki@samsung.com> Date: Sat, 19 Oct 2013 11:48:10 +0200 Subject: [PATCH] exynos4-is: Do not call vb2_fop_release() with queue lock held Currently vb2_fop_release() function doesn't take the queue lock, but it is going to change and then there would happen a deadlock in fimc_capture_release() and fimc_lite_release(), since these function take the video queue lock prior to calling vb2_fop_release(). To avoid a deadlock open code the vb2_fop_release() function in those drivers. Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> --- drivers/media/platform/exynos4-is/fimc-capture.c | 11 ++++++++--- drivers/media/platform/exynos4-is/fimc-lite.c | 8 +++++++- 2 files changed, 15 insertions(+), 4 deletions(-)
Comments
Hello Sylwester On Sat, Oct 19, 2013 at 11:55 AM, Sylwester Nawrocki <sylvester.nawrocki@gmail.com> wrote: > Hi Ricardo, > > > On 10/14/2013 09:41 AM, Ricardo Ribalda Delgado wrote: >> >> vb2_fop_relase does not held the lock although it is modifying the >> queue->owner field. > > [...] > >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c >> b/drivers/media/v4l2-core/videobuf2-core.c >> index 9fc4bab..3a961ee 100644 >> --- a/drivers/media/v4l2-core/videobuf2-core.c >> +++ b/drivers/media/v4l2-core/videobuf2-core.c >> @@ -2588,8 +2588,15 @@ int vb2_fop_release(struct file *file) >> struct video_device *vdev = video_devdata(file); >> >> if (file->private_data == vdev->queue->owner) { >> + struct mutex *lock; >> + >> + lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock; >> + if (lock) >> + mutex_lock(lock); >> vb2_queue_release(vdev->queue); >> vdev->queue->owner = NULL; >> + if (lock) >> + mutex_unlock(lock); >> } >> return v4l2_fh_release(file); >> } > > > It seems you didn't inspect all users of vb2_fop_release(). There are 3 > drivers > that don't assign vb2_fop_release() to struct v4l2_file_operations directly > but > instead call it from within its own release() handler. Two of them do call > vb2_fop_release() with the video queue lock already held. > > $ git grep -n vb2_fop_rel -- drivers/media/ > > drivers/media/platform/exynos4-is/fimc-capture.c:552: ret = > vb2_fop_release(file); > drivers/media/platform/exynos4-is/fimc-lite.c:549: vb2_fop_release(file); > Very good catch, thanks! > A rather ugly solution would be to open code the vb2_fop_release() function > in those driver, like in below patch (untested). Unless there are better > proposals I would queue the patch as below together with the $subject patch > upstream. IMHO this will lead to the same type of mistakes in the future. What about creating a function __vb2_fop_release that does exactly the same as the original function but with an extra parameter bool lock_held vb2_fop_release will be a wrapper for that funtion with lock_held== false drivers that overload the fop_release and need to hold the lock will call the __ function with lock_held= true What do you think? Thanks! > > > From 3617684d759bb021e3cf1d862a91cb6e18d12052 Mon Sep 17 00:00:00 2001 > From: Sylwester Nawrocki <s.nawrocki@samsung.com> > Date: Sat, 19 Oct 2013 11:48:10 +0200 > Subject: [PATCH] exynos4-is: Do not call vb2_fop_release() with queue lock > held > > Currently vb2_fop_release() function doesn't take the queue lock, > but it is going to change and then there would happen a deadlock > in fimc_capture_release() and fimc_lite_release(), since these > function take the video queue lock prior to calling vb2_fop_release(). > > To avoid a deadlock open code the vb2_fop_release() function in those > drivers. > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > --- > drivers/media/platform/exynos4-is/fimc-capture.c | 11 ++++++++--- > drivers/media/platform/exynos4-is/fimc-lite.c | 8 +++++++- > 2 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c > b/drivers/media/platform/exynos4-is/fimc-capture.c > index fb27ff7..e9a5c90 100644 > --- a/drivers/media/platform/exynos4-is/fimc-capture.c > +++ b/drivers/media/platform/exynos4-is/fimc-capture.c > @@ -537,6 +537,7 @@ static int fimc_capture_release(struct file *file) > { > struct fimc_dev *fimc = video_drvdata(file); > struct fimc_vid_cap *vc = &fimc->vid_cap; > + struct video_device *vdev = &vc->ve.vdev; > bool close = v4l2_fh_is_singular_file(file); > int ret; > > @@ -545,11 +546,15 @@ static int fimc_capture_release(struct file *file) > mutex_lock(&fimc->lock); > > if (close && vc->streaming) { > - media_entity_pipeline_stop(&vc->ve.vdev.entity); > + media_entity_pipeline_stop(&vdev->entity); > vc->streaming = false; > } > > - ret = vb2_fop_release(file); > + if (file->private_data == vdev->queue->owner) { > + vb2_queue_release(vdev->queue); > + vdev->queue->owner = NULL; > + } > + ret = v4l2_fh_release(file); > > if (close) { > clear_bit(ST_CAPT_BUSY, &fimc->state); > @@ -557,7 +562,7 @@ static int fimc_capture_release(struct file *file) > clear_bit(ST_CAPT_SUSPENDED, &fimc->state); > > fimc_md_graph_lock(&vc->ve); > - vc->ve.vdev.entity.use_count--; > + vdev->entity.use_count--; > fimc_md_graph_unlock(&vc->ve); > } > > diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c > b/drivers/media/platform/exynos4-is/fimc-lite.c > index e5798f7..182db3c 100644 > --- a/drivers/media/platform/exynos4-is/fimc-lite.c > +++ b/drivers/media/platform/exynos4-is/fimc-lite.c > @@ -528,6 +528,7 @@ static int fimc_lite_release(struct file *file) > { > struct fimc_lite *fimc = video_drvdata(file); > struct media_entity *entity = &fimc->ve.vdev.entity; > + struct video_device *vdev = &fimc->ve.vdev; > > mutex_lock(&fimc->lock); > > @@ -546,7 +547,12 @@ static int fimc_lite_release(struct file *file) > mutex_unlock(&entity->parent->graph_mutex); > } > > - vb2_fop_release(file); > + if (file->private_data == vdev->queue->owner) { > + vb2_queue_release(vdev->queue); > + vdev->queue->owner = NULL; > + } > + v4l2_fh_release(file); > + > pm_runtime_put(&fimc->pdev->dev); > clear_bit(ST_FLITE_SUSPENDED, &fimc->state); > > -- > 1.7.4.1
On 10/19/2013 12:22 PM, Ricardo Ribalda Delgado wrote: > On Sat, Oct 19, 2013 at 11:55 AM, Sylwester Nawrocki > <sylvester.nawrocki@gmail.com> wrote: >> > On 10/14/2013 09:41 AM, Ricardo Ribalda Delgado wrote: >>> >> >>> >> vb2_fop_relase does not held the lock although it is modifying the >>> >> queue->owner field. >> > [...] >>> >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c >>> >> b/drivers/media/v4l2-core/videobuf2-core.c >>> >> index 9fc4bab..3a961ee 100644 >>> >> --- a/drivers/media/v4l2-core/videobuf2-core.c >>> >> +++ b/drivers/media/v4l2-core/videobuf2-core.c >>> >> @@ -2588,8 +2588,15 @@ int vb2_fop_release(struct file *file) >>> >> struct video_device *vdev = video_devdata(file); >>> >> >>> >> if (file->private_data == vdev->queue->owner) { >>> >> + struct mutex *lock; >>> >> + >>> >> + lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock; >>> >> + if (lock) >>> >> + mutex_lock(lock); >>> >> vb2_queue_release(vdev->queue); >>> >> vdev->queue->owner = NULL; >>> >> + if (lock) >>> >> + mutex_unlock(lock); >>> >> } >>> >> return v4l2_fh_release(file); >>> >> } >> > >> > >> > It seems you didn't inspect all users of vb2_fop_release(). There are 3 >> > drivers >> > that don't assign vb2_fop_release() to struct v4l2_file_operations directly >> > but >> > instead call it from within its own release() handler. Two of them do call >> > vb2_fop_release() with the video queue lock already held. >> > >> > $ git grep -n vb2_fop_rel -- drivers/media/ >> > >> > drivers/media/platform/exynos4-is/fimc-capture.c:552: ret = >> > vb2_fop_release(file); >> > drivers/media/platform/exynos4-is/fimc-lite.c:549: vb2_fop_release(file); >> > > > Very good catch, thanks! > >> > A rather ugly solution would be to open code the vb2_fop_release() function >> > in those driver, like in below patch (untested). Unless there are better >> > proposals I would queue the patch as below together with the $subject patch >> > upstream. > > IMHO this will lead to the same type of mistakes in the future. > > What about creating a function __vb2_fop_release that does exactly > the same as the original function but with an extra parameter bool > lock_held > > vb2_fop_release will be a wrapper for that funtion with lock_held== false Hmm, the parameter would be telling whether the lock is already held ? Perhaps we should inverse its meaning and it should indicate whether vb2_fop_release() should be taking the lock internally ? It seems to me more straightforward. > drivers that overload the fop_release and need to hold the lock will > call the __ function with lock_held= true > > What do you think? I was also considering this, it's probably better. I'm not sure about exporting functions prefixed with __ though. And the locking becomes less clear with such functions proliferation. Anyway, I'm in general personally OK with having an additional version like: __vb2_fop_release(struct file *filp, bool lock); Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Sylwester I have just posted a new version. Please take a look to it, it should fix your issue. I havent tried it in hw because I am out of the office. Regards! On Sat, Oct 19, 2013 at 12:51 PM, Sylwester Nawrocki <sylvester.nawrocki@gmail.com> wrote: > On 10/19/2013 12:22 PM, Ricardo Ribalda Delgado wrote: >> >> On Sat, Oct 19, 2013 at 11:55 AM, Sylwester Nawrocki >> <sylvester.nawrocki@gmail.com> wrote: >>> >>> > On 10/14/2013 09:41 AM, Ricardo Ribalda Delgado wrote: >>> >>>> >> >>>> >> vb2_fop_relase does not held the lock although it is modifying the >>>> >> queue->owner field. >>> >>> > [...] >>>> >>>> >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c >>>> >> b/drivers/media/v4l2-core/videobuf2-core.c >>>> >> index 9fc4bab..3a961ee 100644 >>>> >> --- a/drivers/media/v4l2-core/videobuf2-core.c >>>> >> +++ b/drivers/media/v4l2-core/videobuf2-core.c >>>> >> @@ -2588,8 +2588,15 @@ int vb2_fop_release(struct file *file) >>>> >> struct video_device *vdev = video_devdata(file); >>>> >> >>>> >> if (file->private_data == vdev->queue->owner) { >>>> >> + struct mutex *lock; >>>> >> + >>>> >> + lock = vdev->queue->lock ? vdev->queue->lock : >>>> >> vdev->lock; >>>> >> + if (lock) >>>> >> + mutex_lock(lock); >>>> >> vb2_queue_release(vdev->queue); >>>> >> vdev->queue->owner = NULL; >>>> >> + if (lock) >>>> >> + mutex_unlock(lock); >>>> >> } >>>> >> return v4l2_fh_release(file); >>>> >> } >>> >>> > >>> > >>> > It seems you didn't inspect all users of vb2_fop_release(). There are >>> > 3 >>> > drivers >>> > that don't assign vb2_fop_release() to struct v4l2_file_operations >>> > directly >>> > but >>> > instead call it from within its own release() handler. Two of them do >>> > call >>> > vb2_fop_release() with the video queue lock already held. >>> > >>> > $ git grep -n vb2_fop_rel -- drivers/media/ >>> > >>> > drivers/media/platform/exynos4-is/fimc-capture.c:552: ret = >>> > vb2_fop_release(file); >>> > drivers/media/platform/exynos4-is/fimc-lite.c:549: >>> > vb2_fop_release(file); >>> > >> >> >> Very good catch, thanks! >> >>> > A rather ugly solution would be to open code the vb2_fop_release() >>> > function >>> > in those driver, like in below patch (untested). Unless there are >>> > better >>> > proposals I would queue the patch as below together with the $subject >>> > patch >>> > upstream. >> >> >> IMHO this will lead to the same type of mistakes in the future. >> >> What about creating a function __vb2_fop_release that does exactly >> the same as the original function but with an extra parameter bool >> lock_held >> >> vb2_fop_release will be a wrapper for that funtion with lock_held== false > > > Hmm, the parameter would be telling whether the lock is already held ? > Perhaps > we should inverse its meaning and it should indicate whether > vb2_fop_release() > should be taking the lock internally ? It seems to me more straightforward. > > >> drivers that overload the fop_release and need to hold the lock will >> call the __ function with lock_held= true >> >> What do you think? > > > I was also considering this, it's probably better. I'm not sure about > exporting > functions prefixed with __ though. And the locking becomes less clear with > such > functions proliferation. > > Anyway, I'm in general personally OK with having an additional version like: > > __vb2_fop_release(struct file *filp, bool lock); > > > Regards, > Sylwester
diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c index fb27ff7..e9a5c90 100644 --- a/drivers/media/platform/exynos4-is/fimc-capture.c +++ b/drivers/media/platform/exynos4-is/fimc-capture.c @@ -537,6 +537,7 @@ static int fimc_capture_release(struct file *file) { struct fimc_dev *fimc = video_drvdata(file); struct fimc_vid_cap *vc = &fimc->vid_cap; + struct video_device *vdev = &vc->ve.vdev; bool close = v4l2_fh_is_singular_file(file); int ret; @@ -545,11 +546,15 @@ static int fimc_capture_release(struct file *file) mutex_lock(&fimc->lock); if (close && vc->streaming) { - media_entity_pipeline_stop(&vc->ve.vdev.entity); + media_entity_pipeline_stop(&vdev->entity); vc->streaming = false; } - ret = vb2_fop_release(file); + if (file->private_data == vdev->queue->owner) { + vb2_queue_release(vdev->queue); + vdev->queue->owner = NULL; + } + ret = v4l2_fh_release(file); if (close) { clear_bit(ST_CAPT_BUSY, &fimc->state); @@ -557,7 +562,7 @@ static int fimc_capture_release(struct file *file) clear_bit(ST_CAPT_SUSPENDED, &fimc->state); fimc_md_graph_lock(&vc->ve); - vc->ve.vdev.entity.use_count--; + vdev->entity.use_count--; fimc_md_graph_unlock(&vc->ve); } diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c index e5798f7..182db3c 100644 --- a/drivers/media/platform/exynos4-is/fimc-lite.c +++ b/drivers/media/platform/exynos4-is/fimc-lite.c @@ -528,6 +528,7 @@ static int fimc_lite_release(struct file *file) { struct fimc_lite *fimc = video_drvdata(file); struct media_entity *entity = &fimc->ve.vdev.entity; + struct video_device *vdev = &fimc->ve.vdev; mutex_lock(&fimc->lock); @@ -546,7 +547,12 @@ static int fimc_lite_release(struct file *file) mutex_unlock(&entity->parent->graph_mutex); } - vb2_fop_release(file); + if (file->private_data == vdev->queue->owner) { + vb2_queue_release(vdev->queue); + vdev->queue->owner = NULL; + } + v4l2_fh_release(file); + pm_runtime_put(&fimc->pdev->dev); clear_bit(ST_FLITE_SUSPENDED, &fimc->state);