Re: [PATCH v4] dma-buf: fix racing conflict of dma_heap_add()

[Christian König]

Hi Dawei,

from the technical description, coding style etc.. it looks clean to me, 
but I'm the completely wrong person to ask for a background check.

I have a high level understanding of how dma-heaps work, but not a 
single line of this code is from me.

Feel free to add my Acked-by, but Laura, John and others do you have any 
opinion?

Regards,
Christian.

Am 21.11.22 um 16:28 schrieb Dawei Li:
On Sat, Nov 05, 2022 at 12:05:36AM +0800, Dawei Li wrote:

Hi Christian,
May I have your opinion on this change?

Thanks,
Dawei

Racing conflict could be:
task A                 task B
list_for_each_entry
strcmp(h->name))
                        list_for_each_entry
                        strcmp(h->name)
kzalloc                kzalloc
......                 .....
device_create          device_create
list_add
                        list_add

The root cause is that task B has no idea about the fact someone
else(A) has inserted heap with same name when it calls list_add,
so a potential collision occurs.

Fixes: c02a81fba74f ("dma-buf: Add dma-buf heaps framework")
Signed-off-by: Dawei Li <set_pte...@outlook.com>
---
v1: 
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2FTYCP286MB2323950197F60FC3473123B7CA349%40TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM%2F&amp;data=05%7C01%7Cchristian.koenig%40amd.com%7C1989f31257fc458a27c508dacbd5078e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638046413707443203%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=OWPUPrIHGnzwXyQE4WlIthThwSuSK2y3Eq2Wq5zAzbo%3D&amp;reserved=0
v1->v2: Narrow down locking scope, check the existence of heap before
insertion, as suggested by Andrew Davis.
v2->v3: Remove double checking.
v3->v4: Minor coding style and patch formatting adjustment.
---
  drivers/dma-buf/dma-heap.c | 28 +++++++++++++++-------------
  1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index 8f5848aa144f..59d158873f4c 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -233,18 +233,6 @@ struct dma_heap *dma_heap_add(const struct 
dma_heap_export_info *exp_info)
                return ERR_PTR(-EINVAL);
        }
  
-	/* check the name is unique */
-       mutex_lock(&heap_list_lock);
-       list_for_each_entry(h, &heap_list, list) {
-               if (!strcmp(h->name, exp_info->name)) {
-                       mutex_unlock(&heap_list_lock);
-                       pr_err("dma_heap: Already registered heap named %s\n",
-                              exp_info->name);
-                       return ERR_PTR(-EINVAL);
-               }
-       }
-       mutex_unlock(&heap_list_lock);
-
        heap = kzalloc(sizeof(*heap), GFP_KERNEL);
        if (!heap)
                return ERR_PTR(-ENOMEM);
@@ -283,13 +271,27 @@ struct dma_heap *dma_heap_add(const struct 
dma_heap_export_info *exp_info)
                err_ret = ERR_CAST(dev_ret);
                goto err2;
        }
-       /* Add heap to the list */
+
        mutex_lock(&heap_list_lock);
+       /* check the name is unique */
+       list_for_each_entry(h, &heap_list, list) {
+               if (!strcmp(h->name, exp_info->name)) {
+                       mutex_unlock(&heap_list_lock);
+                       pr_err("dma_heap: Already registered heap named %s\n",
+                              exp_info->name);
+                       err_ret = ERR_PTR(-EINVAL);
+                       goto err3;
+               }
+       }
+
+       /* Add heap to the list */
        list_add(&heap->list, &heap_list);
        mutex_unlock(&heap_list_lock);
  
  	return heap;
  
+err3:
+       device_destroy(dma_heap_class, heap->heap_devt);
  err2:
        cdev_del(&heap->heap_cdev);
  err1:
--
2.25.1