Re: [PATCH] drm/amd: fix potential memleak in err branch

2020-06-19 Thread Felix Kuehling
Hi Bernard,

I just applied a patch from another contributor for the kfd_topology
part of this. See
https://cgit.freedesktop.org/~agd5f/linux/commit/?h=amd-staging-drm-next=fc0fe8138309fee303bd12991f12b23f01bbf58c

Please rebase your patch on that. I believe that should only leave the
fixes in kfd_process.c.

Thanks,
  Felix

Am 2020-06-19 um 7:45 a.m. schrieb Bernard Zhao:
> The function kobject_init_and_add alloc memory like:
> kobject_init_and_add->kobject_add_varg->kobject_set_name_vargs
> ->kvasprintf_const->kstrdup_const->kstrdup->kmalloc_track_caller
> ->kmalloc_slab, in err branch this memory not free. If use
> kmemleak, this path maybe catched.
> These changes are to add kobject_put in kobject_init_and_add
> failed branch, fix potential memleak.
>
> Signed-off-by: Bernard Zhao 
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c  |  2 ++
>  drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 20 +++-
>  2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index d27221ddcdeb..5ee4d6cfb16d 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -124,6 +124,7 @@ void kfd_procfs_init(void)
>   if (ret) {
>   pr_warn("Could not create procfs proc folder");
>   /* If we fail to create the procfs, clean up */
> + kobject_put(procfs.kobj);
>   kfd_procfs_shutdown();
>   }
>  }
> @@ -428,6 +429,7 @@ struct kfd_process *kfd_create_process(struct file *filep)
>  (int)process->lead_thread->pid);
>   if (ret) {
>   pr_warn("Creating procfs pid directory failed");
> + kobject_put(process->kobj);
>   goto out;
>   }
>  
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> index bb77f7af2b6d..dc3c4149f860 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> @@ -632,8 +632,10 @@ static int kfd_build_sysfs_node_entry(struct 
> kfd_topology_device *dev,
>  
>   ret = kobject_init_and_add(dev->kobj_node, _type,
>   sys_props.kobj_nodes, "%d", id);
> - if (ret < 0)
> + if (ret < 0) {
> + kobject_put(dev->kobj_node);
>   return ret;
> + }
>  
>   dev->kobj_mem = kobject_create_and_add("mem_banks", dev->kobj_node);
>   if (!dev->kobj_mem)
> @@ -680,8 +682,10 @@ static int kfd_build_sysfs_node_entry(struct 
> kfd_topology_device *dev,
>   return -ENOMEM;
>   ret = kobject_init_and_add(mem->kobj, _type,
>   dev->kobj_mem, "%d", i);
> - if (ret < 0)
> + if (ret < 0) {
> + kobject_put(mem->kobj);
>   return ret;
> + }
>  
>   mem->attr.name = "properties";
>   mem->attr.mode = KFD_SYSFS_FILE_MODE;
> @@ -699,8 +703,10 @@ static int kfd_build_sysfs_node_entry(struct 
> kfd_topology_device *dev,
>   return -ENOMEM;
>   ret = kobject_init_and_add(cache->kobj, _type,
>   dev->kobj_cache, "%d", i);
> - if (ret < 0)
> + if (ret < 0) {
> + kobject_put(cache->kobj);
>   return ret;
> + }
>  
>   cache->attr.name = "properties";
>   cache->attr.mode = KFD_SYSFS_FILE_MODE;
> @@ -718,8 +724,10 @@ static int kfd_build_sysfs_node_entry(struct 
> kfd_topology_device *dev,
>   return -ENOMEM;
>   ret = kobject_init_and_add(iolink->kobj, _type,
>   dev->kobj_iolink, "%d", i);
> - if (ret < 0)
> + if (ret < 0) {
> + kobject_put(iolink->kobj);
>   return ret;
> + }
>  
>   iolink->attr.name = "properties";
>   iolink->attr.mode = KFD_SYSFS_FILE_MODE;
> @@ -798,8 +806,10 @@ static int kfd_topology_update_sysfs(void)
>   ret = kobject_init_and_add(sys_props.kobj_topology,
>   _type,  _device->kobj,
>   "topology");
> - if (ret < 0)
> + if (ret < 0) {
> + kobject_put(sys_props.kobj_topology);
>   return ret;
> + }
>  
>   sys_props.kobj_nodes = kobject_create_and_add("nodes",
>   sys_props.kobj_topology);


[PATCH] drm/amd: fix potential memleak in err branch

2020-06-19 Thread Bernard Zhao
The function kobject_init_and_add alloc memory like:
kobject_init_and_add->kobject_add_varg->kobject_set_name_vargs
->kvasprintf_const->kstrdup_const->kstrdup->kmalloc_track_caller
->kmalloc_slab, in err branch this memory not free. If use
kmemleak, this path maybe catched.
These changes are to add kobject_put in kobject_init_and_add
failed branch, fix potential memleak.

Signed-off-by: Bernard Zhao 
---
 drivers/gpu/drm/amd/amdkfd/kfd_process.c  |  2 ++
 drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 20 +++-
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index d27221ddcdeb..5ee4d6cfb16d 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -124,6 +124,7 @@ void kfd_procfs_init(void)
if (ret) {
pr_warn("Could not create procfs proc folder");
/* If we fail to create the procfs, clean up */
+   kobject_put(procfs.kobj);
kfd_procfs_shutdown();
}
 }
@@ -428,6 +429,7 @@ struct kfd_process *kfd_create_process(struct file *filep)
   (int)process->lead_thread->pid);
if (ret) {
pr_warn("Creating procfs pid directory failed");
+   kobject_put(process->kobj);
goto out;
}
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index bb77f7af2b6d..dc3c4149f860 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -632,8 +632,10 @@ static int kfd_build_sysfs_node_entry(struct 
kfd_topology_device *dev,
 
ret = kobject_init_and_add(dev->kobj_node, _type,
sys_props.kobj_nodes, "%d", id);
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(dev->kobj_node);
return ret;
+   }
 
dev->kobj_mem = kobject_create_and_add("mem_banks", dev->kobj_node);
if (!dev->kobj_mem)
@@ -680,8 +682,10 @@ static int kfd_build_sysfs_node_entry(struct 
kfd_topology_device *dev,
return -ENOMEM;
ret = kobject_init_and_add(mem->kobj, _type,
dev->kobj_mem, "%d", i);
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(mem->kobj);
return ret;
+   }
 
mem->attr.name = "properties";
mem->attr.mode = KFD_SYSFS_FILE_MODE;
@@ -699,8 +703,10 @@ static int kfd_build_sysfs_node_entry(struct 
kfd_topology_device *dev,
return -ENOMEM;
ret = kobject_init_and_add(cache->kobj, _type,
dev->kobj_cache, "%d", i);
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(cache->kobj);
return ret;
+   }
 
cache->attr.name = "properties";
cache->attr.mode = KFD_SYSFS_FILE_MODE;
@@ -718,8 +724,10 @@ static int kfd_build_sysfs_node_entry(struct 
kfd_topology_device *dev,
return -ENOMEM;
ret = kobject_init_and_add(iolink->kobj, _type,
dev->kobj_iolink, "%d", i);
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(iolink->kobj);
return ret;
+   }
 
iolink->attr.name = "properties";
iolink->attr.mode = KFD_SYSFS_FILE_MODE;
@@ -798,8 +806,10 @@ static int kfd_topology_update_sysfs(void)
ret = kobject_init_and_add(sys_props.kobj_topology,
_type,  _device->kobj,
"topology");
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(sys_props.kobj_topology);
return ret;
+   }
 
sys_props.kobj_nodes = kobject_create_and_add("nodes",
sys_props.kobj_topology);
-- 
2.17.1