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

[Laurent Pinchart]

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);
>     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 }

-- 
Regards,

Laurent Pinchart