[v8,52/55,media] media-device: remove interfaces and interface links
diff mbox

Message ID fcb7fe56b016191b35dfc9fbc007ba1a1f35e837.1441540862.git.mchehab@osg.samsung.com
State New
Headers show

Commit Message

Mauro Carvalho Chehab Sept. 6, 2015, 12:03 p.m. UTC
Just like what's done with entities, when the media controller is
unregistered, release any interface and interface links that
might still be there.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

Comments

Hans Verkuil Sept. 11, 2015, 2:20 p.m. UTC | #1
On 09/06/2015 02:03 PM, Mauro Carvalho Chehab wrote:
> Just like what's done with entities, when the media controller is
> unregistered, release any interface and interface links that
> might still be there.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 7c37aeab05bb..0238885fcc74 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -574,6 +574,22 @@ void media_device_unregister(struct media_device *mdev)
>  {
>  	struct media_entity *entity;
>  	struct media_entity *next;
> +	struct media_link *link, *tmp_link;
> +	struct media_interface *intf, *tmp_intf;
> +
> +	/* Remove interface links from the media device */
> +	list_for_each_entry_safe(link, tmp_link, &mdev->links,
> +				 graph_obj.list) {
> +		media_gobj_remove(&link->graph_obj);
> +		kfree(link);
> +	}
> +
> +	/* Remove all interfaces from the media device */
> +	list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
> +				 graph_obj.list) {
> +		media_gobj_remove(&intf->graph_obj);
> +		kfree(intf);
> +	}
>  
>  	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
>  		media_device_unregister_entity(entity);
> @@ -651,7 +667,6 @@ void media_device_unregister_entity(struct media_entity *entity)
>  	/* Remove all data links that belong to this entity */
>  	list_for_each_entry_safe(link, tmp, &entity->links, list) {
>  		media_gobj_remove(&link->graph_obj);
> -		list_del(&link->list);
>  		kfree(link);
>  	}
>  
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index a37ccd2edfd5..cd4d767644df 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -206,6 +206,10 @@ void media_gobj_remove(struct media_gobj *gobj)
>  
>  	/* Remove the object from mdev list */
>  	list_del(&gobj->list);
> +
> +	/* Links have their own list - we need to drop them there too */
> +	if (media_type(gobj) == MEDIA_GRAPH_LINK)
> +		list_del(&gobj_to_link(gobj)->list);
>  }
>  
>  /**
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index ca4a4f23362f..fb5f0e21f137 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -153,7 +153,7 @@ struct media_entity {
>  };
>  
>  /**
> - * struct media_intf_devnode - Define a Kernel API interface
> + * struct media_interface - Define a Kernel API interface
>   *
>   * @graph_obj:		embedded graph object
>   * @list:		Linked list used to find other interfaces that belong
> @@ -163,6 +163,11 @@ struct media_entity {
>   *			uapi/media/media.h header, e. g.
>   *			MEDIA_INTF_T_*
>   * @flags:		Interface flags as defined at uapi/media/media.h
> + *
> + * NOTE: As media_device_unregister() will free the address of the
> + *	 media_interface, this structure should be embedded as the first
> + *	 element of the derived functions, in order for the address to be
> + *	 the same.
>   */
>  struct media_interface {
>  	struct media_gobj		graph_obj;
> @@ -179,11 +184,11 @@ struct media_interface {
>   * @minor:	Minor number of a device node
>   */
>  struct media_intf_devnode {
> -	struct media_interface		intf;
> +	struct media_interface	intf; /* must be first field in struct */
>  
>  	/* Should match the fields at media_v2_intf_devnode */
> -	u32				major;
> -	u32				minor;
> +	u32			major;
> +	u32			minor;
>  };
>  
>  static inline u32 media_entity_id(struct media_entity *entity)
> 

--
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
Laurent Pinchart Nov. 23, 2015, 9:22 p.m. UTC | #2
Hi Mauro,

Thank you for the patch.

On Sunday 06 September 2015 09:03:12 Mauro Carvalho Chehab wrote:
> Just like what's done with entities, when the media controller is
> unregistered, release any interface and interface links that
> might still be there.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 7c37aeab05bb..0238885fcc74 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -574,6 +574,22 @@ void media_device_unregister(struct media_device *mdev)
> {
>  	struct media_entity *entity;
>  	struct media_entity *next;
> +	struct media_link *link, *tmp_link;
> +	struct media_interface *intf, *tmp_intf;
> +
> +	/* Remove interface links from the media device */
> +	list_for_each_entry_safe(link, tmp_link, &mdev->links,
> +				 graph_obj.list) {
> +		media_gobj_remove(&link->graph_obj);
> +		kfree(link);
> +	}
> +
> +	/* Remove all interfaces from the media device */
> +	list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
> +				 graph_obj.list) {
> +		media_gobj_remove(&intf->graph_obj);
> +		kfree(intf);
> +	}
> 
>  	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
>  		media_device_unregister_entity(entity);
> @@ -651,7 +667,6 @@ void media_device_unregister_entity(struct media_entity
> *entity) /* Remove all data links that belong to this entity */
>  	list_for_each_entry_safe(link, tmp, &entity->links, list) {
>  		media_gobj_remove(&link->graph_obj);
> -		list_del(&link->list);
>  		kfree(link);

The link has already been freed in media_device_unregister(). You have access-
after-free and double-free issues here.

>  	}
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index a37ccd2edfd5..cd4d767644df 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -206,6 +206,10 @@ void media_gobj_remove(struct media_gobj *gobj)
> 
>  	/* Remove the object from mdev list */
>  	list_del(&gobj->list);
> +
> +	/* Links have their own list - we need to drop them there too */
> +	if (media_type(gobj) == MEDIA_GRAPH_LINK)
> +		list_del(&gobj_to_link(gobj)->list);

Please... That's a very bad layering violation. Let's not do that. Generic 
graph object code should not contain any type-specific code. You can create a 
media_link_remove() function for links that will remove the link from the 
entity links list and call media_gobj_remove().

>  }
> 
>  /**
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index ca4a4f23362f..fb5f0e21f137 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -153,7 +153,7 @@ struct media_entity {
>  };
> 
>  /**
> - * struct media_intf_devnode - Define a Kernel API interface
> + * struct media_interface - Define a Kernel API interface

This belongs to a different patch ;-)

>   *
>   * @graph_obj:		embedded graph object
>   * @list:		Linked list used to find other interfaces that belong
> @@ -163,6 +163,11 @@ struct media_entity {
>   *			uapi/media/media.h header, e. g.
>   *			MEDIA_INTF_T_*
>   * @flags:		Interface flags as defined at uapi/media/media.h
> + *
> + * NOTE: As media_device_unregister() will free the address of the
> + *	 media_interface, this structure should be embedded as the first
> + *	 element of the derived functions, in order for the address to be
> + *	 the same.

s/NOTE/DIRTY HACK/

Or, much better, let's fix it :-) If you want to be able to destroy graph 
object without needing to know their type, you can add a destroy operation to 
the graph objects and have per-type implementations. There's probably other 
options as well.

>   */
>  struct media_interface {
>  	struct media_gobj		graph_obj;
> @@ -179,11 +184,11 @@ struct media_interface {
>   * @minor:	Minor number of a device node
>   */
>  struct media_intf_devnode {
> -	struct media_interface		intf;
> +	struct media_interface	intf; /* must be first field in struct */
> 
>  	/* Should match the fields at media_v2_intf_devnode */
> -	u32				major;
> -	u32				minor;
> +	u32			major;
> +	u32			minor;

This doesn't belong to this patch either.

>  };
> 
>  static inline u32 media_entity_id(struct media_entity *entity)

Patch
diff mbox

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 7c37aeab05bb..0238885fcc74 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -574,6 +574,22 @@  void media_device_unregister(struct media_device *mdev)
 {
 	struct media_entity *entity;
 	struct media_entity *next;
+	struct media_link *link, *tmp_link;
+	struct media_interface *intf, *tmp_intf;
+
+	/* Remove interface links from the media device */
+	list_for_each_entry_safe(link, tmp_link, &mdev->links,
+				 graph_obj.list) {
+		media_gobj_remove(&link->graph_obj);
+		kfree(link);
+	}
+
+	/* Remove all interfaces from the media device */
+	list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
+				 graph_obj.list) {
+		media_gobj_remove(&intf->graph_obj);
+		kfree(intf);
+	}
 
 	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
 		media_device_unregister_entity(entity);
@@ -651,7 +667,6 @@  void media_device_unregister_entity(struct media_entity *entity)
 	/* Remove all data links that belong to this entity */
 	list_for_each_entry_safe(link, tmp, &entity->links, list) {
 		media_gobj_remove(&link->graph_obj);
-		list_del(&link->list);
 		kfree(link);
 	}
 
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index a37ccd2edfd5..cd4d767644df 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -206,6 +206,10 @@  void media_gobj_remove(struct media_gobj *gobj)
 
 	/* Remove the object from mdev list */
 	list_del(&gobj->list);
+
+	/* Links have their own list - we need to drop them there too */
+	if (media_type(gobj) == MEDIA_GRAPH_LINK)
+		list_del(&gobj_to_link(gobj)->list);
 }
 
 /**
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index ca4a4f23362f..fb5f0e21f137 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -153,7 +153,7 @@  struct media_entity {
 };
 
 /**
- * struct media_intf_devnode - Define a Kernel API interface
+ * struct media_interface - Define a Kernel API interface
  *
  * @graph_obj:		embedded graph object
  * @list:		Linked list used to find other interfaces that belong
@@ -163,6 +163,11 @@  struct media_entity {
  *			uapi/media/media.h header, e. g.
  *			MEDIA_INTF_T_*
  * @flags:		Interface flags as defined at uapi/media/media.h
+ *
+ * NOTE: As media_device_unregister() will free the address of the
+ *	 media_interface, this structure should be embedded as the first
+ *	 element of the derived functions, in order for the address to be
+ *	 the same.
  */
 struct media_interface {
 	struct media_gobj		graph_obj;
@@ -179,11 +184,11 @@  struct media_interface {
  * @minor:	Minor number of a device node
  */
 struct media_intf_devnode {
-	struct media_interface		intf;
+	struct media_interface	intf; /* must be first field in struct */
 
 	/* Should match the fields at media_v2_intf_devnode */
-	u32				major;
-	u32				minor;
+	u32			major;
+	u32			minor;
 };
 
 static inline u32 media_entity_id(struct media_entity *entity)