All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sylwester Nawrocki <s.nawrocki@samsung.com>
To: linux-media@vger.kernel.org
Cc: hverkuil@xs4all.nl, laurent.pinchart@ideasonboard.com,
	Sylwester Nawrocki <s.nawrocki@samsung.com>,
	Kyungmin Park <kyungmin.park@samsung.com>
Subject: [PATCH] V4L: Drop meaningless video_is_registered() call in v4l2_open()
Date: Fri, 02 Aug 2013 14:27:29 +0200	[thread overview]
Message-ID: <1375446449-27066-1-git-send-email-s.nawrocki@samsung.com> (raw)

As it currently stands this code doesn't protect against any races
between video device open() and its unregistration. Races could be
avoided by doing the video_is_registered() check protected by the
core mutex, while the video device unregistration is also done with
this mutex held.

The history of this code is that the second video_is_registered()
call has been added in commit ee6869afc922a9849979e49bb3bbcad7948
"V4L/DVB: v4l2: add core serialization lock" together with addition
of the core mutex support in fops:

        mutex_unlock(&videodev_lock);
-       if (vdev->fops->open)
-               ret = vdev->fops->open(filp);
+       if (vdev->fops->open) {
+               if (vdev->lock)
+                       mutex_lock(vdev->lock);
+               if (video_is_registered(vdev))
+                       ret = vdev->fops->open(filp);
+               else
+                       ret = -ENODEV;
+               if (vdev->lock)
+                       mutex_unlock(vdev->lock);
+       }

While commit cf5337358548b813479b58478539fc20ee86556c
"[media] v4l2-dev: remove V4L2_FL_LOCK_ALL_FOPS"
removed only code touching the mutex:

        mutex_unlock(&videodev_lock);
        if (vdev->fops->open) {
-               if (test_bit(V4L2_FL_LOCK_ALL_FOPS, &vdev->flags) &&
-                   mutex_lock_interruptible(vdev->lock)) {
-                       ret = -ERESTARTSYS;
-                       goto err;
-               }
                if (video_is_registered(vdev))
                        ret = vdev->fops->open(filp);
                else
                        ret = -ENODEV;
-               if (test_bit(V4L2_FL_LOCK_ALL_FOPS, &vdev->flags))
-                       mutex_unlock(vdev->lock);
        }

Remove the remaining video_is_registered() call as it doesn't provide
any real protection and just adds unnecessary overhead.

The drivers need to perform the unregistration check themselves inside
their file operation handlers, while holding respective mutex.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/v4l2-core/v4l2-dev.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index c8859d6..1743119 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -444,13 +444,9 @@ static int v4l2_open(struct inode *inode, struct file *filp)
 	/* and increase the device refcount */
 	video_get(vdev);
 	mutex_unlock(&videodev_lock);
-	if (vdev->fops->open) {
-		if (video_is_registered(vdev))
-			ret = vdev->fops->open(filp);
-		else
-			ret = -ENODEV;
-	}

+	if (vdev->fops->open)
+		ret = vdev->fops->open(filp);
 	if (vdev->debug)
 		printk(KERN_DEBUG "%s: open (%d)\n",
 			video_device_node_name(vdev), ret);
--
1.7.9.5


             reply	other threads:[~2013-08-02 12:27 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-08-02 12:27 Sylwester Nawrocki [this message]
2013-08-02 13:00 ` [PATCH] V4L: Drop meaningless video_is_registered() call in v4l2_open() Hans Verkuil
2013-08-07 16:49   ` Sylwester Nawrocki
2013-08-07 17:49     ` Hans Verkuil
2013-08-07 22:16       ` Laurent Pinchart
2013-08-08 12:36       ` Andrzej Hajda
2013-09-05 22:33       ` Sylwester Nawrocki
2013-09-09  9:07         ` Hans Verkuil
2013-09-09 10:00           ` Laurent Pinchart
2013-09-09 10:07             ` Hans Verkuil
2013-09-09 10:10               ` Laurent Pinchart
2013-09-09 10:17                 ` Hans Verkuil
2013-09-09 10:24                   ` Laurent Pinchart
2013-09-09 10:37                     ` Hans Verkuil
2013-09-09 10:48                       ` Hans Verkuil
2013-09-11 13:07           ` Sylwester Nawrocki
2013-09-11 14:01             ` Hans Verkuil
2013-09-12 10:19               ` Sylwester Nawrocki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1375446449-27066-1-git-send-email-s.nawrocki@samsung.com \
    --to=s.nawrocki@samsung.com \
    --cc=hverkuil@xs4all.nl \
    --cc=kyungmin.park@samsung.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-media@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.