[2/2,media] media-device: better lock media_device_unregister()
diff mbox

Message ID cda0486f763fc1c2f5267c3a0806cf297317301b.1450176187.git.mchehab@osg.samsung.com
State New
Headers show

Commit Message

Mauro Carvalho Chehab Dec. 15, 2015, 10:43 a.m. UTC
If media_device_unregister() is called by two different
drivers, a race condition may happen, as the check if the
device is not registered is not protected.

Move the spin_lock() to happen earlier in the function, in order
to prevent such race condition.

Reported-by: Shuah Khan <shuahkh@osg.samsung.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
 drivers/media/media-device.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

Comments

Javier Martinez Canillas Dec. 21, 2015, 12:38 p.m. UTC | #1
Hello Mauro,

On 12/15/2015 07:43 AM, Mauro Carvalho Chehab wrote:
> If media_device_unregister() is called by two different
> drivers, a race condition may happen, as the check if the
> device is not registered is not protected.
> 
> Move the spin_lock() to happen earlier in the function, in order
> to prevent such race condition.
> 
> Reported-by: Shuah Khan <shuahkh@osg.samsung.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---

The patch looks good and I also tested it on an OMAP3 IGEPv2 board:

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,

Patch
diff mbox

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 1222fa642ad8..189c2ba8c3d3 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -573,18 +573,13 @@  EXPORT_SYMBOL_GPL(media_device_register_entity);
  * If the entity has never been registered this function will return
  * immediately.
  */
-void media_device_unregister_entity(struct media_entity *entity)
+static void __media_device_unregister_entity(struct media_entity *entity)
 {
 	struct media_device *mdev = entity->graph_obj.mdev;
 	struct media_link *link, *tmp;
 	struct media_interface *intf;
 	unsigned int i;
 
-	if (mdev == NULL)
-		return;
-
-	spin_lock(&mdev->lock);
-
 	/* Remove all interface links pointing to this entity */
 	list_for_each_entry(intf, &mdev->interfaces, graph_obj.list) {
 		list_for_each_entry_safe(link, tmp, &intf->links, list) {
@@ -603,11 +598,23 @@  void media_device_unregister_entity(struct media_entity *entity)
 	/* Remove the entity */
 	media_gobj_destroy(&entity->graph_obj);
 
-	spin_unlock(&mdev->lock);
 	entity->graph_obj.mdev = NULL;
 }
+
+void media_device_unregister_entity(struct media_entity *entity)
+{
+	struct media_device *mdev = entity->graph_obj.mdev;
+
+	if (mdev == NULL)
+		return;
+
+	spin_lock(&mdev->lock);
+	__media_device_unregister_entity(entity);
+	spin_unlock(&mdev->lock);
+}
 EXPORT_SYMBOL_GPL(media_device_unregister_entity);
 
+
 /**
  * media_device_register - register a media device
  * @mdev:	The media device
@@ -666,22 +673,29 @@  void media_device_unregister(struct media_device *mdev)
 	struct media_entity *next;
 	struct media_interface *intf, *tmp_intf;
 
+	if (mdev == NULL)
+		return;
+
+	spin_lock(&mdev->lock);
+
 	/* Check if mdev was ever registered at all */
-	if (!media_devnode_is_registered(&mdev->devnode))
+	if (!media_devnode_is_registered(&mdev->devnode)) {
+		spin_unlock(&mdev->lock);
 		return;
+	}
 
 	/* Remove all entities from the media device */
 	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
-		media_device_unregister_entity(entity);
+		__media_device_unregister_entity(entity);
 
 	/* Remove all interfaces from the media device */
-	spin_lock(&mdev->lock);
 	list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
 				 graph_obj.list) {
 		__media_remove_intf_links(intf);
 		media_gobj_destroy(&intf->graph_obj);
 		kfree(intf);
 	}
+
 	spin_unlock(&mdev->lock);
 
 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);