[bug report] drm/omap: gem: Replace struct_mutex usage with omap_obj private lock

[Tomi Valkeinen]

On 31/07/2024 15:26, Laurent Pinchart wrote:
> Hi Dan,
> 
> (CC'ing Tomi)
> 
> Thank for the report. It indeed seems that something is wrong.
> 
> Tomi, could you handle this and send a fix ?
> 
> On Tue, Jul 30, 2024 at 05:01:35PM -0500, Dan Carpenter wrote:
>> Hello Laurent Pinchart,
>>
>> Commit 3cbd0c587b12 ("drm/omap: gem: Replace struct_mutex usage with
>> omap_obj private lock") from May 26, 2018 (linux-next), leads to the
>> following Smatch static checker warning:
>>
>> 	drivers/gpu/drm/omapdrm/omap_gem.c:1435 omap_gem_new_dmabuf()
>> 	warn: 'omap_obj' was already freed. (line 1434)
>>
>> drivers/gpu/drm/omapdrm/omap_gem.c
>>      1386 struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size,
>>      1387                                            struct sg_table *sgt)
>>      1388 {
>>      1389         struct omap_drm_private *priv = dev->dev_private;
>>      1390         struct omap_gem_object *omap_obj;
>>      1391         struct drm_gem_object *obj;
>>      1392         union omap_gem_size gsize;
>>      1393
>>      1394         /* Without a DMM only physically contiguous buffers can be supported. */
>>      1395         if (!omap_gem_sgt_is_contiguous(sgt, size) && !priv->has_dmm)
>>      1396                 return ERR_PTR(-EINVAL);
>>      1397
>>      1398         gsize.bytes = PAGE_ALIGN(size);
>>      1399         obj = omap_gem_new(dev, gsize, OMAP_BO_MEM_DMABUF | OMAP_BO_WC);
>>      1400         if (!obj)
>>      1401                 return ERR_PTR(-ENOMEM);
>>      1402
>>      1403         omap_obj = to_omap_bo(obj);
>>                   ^^^^^^^^^^^^^^^^^^^^^^^^^^
>> This is omap_obj
>>
>>      1404
>>      1405         mutex_lock(&omap_obj->lock);

I wonder why we even lock it. We just allocated it above, no one else 
can be using it.

Not only that, but omap_gem_free_object(), which is called in the error 
paths, _also_ takes the same lock.

I think we can just drop the locking from this function. But first I 
need to figure out how to run this piece of code to test it...

  Tomi

>>      1406
>>      1407         omap_obj->sgt = sgt;
>>      1408
>>      1409         if (omap_gem_sgt_is_contiguous(sgt, size)) {
>>      1410                 omap_obj->dma_addr = sg_dma_address(sgt->sgl);
>>      1411         } else {
>>      1412                 /* Create pages list from sgt */
>>      1413                 struct page **pages;
>>      1414                 unsigned int npages;
>>      1415                 unsigned int ret;
>>      1416
>>      1417                 npages = DIV_ROUND_UP(size, PAGE_SIZE);
>>      1418                 pages = kcalloc(npages, sizeof(*pages), GFP_KERNEL);
>>      1419                 if (!pages) {
>>      1420                         omap_gem_free_object(obj);
>>                                                        ^^^
>> It gets free inside this function
>>
>>      1421                         obj = ERR_PTR(-ENOMEM);
>>      1422                         goto done;
>>      1423                 }
>>      1424
>>      1425                 omap_obj->pages = pages;
>>      1426                 ret = drm_prime_sg_to_page_array(sgt, pages, npages);
>>      1427                 if (ret) {
>>      1428                         omap_gem_free_object(obj);
>>                                                        ^^^
>> Same
>>
>>      1429                         obj = ERR_PTR(-ENOMEM);
>>      1430                         goto done;
>>
>> So I think we can just return directly instead of unlocking.
>>
>>      1431                 }
>>      1432         }
>>      1433
>>      1434 done:
>> --> 1435         mutex_unlock(&omap_obj->lock);
>>      1436         return obj;
>>      1437 }
>