[PATCH v2] drm/gem: Internally test import_attach for imported objects

[PATCH v2] drm/gem: Internally test import_attach for imported objects

[Thomas Zimmermann]

Test struct drm_gem_object.import_attach.dmabuf to detect imported
objects. Warn if the stored state is inconsistent.

During object clenaup, the dma_buf field might be NULL. Testing it in
an object's free callback then incorrectly does a cleanup as for native
objects. Happens for calls to drm_mode_destroy_dumb_ioctl() that
clears the dma_buf field in drm_gem_object_exported_dma_buf_free().

v2:
- use import_attach.dmabuf instead of dma_buf (Christian)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
Reported-by: Andy Yan <andyshrk@163.com>
Closes: https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.Coremail.andyshrk@163.com/
Tested-by: Andy Yan <andyshrk@163.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Anusha Srivatsa <asrivats@redhat.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
---
 include/drm/drm_gem.h | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 9b71f7a9f3f8..464b9c7feec0 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -579,6 +579,21 @@ static inline bool drm_gem_object_is_shared_for_memory_stats(struct drm_gem_obje
 	return (obj->handle_count > 1) || obj->dma_buf;
 }
 
+/**
+ * drm_gem_owns_dma_buf() - Tests if GEM object backs a DMA-buffer object
+ * @obj: the GEM object
+ * @obj: the DMA buffer
+ *
+ * Returns:
+ * True if the DMA buffer refers to the GEM object's buffer.
+ */
+static inline bool drm_gem_owns_dma_buf(const struct drm_gem_object *obj,
+					const struct dma_buf *dma_buf)
+{
+	/* The dma-buf's priv field points to the original GEM object. */
+	return dma_buf->priv == obj;
+}
+
 /**
  * drm_gem_is_imported() - Tests if GEM object's buffer has been imported
  * @obj: the GEM object
@@ -588,8 +603,15 @@ static inline bool drm_gem_object_is_shared_for_memory_stats(struct drm_gem_obje
  */
 static inline bool drm_gem_is_imported(const struct drm_gem_object *obj)
 {
-	/* The dma-buf's priv field points to the original GEM object. */
-	return obj->dma_buf && (obj->dma_buf->priv != obj);
+	const struct dma_buf *dma_buf = NULL;
+
+	if (!obj->import_attach)
+		return false;
+
+	dma_buf = obj->import_attach->dmabuf;
+
+	/* Warn if we somehow reimported our own buffer. */
+	return !drm_WARN_ON_ONCE(obj->dev, !dma_buf || drm_gem_owns_dma_buf(obj, dma_buf));
 }
 
 #ifdef CONFIG_LOCKDEP
-- 
2.49.0

Re: [PATCH v2] drm/gem: Internally test import_attach for imported objects

[Boris Brezillon]

On Tue, 15 Apr 2025 16:02:20 +0200
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Test struct drm_gem_object.import_attach.dmabuf to detect imported
> objects. Warn if the stored state is inconsistent.
> 
> During object clenaup, the dma_buf field might be NULL. Testing it in
> an object's free callback then incorrectly does a cleanup as for native
> objects. Happens for calls to drm_mode_destroy_dumb_ioctl() that
> clears the dma_buf field in drm_gem_object_exported_dma_buf_free().
> 
> v2:
> - use import_attach.dmabuf instead of dma_buf (Christian)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
> Reported-by: Andy Yan <andyshrk@163.com>
> Closes: https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.Coremail.andyshrk@163.com/
> Tested-by: Andy Yan <andyshrk@163.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Anusha Srivatsa <asrivats@redhat.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-media@vger.kernel.org
> Cc: linaro-mm-sig@lists.linaro.org
> ---
>  include/drm/drm_gem.h | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 9b71f7a9f3f8..464b9c7feec0 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -579,6 +579,21 @@ static inline bool drm_gem_object_is_shared_for_memory_stats(struct drm_gem_obje
>  	return (obj->handle_count > 1) || obj->dma_buf;
>  }
>  
> +/**
> + * drm_gem_owns_dma_buf() - Tests if GEM object backs a DMA-buffer object
> + * @obj: the GEM object
> + * @obj: the DMA buffer
> + *
> + * Returns:
> + * True if the DMA buffer refers to the GEM object's buffer.
> + */
> +static inline bool drm_gem_owns_dma_buf(const struct drm_gem_object *obj,
> +					const struct dma_buf *dma_buf)
> +{
> +	/* The dma-buf's priv field points to the original GEM object. */
> +	return dma_buf->priv == obj;
> +}
> +
>  /**
>   * drm_gem_is_imported() - Tests if GEM object's buffer has been imported
>   * @obj: the GEM object
> @@ -588,8 +603,15 @@ static inline bool drm_gem_object_is_shared_for_memory_stats(struct drm_gem_obje
>   */
>  static inline bool drm_gem_is_imported(const struct drm_gem_object *obj)
>  {
> -	/* The dma-buf's priv field points to the original GEM object. */
> -	return obj->dma_buf && (obj->dma_buf->priv != obj);
> +	const struct dma_buf *dma_buf = NULL;
> +
> +	if (!obj->import_attach)
> +		return false;
> +
> +	dma_buf = obj->import_attach->dmabuf;
> +
> +	/* Warn if we somehow reimported our own buffer. */
> +	return !drm_WARN_ON_ONCE(obj->dev, !dma_buf || drm_gem_owns_dma_buf(obj, dma_buf));

I'm honestly not sure I see the point of checking
obj->import_attach->dmabuf. If obj->import_attach != NULL, we're sure
it's a foreign buffer already, otherwise we would get the original GEM
object which has ->import_attach=NULL. So why not go for a simple

	return obj->import_attach != NULL;

check, and extend the check when you get to implement imports without
import attachments (not sure what those would look like BTW).

Re: [PATCH v2] drm/gem: Internally test import_attach for imported objects

[Thomas Zimmermann]

Hi

Am 15.04.25 um 16:19 schrieb Boris Brezillon:
> On Tue, 15 Apr 2025 16:02:20 +0200
> Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
>> Test struct drm_gem_object.import_attach.dmabuf to detect imported
>> objects. Warn if the stored state is inconsistent.
>>
>> During object clenaup, the dma_buf field might be NULL. Testing it in
>> an object's free callback then incorrectly does a cleanup as for native
>> objects. Happens for calls to drm_mode_destroy_dumb_ioctl() that
>> clears the dma_buf field in drm_gem_object_exported_dma_buf_free().
>>
>> v2:
>> - use import_attach.dmabuf instead of dma_buf (Christian)
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper")
>> Reported-by: Andy Yan <andyshrk@163.com>
>> Closes: https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.Coremail.andyshrk@163.com/
>> Tested-by: Andy Yan <andyshrk@163.com>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Anusha Srivatsa <asrivats@redhat.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Simona Vetter <simona@ffwll.ch>
>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>> Cc: "Christian König" <christian.koenig@amd.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: linux-media@vger.kernel.org
>> Cc: linaro-mm-sig@lists.linaro.org
>> ---
>>   include/drm/drm_gem.h | 26 ++++++++++++++++++++++++--
>>   1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>> index 9b71f7a9f3f8..464b9c7feec0 100644
>> --- a/include/drm/drm_gem.h
>> +++ b/include/drm/drm_gem.h
>> @@ -579,6 +579,21 @@ static inline bool drm_gem_object_is_shared_for_memory_stats(struct drm_gem_obje
>>   	return (obj->handle_count > 1) || obj->dma_buf;
>>   }
>>   
>> +/**
>> + * drm_gem_owns_dma_buf() - Tests if GEM object backs a DMA-buffer object
>> + * @obj: the GEM object
>> + * @obj: the DMA buffer
>> + *
>> + * Returns:
>> + * True if the DMA buffer refers to the GEM object's buffer.
>> + */
>> +static inline bool drm_gem_owns_dma_buf(const struct drm_gem_object *obj,
>> +					const struct dma_buf *dma_buf)
>> +{
>> +	/* The dma-buf's priv field points to the original GEM object. */
>> +	return dma_buf->priv == obj;
>> +}
>> +
>>   /**
>>    * drm_gem_is_imported() - Tests if GEM object's buffer has been imported
>>    * @obj: the GEM object
>> @@ -588,8 +603,15 @@ static inline bool drm_gem_object_is_shared_for_memory_stats(struct drm_gem_obje
>>    */
>>   static inline bool drm_gem_is_imported(const struct drm_gem_object *obj)
>>   {
>> -	/* The dma-buf's priv field points to the original GEM object. */
>> -	return obj->dma_buf && (obj->dma_buf->priv != obj);
>> +	const struct dma_buf *dma_buf = NULL;
>> +
>> +	if (!obj->import_attach)
>> +		return false;
>> +
>> +	dma_buf = obj->import_attach->dmabuf;
>> +
>> +	/* Warn if we somehow reimported our own buffer. */
>> +	return !drm_WARN_ON_ONCE(obj->dev, !dma_buf || drm_gem_owns_dma_buf(obj, dma_buf));
> I'm honestly not sure I see the point of checking
> obj->import_attach->dmabuf. If obj->import_attach != NULL, we're sure
> it's a foreign buffer already, otherwise we would get the original GEM
> object which has ->import_attach=NULL. So why not go for a simple
>
> 	return obj->import_attach != NULL;
>
> check, and extend the check when you get to implement imports without
> import attachments (not sure what those would look like BTW).

I have no strong opinion. I just found it confusing to rely on 
import_attach when the dma_buf is what we originally imported.

Best regards
Thomas


-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)