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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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