| Message ID | 1383385994-11422-1-git-send-email-ricardo.ribalda@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, 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 1VcXtX-0000Wo-Ee; Sat, 02 Nov 2013 10:53:27 +0100 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-5) with esmtp id 1VcXtV-0001bN-7l; Sat, 02 Nov 2013 10:53:27 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751607Ab3KBJxX (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Sat, 2 Nov 2013 05:53:23 -0400 Received: from mail-lb0-f176.google.com ([209.85.217.176]:46205 "EHLO mail-lb0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751330Ab3KBJxW (ORCPT <rfc822; linux-media@vger.kernel.org>); Sat, 2 Nov 2013 05:53:22 -0400 Received: by mail-lb0-f176.google.com with SMTP id z5so4181034lbh.35 for <multiple recipients>; Sat, 02 Nov 2013 02:53:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:subject:date:message-id; bh=fmIjxrXopQXN4sp8REMJ+I7gkNXVRiSGFkownUMsYuQ=; b=zXbDI+x26tAt/oqwDKNNqUeQM/1CdVrhPEolLQT0rnhnTNy8ZvJhuyUfKlBbOoljpp WBWzM/zOMo6A5OZ/HIp/TPREBf/vH8AUDN+Op5zoEJkeDOTLGMVJLs9BU5UeR8X6CmoW Jwsuns2EhoG22boCvoJFe1hH1AO8aen48XjA6lGNcMJKEA04NH9kytJMRCGxHLtEd2J1 lmJlOWVhRxYJG16UkkcM0pOyf5GhJBLHMROMOc53gekNSSPBAT4pZ3VT8Zru/eS8d2Ms yQhtanAvVCOusi0Gx4TRSRfUnYbOW5/Rj8K3tVCdvTVkoXUOYF/wKE1GDL9NMACCuOQe ljyg== X-Received: by 10.112.198.39 with SMTP id iz7mr4168932lbc.24.1383385999965; Sat, 02 Nov 2013 02:53:19 -0700 (PDT) Received: from neopili.wiz.cph.dk ([193.110.198.7]) by mx.google.com with ESMTPSA id pw4sm9227138lbb.9.2013.11.02.02.53.18 for <multiple recipients> (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 02 Nov 2013 02:53:19 -0700 (PDT) From: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> To: Mauro Carvalho Chehab <m.chehab@samsung.com>, Ricardo Ribalda <ricardo.ribalda@gmail.com>, Kyungmin Park <kyungmin.park@samsung.com>, Sylwester Nawrocki <s.nawrocki@samsung.com>, Kukjin Kim <kgene.kim@samsung.com>, Pawel Osciak <pawel@osciak.com>, Marek Szyprowski <m.szyprowski@samsung.com>, "open list:SAMSUNG S5P/EXYNO..." <linux-media@vger.kernel.org>, "moderated list:ARM/S5P EXYNOS AR..." <linux-arm-kernel@lists.infradead.org>, "moderated list:ARM/S5P EXYNOS AR..." <linux-samsung-soc@vger.kernel.org> Subject: [PATCH v4] videobuf2: Add missing lock held on vb2_fop_relase Date: Sat, 2 Nov 2013 10:53:14 +0100 Message-Id: <1383385994-11422-1-git-send-email-ricardo.ribalda@gmail.com> X-Mailer: git-send-email 1.8.4.rc3 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.11.2.94215 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_6000_6999 0, BODY_SIZE_7000_LESS 0, DKIM_SIGNATURE 0, FROM_NAME_PHRASE 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __CP_MEDIA_BODY 0, __CP_URI_IN_BODY 0, __FRAUD_BODY_WEBMAIL 0, __FRAUD_CONTACT_NAME 0, __FRAUD_WEBMAIL 0, __FRAUD_WEBMAIL_FROM 0, __FROM_GMAIL 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __MIME_TEXT_ONLY 0, __MULTIPLE_RCPTS_TO_X5 0, __PHISH_SPEAR_STRUCTURE_1 0, __SANE_MSGID 0, __STOCK_PHRASE_24 0, __SUBJ_ALPHA_NEGATE 0, __TO_MALFORMED_2 0, __URI_NO_WWW 0, __URI_NS , __YOUTUBE_RCVD 0' |
Commit Message
Ricardo Ribalda Delgado
Nov. 2, 2013, 9:53 a.m. UTC
From: Ricardo Ribalda <ricardo.ribalda@gmail.com> vb2_fop_relase does not held the lock although it is modifying the queue->owner field. This could lead to race conditions on the vb2_perform_io function when multiple applications are accessing the video device via read/write API: [ 308.297741] BUG: unable to handle kernel NULL pointer dereference at 0000000000000260 [ 308.297759] IP: [<ffffffffa07a9fd2>] vb2_perform_fileio+0x372/0x610 [videobuf2_core] [ 308.297794] PGD 159719067 PUD 158119067 PMD 0 [ 308.297812] Oops: 0000 #1 SMP [ 308.297826] Modules linked in: qt5023_video videobuf2_dma_sg qtec_xform videobuf2_vmalloc videobuf2_memops videobuf2_core qtec_white qtec_mem gpio_xilinx qtec_cmosis qtec_pcie fglrx(PO) spi_xilinx spi_bitbang qt5023 [ 308.297888] CPU: 1 PID: 2189 Comm: java Tainted: P O 3.11.0-qtec-standard #1 [ 308.297919] Hardware name: QTechnology QT5022/QT5022, BIOS PM_2.1.0.309 X64 05/23/2013 [ 308.297952] task: ffff8801564e1690 ti: ffff88014dc02000 task.ti: ffff88014dc02000 [ 308.297962] RIP: 0010:[<ffffffffa07a9fd2>] [<ffffffffa07a9fd2>] vb2_perform_fileio+0x372/0x610 [videobuf2_core] [ 308.297985] RSP: 0018:ffff88014dc03df8 EFLAGS: 00010202 [ 308.297995] RAX: 0000000000000000 RBX: ffff880158a23000 RCX: dead000000100100 [ 308.298003] RDX: 0000000000000000 RSI: dead000000200200 RDI: 0000000000000000 [ 308.298012] RBP: ffff88014dc03e58 R08: 0000000000000000 R09: 0000000000000001 [ 308.298020] R10: ffffea00051e8380 R11: ffff88014dc03fd8 R12: ffff880158a23070 [ 308.298029] R13: ffff8801549040b8 R14: 0000000000198000 R15: 0000000001887e60 [ 308.298040] FS: 00007f65130d5700(0000) GS:ffff88015ed00000(0000) knlGS:0000000000000000 [ 308.298049] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 308.298057] CR2: 0000000000000260 CR3: 0000000159630000 CR4: 00000000000007e0 [ 308.298064] Stack: [ 308.298071] ffff880156416c00 0000000000198000 0000000000000000 ffff880100000001 [ 308.298087] ffff88014dc03f50 00000000810a79ca 0002000000000001 ffff880154904718 [ 308.298101] ffff880156416c00 0000000000198000 ffff880154904338 ffff88014dc03f50 [ 308.298116] Call Trace: [ 308.298143] [<ffffffffa07aa3c4>] vb2_read+0x14/0x20 [videobuf2_core] [ 308.298198] [<ffffffffa07aa494>] vb2_fop_read+0xc4/0x120 [videobuf2_core] [ 308.298252] [<ffffffff8154ee9e>] v4l2_read+0x7e/0xc0 [ 308.298296] [<ffffffff8116e639>] vfs_read+0xa9/0x160 [ 308.298312] [<ffffffff8116e882>] SyS_read+0x52/0xb0 [ 308.298328] [<ffffffff81784179>] tracesys+0xd0/0xd5 [ 308.298335] Code: e5 d6 ff ff 83 3d be 24 00 00 04 89 c2 4c 8b 45 b0 44 8b 4d b8 0f 8f 20 02 00 00 85 d2 75 32 83 83 78 03 00 00 01 4b 8b 44 c5 48 <8b> 88 60 02 00 00 85 c9 0f 84 b0 00 00 00 8b 40 58 89 c2 41 89 [ 308.298487] RIP [<ffffffffa07a9fd2>] vb2_perform_fileio+0x372/0x610 [videobuf2_core] [ 308.298507] RSP <ffff88014dc03df8> [ 308.298514] CR2: 0000000000000260 [ 308.298526] ---[ end trace e8f01717c96d1e41 ]--- Signed-off-by: Ricardo Ribalda <ricardo.ribalda@gmail.com> --- v2: Comments by Sylvester Nawrocki fimc-capture and fimc-lite where calling vb2_fop_release with the lock held. Therefore a new __vb2_fop_release function has been created to be used by drivers that overload the release function. v3: Comments by Sylvester Nawrocki and Mauro Carvalho Chehab Use vb2_fop_release_locked instead of __vb2_fop_release v4: Comments by Sylvester Nawrocki Rename vb2_fop_release_locked to __vb2_fop_release and fix patch format drivers/media/platform/exynos4-is/fimc-capture.c | 2 +- drivers/media/platform/exynos4-is/fimc-lite.c | 2 +- drivers/media/v4l2-core/videobuf2-core.c | 23 ++++++++++++++++++++++- include/media/videobuf2-core.h | 1 + 4 files changed, 25 insertions(+), 3 deletions(-)
Comments
On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote: > From: Ricardo Ribalda <ricardo.ribalda@gmail.com> > > vb2_fop_relase does not held the lock although it is modifying the > queue->owner field. > > This could lead to race conditions on the vb2_perform_io function > when multiple applications are accessing the video device via > read/write API: It's also called directly by drivers/media/usb/em28xx/em28xx-video.c! > > [ 308.297741] BUG: unable to handle kernel NULL pointer dereference at > 0000000000000260 > [ 308.297759] IP: [<ffffffffa07a9fd2>] vb2_perform_fileio+0x372/0x610 > [videobuf2_core] > [ 308.297794] PGD 159719067 PUD 158119067 PMD 0 > [ 308.297812] Oops: 0000 #1 SMP > [ 308.297826] Modules linked in: qt5023_video videobuf2_dma_sg > qtec_xform videobuf2_vmalloc videobuf2_memops videobuf2_core > qtec_white qtec_mem gpio_xilinx qtec_cmosis qtec_pcie fglrx(PO) > spi_xilinx spi_bitbang qt5023 > [ 308.297888] CPU: 1 PID: 2189 Comm: java Tainted: P O 3.11.0-qtec-standard #1 > [ 308.297919] Hardware name: QTechnology QT5022/QT5022, BIOS > PM_2.1.0.309 X64 05/23/2013 > [ 308.297952] task: ffff8801564e1690 ti: ffff88014dc02000 task.ti: > ffff88014dc02000 > [ 308.297962] RIP: 0010:[<ffffffffa07a9fd2>] [<ffffffffa07a9fd2>] > vb2_perform_fileio+0x372/0x610 [videobuf2_core] > [ 308.297985] RSP: 0018:ffff88014dc03df8 EFLAGS: 00010202 > [ 308.297995] RAX: 0000000000000000 RBX: ffff880158a23000 RCX: dead000000100100 > [ 308.298003] RDX: 0000000000000000 RSI: dead000000200200 RDI: 0000000000000000 > [ 308.298012] RBP: ffff88014dc03e58 R08: 0000000000000000 R09: 0000000000000001 > [ 308.298020] R10: ffffea00051e8380 R11: ffff88014dc03fd8 R12: ffff880158a23070 > [ 308.298029] R13: ffff8801549040b8 R14: 0000000000198000 R15: 0000000001887e60 > [ 308.298040] FS: 00007f65130d5700(0000) GS:ffff88015ed00000(0000) > knlGS:0000000000000000 > [ 308.298049] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 308.298057] CR2: 0000000000000260 CR3: 0000000159630000 CR4: 00000000000007e0 > [ 308.298064] Stack: > [ 308.298071] ffff880156416c00 0000000000198000 0000000000000000 > ffff880100000001 > [ 308.298087] ffff88014dc03f50 00000000810a79ca 0002000000000001 > ffff880154904718 > [ 308.298101] ffff880156416c00 0000000000198000 ffff880154904338 > ffff88014dc03f50 > [ 308.298116] Call Trace: > [ 308.298143] [<ffffffffa07aa3c4>] vb2_read+0x14/0x20 [videobuf2_core] > [ 308.298198] [<ffffffffa07aa494>] vb2_fop_read+0xc4/0x120 [videobuf2_core] > [ 308.298252] [<ffffffff8154ee9e>] v4l2_read+0x7e/0xc0 > [ 308.298296] [<ffffffff8116e639>] vfs_read+0xa9/0x160 > [ 308.298312] [<ffffffff8116e882>] SyS_read+0x52/0xb0 > [ 308.298328] [<ffffffff81784179>] tracesys+0xd0/0xd5 > [ 308.298335] Code: e5 d6 ff ff 83 3d be 24 00 00 04 89 c2 4c 8b 45 b0 > 44 8b 4d b8 0f 8f 20 02 00 00 85 d2 75 32 83 83 78 03 00 00 01 4b 8b > 44 c5 48 <8b> 88 60 02 00 00 85 c9 0f 84 b0 00 00 00 8b 40 58 89 c2 41 > 89 > [ 308.298487] RIP [<ffffffffa07a9fd2>] vb2_perform_fileio+0x372/0x610 > [videobuf2_core] > [ 308.298507] RSP <ffff88014dc03df8> > [ 308.298514] CR2: 0000000000000260 > [ 308.298526] ---[ end trace e8f01717c96d1e41 ]--- > > Signed-off-by: Ricardo Ribalda <ricardo.ribalda@gmail.com> > --- > v2: Comments by Sylvester Nawrocki > > fimc-capture and fimc-lite where calling vb2_fop_release with the lock held. > Therefore a new __vb2_fop_release function has been created to be used by > drivers that overload the release function. > > v3: Comments by Sylvester Nawrocki and Mauro Carvalho Chehab > > Use vb2_fop_release_locked instead of __vb2_fop_release > > v4: Comments by Sylvester Nawrocki > > Rename vb2_fop_release_locked to __vb2_fop_release and fix patch format > > drivers/media/platform/exynos4-is/fimc-capture.c | 2 +- > drivers/media/platform/exynos4-is/fimc-lite.c | 2 +- > drivers/media/v4l2-core/videobuf2-core.c | 23 ++++++++++++++++++++++- > include/media/videobuf2-core.h | 1 + > 4 files changed, 25 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c > index fb27ff7..8192fe0 100644 > --- a/drivers/media/platform/exynos4-is/fimc-capture.c > +++ b/drivers/media/platform/exynos4-is/fimc-capture.c > @@ -549,7 +549,7 @@ static int fimc_capture_release(struct file *file) > vc->streaming = false; > } > > - ret = vb2_fop_release(file); > + ret = __vb2_fop_release(file); > > if (close) { > clear_bit(ST_CAPT_BUSY, &fimc->state); > diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c > index e5798f7..cbe51cd 100644 > --- a/drivers/media/platform/exynos4-is/fimc-lite.c > +++ b/drivers/media/platform/exynos4-is/fimc-lite.c > @@ -546,7 +546,7 @@ static int fimc_lite_release(struct file *file) > mutex_unlock(&entity->parent->graph_mutex); > } > > - vb2_fop_release(file); > + __vb2_fop_release(file); > pm_runtime_put(&fimc->pdev->dev); > clear_bit(ST_FLITE_SUSPENDED, &fimc->state); > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > index 594c75e..f48d72a 100644 > --- a/drivers/media/v4l2-core/videobuf2-core.c > +++ b/drivers/media/v4l2-core/videobuf2-core.c > @@ -2619,18 +2619,39 @@ int vb2_fop_mmap(struct file *file, struct vm_area_struct *vma) > } > EXPORT_SYMBOL_GPL(vb2_fop_mmap); > > -int vb2_fop_release(struct file *file) > +static int _vb2_fop_release(struct file *file, bool lock_is_held) > { > struct video_device *vdev = video_devdata(file); > + struct mutex *lock; > > if (file->private_data == vdev->queue->owner) { > + if (lock_is_held) > + lock = NULL; > + else > + 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); > } > + > +int vb2_fop_release(struct file *file) > +{ > + return _vb2_fop_release(file, false); > +} > EXPORT_SYMBOL_GPL(vb2_fop_release); > > +int __vb2_fop_release(struct file *file) > +{ > + return _vb2_fop_release(file, true); > +} > +EXPORT_SYMBOL_GPL(__vb2_fop_release); Sorry for introducing yet another opinion, but I think this is very confusing. I would do this: static int __vb2_fop_release(struct file *file, struct mutex *lock) { struct video_device *vdev = video_devdata(file); if (file->private_data == vdev->queue->owner) { if (lock) mutex_lock(lock); vb2_queue_release(vdev->queue); vdev->queue->owner = NULL; if (lock) mutex_unlock(lock); } return v4l2_fh_release(file); } int vb2_fop_release(struct file *file) { struct video_device *vdev = video_devdata(file); struct mutex *lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock; return __vb2_fop_release(file, lock); } EXPORT_SYMBOL_GPL(vb2_fop_release); int vb2_fop_release_unlock(struct file *file) { return __vb2_fop_release(file, NULL); } EXPORT_SYMBOL_GPL(vb2_fop_release_unlock); Optionally, __vb2_fop_release can be exported and then vb2_fop_release_unlock isn't necessary. Regards, Hans > + > ssize_t vb2_fop_write(struct file *file, char __user *buf, > size_t count, loff_t *ppos) > { > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 6781258..76400fa 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -491,6 +491,7 @@ int vb2_ioctl_expbuf(struct file *file, void *priv, > > int vb2_fop_mmap(struct file *file, struct vm_area_struct *vma); > int vb2_fop_release(struct file *file); > +int __vb2_fop_release(struct file *file); > ssize_t vb2_fop_write(struct file *file, char __user *buf, > size_t count, loff_t *ppos); > ssize_t vb2_fop_read(struct file *file, char __user *buf, > -- 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 Hans Thanks for your comments. Please take a look to v4 of this patch https://patchwork.linuxtv.org/patch/20529/ On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote: >> From: Ricardo Ribalda <ricardo.ribalda@gmail.com> >> >> vb2_fop_relase does not held the lock although it is modifying the >> queue->owner field. >> >> This could lead to race conditions on the vb2_perform_io function >> when multiple applications are accessing the video device via >> read/write API: > > It's also called directly by drivers/media/usb/em28xx/em28xx-video.c! > em28xx-video does not hold the lock, therefore it can call the normal function. On v2 we made a internal function that should be called if the funciton is called directly by the driver. Please take a look to the old comments. https://patchwork.linuxtv.org/patch/20460/ >> >> [ 308.297741] BUG: unable to handle kernel NULL pointer dereference at >> 0000000000000260 >> [ 308.297759] IP: [<ffffffffa07a9fd2>] vb2_perform_fileio+0x372/0x610 >> [videobuf2_core] >> [ 308.297794] PGD 159719067 PUD 158119067 PMD 0 >> [ 308.297812] Oops: 0000 #1 SMP >> [ 308.297826] Modules linked in: qt5023_video videobuf2_dma_sg >> qtec_xform videobuf2_vmalloc videobuf2_memops videobuf2_core >> qtec_white qtec_mem gpio_xilinx qtec_cmosis qtec_pcie fglrx(PO) >> spi_xilinx spi_bitbang qt5023 >> [ 308.297888] CPU: 1 PID: 2189 Comm: java Tainted: P O 3.11.0-qtec-standard #1 >> [ 308.297919] Hardware name: QTechnology QT5022/QT5022, BIOS >> PM_2.1.0.309 X64 05/23/2013 >> [ 308.297952] task: ffff8801564e1690 ti: ffff88014dc02000 task.ti: >> ffff88014dc02000 >> [ 308.297962] RIP: 0010:[<ffffffffa07a9fd2>] [<ffffffffa07a9fd2>] >> vb2_perform_fileio+0x372/0x610 [videobuf2_core] >> [ 308.297985] RSP: 0018:ffff88014dc03df8 EFLAGS: 00010202 >> [ 308.297995] RAX: 0000000000000000 RBX: ffff880158a23000 RCX: dead000000100100 >> [ 308.298003] RDX: 0000000000000000 RSI: dead000000200200 RDI: 0000000000000000 >> [ 308.298012] RBP: ffff88014dc03e58 R08: 0000000000000000 R09: 0000000000000001 >> [ 308.298020] R10: ffffea00051e8380 R11: ffff88014dc03fd8 R12: ffff880158a23070 >> [ 308.298029] R13: ffff8801549040b8 R14: 0000000000198000 R15: 0000000001887e60 >> [ 308.298040] FS: 00007f65130d5700(0000) GS:ffff88015ed00000(0000) >> knlGS:0000000000000000 >> [ 308.298049] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> [ 308.298057] CR2: 0000000000000260 CR3: 0000000159630000 CR4: 00000000000007e0 >> [ 308.298064] Stack: >> [ 308.298071] ffff880156416c00 0000000000198000 0000000000000000 >> ffff880100000001 >> [ 308.298087] ffff88014dc03f50 00000000810a79ca 0002000000000001 >> ffff880154904718 >> [ 308.298101] ffff880156416c00 0000000000198000 ffff880154904338 >> ffff88014dc03f50 >> [ 308.298116] Call Trace: >> [ 308.298143] [<ffffffffa07aa3c4>] vb2_read+0x14/0x20 [videobuf2_core] >> [ 308.298198] [<ffffffffa07aa494>] vb2_fop_read+0xc4/0x120 [videobuf2_core] >> [ 308.298252] [<ffffffff8154ee9e>] v4l2_read+0x7e/0xc0 >> [ 308.298296] [<ffffffff8116e639>] vfs_read+0xa9/0x160 >> [ 308.298312] [<ffffffff8116e882>] SyS_read+0x52/0xb0 >> [ 308.298328] [<ffffffff81784179>] tracesys+0xd0/0xd5 >> [ 308.298335] Code: e5 d6 ff ff 83 3d be 24 00 00 04 89 c2 4c 8b 45 b0 >> 44 8b 4d b8 0f 8f 20 02 00 00 85 d2 75 32 83 83 78 03 00 00 01 4b 8b >> 44 c5 48 <8b> 88 60 02 00 00 85 c9 0f 84 b0 00 00 00 8b 40 58 89 c2 41 >> 89 >> [ 308.298487] RIP [<ffffffffa07a9fd2>] vb2_perform_fileio+0x372/0x610 >> [videobuf2_core] >> [ 308.298507] RSP <ffff88014dc03df8> >> [ 308.298514] CR2: 0000000000000260 >> [ 308.298526] ---[ end trace e8f01717c96d1e41 ]--- >> >> Signed-off-by: Ricardo Ribalda <ricardo.ribalda@gmail.com> >> --- >> v2: Comments by Sylvester Nawrocki >> >> fimc-capture and fimc-lite where calling vb2_fop_release with the lock held. >> Therefore a new __vb2_fop_release function has been created to be used by >> drivers that overload the release function. >> >> v3: Comments by Sylvester Nawrocki and Mauro Carvalho Chehab >> >> Use vb2_fop_release_locked instead of __vb2_fop_release >> >> v4: Comments by Sylvester Nawrocki >> >> Rename vb2_fop_release_locked to __vb2_fop_release and fix patch format >> >> drivers/media/platform/exynos4-is/fimc-capture.c | 2 +- >> drivers/media/platform/exynos4-is/fimc-lite.c | 2 +- >> drivers/media/v4l2-core/videobuf2-core.c | 23 ++++++++++++++++++++++- >> include/media/videobuf2-core.h | 1 + >> 4 files changed, 25 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c >> index fb27ff7..8192fe0 100644 >> --- a/drivers/media/platform/exynos4-is/fimc-capture.c >> +++ b/drivers/media/platform/exynos4-is/fimc-capture.c >> @@ -549,7 +549,7 @@ static int fimc_capture_release(struct file *file) >> vc->streaming = false; >> } >> >> - ret = vb2_fop_release(file); >> + ret = __vb2_fop_release(file); >> >> if (close) { >> clear_bit(ST_CAPT_BUSY, &fimc->state); >> diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c >> index e5798f7..cbe51cd 100644 >> --- a/drivers/media/platform/exynos4-is/fimc-lite.c >> +++ b/drivers/media/platform/exynos4-is/fimc-lite.c >> @@ -546,7 +546,7 @@ static int fimc_lite_release(struct file *file) >> mutex_unlock(&entity->parent->graph_mutex); >> } >> >> - vb2_fop_release(file); >> + __vb2_fop_release(file); >> pm_runtime_put(&fimc->pdev->dev); >> clear_bit(ST_FLITE_SUSPENDED, &fimc->state); >> >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c >> index 594c75e..f48d72a 100644 >> --- a/drivers/media/v4l2-core/videobuf2-core.c >> +++ b/drivers/media/v4l2-core/videobuf2-core.c >> @@ -2619,18 +2619,39 @@ int vb2_fop_mmap(struct file *file, struct vm_area_struct *vma) >> } >> EXPORT_SYMBOL_GPL(vb2_fop_mmap); >> >> -int vb2_fop_release(struct file *file) >> +static int _vb2_fop_release(struct file *file, bool lock_is_held) >> { >> struct video_device *vdev = video_devdata(file); >> + struct mutex *lock; >> >> if (file->private_data == vdev->queue->owner) { >> + if (lock_is_held) >> + lock = NULL; >> + else >> + 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); >> } >> + >> +int vb2_fop_release(struct file *file) >> +{ >> + return _vb2_fop_release(file, false); >> +} >> EXPORT_SYMBOL_GPL(vb2_fop_release); >> >> +int __vb2_fop_release(struct file *file) >> +{ >> + return _vb2_fop_release(file, true); >> +} >> +EXPORT_SYMBOL_GPL(__vb2_fop_release); > > Sorry for introducing yet another opinion, but I think this is very confusing. It is confusing the lock_held parameter or the __ naming for unlocked versions? > > I would do this: > > static int __vb2_fop_release(struct file *file, struct mutex *lock) > { > struct video_device *vdev = video_devdata(file); > > if (file->private_data == vdev->queue->owner) { > if (lock) > mutex_lock(lock); > vb2_queue_release(vdev->queue); > vdev->queue->owner = NULL; > if (lock) > mutex_unlock(lock); > } > return v4l2_fh_release(file); > } > > int vb2_fop_release(struct file *file) > { > struct video_device *vdev = video_devdata(file); > struct mutex *lock = vdev->queue->lock ? > vdev->queue->lock : vdev->lock; > > return __vb2_fop_release(file, lock); > } > EXPORT_SYMBOL_GPL(vb2_fop_release); > > int vb2_fop_release_unlock(struct file *file) > { > return __vb2_fop_release(file, NULL); > } > EXPORT_SYMBOL_GPL(vb2_fop_release_unlock); > > Optionally, __vb2_fop_release can be exported and then vb2_fop_release_unlock > isn't necessary. > i dont have any strong opinion in any direction. All I really care is that the oops is fixed :). If your concern about the patch is the is_lock_held function, I can make a patch with the params on your proposal and the __naming as Sylvester suggested, so everyone is happy. Sylvester, Hanshat do you think? Thanks for your comments! > Regards, > > Hans > >> + >> ssize_t vb2_fop_write(struct file *file, char __user *buf, >> size_t count, loff_t *ppos) >> { >> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h >> index 6781258..76400fa 100644 >> --- a/include/media/videobuf2-core.h >> +++ b/include/media/videobuf2-core.h >> @@ -491,6 +491,7 @@ int vb2_ioctl_expbuf(struct file *file, void *priv, >> >> int vb2_fop_mmap(struct file *file, struct vm_area_struct *vma); >> int vb2_fop_release(struct file *file); >> +int __vb2_fop_release(struct file *file); >> ssize_t vb2_fop_write(struct file *file, char __user *buf, >> size_t count, loff_t *ppos); >> ssize_t vb2_fop_read(struct file *file, char __user *buf, >> >
Hi, On 04/11/13 14:54, Ricardo Ribalda Delgado wrote: > Hello Hans > > Thanks for your comments. > > Please take a look to v4 of this patch > https://patchwork.linuxtv.org/patch/20529/ We're discussing v4 actually ;) > On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote: >>> From: Ricardo Ribalda <ricardo.ribalda@gmail.com> >>> >>> vb2_fop_relase does not held the lock although it is modifying the >>> queue->owner field. >>> >>> This could lead to race conditions on the vb2_perform_io function >>> when multiple applications are accessing the video device via >>> read/write API: >> >> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c! >> > > em28xx-video does not hold the lock, therefore it can call the normal > function. On v2 we made a internal function that should be called if > the funciton is called directly by the driver. Please take a look to > the old comments. https://patchwork.linuxtv.org/patch/20460/ [...] >>> -int vb2_fop_release(struct file *file) >>> +static int _vb2_fop_release(struct file *file, bool lock_is_held) >>> { >>> struct video_device *vdev = video_devdata(file); >>> + struct mutex *lock; >>> >>> if (file->private_data == vdev->queue->owner) { >>> + if (lock_is_held) >>> + lock = NULL; >>> + else >>> + 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); >>> } >>> + >>> +int vb2_fop_release(struct file *file) >>> +{ >>> + return _vb2_fop_release(file, false); >>> +} >>> EXPORT_SYMBOL_GPL(vb2_fop_release); >>> >>> +int __vb2_fop_release(struct file *file) >>> +{ >>> + return _vb2_fop_release(file, true); >>> +} >>> +EXPORT_SYMBOL_GPL(__vb2_fop_release); >> >> Sorry for introducing yet another opinion, but I think this is very confusing. > > It is confusing the lock_held parameter or the __ naming for unlocked versions? > >> >> I would do this: >> >> static int __vb2_fop_release(struct file *file, struct mutex *lock) >> { >> struct video_device *vdev = video_devdata(file); >> >> if (file->private_data == vdev->queue->owner) { >> if (lock) >> mutex_lock(lock); >> vb2_queue_release(vdev->queue); >> vdev->queue->owner = NULL; >> if (lock) >> mutex_unlock(lock); >> } >> return v4l2_fh_release(file); >> } >> >> int vb2_fop_release(struct file *file) >> { >> struct video_device *vdev = video_devdata(file); >> struct mutex *lock = vdev->queue->lock ? >> vdev->queue->lock : vdev->lock; >> >> return __vb2_fop_release(file, lock); >> } >> EXPORT_SYMBOL_GPL(vb2_fop_release); >> >> int vb2_fop_release_unlock(struct file *file) >> { >> return __vb2_fop_release(file, NULL); >> } >> EXPORT_SYMBOL_GPL(vb2_fop_release_unlock); >> >> Optionally, __vb2_fop_release can be exported and then vb2_fop_release_unlock >> isn't necessary. >> > > i dont have any strong opinion in any direction. All I really care is > that the oops is fixed :). > > If your concern about the patch is the is_lock_held function, I can > make a patch with the params on your proposal and the __naming as > Sylvester suggested, so everyone is happy. > > Sylvester, Hanshat do you think? I like Hans' proposal, probably it's better to export __vb2_fop_release(), so it can be also used in cases like em28xx, to make things a bit more clear. But I'm fine with vb2_fop_release_unlock() as well, don't really have strong preference. It's up to you. -- Thanks, 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
On 11/04/2013 02:54 PM, Ricardo Ribalda Delgado wrote: > Hello Hans > > Thanks for your comments. > > Please take a look to v4 of this patch > https://patchwork.linuxtv.org/patch/20529/ > > On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote: >>> From: Ricardo Ribalda <ricardo.ribalda@gmail.com> >>> >>> vb2_fop_relase does not held the lock although it is modifying the >>> queue->owner field. >>> >>> This could lead to race conditions on the vb2_perform_io function >>> when multiple applications are accessing the video device via >>> read/write API: >> >> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c! >> > > em28xx-video does not hold the lock, therefore it can call the normal > function. On v2 we made a internal function that should be called if > the funciton is called directly by the driver. Please take a look to > the old comments. https://patchwork.linuxtv.org/patch/20460/ static int em28xx_v4l2_close(struct file *filp) { struct em28xx_fh *fh = filp->private_data; struct em28xx *dev = fh->dev; int errCode; em28xx_videodbg("users=%d\n", dev->users); mutex_lock(&dev->lock); vb2_fop_release(filp); ... vb2_fop_release(filp) will, with your patch, also try to get dev->lock. Sylwester's comment re em28xx is incorrect. > >>> >>> [ 308.297741] BUG: unable to handle kernel NULL pointer dereference at >>> 0000000000000260 >>> [ 308.297759] IP: [<ffffffffa07a9fd2>] vb2_perform_fileio+0x372/0x610 >>> [videobuf2_core] >>> [ 308.297794] PGD 159719067 PUD 158119067 PMD 0 >>> [ 308.297812] Oops: 0000 #1 SMP >>> [ 308.297826] Modules linked in: qt5023_video videobuf2_dma_sg >>> qtec_xform videobuf2_vmalloc videobuf2_memops videobuf2_core >>> qtec_white qtec_mem gpio_xilinx qtec_cmosis qtec_pcie fglrx(PO) >>> spi_xilinx spi_bitbang qt5023 >>> [ 308.297888] CPU: 1 PID: 2189 Comm: java Tainted: P O 3.11.0-qtec-standard #1 >>> [ 308.297919] Hardware name: QTechnology QT5022/QT5022, BIOS >>> PM_2.1.0.309 X64 05/23/2013 >>> [ 308.297952] task: ffff8801564e1690 ti: ffff88014dc02000 task.ti: >>> ffff88014dc02000 >>> [ 308.297962] RIP: 0010:[<ffffffffa07a9fd2>] [<ffffffffa07a9fd2>] >>> vb2_perform_fileio+0x372/0x610 [videobuf2_core] >>> [ 308.297985] RSP: 0018:ffff88014dc03df8 EFLAGS: 00010202 >>> [ 308.297995] RAX: 0000000000000000 RBX: ffff880158a23000 RCX: dead000000100100 >>> [ 308.298003] RDX: 0000000000000000 RSI: dead000000200200 RDI: 0000000000000000 >>> [ 308.298012] RBP: ffff88014dc03e58 R08: 0000000000000000 R09: 0000000000000001 >>> [ 308.298020] R10: ffffea00051e8380 R11: ffff88014dc03fd8 R12: ffff880158a23070 >>> [ 308.298029] R13: ffff8801549040b8 R14: 0000000000198000 R15: 0000000001887e60 >>> [ 308.298040] FS: 00007f65130d5700(0000) GS:ffff88015ed00000(0000) >>> knlGS:0000000000000000 >>> [ 308.298049] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >>> [ 308.298057] CR2: 0000000000000260 CR3: 0000000159630000 CR4: 00000000000007e0 >>> [ 308.298064] Stack: >>> [ 308.298071] ffff880156416c00 0000000000198000 0000000000000000 >>> ffff880100000001 >>> [ 308.298087] ffff88014dc03f50 00000000810a79ca 0002000000000001 >>> ffff880154904718 >>> [ 308.298101] ffff880156416c00 0000000000198000 ffff880154904338 >>> ffff88014dc03f50 >>> [ 308.298116] Call Trace: >>> [ 308.298143] [<ffffffffa07aa3c4>] vb2_read+0x14/0x20 [videobuf2_core] >>> [ 308.298198] [<ffffffffa07aa494>] vb2_fop_read+0xc4/0x120 [videobuf2_core] >>> [ 308.298252] [<ffffffff8154ee9e>] v4l2_read+0x7e/0xc0 >>> [ 308.298296] [<ffffffff8116e639>] vfs_read+0xa9/0x160 >>> [ 308.298312] [<ffffffff8116e882>] SyS_read+0x52/0xb0 >>> [ 308.298328] [<ffffffff81784179>] tracesys+0xd0/0xd5 >>> [ 308.298335] Code: e5 d6 ff ff 83 3d be 24 00 00 04 89 c2 4c 8b 45 b0 >>> 44 8b 4d b8 0f 8f 20 02 00 00 85 d2 75 32 83 83 78 03 00 00 01 4b 8b >>> 44 c5 48 <8b> 88 60 02 00 00 85 c9 0f 84 b0 00 00 00 8b 40 58 89 c2 41 >>> 89 >>> [ 308.298487] RIP [<ffffffffa07a9fd2>] vb2_perform_fileio+0x372/0x610 >>> [videobuf2_core] >>> [ 308.298507] RSP <ffff88014dc03df8> >>> [ 308.298514] CR2: 0000000000000260 >>> [ 308.298526] ---[ end trace e8f01717c96d1e41 ]--- >>> >>> Signed-off-by: Ricardo Ribalda <ricardo.ribalda@gmail.com> >>> --- >>> v2: Comments by Sylvester Nawrocki >>> >>> fimc-capture and fimc-lite where calling vb2_fop_release with the lock held. >>> Therefore a new __vb2_fop_release function has been created to be used by >>> drivers that overload the release function. >>> >>> v3: Comments by Sylvester Nawrocki and Mauro Carvalho Chehab >>> >>> Use vb2_fop_release_locked instead of __vb2_fop_release >>> >>> v4: Comments by Sylvester Nawrocki >>> >>> Rename vb2_fop_release_locked to __vb2_fop_release and fix patch format >>> >>> drivers/media/platform/exynos4-is/fimc-capture.c | 2 +- >>> drivers/media/platform/exynos4-is/fimc-lite.c | 2 +- >>> drivers/media/v4l2-core/videobuf2-core.c | 23 ++++++++++++++++++++++- >>> include/media/videobuf2-core.h | 1 + >>> 4 files changed, 25 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c >>> index fb27ff7..8192fe0 100644 >>> --- a/drivers/media/platform/exynos4-is/fimc-capture.c >>> +++ b/drivers/media/platform/exynos4-is/fimc-capture.c >>> @@ -549,7 +549,7 @@ static int fimc_capture_release(struct file *file) >>> vc->streaming = false; >>> } >>> >>> - ret = vb2_fop_release(file); >>> + ret = __vb2_fop_release(file); >>> >>> if (close) { >>> clear_bit(ST_CAPT_BUSY, &fimc->state); >>> diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c >>> index e5798f7..cbe51cd 100644 >>> --- a/drivers/media/platform/exynos4-is/fimc-lite.c >>> +++ b/drivers/media/platform/exynos4-is/fimc-lite.c >>> @@ -546,7 +546,7 @@ static int fimc_lite_release(struct file *file) >>> mutex_unlock(&entity->parent->graph_mutex); >>> } >>> >>> - vb2_fop_release(file); >>> + __vb2_fop_release(file); >>> pm_runtime_put(&fimc->pdev->dev); >>> clear_bit(ST_FLITE_SUSPENDED, &fimc->state); >>> >>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c >>> index 594c75e..f48d72a 100644 >>> --- a/drivers/media/v4l2-core/videobuf2-core.c >>> +++ b/drivers/media/v4l2-core/videobuf2-core.c >>> @@ -2619,18 +2619,39 @@ int vb2_fop_mmap(struct file *file, struct vm_area_struct *vma) >>> } >>> EXPORT_SYMBOL_GPL(vb2_fop_mmap); >>> >>> -int vb2_fop_release(struct file *file) >>> +static int _vb2_fop_release(struct file *file, bool lock_is_held) >>> { >>> struct video_device *vdev = video_devdata(file); >>> + struct mutex *lock; >>> >>> if (file->private_data == vdev->queue->owner) { >>> + if (lock_is_held) >>> + lock = NULL; >>> + else >>> + 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); >>> } >>> + >>> +int vb2_fop_release(struct file *file) >>> +{ >>> + return _vb2_fop_release(file, false); >>> +} >>> EXPORT_SYMBOL_GPL(vb2_fop_release); >>> >>> +int __vb2_fop_release(struct file *file) >>> +{ >>> + return _vb2_fop_release(file, true); >>> +} >>> +EXPORT_SYMBOL_GPL(__vb2_fop_release); >> >> Sorry for introducing yet another opinion, but I think this is very confusing. > > It is confusing the lock_held parameter or the __ naming for unlocked versions? The naming for the most part. Having a normal function, a '_' prefix and a '__' prefix is a bit over the top IMHO. I prefer to have a clearly named _unlock version. Regards, Hans > >> >> I would do this: >> >> static int __vb2_fop_release(struct file *file, struct mutex *lock) >> { >> struct video_device *vdev = video_devdata(file); >> >> if (file->private_data == vdev->queue->owner) { >> if (lock) >> mutex_lock(lock); >> vb2_queue_release(vdev->queue); >> vdev->queue->owner = NULL; >> if (lock) >> mutex_unlock(lock); >> } >> return v4l2_fh_release(file); >> } >> >> int vb2_fop_release(struct file *file) >> { >> struct video_device *vdev = video_devdata(file); >> struct mutex *lock = vdev->queue->lock ? >> vdev->queue->lock : vdev->lock; >> >> return __vb2_fop_release(file, lock); >> } >> EXPORT_SYMBOL_GPL(vb2_fop_release); >> >> int vb2_fop_release_unlock(struct file *file) >> { >> return __vb2_fop_release(file, NULL); >> } >> EXPORT_SYMBOL_GPL(vb2_fop_release_unlock); >> >> Optionally, __vb2_fop_release can be exported and then vb2_fop_release_unlock >> isn't necessary. >> > > i dont have any strong opinion in any direction. All I really care is > that the oops is fixed :). > > If your concern about the patch is the is_lock_held function, I can > make a patch with the params on your proposal and the __naming as > Sylvester suggested, so everyone is happy. > > Sylvester, Hanshat do you think? > > Thanks for your comments! > >> Regards, >> >> Hans >> >>> + >>> ssize_t vb2_fop_write(struct file *file, char __user *buf, >>> size_t count, loff_t *ppos) >>> { >>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h >>> index 6781258..76400fa 100644 >>> --- a/include/media/videobuf2-core.h >>> +++ b/include/media/videobuf2-core.h >>> @@ -491,6 +491,7 @@ int vb2_ioctl_expbuf(struct file *file, void *priv, >>> >>> int vb2_fop_mmap(struct file *file, struct vm_area_struct *vma); >>> int vb2_fop_release(struct file *file); >>> +int __vb2_fop_release(struct file *file); >>> ssize_t vb2_fop_write(struct file *file, char __user *buf, >>> size_t count, loff_t *ppos); >>> ssize_t vb2_fop_read(struct file *file, char __user *buf, >>> >> > > > -- 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
On 04/11/13 15:12, Hans Verkuil wrote: > On 11/04/2013 02:54 PM, Ricardo Ribalda Delgado wrote: >> > Hello Hans >> > >> > Thanks for your comments. >> > >> > Please take a look to v4 of this patch >> > https://patchwork.linuxtv.org/patch/20529/ >> > >> > On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >>> >> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote: >>>> >>> From: Ricardo Ribalda <ricardo.ribalda@gmail.com> >>>> >>> >>>> >>> vb2_fop_relase does not held the lock although it is modifying the >>>> >>> queue->owner field. >>>> >>> >>>> >>> This could lead to race conditions on the vb2_perform_io function >>>> >>> when multiple applications are accessing the video device via >>>> >>> read/write API: >>> >> >>> >> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c! >>> >> >> > >> > em28xx-video does not hold the lock, therefore it can call the normal >> > function. On v2 we made a internal function that should be called if >> > the funciton is called directly by the driver. Please take a look to >> > the old comments. https://patchwork.linuxtv.org/patch/20460/ > > static int em28xx_v4l2_close(struct file *filp) > { > struct em28xx_fh *fh = filp->private_data; > struct em28xx *dev = fh->dev; > int errCode; > > em28xx_videodbg("users=%d\n", dev->users); > > mutex_lock(&dev->lock); > vb2_fop_release(filp); > ... > > vb2_fop_release(filp) will, with your patch, also try to get dev->lock. > > Sylwester's comment re em28xx is incorrect. dev->lock is not used as the video queue lock: $ git grep "lock =" drivers/media/usb/em28xx/ ... drivers/media/usb/em28xx/em28xx-video.c: dev->vdev->queue->lock = &dev->vb_queue_lock; drivers/media/usb/em28xx/em28xx-video.c: dev->vbi_dev->queue->lock = &dev->vb_vbi_queue_lock; There is a separate mutex for the video queue which needs to be acquired independently. -- 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
On 11/04/2013 03:24 PM, Sylwester Nawrocki wrote: > On 04/11/13 15:12, Hans Verkuil wrote: >> On 11/04/2013 02:54 PM, Ricardo Ribalda Delgado wrote: >>>> Hello Hans >>>> >>>> Thanks for your comments. >>>> >>>> Please take a look to v4 of this patch >>>> https://patchwork.linuxtv.org/patch/20529/ >>>> >>>> On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >>>>>> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote: >>>>>>>> From: Ricardo Ribalda <ricardo.ribalda@gmail.com> >>>>>>>> >>>>>>>> vb2_fop_relase does not held the lock although it is modifying the >>>>>>>> queue->owner field. >>>>>>>> >>>>>>>> This could lead to race conditions on the vb2_perform_io function >>>>>>>> when multiple applications are accessing the video device via >>>>>>>> read/write API: >>>>>> >>>>>> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c! >>>>>> >>>> >>>> em28xx-video does not hold the lock, therefore it can call the normal >>>> function. On v2 we made a internal function that should be called if >>>> the funciton is called directly by the driver. Please take a look to >>>> the old comments. https://patchwork.linuxtv.org/patch/20460/ >> >> static int em28xx_v4l2_close(struct file *filp) >> { >> struct em28xx_fh *fh = filp->private_data; >> struct em28xx *dev = fh->dev; >> int errCode; >> >> em28xx_videodbg("users=%d\n", dev->users); >> >> mutex_lock(&dev->lock); >> vb2_fop_release(filp); >> ... >> >> vb2_fop_release(filp) will, with your patch, also try to get dev->lock. >> >> Sylwester's comment re em28xx is incorrect. > > dev->lock is not used as the video queue lock: > > $ git grep "lock =" drivers/media/usb/em28xx/ > ... > drivers/media/usb/em28xx/em28xx-video.c: dev->vdev->queue->lock = &dev->vb_queue_lock; > drivers/media/usb/em28xx/em28xx-video.c: dev->vbi_dev->queue->lock = &dev->vb_vbi_queue_lock; > > There is a separate mutex for the video queue which needs to be acquired > independently. Darn, I missed that one. I was looking for it in em28xx_vdev_init(), which is where I expected the queue->lock to be set, if there was any. That said, wouldn't it be a good idea to swap the order: vb2_fop_release(filp); mutex_lock(&dev->lock); I don't believe there is a good reason for nesting mutexes here. Regards, Hans > > -- > 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 > -- 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 Hans and Sywester I have just posted a new patch. I think it fits the suggestions from both of you, please take a look to it and please post any comment. I will also send a patch about the em28xx, to swap the lock order. Thanks for your comments On Mon, Nov 4, 2013 at 4:19 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > On 11/04/2013 03:24 PM, Sylwester Nawrocki wrote: >> On 04/11/13 15:12, Hans Verkuil wrote: >>> On 11/04/2013 02:54 PM, Ricardo Ribalda Delgado wrote: >>>>> Hello Hans >>>>> >>>>> Thanks for your comments. >>>>> >>>>> Please take a look to v4 of this patch >>>>> https://patchwork.linuxtv.org/patch/20529/ >>>>> >>>>> On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >>>>>>> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote: >>>>>>>>> From: Ricardo Ribalda <ricardo.ribalda@gmail.com> >>>>>>>>> >>>>>>>>> vb2_fop_relase does not held the lock although it is modifying the >>>>>>>>> queue->owner field. >>>>>>>>> >>>>>>>>> This could lead to race conditions on the vb2_perform_io function >>>>>>>>> when multiple applications are accessing the video device via >>>>>>>>> read/write API: >>>>>>> >>>>>>> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c! >>>>>>> >>>>> >>>>> em28xx-video does not hold the lock, therefore it can call the normal >>>>> function. On v2 we made a internal function that should be called if >>>>> the funciton is called directly by the driver. Please take a look to >>>>> the old comments. https://patchwork.linuxtv.org/patch/20460/ >>> >>> static int em28xx_v4l2_close(struct file *filp) >>> { >>> struct em28xx_fh *fh = filp->private_data; >>> struct em28xx *dev = fh->dev; >>> int errCode; >>> >>> em28xx_videodbg("users=%d\n", dev->users); >>> >>> mutex_lock(&dev->lock); >>> vb2_fop_release(filp); >>> ... >>> >>> vb2_fop_release(filp) will, with your patch, also try to get dev->lock. >>> >>> Sylwester's comment re em28xx is incorrect. >> >> dev->lock is not used as the video queue lock: >> >> $ git grep "lock =" drivers/media/usb/em28xx/ >> ... >> drivers/media/usb/em28xx/em28xx-video.c: dev->vdev->queue->lock = &dev->vb_queue_lock; >> drivers/media/usb/em28xx/em28xx-video.c: dev->vbi_dev->queue->lock = &dev->vb_vbi_queue_lock; >> >> There is a separate mutex for the video queue which needs to be acquired >> independently. > > Darn, I missed that one. I was looking for it in em28xx_vdev_init(), which is > where I expected the queue->lock to be set, if there was any. > > That said, wouldn't it be a good idea to swap the order: > > vb2_fop_release(filp); > mutex_lock(&dev->lock); > > I don't believe there is a good reason for nesting mutexes here. > > Regards, > > Hans > >> >> -- >> 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 >> >
Here are the patches https://patchwork.linuxtv.org/patch/20668/ https://patchwork.linuxtv.org/patch/20669/ Thanks! On Wed, Nov 6, 2013 at 9:26 AM, Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote: > Hello Hans and Sywester > > I have just posted a new patch. I think it fits the suggestions from > both of you, please take a look to it and please post any comment. > > I will also send a patch about the em28xx, to swap the lock order. > > Thanks for your comments > > On Mon, Nov 4, 2013 at 4:19 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >> On 11/04/2013 03:24 PM, Sylwester Nawrocki wrote: >>> On 04/11/13 15:12, Hans Verkuil wrote: >>>> On 11/04/2013 02:54 PM, Ricardo Ribalda Delgado wrote: >>>>>> Hello Hans >>>>>> >>>>>> Thanks for your comments. >>>>>> >>>>>> Please take a look to v4 of this patch >>>>>> https://patchwork.linuxtv.org/patch/20529/ >>>>>> >>>>>> On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >>>>>>>> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote: >>>>>>>>>> From: Ricardo Ribalda <ricardo.ribalda@gmail.com> >>>>>>>>>> >>>>>>>>>> vb2_fop_relase does not held the lock although it is modifying the >>>>>>>>>> queue->owner field. >>>>>>>>>> >>>>>>>>>> This could lead to race conditions on the vb2_perform_io function >>>>>>>>>> when multiple applications are accessing the video device via >>>>>>>>>> read/write API: >>>>>>>> >>>>>>>> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c! >>>>>>>> >>>>>> >>>>>> em28xx-video does not hold the lock, therefore it can call the normal >>>>>> function. On v2 we made a internal function that should be called if >>>>>> the funciton is called directly by the driver. Please take a look to >>>>>> the old comments. https://patchwork.linuxtv.org/patch/20460/ >>>> >>>> static int em28xx_v4l2_close(struct file *filp) >>>> { >>>> struct em28xx_fh *fh = filp->private_data; >>>> struct em28xx *dev = fh->dev; >>>> int errCode; >>>> >>>> em28xx_videodbg("users=%d\n", dev->users); >>>> >>>> mutex_lock(&dev->lock); >>>> vb2_fop_release(filp); >>>> ... >>>> >>>> vb2_fop_release(filp) will, with your patch, also try to get dev->lock. >>>> >>>> Sylwester's comment re em28xx is incorrect. >>> >>> dev->lock is not used as the video queue lock: >>> >>> $ git grep "lock =" drivers/media/usb/em28xx/ >>> ... >>> drivers/media/usb/em28xx/em28xx-video.c: dev->vdev->queue->lock = &dev->vb_queue_lock; >>> drivers/media/usb/em28xx/em28xx-video.c: dev->vbi_dev->queue->lock = &dev->vb_vbi_queue_lock; >>> >>> There is a separate mutex for the video queue which needs to be acquired >>> independently. >> >> Darn, I missed that one. I was looking for it in em28xx_vdev_init(), which is >> where I expected the queue->lock to be set, if there was any. >> >> That said, wouldn't it be a good idea to swap the order: >> >> vb2_fop_release(filp); >> mutex_lock(&dev->lock); >> >> I don't believe there is a good reason for nesting mutexes here. >> >> Regards, >> >> Hans >> >>> >>> -- >>> 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 >>> >> > > > > -- > Ricardo Ribalda
diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c index fb27ff7..8192fe0 100644 --- a/drivers/media/platform/exynos4-is/fimc-capture.c +++ b/drivers/media/platform/exynos4-is/fimc-capture.c @@ -549,7 +549,7 @@ static int fimc_capture_release(struct file *file) vc->streaming = false; } - ret = vb2_fop_release(file); + ret = __vb2_fop_release(file); if (close) { clear_bit(ST_CAPT_BUSY, &fimc->state); diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c index e5798f7..cbe51cd 100644 --- a/drivers/media/platform/exynos4-is/fimc-lite.c +++ b/drivers/media/platform/exynos4-is/fimc-lite.c @@ -546,7 +546,7 @@ static int fimc_lite_release(struct file *file) mutex_unlock(&entity->parent->graph_mutex); } - vb2_fop_release(file); + __vb2_fop_release(file); pm_runtime_put(&fimc->pdev->dev); clear_bit(ST_FLITE_SUSPENDED, &fimc->state); diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c index 594c75e..f48d72a 100644 --- a/drivers/media/v4l2-core/videobuf2-core.c +++ b/drivers/media/v4l2-core/videobuf2-core.c @@ -2619,18 +2619,39 @@ int vb2_fop_mmap(struct file *file, struct vm_area_struct *vma) } EXPORT_SYMBOL_GPL(vb2_fop_mmap); -int vb2_fop_release(struct file *file) +static int _vb2_fop_release(struct file *file, bool lock_is_held) { struct video_device *vdev = video_devdata(file); + struct mutex *lock; if (file->private_data == vdev->queue->owner) { + if (lock_is_held) + lock = NULL; + else + 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); } + +int vb2_fop_release(struct file *file) +{ + return _vb2_fop_release(file, false); +} EXPORT_SYMBOL_GPL(vb2_fop_release); +int __vb2_fop_release(struct file *file) +{ + return _vb2_fop_release(file, true); +} +EXPORT_SYMBOL_GPL(__vb2_fop_release); + ssize_t vb2_fop_write(struct file *file, char __user *buf, size_t count, loff_t *ppos) { diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 6781258..76400fa 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -491,6 +491,7 @@ int vb2_ioctl_expbuf(struct file *file, void *priv, int vb2_fop_mmap(struct file *file, struct vm_area_struct *vma); int vb2_fop_release(struct file *file); +int __vb2_fop_release(struct file *file); ssize_t vb2_fop_write(struct file *file, char __user *buf, size_t count, loff_t *ppos); ssize_t vb2_fop_read(struct file *file, char __user *buf,