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

[Andrew Davis]

On 10/30/22 6:37 AM, Dawei Li wrote:

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")

base-commit: 447fb14bf07905b880c9ed1ea92c53d6dd0649d7

Signed-off-by: Dawei Li <set_pte...@outlook.com>
---
  drivers/dma-buf/dma-heap.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index 8f5848aa144f..ff44c2777b04 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -243,11 +243,12 @@ struct dma_heap *dma_heap_add(const struct 
dma_heap_export_info *exp_info)
                        return ERR_PTR(-EINVAL);
                }
        }
-       mutex_unlock(&heap_list_lock);

heap = kzalloc(sizeof(*heap), GFP_KERNEL);

-       if (!heap)
+       if (!heap) {
+               mutex_unlock(&heap_list_lock);
                return ERR_PTR(-ENOMEM);
+       }

heap->name = exp_info->name;

        heap->ops = exp_info->ops;
@@ -284,7 +285,6 @@ struct dma_heap *dma_heap_add(const struct 
dma_heap_export_info *exp_info)
                goto err2;
        }
        /* Add heap to the list */
-       mutex_lock(&heap_list_lock);

Good catch!

In general I'd like to hold locks for as short a time as possible and
only bracket the lock associated structure (heap_list). How about we
move the duplicate name check to down here so they are both inside
this one locked section here.

I know this will mean we take a longer unwind error path
if the names are duplicated, but that should be rare and
this will keep all heap_list accesses together.

Thanks,
Andrew

        list_add(&heap->list, &heap_list);
        mutex_unlock(&heap_list_lock);

@@ -296,6 +296,7 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)

        xa_erase(&dma_heap_minors, minor);
  err0:
        kfree(heap);
+       mutex_unlock(&heap_list_lock);
        return err_ret;
  }