[PATCH] drm/shmem-helper: Reset vma->vm_ops before calling dma_buf_mmap()

[PATCH] drm/shmem-helper: Reset vma->vm_ops before calling dma_buf_mmap()

[Boris Brezillon]

The dma-buf backend is supposed to provide its own vm_ops, but some
implementation just have nothing special to do and leave vm_ops
untouched, probably expecting this field to be zero initialized (this
is the case with the system_heap implementation for instance).
Let's reset vma->vm_ops to NULL to keep things working with these
implementations.

Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf")
Cc: <stable@vger.kernel.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Reported-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/drm_gem_shmem_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 4ea6507a77e5..baaf0e0feb06 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -623,7 +623,13 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
 	int ret;
 
 	if (obj->import_attach) {
+		/* Reset both vm_ops and vm_private_data, so we don't end up with
+		 * vm_ops pointing to our implementation if the dma-buf backend
+		 * doesn't set those fields.
+		 */
 		vma->vm_private_data = NULL;
+		vma->vm_ops = NULL;
+
 		ret = dma_buf_mmap(obj->dma_buf, vma, 0);
 
 		/* Drop the reference drm_gem_mmap_obj() acquired.*/
-- 
2.41.0

Re: [PATCH] drm/shmem-helper: Reset vma->vm_ops before calling dma_buf_mmap()

[Boris Brezillon]

On Mon, 24 Jul 2023 13:26:10 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> The dma-buf backend is supposed to provide its own vm_ops, but some
> implementation just have nothing special to do and leave vm_ops
> untouched, probably expecting this field to be zero initialized (this
> is the case with the system_heap implementation for instance).
> Let's reset vma->vm_ops to NULL to keep things working with these
> implementations.
> 
> Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf")
> Cc: <stable@vger.kernel.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reported-by: Roman Stratiienko <roman.stratiienko@globallogic.com>

Adding Roman's tested-by coming from [1]

Tested-by: Roman Stratiienko <r.stratiienko@gmail.com>

[1]https://gitlab.freedesktop.org/mesa/mesa/-/issues/9416#note_2013722

> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/gpu/drm/drm_gem_shmem_helper.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 4ea6507a77e5..baaf0e0feb06 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -623,7 +623,13 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
>  	int ret;
>  
>  	if (obj->import_attach) {
> +		/* Reset both vm_ops and vm_private_data, so we don't end up with
> +		 * vm_ops pointing to our implementation if the dma-buf backend
> +		 * doesn't set those fields.
> +		 */
>  		vma->vm_private_data = NULL;
> +		vma->vm_ops = NULL;
> +
>  		ret = dma_buf_mmap(obj->dma_buf, vma, 0);
>  
>  		/* Drop the reference drm_gem_mmap_obj() acquired.*/

Re: [PATCH] drm/shmem-helper: Reset vma->vm_ops before calling dma_buf_mmap()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2248 bytes --]

Hi

Am 24.07.23 um 13:26 schrieb Boris Brezillon:
> The dma-buf backend is supposed to provide its own vm_ops, but some
> implementation just have nothing special to do and leave vm_ops
> untouched, probably expecting this field to be zero initialized (this
> is the case with the system_heap implementation for instance).
> Let's reset vma->vm_ops to NULL to keep things working with these
> implementations.

Thanks for your patch. This bug could affect a number of GEM 
implementations. Instead of fixing this individually, could we set the 
fields conditionally at

 
https://elixir.bootlin.com/linux/v6.4/source/drivers/gpu/drm/drm_gem.c#L1042

?

Something like

   if (!object->import_attach) {
     vma->priv =
     vma->ops =
   }

plus a descriptive comment like the one you have in your patch.

Best regards
Thomas

> 
> Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf")
> Cc: <stable@vger.kernel.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reported-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>   drivers/gpu/drm/drm_gem_shmem_helper.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 4ea6507a77e5..baaf0e0feb06 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -623,7 +623,13 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
>   	int ret;
>   
>   	if (obj->import_attach) {
> +		/* Reset both vm_ops and vm_private_data, so we don't end up with
> +		 * vm_ops pointing to our implementation if the dma-buf backend
> +		 * doesn't set those fields.
> +		 */
>   		vma->vm_private_data = NULL;
> +		vma->vm_ops = NULL;
> +
>   		ret = dma_buf_mmap(obj->dma_buf, vma, 0);
>   
>   		/* Drop the reference drm_gem_mmap_obj() acquired.*/

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/shmem-helper: Reset vma->vm_ops before calling dma_buf_mmap()

[Boris Brezillon]

On Tue, 25 Jul 2023 20:50:43 +0200
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Hi
> 
> Am 24.07.23 um 13:26 schrieb Boris Brezillon:
> > The dma-buf backend is supposed to provide its own vm_ops, but some
> > implementation just have nothing special to do and leave vm_ops
> > untouched, probably expecting this field to be zero initialized (this
> > is the case with the system_heap implementation for instance).
> > Let's reset vma->vm_ops to NULL to keep things working with these
> > implementations.  
> 
> Thanks for your patch. This bug could affect a number of GEM 
> implementations.

The one I found that is probably hit by the same problem is
exynos_drm_gem.c, but there might be others...

> Instead of fixing this individually, could we set the 
> fields conditionally at
> 
>  
> https://elixir.bootlin.com/linux/v6.4/source/drivers/gpu/drm/drm_gem.c#L1042
> 
> ?
> 
> Something like
> 
>    if (!object->import_attach) {

If guess you meant the opposite: if (object->import_attach)

>      vma->priv =
>      vma->ops =
>    }

I suspect it will break other drivers relying on the fact vma->vm_ops
is auto-magically assigned to obj->funcs->vm_ops, even for prime
buffers. The one I'm looking at right now is amdgpu: it has its own way
of mapping imported dma-bufs, and resetting vma->vm_ops to NULL means
the ttm layer will fallback to the default ttm_bo_vm_ops, which is not
what amdgpu wants.

AFAICT, etnaviv is in the same situtation, though it's probably easier
to fix, given the open/close hooks for imported objects doesn't do much.

TLDR; yes, it'd be great to have this 'fix' moved at the core level, or
even have a dedicated path for dma-buf objects, but I fear it's going
to fall apart if we do that.

One option would be to add a dma_buf_vm_ops field to
drm_gem_object_funcs, add a
DRM_GEM_OBJ_FUNCS_SET_VM_OPS(vm_ops, dma_buf_vm_ops) macro that would
assign both dma_buf_vm_ops and vm_ops, patch all existing drivers
to use this macro (mechanical change where we assign both fields to the
same value, so we don't break anything, but don't fix broken
implementations either). Once this is in place, we can have the
following in drm_gem_mmap_obj():

	vma->vm_ops = object->import_attach ?
		      object->funcs->dma_buf_vm_ops :
		      object->funcs->vm_ops;
	vma->vm_private_data = vma->vm_ops ? obj : NULL;

And then we can specialize the shmem and exynos implementations
(actually, any implementation that's entirely deferring the mmap to the
dma-buf layer), so they explicitly set dma_buf_vm_ops to NULL.

Honestly, I'm not sure this is better than manually assigning
vma->vm_ops to NULL in the driver mmap function, but at least people
will have to consider it when they write their driver ('do I want
the same mmap behavior for dmabuf and !dmabuf?').

Anyway, I think this fix is worth applying, because it's self-contained
and easy to backport. We can discuss and sort out how we want to fix the
problem more generically later on.

> 
> plus a descriptive comment like the one you have in your patch.
> 
> Best regards
> Thomas
> 
> > 
> > Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf")
> > Cc: <stable@vger.kernel.org>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Reported-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >   drivers/gpu/drm/drm_gem_shmem_helper.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > index 4ea6507a77e5..baaf0e0feb06 100644
> > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > @@ -623,7 +623,13 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
> >   	int ret;
> >   
> >   	if (obj->import_attach) {
> > +		/* Reset both vm_ops and vm_private_data, so we don't end up with
> > +		 * vm_ops pointing to our implementation if the dma-buf backend
> > +		 * doesn't set those fields.
> > +		 */
> >   		vma->vm_private_data = NULL;
> > +		vma->vm_ops = NULL;
> > +
> >   		ret = dma_buf_mmap(obj->dma_buf, vma, 0);
> >   
> >   		/* Drop the reference drm_gem_mmap_obj() acquired.*/  
> 

Re: [PATCH] drm/shmem-helper: Reset vma->vm_ops before calling dma_buf_mmap()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 5072 bytes --]

Hi

Am 26.07.23 um 09:57 schrieb Boris Brezillon:
> On Tue, 25 Jul 2023 20:50:43 +0200
> Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 
>> Hi
>>
>> Am 24.07.23 um 13:26 schrieb Boris Brezillon:
>>> The dma-buf backend is supposed to provide its own vm_ops, but some
>>> implementation just have nothing special to do and leave vm_ops
>>> untouched, probably expecting this field to be zero initialized (this
>>> is the case with the system_heap implementation for instance).
>>> Let's reset vma->vm_ops to NULL to keep things working with these
>>> implementations.
>>
>> Thanks for your patch. This bug could affect a number of GEM
>> implementations.
> 
> The one I found that is probably hit by the same problem is
> exynos_drm_gem.c, but there might be others...
> 
>> Instead of fixing this individually, could we set the
>> fields conditionally at
>>
>>   
>> https://elixir.bootlin.com/linux/v6.4/source/drivers/gpu/drm/drm_gem.c#L1042
>>
>> ?
>>
>> Something like
>>
>>     if (!object->import_attach) {
> 
> If guess you meant the opposite: if (object->import_attach)

No, we'd want to assign iff it's not an imported buffer.

> 
>>       vma->priv =
>>       vma->ops =
>>     }
> 
> I suspect it will break other drivers relying on the fact vma->vm_ops
> is auto-magically assigned to obj->funcs->vm_ops, even for prime
> buffers. The one I'm looking at right now is amdgpu: it has its own way
> of mapping imported dma-bufs, and resetting vma->vm_ops to NULL means
> the ttm layer will fallback to the default ttm_bo_vm_ops, which is not
> what amdgpu wants.
> 
> AFAICT, etnaviv is in the same situtation, though it's probably easier
> to fix, given the open/close hooks for imported objects doesn't do much.
> 
> TLDR; yes, it'd be great to have this 'fix' moved at the core level, or
> even have a dedicated path for dma-buf objects, but I fear it's going
> to fall apart if we do that.

I see. So for the current patch, you can add

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

I think we should keep the general solution in mind. Maybe this can be 
tried later.

Best regards
Thomas

> 
> One option would be to add a dma_buf_vm_ops field to
> drm_gem_object_funcs, add a
> DRM_GEM_OBJ_FUNCS_SET_VM_OPS(vm_ops, dma_buf_vm_ops) macro that would
> assign both dma_buf_vm_ops and vm_ops, patch all existing drivers
> to use this macro (mechanical change where we assign both fields to the
> same value, so we don't break anything, but don't fix broken
> implementations either). Once this is in place, we can have the
> following in drm_gem_mmap_obj():
> 
> 	vma->vm_ops = object->import_attach ?
> 		      object->funcs->dma_buf_vm_ops :
> 		      object->funcs->vm_ops;
> 	vma->vm_private_data = vma->vm_ops ? obj : NULL;
> 
> And then we can specialize the shmem and exynos implementations
> (actually, any implementation that's entirely deferring the mmap to the
> dma-buf layer), so they explicitly set dma_buf_vm_ops to NULL.
> 
> Honestly, I'm not sure this is better than manually assigning
> vma->vm_ops to NULL in the driver mmap function, but at least people
> will have to consider it when they write their driver ('do I want
> the same mmap behavior for dmabuf and !dmabuf?').
> 
> Anyway, I think this fix is worth applying, because it's self-contained
> and easy to backport. We can discuss and sort out how we want to fix the
> problem more generically later on.
> 
>>
>> plus a descriptive comment like the one you have in your patch.
>>
>> Best regards
>> Thomas
>>
>>>
>>> Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf")
>>> Cc: <stable@vger.kernel.org>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Reported-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
>>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
>>> ---
>>>    drivers/gpu/drm/drm_gem_shmem_helper.c | 6 ++++++
>>>    1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>> index 4ea6507a77e5..baaf0e0feb06 100644
>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>> @@ -623,7 +623,13 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
>>>    	int ret;
>>>    
>>>    	if (obj->import_attach) {
>>> +		/* Reset both vm_ops and vm_private_data, so we don't end up with
>>> +		 * vm_ops pointing to our implementation if the dma-buf backend
>>> +		 * doesn't set those fields.
>>> +		 */
>>>    		vma->vm_private_data = NULL;
>>> +		vma->vm_ops = NULL;
>>> +
>>>    		ret = dma_buf_mmap(obj->dma_buf, vma, 0);
>>>    
>>>    		/* Drop the reference drm_gem_mmap_obj() acquired.*/
>>
> 

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/shmem-helper: Reset vma->vm_ops before calling dma_buf_mmap()

[Boris Brezillon]

On Mon, 24 Jul 2023 13:26:10 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> The dma-buf backend is supposed to provide its own vm_ops, but some
> implementation just have nothing special to do and leave vm_ops
> untouched, probably expecting this field to be zero initialized (this
> is the case with the system_heap implementation for instance).
> Let's reset vma->vm_ops to NULL to keep things working with these
> implementations.
> 
> Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf")
> Cc: <stable@vger.kernel.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reported-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Queued to drm-misc-fixes.

> ---
>  drivers/gpu/drm/drm_gem_shmem_helper.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 4ea6507a77e5..baaf0e0feb06 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -623,7 +623,13 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
>  	int ret;
>  
>  	if (obj->import_attach) {
> +		/* Reset both vm_ops and vm_private_data, so we don't end up with
> +		 * vm_ops pointing to our implementation if the dma-buf backend
> +		 * doesn't set those fields.
> +		 */
>  		vma->vm_private_data = NULL;
> +		vma->vm_ops = NULL;
> +
>  		ret = dma_buf_mmap(obj->dma_buf, vma, 0);
>  
>  		/* Drop the reference drm_gem_mmap_obj() acquired.*/