[PATCH v2] media: mc-device.c: fix memleak in media_device_register_entity

[PATCH v2] media: mc-device.c: fix memleak in media_device_register_entity

[zhengbin]

In media_device_register_entity, if media_graph_walk_init fails,
need to free the previously memory.

Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
 drivers/media/mc/mc-device.c | 65 ++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 32 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index e19df51..da80883 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -575,6 +575,38 @@ static void media_device_release(struct media_devnode *devnode)
 	dev_dbg(devnode->parent, "Media device released\n");
 }

+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;
+
+	ida_free(&mdev->entity_internal_idx, entity->internal_idx);
+
+	/* 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) {
+			if (link->entity == entity)
+				__media_remove_intf_link(link);
+		}
+	}
+
+	/* Remove all data links that belong to this entity */
+	__media_entity_remove_links(entity);
+
+	/* Remove all pads that belong to this entity */
+	for (i = 0; i < entity->num_pads; i++)
+		media_gobj_destroy(&entity->pads[i].graph_obj);
+
+	/* Remove the entity */
+	media_gobj_destroy(&entity->graph_obj);
+
+	/* invoke entity_notify callbacks to handle entity removal?? */
+
+	entity->graph_obj.mdev = NULL;
+}
+
 /**
  * media_device_register_entity - Register an entity with a media device
  * @mdev:	The media device
@@ -632,6 +664,7 @@ int __must_check media_device_register_entity(struct media_device *mdev,
 		 */
 		ret = media_graph_walk_init(&new, mdev);
 		if (ret) {
+			__media_device_unregister_entity(entity);
 			mutex_unlock(&mdev->graph_mutex);
 			return ret;
 		}
@@ -644,38 +677,6 @@ int __must_check media_device_register_entity(struct media_device *mdev,
 }
 EXPORT_SYMBOL_GPL(media_device_register_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;
-
-	ida_free(&mdev->entity_internal_idx, entity->internal_idx);
-
-	/* 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) {
-			if (link->entity == entity)
-				__media_remove_intf_link(link);
-		}
-	}
-
-	/* Remove all data links that belong to this entity */
-	__media_entity_remove_links(entity);
-
-	/* Remove all pads that belong to this entity */
-	for (i = 0; i < entity->num_pads; i++)
-		media_gobj_destroy(&entity->pads[i].graph_obj);
-
-	/* Remove the entity */
-	media_gobj_destroy(&entity->graph_obj);
-
-	/* invoke entity_notify callbacks to handle entity removal?? */
-
-	entity->graph_obj.mdev = NULL;
-}
-
 void media_device_unregister_entity(struct media_entity *entity)
 {
 	struct media_device *mdev = entity->graph_obj.mdev;
--
2.7.4

Re: [PATCH v2] media: mc-device.c: fix memleak in media_device_register_entity

[zhengbin (A)]

ping

On 2019/8/19 9:51, zhengbin wrote:
> In media_device_register_entity, if media_graph_walk_init fails,
> need to free the previously memory.
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: zhengbin <zhengbin13@huawei.com>
> ---
>  drivers/media/mc/mc-device.c | 65 ++++++++++++++++++++++----------------------
>  1 file changed, 33 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> index e19df51..da80883 100644
> --- a/drivers/media/mc/mc-device.c
> +++ b/drivers/media/mc/mc-device.c
> @@ -575,6 +575,38 @@ static void media_device_release(struct media_devnode *devnode)
>  	dev_dbg(devnode->parent, "Media device released\n");
>  }
>
> +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;
> +
> +	ida_free(&mdev->entity_internal_idx, entity->internal_idx);
> +
> +	/* 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) {
> +			if (link->entity == entity)
> +				__media_remove_intf_link(link);
> +		}
> +	}
> +
> +	/* Remove all data links that belong to this entity */
> +	__media_entity_remove_links(entity);
> +
> +	/* Remove all pads that belong to this entity */
> +	for (i = 0; i < entity->num_pads; i++)
> +		media_gobj_destroy(&entity->pads[i].graph_obj);
> +
> +	/* Remove the entity */
> +	media_gobj_destroy(&entity->graph_obj);
> +
> +	/* invoke entity_notify callbacks to handle entity removal?? */
> +
> +	entity->graph_obj.mdev = NULL;
> +}
> +
>  /**
>   * media_device_register_entity - Register an entity with a media device
>   * @mdev:	The media device
> @@ -632,6 +664,7 @@ int __must_check media_device_register_entity(struct media_device *mdev,
>  		 */
>  		ret = media_graph_walk_init(&new, mdev);
>  		if (ret) {
> +			__media_device_unregister_entity(entity);
>  			mutex_unlock(&mdev->graph_mutex);
>  			return ret;
>  		}
> @@ -644,38 +677,6 @@ int __must_check media_device_register_entity(struct media_device *mdev,
>  }
>  EXPORT_SYMBOL_GPL(media_device_register_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;
> -
> -	ida_free(&mdev->entity_internal_idx, entity->internal_idx);
> -
> -	/* 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) {
> -			if (link->entity == entity)
> -				__media_remove_intf_link(link);
> -		}
> -	}
> -
> -	/* Remove all data links that belong to this entity */
> -	__media_entity_remove_links(entity);
> -
> -	/* Remove all pads that belong to this entity */
> -	for (i = 0; i < entity->num_pads; i++)
> -		media_gobj_destroy(&entity->pads[i].graph_obj);
> -
> -	/* Remove the entity */
> -	media_gobj_destroy(&entity->graph_obj);
> -
> -	/* invoke entity_notify callbacks to handle entity removal?? */
> -
> -	entity->graph_obj.mdev = NULL;
> -}
> -
>  void media_device_unregister_entity(struct media_entity *entity)
>  {
>  	struct media_device *mdev = entity->graph_obj.mdev;
> --
> 2.7.4
>
>
> .
>

Re: [PATCH v2] media: mc-device.c: fix memleak in media_device_register_entity

[Laurent Pinchart]

Hello Zhengbin,

On Mon, Aug 19, 2019 at 09:51:30AM +0800, zhengbin wrote:
> In media_device_register_entity, if media_graph_walk_init fails,
> need to free the previously memory.
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: zhengbin <zhengbin13@huawei.com>

This looks good to me.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

and applied to my tree, for v5.5.

> ---
>  drivers/media/mc/mc-device.c | 65 ++++++++++++++++++++++----------------------
>  1 file changed, 33 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> index e19df51..da80883 100644
> --- a/drivers/media/mc/mc-device.c
> +++ b/drivers/media/mc/mc-device.c
> @@ -575,6 +575,38 @@ static void media_device_release(struct media_devnode *devnode)
>  	dev_dbg(devnode->parent, "Media device released\n");
>  }
> 
> +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;
> +
> +	ida_free(&mdev->entity_internal_idx, entity->internal_idx);
> +
> +	/* 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) {
> +			if (link->entity == entity)
> +				__media_remove_intf_link(link);
> +		}
> +	}
> +
> +	/* Remove all data links that belong to this entity */
> +	__media_entity_remove_links(entity);
> +
> +	/* Remove all pads that belong to this entity */
> +	for (i = 0; i < entity->num_pads; i++)
> +		media_gobj_destroy(&entity->pads[i].graph_obj);
> +
> +	/* Remove the entity */
> +	media_gobj_destroy(&entity->graph_obj);
> +
> +	/* invoke entity_notify callbacks to handle entity removal?? */
> +
> +	entity->graph_obj.mdev = NULL;
> +}
> +
>  /**
>   * media_device_register_entity - Register an entity with a media device
>   * @mdev:	The media device
> @@ -632,6 +664,7 @@ int __must_check media_device_register_entity(struct media_device *mdev,
>  		 */
>  		ret = media_graph_walk_init(&new, mdev);
>  		if (ret) {
> +			__media_device_unregister_entity(entity);
>  			mutex_unlock(&mdev->graph_mutex);
>  			return ret;
>  		}
> @@ -644,38 +677,6 @@ int __must_check media_device_register_entity(struct media_device *mdev,
>  }
>  EXPORT_SYMBOL_GPL(media_device_register_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;
> -
> -	ida_free(&mdev->entity_internal_idx, entity->internal_idx);
> -
> -	/* 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) {
> -			if (link->entity == entity)
> -				__media_remove_intf_link(link);
> -		}
> -	}
> -
> -	/* Remove all data links that belong to this entity */
> -	__media_entity_remove_links(entity);
> -
> -	/* Remove all pads that belong to this entity */
> -	for (i = 0; i < entity->num_pads; i++)
> -		media_gobj_destroy(&entity->pads[i].graph_obj);
> -
> -	/* Remove the entity */
> -	media_gobj_destroy(&entity->graph_obj);
> -
> -	/* invoke entity_notify callbacks to handle entity removal?? */
> -
> -	entity->graph_obj.mdev = NULL;
> -}
> -
>  void media_device_unregister_entity(struct media_entity *entity)
>  {
>  	struct media_device *mdev = entity->graph_obj.mdev;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: mc-device.c: fix memleak in media_device_register_entity

[Sakari Ailus]

On Fri, Sep 06, 2019 at 12:12:03PM +0300, Laurent Pinchart wrote:
> Hello Zhengbin,
> 
> On Mon, Aug 19, 2019 at 09:51:30AM +0800, zhengbin wrote:
> > In media_device_register_entity, if media_graph_walk_init fails,
> > need to free the previously memory.
> > 
> > Reported-by: Hulk Robot <hulkci@huawei.com>
> > Signed-off-by: zhengbin <zhengbin13@huawei.com>
> 
> This looks good to me.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> and applied to my tree, for v5.5.

Hmm. This is in my tree as well. Would you like to drop it from yours? :-)

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2] media: mc-device.c: fix memleak in media_device_register_entity

[Laurent Pinchart]

Hi Sakari,

On Fri, Sep 06, 2019 at 01:11:34PM +0300, Sakari Ailus wrote:
> On Fri, Sep 06, 2019 at 12:12:03PM +0300, Laurent Pinchart wrote:
> > On Mon, Aug 19, 2019 at 09:51:30AM +0800, zhengbin wrote:
> > > In media_device_register_entity, if media_graph_walk_init fails,
> > > need to free the previously memory.
> > > 
> > > Reported-by: Hulk Robot <hulkci@huawei.com>
> > > Signed-off-by: zhengbin <zhengbin13@huawei.com>
> > 
> > This looks good to me.
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > and applied to my tree, for v5.5.
> 
> Hmm. This is in my tree as well. Would you like to drop it from yours? :-)

Sure :-)

I wonder if we should setup a shared git tree for this.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: mc-device.c: fix memleak in media_device_register_entity

[Sakari Ailus]

On Fri, Sep 06, 2019 at 01:36:02PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Fri, Sep 06, 2019 at 01:11:34PM +0300, Sakari Ailus wrote:
> > On Fri, Sep 06, 2019 at 12:12:03PM +0300, Laurent Pinchart wrote:
> > > On Mon, Aug 19, 2019 at 09:51:30AM +0800, zhengbin wrote:
> > > > In media_device_register_entity, if media_graph_walk_init fails,
> > > > need to free the previously memory.
> > > > 
> > > > Reported-by: Hulk Robot <hulkci@huawei.com>
> > > > Signed-off-by: zhengbin <zhengbin13@huawei.com>
> > > 
> > > This looks good to me.
> > > 
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > and applied to my tree, for v5.5.
> > 
> > Hmm. This is in my tree as well. Would you like to drop it from yours? :-)
> 
> Sure :-)
> 
> I wonder if we should setup a shared git tree for this.

I think following the patchwork status should be enough for now.

I marked it as accepted but forgot to assign it myself in this case. I'll
try assign them as well. It'd be actually nice if Patchwork did that by
default, as this is generally what needs to be done.

What do you think?

If there would be more patches, it'd make sense to rethink this IMO. But
two people eagerly merging patches is much better than none at all. ;)

-- 
Sakari Ailus
sakari.ailus@linux.intel.com