Re: [PATCH 2/2] [media] media-device: better lock media_device_unregister()

[Javier Martinez Canillas]

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 
> Signed-off-by: Mauro Carvalho Chehab 
> ---

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

Reviewed-by: Javier Martinez Canillas 
Tested-by: Javier Martinez Canillas 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] [media] media-device: better lock media_device_unregister()

[Mauro Carvalho Chehab]

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 
Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/media-device.c | 34 --
 1 file changed, 24 insertions(+), 10 deletions(-)

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(>lock);
-
/* Remove all interface links pointing to this entity */
list_for_each_entry(intf, >interfaces, graph_obj.list) {
list_for_each_entry_safe(link, tmp, >links, list) {
@@ -603,11 +598,23 @@ void media_device_unregister_entity(struct media_entity 
*entity)
/* Remove the entity */
media_gobj_destroy(>graph_obj);
 
-   spin_unlock(>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(>lock);
+   __media_device_unregister_entity(entity);
+   spin_unlock(>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(>lock);
+
/* Check if mdev was ever registered at all */
-   if (!media_devnode_is_registered(>devnode))
+   if (!media_devnode_is_registered(>devnode)) {
+   spin_unlock(>lock);
return;
+   }
 
/* Remove all entities from the media device */
list_for_each_entry_safe(entity, next, >entities, graph_obj.list)
-   media_device_unregister_entity(entity);
+   __media_device_unregister_entity(entity);
 
/* Remove all interfaces from the media device */
-   spin_lock(>lock);
list_for_each_entry_safe(intf, tmp_intf, >interfaces,
 graph_obj.list) {
__media_remove_intf_links(intf);
media_gobj_destroy(>graph_obj);
kfree(intf);
}
+
spin_unlock(>lock);
 
device_remove_file(>devnode.dev, _attr_model);
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html