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

[PATCH] 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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index e19df51..939be00 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -632,6 +632,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;
 		}
--
2.7.4

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

[Sakari Ailus]

Hi Zhenbin,

On Fri, Aug 16, 2019 at 11:33:02AM +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>
> ---
>  drivers/media/mc/mc-device.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> index e19df51..939be00 100644
> --- a/drivers/media/mc/mc-device.c
> +++ b/drivers/media/mc/mc-device.c
> @@ -632,6 +632,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;
>  		}

This does not compile; the function is defined after it's used here.

Could you move the function definition up, just above this function,
please?

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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5929 bytes --]

Hi zhengbin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[cannot apply to v5.3-rc4 next-20190814]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/zhengbin/media-mc-device-c-fix-memleak-in-media_device_register_entity/20190816-191628
base:   git://linuxtv.org/media_tree.git master
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=sparc64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   drivers/media//mc/mc-device.c: In function 'media_device_register_entity':
>> drivers/media//mc/mc-device.c:635:4: error: implicit declaration of function '__media_device_unregister_entity'; did you mean 'media_device_unregister_entity'? [-Werror=implicit-function-declaration]
       __media_device_unregister_entity(entity);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       media_device_unregister_entity
   drivers/media//mc/mc-device.c: At top level:
>> drivers/media//mc/mc-device.c:648:13: warning: conflicting types for '__media_device_unregister_entity'
    static void __media_device_unregister_entity(struct media_entity *entity)
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/media//mc/mc-device.c:648:13: error: static declaration of '__media_device_unregister_entity' follows non-static declaration
   drivers/media//mc/mc-device.c:635:4: note: previous implicit declaration of '__media_device_unregister_entity' was here
       __media_device_unregister_entity(entity);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +635 drivers/media//mc/mc-device.c

   577	
   578	/**
   579	 * media_device_register_entity - Register an entity with a media device
   580	 * @mdev:	The media device
   581	 * @entity:	The entity
   582	 */
   583	int __must_check media_device_register_entity(struct media_device *mdev,
   584						      struct media_entity *entity)
   585	{
   586		struct media_entity_notify *notify, *next;
   587		unsigned int i;
   588		int ret;
   589	
   590		if (entity->function == MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN ||
   591		    entity->function == MEDIA_ENT_F_UNKNOWN)
   592			dev_warn(mdev->dev,
   593				 "Entity type for entity %s was not initialized!\n",
   594				 entity->name);
   595	
   596		/* Warn if we apparently re-register an entity */
   597		WARN_ON(entity->graph_obj.mdev != NULL);
   598		entity->graph_obj.mdev = mdev;
   599		INIT_LIST_HEAD(&entity->links);
   600		entity->num_links = 0;
   601		entity->num_backlinks = 0;
   602	
   603		ret = ida_alloc_min(&mdev->entity_internal_idx, 1, GFP_KERNEL);
   604		if (ret < 0)
   605			return ret;
   606		entity->internal_idx = ret;
   607	
   608		mutex_lock(&mdev->graph_mutex);
   609		mdev->entity_internal_idx_max =
   610			max(mdev->entity_internal_idx_max, entity->internal_idx);
   611	
   612		/* Initialize media_gobj embedded at the entity */
   613		media_gobj_create(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj);
   614	
   615		/* Initialize objects at the pads */
   616		for (i = 0; i < entity->num_pads; i++)
   617			media_gobj_create(mdev, MEDIA_GRAPH_PAD,
   618				       &entity->pads[i].graph_obj);
   619	
   620		/* invoke entity_notify callbacks */
   621		list_for_each_entry_safe(notify, next, &mdev->entity_notify, list)
   622			notify->notify(entity, notify->notify_data);
   623	
   624		if (mdev->entity_internal_idx_max
   625		    >= mdev->pm_count_walk.ent_enum.idx_max) {
   626			struct media_graph new = { .top = 0 };
   627	
   628			/*
   629			 * Initialise the new graph walk before cleaning up
   630			 * the old one in order not to spoil the graph walk
   631			 * object of the media device if graph walk init fails.
   632			 */
   633			ret = media_graph_walk_init(&new, mdev);
   634			if (ret) {
 > 635				__media_device_unregister_entity(entity);
   636				mutex_unlock(&mdev->graph_mutex);
   637				return ret;
   638			}
   639			media_graph_walk_cleanup(&mdev->pm_count_walk);
   640			mdev->pm_count_walk = new;
   641		}
   642		mutex_unlock(&mdev->graph_mutex);
   643	
   644		return 0;
   645	}
   646	EXPORT_SYMBOL_GPL(media_device_register_entity);
   647	
 > 648	static void __media_device_unregister_entity(struct media_entity *entity)
   649	{
   650		struct media_device *mdev = entity->graph_obj.mdev;
   651		struct media_link *link, *tmp;
   652		struct media_interface *intf;
   653		unsigned int i;
   654	
   655		ida_free(&mdev->entity_internal_idx, entity->internal_idx);
   656	
   657		/* Remove all interface links pointing to this entity */
   658		list_for_each_entry(intf, &mdev->interfaces, graph_obj.list) {
   659			list_for_each_entry_safe(link, tmp, &intf->links, list) {
   660				if (link->entity == entity)
   661					__media_remove_intf_link(link);
   662			}
   663		}
   664	
   665		/* Remove all data links that belong to this entity */
   666		__media_entity_remove_links(entity);
   667	
   668		/* Remove all pads that belong to this entity */
   669		for (i = 0; i < entity->num_pads; i++)
   670			media_gobj_destroy(&entity->pads[i].graph_obj);
   671	
   672		/* Remove the entity */
   673		media_gobj_destroy(&entity->graph_obj);
   674	
   675		/* invoke entity_notify callbacks to handle entity removal?? */
   676	
   677		entity->graph_obj.mdev = NULL;
   678	}
   679	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 58651 bytes --]