Re: [PATCH v6 10/16] drm/msm/gem: Extract bookkeeping init helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Imported dma-buf objects are incorrectly placed into the unbacked LRU list instead of the pinned LRU list.

Pre-existing issues:
- [Critical] Failure path in `msm_gem_import` incorrectly treats the imported object as a native object, erroneously freeing the exporter's `sg_table` and dropping page references, leading to a NULL pointer dereference.
--

--- Patch [10]: [PATCH v6 10/16] drm/msm/gem: Extract bookkeeping init helper ---
commit e9867494af3de09952793735c8d5be5dc88c19a0
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/gem: Extract bookkeeping init helper

Clean up duplicated logic between import and new paths.

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index a358fb5ca6d73..961e22d3c7292 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -1233,10 +1233,21 @@ static int msm_gem_new_impl(struct drm_device *dev, uint32_t flags,
>  	return 0;
>  }
>  
> +static int msm_gem_init_bookkeeping(struct drm_gem_object *obj)
> +{
> +	struct msm_drm_private *priv = obj->dev->dev_private;
> +
> +	drm_gem_lru_move_tail(&priv->lru.unbacked, obj);

[Severity: Low]
Does this hardcode the unbacked LRU list for all callers?  The previous
code for imported objects placed them in the pinned LRU.  Will this now
route an imported, page-backed object into the unbacked LRU?

> +
> +	mutex_lock(&priv->obj_lock);
> +	list_add_tail(&to_msm_bo(obj)->node, &priv->objects);
> +	mutex_unlock(&priv->obj_lock);
> +
> +	return drm_gem_create_mmap_offset(obj);
> +}

[ ... ]

> @@ -1314,13 +1316,7 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
>  		goto fail;
>  	}
>  
> -	drm_gem_lru_move_tail(&priv->lru.pinned, obj);
> -
> -	mutex_lock(&priv->obj_lock);
> -	list_add_tail(&msm_obj->node, &priv->objects);
> -	mutex_unlock(&priv->obj_lock);
> -
> -	ret = drm_gem_create_mmap_offset(obj);
> +	ret = msm_gem_init_bookkeeping(obj);
>  	if (ret)
>  		goto fail;

[Severity: Critical]
This is a pre-existing issue, but can this failure path lead to a NULL
pointer dereference and double-free?

If msm_gem_init_bookkeeping() fails (e.g. from -ENOSPC) and we take this
goto fail path, drm_gem_object_put(obj) will be called.

Because import_attach is still NULL at this point, msm_gem_free_object()
will see drm_gem_is_imported(obj) as false and treat it as a native
object:

drivers/gpu/drm/msm/msm_gem.c:msm_gem_free_object() {
    if (drm_gem_is_imported(obj)) {
    ...
    } else {
        msm_gem_vunmap(obj);
        put_pages(obj);
    }
}

This would free the exporter's sgt using sg_free_table() and kfree().

It would also call drm_gem_put_pages() which unconditionally dereferences
obj->filp:

drivers/gpu/drm/drm_gem.c:drm_gem_put_pages() {
    mapping = file_inode(obj->filp)->i_mapping;
    ...
}

Since this object was initialized with drm_gem_private_object_init(),
obj->filp is NULL, causing a panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717204841.26120-1-robin.clark@oss.qualcomm.com?part=10