diff mbox series

[v2,1/2] drm/prime: reject DMA-BUF attach when get_sg_table is missing

Message ID 20230302143502.500661-1-contact@emersion.fr (mailing list archive)
State New, archived
Headers show
Series [v2,1/2] drm/prime: reject DMA-BUF attach when get_sg_table is missing | expand

Commit Message

Simon Ser March 2, 2023, 2:35 p.m. UTC
drm_gem_map_dma_buf() requires drm_gem_object_funcs.get_sg_table
to be implemented, or else WARNs.

Allow drivers to leave this hook unimplemented to implement purely
local DMA-BUFs (ie, DMA-BUFs which cannot be imported anywhere
else but the device which allocated them). In that case, reject
imports to other devices in drm_gem_map_attach().

v2: new patch

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Tian Tao <tiantao6@hisilicon.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Christian König <christian.koenig@amd.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/drm_prime.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Thomas Zimmermann March 2, 2023, 3:03 p.m. UTC | #1
Hi

Am 02.03.23 um 15:35 schrieb Simon Ser:
> drm_gem_map_dma_buf() requires drm_gem_object_funcs.get_sg_table
> to be implemented, or else WARNs.
> 
> Allow drivers to leave this hook unimplemented to implement purely
> local DMA-BUFs (ie, DMA-BUFs which cannot be imported anywhere
> else but the device which allocated them). In that case, reject
> imports to other devices in drm_gem_map_attach().
> 
> v2: new patch
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Tian Tao <tiantao6@hisilicon.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/gpu/drm/drm_prime.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index f924b8b4ab6b..ab1d21d63a03 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -544,7 +544,8 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
>    * Optional pinning of buffers is handled at dma-buf attach and detach time in
>    * drm_gem_map_attach() and drm_gem_map_detach(). Backing storage itself is
>    * handled by drm_gem_map_dma_buf() and drm_gem_unmap_dma_buf(), which relies on
> - * &drm_gem_object_funcs.get_sg_table.
> + * &drm_gem_object_funcs.get_sg_table. If &drm_gem_object_funcs.get_sg_table is
> + * unimplemented, exports into another device are rejected.
>    *
>    * For kernel-internal access there's drm_gem_dmabuf_vmap() and
>    * drm_gem_dmabuf_vunmap(). Userspace mmap support is provided by
> @@ -583,6 +584,9 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
>   {
>   	struct drm_gem_object *obj = dma_buf->priv;
>   
> +	if (!obj->funcs->get_sg_table)
> +		return -EOPNOTSUPP;

-ENOSYS please.

Best regards
Thomas

> +
>   	return drm_gem_pin(obj);
>   }
>   EXPORT_SYMBOL(drm_gem_map_attach);
Thomas Zimmermann March 2, 2023, 3:05 p.m. UTC | #2
Hi

Am 02.03.23 um 16:03 schrieb Thomas Zimmermann:
> Hi
> 
> Am 02.03.23 um 15:35 schrieb Simon Ser:
>> drm_gem_map_dma_buf() requires drm_gem_object_funcs.get_sg_table
>> to be implemented, or else WARNs.
>>
>> Allow drivers to leave this hook unimplemented to implement purely
>> local DMA-BUFs (ie, DMA-BUFs which cannot be imported anywhere
>> else but the device which allocated them). In that case, reject
>> imports to other devices in drm_gem_map_attach().
>>
>> v2: new patch
>>
>> Signed-off-by: Simon Ser <contact@emersion.fr>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Tian Tao <tiantao6@hisilicon.com>
>> Cc: Maxime Ripard <maxime@cerno.tech>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/gpu/drm/drm_prime.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index f924b8b4ab6b..ab1d21d63a03 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -544,7 +544,8 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device 
>> *dev, void *data,
>>    * Optional pinning of buffers is handled at dma-buf attach and 
>> detach time in
>>    * drm_gem_map_attach() and drm_gem_map_detach(). Backing storage 
>> itself is
>>    * handled by drm_gem_map_dma_buf() and drm_gem_unmap_dma_buf(), 
>> which relies on
>> - * &drm_gem_object_funcs.get_sg_table.
>> + * &drm_gem_object_funcs.get_sg_table. If 
>> &drm_gem_object_funcs.get_sg_table is
>> + * unimplemented, exports into another device are rejected.
>>    *
>>    * For kernel-internal access there's drm_gem_dmabuf_vmap() and
>>    * drm_gem_dmabuf_vunmap(). Userspace mmap support is provided by
>> @@ -583,6 +584,9 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
>>   {
>>       struct drm_gem_object *obj = dma_buf->priv;
>> +    if (!obj->funcs->get_sg_table)
>> +        return -EOPNOTSUPP;
> 
> -ENOSYS please.

With this changed:

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> 
> Best regards
> Thomas
> 
>> +
>>       return drm_gem_pin(obj);
>>   }
>>   EXPORT_SYMBOL(drm_gem_map_attach);
>
Christian König March 2, 2023, 3:56 p.m. UTC | #3
Am 02.03.23 um 15:35 schrieb Simon Ser:
> drm_gem_map_dma_buf() requires drm_gem_object_funcs.get_sg_table
> to be implemented, or else WARNs.
>
> Allow drivers to leave this hook unimplemented to implement purely
> local DMA-BUFs (ie, DMA-BUFs which cannot be imported anywhere
> else but the device which allocated them). In that case, reject
> imports to other devices in drm_gem_map_attach().
>
> v2: new patch
>
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Tian Tao <tiantao6@hisilicon.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Hans de Goede <hdegoede@redhat.com>

With Thomas comment addressed: Reviewed-by: Christian König 
<christian.koenig@amd.com> for the series.

> ---
>   drivers/gpu/drm/drm_prime.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index f924b8b4ab6b..ab1d21d63a03 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -544,7 +544,8 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
>    * Optional pinning of buffers is handled at dma-buf attach and detach time in
>    * drm_gem_map_attach() and drm_gem_map_detach(). Backing storage itself is
>    * handled by drm_gem_map_dma_buf() and drm_gem_unmap_dma_buf(), which relies on
> - * &drm_gem_object_funcs.get_sg_table.
> + * &drm_gem_object_funcs.get_sg_table. If &drm_gem_object_funcs.get_sg_table is
> + * unimplemented, exports into another device are rejected.
>    *
>    * For kernel-internal access there's drm_gem_dmabuf_vmap() and
>    * drm_gem_dmabuf_vunmap(). Userspace mmap support is provided by
> @@ -583,6 +584,9 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
>   {
>   	struct drm_gem_object *obj = dma_buf->priv;
>   
> +	if (!obj->funcs->get_sg_table)
> +		return -EOPNOTSUPP;
> +
>   	return drm_gem_pin(obj);
>   }
>   EXPORT_SYMBOL(drm_gem_map_attach);
Thomas Zimmermann June 9, 2023, 11:31 a.m. UTC | #4
Hi Simon

Am 02.03.23 um 15:35 schrieb Simon Ser:
> drm_gem_map_dma_buf() requires drm_gem_object_funcs.get_sg_table
> to be implemented, or else WARNs.
> 
> Allow drivers to leave this hook unimplemented to implement purely
> local DMA-BUFs (ie, DMA-BUFs which cannot be imported anywhere
> else but the device which allocated them). In that case, reject
> imports to other devices in drm_gem_map_attach().
> 
> v2: new patch

Is there a v3 of this patchset?  It was Acked with the one errno code 
changed.

Best regards
Thomas

> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Tian Tao <tiantao6@hisilicon.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/gpu/drm/drm_prime.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index f924b8b4ab6b..ab1d21d63a03 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -544,7 +544,8 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
>    * Optional pinning of buffers is handled at dma-buf attach and detach time in
>    * drm_gem_map_attach() and drm_gem_map_detach(). Backing storage itself is
>    * handled by drm_gem_map_dma_buf() and drm_gem_unmap_dma_buf(), which relies on
> - * &drm_gem_object_funcs.get_sg_table.
> + * &drm_gem_object_funcs.get_sg_table. If &drm_gem_object_funcs.get_sg_table is
> + * unimplemented, exports into another device are rejected.
>    *
>    * For kernel-internal access there's drm_gem_dmabuf_vmap() and
>    * drm_gem_dmabuf_vunmap(). Userspace mmap support is provided by
> @@ -583,6 +584,9 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
>   {
>   	struct drm_gem_object *obj = dma_buf->priv;
>   
> +	if (!obj->funcs->get_sg_table)
> +		return -EOPNOTSUPP;
> +
>   	return drm_gem_pin(obj);
>   }
>   EXPORT_SYMBOL(drm_gem_map_attach);
Simon Ser June 9, 2023, 1:04 p.m. UTC | #5
Hi,

On Friday, June 9th, 2023 at 13:31, Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Is there a v3 of this patchset? It was Acked with the one errno code
> changed.

Since this was a minor change, I did it locally and pushed the patch
to drm-misc-next already.

Simon
Thomas Zimmermann June 12, 2023, 3:10 p.m. UTC | #6
Hi

Am 09.06.23 um 15:04 schrieb Simon Ser:
> Hi,
> 
> On Friday, June 9th, 2023 at 13:31, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 
>> Is there a v3 of this patchset? It was Acked with the one errno code
>> changed.
> 
> Since this was a minor change, I did it locally and pushed the patch
> to drm-misc-next already.

Found it. Thanks for committing and sorry for the noise.

Best regards
Thomas

> 
> Simon
Rob Clark March 20, 2024, 2:16 p.m. UTC | #7
On Thu, Mar 2, 2023 at 6:35 AM Simon Ser <contact@emersion.fr> wrote:
>
> drm_gem_map_dma_buf() requires drm_gem_object_funcs.get_sg_table
> to be implemented, or else WARNs.
>
> Allow drivers to leave this hook unimplemented to implement purely
> local DMA-BUFs (ie, DMA-BUFs which cannot be imported anywhere
> else but the device which allocated them). In that case, reject
> imports to other devices in drm_gem_map_attach().
>
> v2: new patch
>
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Tian Tao <tiantao6@hisilicon.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/drm_prime.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index f924b8b4ab6b..ab1d21d63a03 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -544,7 +544,8 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
>   * Optional pinning of buffers is handled at dma-buf attach and detach time in
>   * drm_gem_map_attach() and drm_gem_map_detach(). Backing storage itself is
>   * handled by drm_gem_map_dma_buf() and drm_gem_unmap_dma_buf(), which relies on
> - * &drm_gem_object_funcs.get_sg_table.
> + * &drm_gem_object_funcs.get_sg_table. If &drm_gem_object_funcs.get_sg_table is
> + * unimplemented, exports into another device are rejected.
>   *
>   * For kernel-internal access there's drm_gem_dmabuf_vmap() and
>   * drm_gem_dmabuf_vunmap(). Userspace mmap support is provided by
> @@ -583,6 +584,9 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
>  {
>         struct drm_gem_object *obj = dma_buf->priv;
>
> +       if (!obj->funcs->get_sg_table)
> +               return -EOPNOTSUPP;

This breaks virtgpu, where buffers may not necessarily have guest
backing pages, but still may be shared with other virtual devices
(because the host side buffer _does_ have backing pages)

BR,
-R

> +
>         return drm_gem_pin(obj);
>  }
>  EXPORT_SYMBOL(drm_gem_map_attach);
> --
> 2.39.2
>
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index f924b8b4ab6b..ab1d21d63a03 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -544,7 +544,8 @@  int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
  * Optional pinning of buffers is handled at dma-buf attach and detach time in
  * drm_gem_map_attach() and drm_gem_map_detach(). Backing storage itself is
  * handled by drm_gem_map_dma_buf() and drm_gem_unmap_dma_buf(), which relies on
- * &drm_gem_object_funcs.get_sg_table.
+ * &drm_gem_object_funcs.get_sg_table. If &drm_gem_object_funcs.get_sg_table is
+ * unimplemented, exports into another device are rejected.
  *
  * For kernel-internal access there's drm_gem_dmabuf_vmap() and
  * drm_gem_dmabuf_vunmap(). Userspace mmap support is provided by
@@ -583,6 +584,9 @@  int drm_gem_map_attach(struct dma_buf *dma_buf,
 {
 	struct drm_gem_object *obj = dma_buf->priv;
 
+	if (!obj->funcs->get_sg_table)
+		return -EOPNOTSUPP;
+
 	return drm_gem_pin(obj);
 }
 EXPORT_SYMBOL(drm_gem_map_attach);