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
