Re: [PATCH v8 1/2] drm/gem: Properly annotate WW context on drm_gem_lock_reservations() error

Re: [PATCH v8 1/2] drm/gem: Properly annotate WW context on drm_gem_lock_reservations() error

[Christian König via Virtualization]

Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> Use ww_acquire_fini() in the error code paths. Otherwise lockdep
> thinks that lock is held when lock's memory is freed after the
> drm_gem_lock_reservations() error. The ww_acquire_context needs to be
> annotated as "released", which fixes the noisy "WARNING: held lock freed!"
> splat of VirtIO-GPU driver with CONFIG_DEBUG_MUTEXES=y and enabled lockdep.
>
> Cc: stable@vger.kernel.org
> Fixes: 7edc3e3b975b5 ("drm: Add helpers for locking an array of BO reservations.")
> Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/drm_gem.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index eb0c2d041f13..86d670c71286 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1226,7 +1226,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
>   		ret = dma_resv_lock_slow_interruptible(obj->resv,
>   								 acquire_ctx);
>   		if (ret) {
> -			ww_acquire_done(acquire_ctx);
> +			ww_acquire_fini(acquire_ctx);
>   			return ret;
>   		}
>   	}
> @@ -1251,7 +1251,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
>   				goto retry;
>   			}
>   
> -			ww_acquire_done(acquire_ctx);
> +			ww_acquire_fini(acquire_ctx);
>   			return ret;
>   		}
>   	}

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 1/2] drm/gem: Properly annotate WW context on drm_gem_lock_reservations() error

[Daniel Vetter]

On Tue, Jul 05, 2022 at 01:33:51PM +0200, Christian König wrote:
> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> > Use ww_acquire_fini() in the error code paths. Otherwise lockdep
> > thinks that lock is held when lock's memory is freed after the
> > drm_gem_lock_reservations() error. The ww_acquire_context needs to be
> > annotated as "released", which fixes the noisy "WARNING: held lock freed!"
> > splat of VirtIO-GPU driver with CONFIG_DEBUG_MUTEXES=y and enabled lockdep.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 7edc3e3b975b5 ("drm: Add helpers for locking an array of BO reservations.")
> > Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> 
> Reviewed-by: Christian König <christian.koenig@amd.com>

Also added this r-b tag when merging to drm-misc-next-fixes.
-Daniel

> 
> > ---
> >   drivers/gpu/drm/drm_gem.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index eb0c2d041f13..86d670c71286 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -1226,7 +1226,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
> >   		ret = dma_resv_lock_slow_interruptible(obj->resv,
> >   								 acquire_ctx);
> >   		if (ret) {
> > -			ww_acquire_done(acquire_ctx);
> > +			ww_acquire_fini(acquire_ctx);
> >   			return ret;
> >   		}
> >   	}
> > @@ -1251,7 +1251,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
> >   				goto retry;
> >   			}
> > -			ww_acquire_done(acquire_ctx);
> > +			ww_acquire_fini(acquire_ctx);
> >   			return ret;
> >   		}
> >   	}
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 1/2] drm/gem: Properly annotate WW context on drm_gem_lock_reservations() error

[Christian König via Virtualization]

Am 09.08.22 um 18:44 schrieb Daniel Vetter:
> On Tue, Jul 05, 2022 at 01:33:51PM +0200, Christian König wrote:
>> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
>>> Use ww_acquire_fini() in the error code paths. Otherwise lockdep
>>> thinks that lock is held when lock's memory is freed after the
>>> drm_gem_lock_reservations() error. The ww_acquire_context needs to be
>>> annotated as "released", which fixes the noisy "WARNING: held lock freed!"
>>> splat of VirtIO-GPU driver with CONFIG_DEBUG_MUTEXES=y and enabled lockdep.
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: 7edc3e3b975b5 ("drm: Add helpers for locking an array of BO reservations.")
>>> Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
> Also added this r-b tag when merging to drm-misc-next-fixes.

IIRC I've already pushed this to drm-misc-fixes with a CC stable tag 
about 2 weeks ago.

Please double check, it probably just hasn't come down the stream again yet.

Christian.

> -Daniel
>
>>> ---
>>>    drivers/gpu/drm/drm_gem.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>>> index eb0c2d041f13..86d670c71286 100644
>>> --- a/drivers/gpu/drm/drm_gem.c
>>> +++ b/drivers/gpu/drm/drm_gem.c
>>> @@ -1226,7 +1226,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
>>>    		ret = dma_resv_lock_slow_interruptible(obj->resv,
>>>    								 acquire_ctx);
>>>    		if (ret) {
>>> -			ww_acquire_done(acquire_ctx);
>>> +			ww_acquire_fini(acquire_ctx);
>>>    			return ret;
>>>    		}
>>>    	}
>>> @@ -1251,7 +1251,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
>>>    				goto retry;
>>>    			}
>>> -			ww_acquire_done(acquire_ctx);
>>> +			ww_acquire_fini(acquire_ctx);
>>>    			return ret;
>>>    		}
>>>    	}

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 1/2] drm/gem: Properly annotate WW context on drm_gem_lock_reservations() error

[Daniel Vetter]

On Wed, 10 Aug 2022 at 08:52, Christian König <christian.koenig@amd.com> wrote:
>
> Am 09.08.22 um 18:44 schrieb Daniel Vetter:
> > On Tue, Jul 05, 2022 at 01:33:51PM +0200, Christian König wrote:
> >> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> >>> Use ww_acquire_fini() in the error code paths. Otherwise lockdep
> >>> thinks that lock is held when lock's memory is freed after the
> >>> drm_gem_lock_reservations() error. The ww_acquire_context needs to be
> >>> annotated as "released", which fixes the noisy "WARNING: held lock freed!"
> >>> splat of VirtIO-GPU driver with CONFIG_DEBUG_MUTEXES=y and enabled lockdep.
> >>>
> >>> Cc: stable@vger.kernel.org
> >>> Fixes: 7edc3e3b975b5 ("drm: Add helpers for locking an array of BO reservations.")
> >>> Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> >>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> >> Reviewed-by: Christian König <christian.koenig@amd.com>
> > Also added this r-b tag when merging to drm-misc-next-fixes.
>
> IIRC I've already pushed this to drm-misc-fixes with a CC stable tag
> about 2 weeks ago.
>
> Please double check, it probably just hasn't come down the stream again yet.

Hm quickly check and I didn't spot it? There's a few patches from
Dmitry in the last few pulls, and some more stuff pending, but not
these two afaics?
-Daniel

>
> Christian.
>
> > -Daniel
> >
> >>> ---
> >>>    drivers/gpu/drm/drm_gem.c | 4 ++--
> >>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> >>> index eb0c2d041f13..86d670c71286 100644
> >>> --- a/drivers/gpu/drm/drm_gem.c
> >>> +++ b/drivers/gpu/drm/drm_gem.c
> >>> @@ -1226,7 +1226,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
> >>>             ret = dma_resv_lock_slow_interruptible(obj->resv,
> >>>                                                              acquire_ctx);
> >>>             if (ret) {
> >>> -                   ww_acquire_done(acquire_ctx);
> >>> +                   ww_acquire_fini(acquire_ctx);
> >>>                     return ret;
> >>>             }
> >>>     }
> >>> @@ -1251,7 +1251,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
> >>>                             goto retry;
> >>>                     }
> >>> -                   ww_acquire_done(acquire_ctx);
> >>> +                   ww_acquire_fini(acquire_ctx);
> >>>                     return ret;
> >>>             }
> >>>     }
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 1/2] drm/gem: Properly annotate WW context on drm_gem_lock_reservations() error

[Christian König via Virtualization]

Am 10.08.22 um 10:33 schrieb Daniel Vetter:
> On Wed, 10 Aug 2022 at 08:52, Christian König <christian.koenig@amd.com> wrote:
>> Am 09.08.22 um 18:44 schrieb Daniel Vetter:
>>> On Tue, Jul 05, 2022 at 01:33:51PM +0200, Christian König wrote:
>>>> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
>>>>> Use ww_acquire_fini() in the error code paths. Otherwise lockdep
>>>>> thinks that lock is held when lock's memory is freed after the
>>>>> drm_gem_lock_reservations() error. The ww_acquire_context needs to be
>>>>> annotated as "released", which fixes the noisy "WARNING: held lock freed!"
>>>>> splat of VirtIO-GPU driver with CONFIG_DEBUG_MUTEXES=y and enabled lockdep.
>>>>>
>>>>> Cc: stable@vger.kernel.org
>>>>> Fixes: 7edc3e3b975b5 ("drm: Add helpers for locking an array of BO reservations.")
>>>>> Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>> Also added this r-b tag when merging to drm-misc-next-fixes.
>> IIRC I've already pushed this to drm-misc-fixes with a CC stable tag
>> about 2 weeks ago.
>>
>> Please double check, it probably just hasn't come down the stream again yet.
> Hm quickly check and I didn't spot it? There's a few patches from
> Dmitry in the last few pulls, and some more stuff pending, but not
> these two afaics?

Mhm, there is some potential that I wanted to push it but got distracted 
by the re-occurring drm-tip build breakages.

Anyway what I wanted to say is that this stuff should probably go to 
drm-misc-fixes with a CC: stable tag :)

Christian.

> -Daniel
>
>> Christian.
>>
>>> -Daniel
>>>
>>>>> ---
>>>>>     drivers/gpu/drm/drm_gem.c | 4 ++--
>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>>>>> index eb0c2d041f13..86d670c71286 100644
>>>>> --- a/drivers/gpu/drm/drm_gem.c
>>>>> +++ b/drivers/gpu/drm/drm_gem.c
>>>>> @@ -1226,7 +1226,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
>>>>>              ret = dma_resv_lock_slow_interruptible(obj->resv,
>>>>>                                                               acquire_ctx);
>>>>>              if (ret) {
>>>>> -                   ww_acquire_done(acquire_ctx);
>>>>> +                   ww_acquire_fini(acquire_ctx);
>>>>>                      return ret;
>>>>>              }
>>>>>      }
>>>>> @@ -1251,7 +1251,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
>>>>>                              goto retry;
>>>>>                      }
>>>>> -                   ww_acquire_done(acquire_ctx);
>>>>> +                   ww_acquire_fini(acquire_ctx);
>>>>>                      return ret;
>>>>>              }
>>>>>      }
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs

[Christian König via Virtualization]

Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> handle imported dma-bufs properly, which results in mapping of something
> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
> userspace writes to the memory mapping of a dma-buf that was imported into
> Tegra's DRM GEM.
>
> Majority of DRM drivers prohibit mapping of the imported GEM objects.
> Mapping of imported GEMs require special care from userspace since it
> should sync dma-buf because mapping coherency of the exporter device may
> not match the DRM device. Let's prohibit the mapping for all DRM drivers
> for consistency.
>
> Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

I'm pretty sure that this is the right approach, but it's certainly more 
than possible that somebody abused this already.

Anyway patch is Reviewed-by: Christian König <christian.koenig@amd.com> 
since you are really fixing a major stability problem here.

Regards,
Christian.

> ---
>   drivers/gpu/drm/drm_gem.c              | 4 ++++
>   drivers/gpu/drm/drm_gem_shmem_helper.c | 9 ---------
>   2 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 86d670c71286..fc9ec42fa0ab 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>   {
>   	int ret;
>   
> +	/* Don't allow imported objects to be mapped */
> +	if (obj->import_attach)
> +		return -EINVAL;
> +
>   	/* Check for valid size. */
>   	if (obj_size < vma->vm_end - vma->vm_start)
>   		return -EINVAL;
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 8ad0e02991ca..6190f5018986 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -609,17 +609,8 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
>    */
>   int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct *vma)
>   {
> -	struct drm_gem_object *obj = &shmem->base;
>   	int ret;
>   
> -	if (obj->import_attach) {
> -		/* Drop the reference drm_gem_mmap_obj() acquired.*/
> -		drm_gem_object_put(obj);
> -		vma->vm_private_data = NULL;
> -
> -		return dma_buf_mmap(obj->dma_buf, vma, 0);
> -	}
> -
>   	ret = drm_gem_shmem_get_pages(shmem);
>   	if (ret) {
>   		drm_gem_vm_close(vma);

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs

[Rob Clark]

On Tue, Jul 5, 2022 at 4:51 AM Christian König <christian.koenig@amd.com> wrote:
>
> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> > Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> > handle imported dma-bufs properly, which results in mapping of something
> > else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
> > userspace writes to the memory mapping of a dma-buf that was imported into
> > Tegra's DRM GEM.
> >
> > Majority of DRM drivers prohibit mapping of the imported GEM objects.
> > Mapping of imported GEMs require special care from userspace since it
> > should sync dma-buf because mapping coherency of the exporter device may
> > not match the DRM device. Let's prohibit the mapping for all DRM drivers
> > for consistency.
> >
> > Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>
> I'm pretty sure that this is the right approach, but it's certainly more
> than possible that somebody abused this already.

I suspect that this is abused if you run deqp cts on android.. ie. all
winsys buffers are dma-buf imports from gralloc.  And then when you
hit readpix...

You might only hit this in scenarios with separate gpu and display (or
dGPU+iGPU) because self-imports are handled differently in
drm_gem_prime_import_dev().. and maybe not in cases where you end up
with a blit from tiled/compressed to linear.. maybe that narrows the
scope enough to just fix it in userspace?

BR,
-R

> Anyway patch is Reviewed-by: Christian König <christian.koenig@amd.com>
> since you are really fixing a major stability problem here.
>
> Regards,
> Christian.
>
> > ---
> >   drivers/gpu/drm/drm_gem.c              | 4 ++++
> >   drivers/gpu/drm/drm_gem_shmem_helper.c | 9 ---------
> >   2 files changed, 4 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 86d670c71286..fc9ec42fa0ab 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
> >   {
> >       int ret;
> >
> > +     /* Don't allow imported objects to be mapped */
> > +     if (obj->import_attach)
> > +             return -EINVAL;
> > +
> >       /* Check for valid size. */
> >       if (obj_size < vma->vm_end - vma->vm_start)
> >               return -EINVAL;
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > index 8ad0e02991ca..6190f5018986 100644
> > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > @@ -609,17 +609,8 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
> >    */
> >   int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct *vma)
> >   {
> > -     struct drm_gem_object *obj = &shmem->base;
> >       int ret;
> >
> > -     if (obj->import_attach) {
> > -             /* Drop the reference drm_gem_mmap_obj() acquired.*/
> > -             drm_gem_object_put(obj);
> > -             vma->vm_private_data = NULL;
> > -
> > -             return dma_buf_mmap(obj->dma_buf, vma, 0);
> > -     }
> > -
> >       ret = drm_gem_shmem_get_pages(shmem);
> >       if (ret) {
> >               drm_gem_vm_close(vma);
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs

[Daniel Vetter]

On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
> On 7/6/22 00:48, Rob Clark wrote:
> > On Tue, Jul 5, 2022 at 4:51 AM Christian König <christian.koenig@amd.com> wrote:
> >>
> >> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> >>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> >>> handle imported dma-bufs properly, which results in mapping of something
> >>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
> >>> userspace writes to the memory mapping of a dma-buf that was imported into
> >>> Tegra's DRM GEM.
> >>>
> >>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
> >>> Mapping of imported GEMs require special care from userspace since it
> >>> should sync dma-buf because mapping coherency of the exporter device may
> >>> not match the DRM device. Let's prohibit the mapping for all DRM drivers
> >>> for consistency.
> >>>
> >>> Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> >>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> >>
> >> I'm pretty sure that this is the right approach, but it's certainly more
> >> than possible that somebody abused this already.
> > 
> > I suspect that this is abused if you run deqp cts on android.. ie. all
> > winsys buffers are dma-buf imports from gralloc.  And then when you
> > hit readpix...
> > 
> > You might only hit this in scenarios with separate gpu and display (or
> > dGPU+iGPU) because self-imports are handled differently in
> > drm_gem_prime_import_dev().. and maybe not in cases where you end up
> > with a blit from tiled/compressed to linear.. maybe that narrows the
> > scope enough to just fix it in userspace?
> 
> Given that that only drivers which use DRM-SHMEM potentially could've
> map imported dma-bufs (Panfrost, Lima) and they already don't allow to
> do that, I think we're good.

So can I have an ack from Rob here or are there still questions that this
might go boom?

Dmitry, since you have a bunch of patches merged now I think would also be
good to get commit rights so you can drive this more yourself. I've asked
Daniel Stone to help you out with getting that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs

[Rob Clark]

On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
> > On 7/6/22 00:48, Rob Clark wrote:
> > > On Tue, Jul 5, 2022 at 4:51 AM Christian König <christian.koenig@amd.com> wrote:
> > >>
> > >> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> > >>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> > >>> handle imported dma-bufs properly, which results in mapping of something
> > >>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
> > >>> userspace writes to the memory mapping of a dma-buf that was imported into
> > >>> Tegra's DRM GEM.
> > >>>
> > >>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
> > >>> Mapping of imported GEMs require special care from userspace since it
> > >>> should sync dma-buf because mapping coherency of the exporter device may
> > >>> not match the DRM device. Let's prohibit the mapping for all DRM drivers
> > >>> for consistency.
> > >>>
> > >>> Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > >>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> > >>
> > >> I'm pretty sure that this is the right approach, but it's certainly more
> > >> than possible that somebody abused this already.
> > >
> > > I suspect that this is abused if you run deqp cts on android.. ie. all
> > > winsys buffers are dma-buf imports from gralloc.  And then when you
> > > hit readpix...
> > >
> > > You might only hit this in scenarios with separate gpu and display (or
> > > dGPU+iGPU) because self-imports are handled differently in
> > > drm_gem_prime_import_dev().. and maybe not in cases where you end up
> > > with a blit from tiled/compressed to linear.. maybe that narrows the
> > > scope enough to just fix it in userspace?
> >
> > Given that that only drivers which use DRM-SHMEM potentially could've
> > map imported dma-bufs (Panfrost, Lima) and they already don't allow to
> > do that, I think we're good.
>
> So can I have an ack from Rob here or are there still questions that this
> might go boom?
>
> Dmitry, since you have a bunch of patches merged now I think would also be
> good to get commit rights so you can drive this more yourself. I've asked
> Daniel Stone to help you out with getting that.

I *think* we'd be ok with this on msm, mostly just by dumb luck.
Because the dma-buf's we import will be self-import.  I'm less sure
about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
special path for imported dma-bufs either, and in that case they won't
be self-imports.. but I guess no one has tried to run android cts on
panfrost).

What about something less drastic to start, like (apologies for
hand-edited patch):

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 86d670c71286..fc9ec42fa0ab 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
*obj, unsigned long obj_size,
 {
        int ret;

+       WARN_ON_ONCE(obj->import_attach);
+
        /* Check for valid size. */
        if (obj_size < vma->vm_end - vma->vm_start)
                return -EINVAL;
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 8ad0e02991ca..6190f5018986 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -609,17 +609,8 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
  */
 int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct
vm_area_struct *vma)
 {
-       struct drm_gem_object *obj = &shmem->base;
        int ret;

        if (obj->import_attach) {
-               /* Drop the reference drm_gem_mmap_obj() acquired.*/
-               drm_gem_object_put(obj);
-               vma->vm_private_data = NULL;
-
-               return dma_buf_mmap(obj->dma_buf, vma, 0);
+               return -EINVAL;
        }

        ret = drm_gem_shmem_get_pages(shmem);
        if (ret) {
                drm_gem_vm_close(vma);
--
2.36.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs

[Rob Clark]

On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 8/10/22 18:08, Rob Clark wrote:
> > On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>
> >> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
> >>> On 7/6/22 00:48, Rob Clark wrote:
> >>>> On Tue, Jul 5, 2022 at 4:51 AM Christian König <christian.koenig@amd.com> wrote:
> >>>>>
> >>>>> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> >>>>>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> >>>>>> handle imported dma-bufs properly, which results in mapping of something
> >>>>>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
> >>>>>> userspace writes to the memory mapping of a dma-buf that was imported into
> >>>>>> Tegra's DRM GEM.
> >>>>>>
> >>>>>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
> >>>>>> Mapping of imported GEMs require special care from userspace since it
> >>>>>> should sync dma-buf because mapping coherency of the exporter device may
> >>>>>> not match the DRM device. Let's prohibit the mapping for all DRM drivers
> >>>>>> for consistency.
> >>>>>>
> >>>>>> Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> >>>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> >>>>>
> >>>>> I'm pretty sure that this is the right approach, but it's certainly more
> >>>>> than possible that somebody abused this already.
> >>>>
> >>>> I suspect that this is abused if you run deqp cts on android.. ie. all
> >>>> winsys buffers are dma-buf imports from gralloc.  And then when you
> >>>> hit readpix...
> >>>>
> >>>> You might only hit this in scenarios with separate gpu and display (or
> >>>> dGPU+iGPU) because self-imports are handled differently in
> >>>> drm_gem_prime_import_dev().. and maybe not in cases where you end up
> >>>> with a blit from tiled/compressed to linear.. maybe that narrows the
> >>>> scope enough to just fix it in userspace?
> >>>
> >>> Given that that only drivers which use DRM-SHMEM potentially could've
> >>> map imported dma-bufs (Panfrost, Lima) and they already don't allow to
> >>> do that, I think we're good.
> >>
> >> So can I have an ack from Rob here or are there still questions that this
> >> might go boom?
> >>
> >> Dmitry, since you have a bunch of patches merged now I think would also be
> >> good to get commit rights so you can drive this more yourself. I've asked
> >> Daniel Stone to help you out with getting that.
> >
> > I *think* we'd be ok with this on msm, mostly just by dumb luck.
> > Because the dma-buf's we import will be self-import.  I'm less sure
> > about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
> > special path for imported dma-bufs either, and in that case they won't
> > be self-imports.. but I guess no one has tried to run android cts on
> > panfrost).
>
> The last time I tried to mmap dma-buf imported to Panfrost didn't work
> because Panfrost didn't implement something needed for that. I'll need
> to take a look again because can't recall what it was.
>
> > What about something less drastic to start, like (apologies for
> > hand-edited patch):
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 86d670c71286..fc9ec42fa0ab 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
> > *obj, unsigned long obj_size,
> >  {
> >         int ret;
> >
> > +       WARN_ON_ONCE(obj->import_attach);
>
> This will hang NVIDIA Tegra, which is what this patch fixed initially.
> If neither of upstream DRM drivers need to map imported dma-bufs and
> never needed, then why do we need this?

oh, tegra isn't using shmem helpers?  I assumed it was.  Well my point
was to make a more targeted fail on tegra, and a WARN_ON for everyone
else to make it clear that what they are doing is undefined behavior.
Because so far existing userspace (or well, panfrost and freedreno at
least, those are the two I know or checked) don't make special cases
for mmap'ing against the dmabuf fd against the dmabuf fd instead of
the drm device fd.

I *think* it should work out that we don't hit this path with
freedreno but on android I can't really guarantee or prove it.  So
your patch would potentially break existing working userspace.  Maybe
it is userspace that isn't portable (but OTOH it isn't like you are
going to be using freedreno on tegra).  So why don't you go for a more
targeted fix that only returns an error on hw where this is
problematic?

BR,
-R
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs

[Rob Clark]

On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 8/11/22 01:03, Rob Clark wrote:
> > On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
> > <dmitry.osipenko@collabora.com> wrote:
> >>
> >> On 8/10/22 18:08, Rob Clark wrote:
> >>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>>>
> >>>> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
> >>>>> On 7/6/22 00:48, Rob Clark wrote:
> >>>>>> On Tue, Jul 5, 2022 at 4:51 AM Christian König <christian.koenig@amd.com> wrote:
> >>>>>>>
> >>>>>>> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> >>>>>>>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> >>>>>>>> handle imported dma-bufs properly, which results in mapping of something
> >>>>>>>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
> >>>>>>>> userspace writes to the memory mapping of a dma-buf that was imported into
> >>>>>>>> Tegra's DRM GEM.
> >>>>>>>>
> >>>>>>>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
> >>>>>>>> Mapping of imported GEMs require special care from userspace since it
> >>>>>>>> should sync dma-buf because mapping coherency of the exporter device may
> >>>>>>>> not match the DRM device. Let's prohibit the mapping for all DRM drivers
> >>>>>>>> for consistency.
> >>>>>>>>
> >>>>>>>> Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> >>>>>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> >>>>>>>
> >>>>>>> I'm pretty sure that this is the right approach, but it's certainly more
> >>>>>>> than possible that somebody abused this already.
> >>>>>>
> >>>>>> I suspect that this is abused if you run deqp cts on android.. ie. all
> >>>>>> winsys buffers are dma-buf imports from gralloc.  And then when you
> >>>>>> hit readpix...
> >>>>>>
> >>>>>> You might only hit this in scenarios with separate gpu and display (or
> >>>>>> dGPU+iGPU) because self-imports are handled differently in
> >>>>>> drm_gem_prime_import_dev().. and maybe not in cases where you end up
> >>>>>> with a blit from tiled/compressed to linear.. maybe that narrows the
> >>>>>> scope enough to just fix it in userspace?
> >>>>>
> >>>>> Given that that only drivers which use DRM-SHMEM potentially could've
> >>>>> map imported dma-bufs (Panfrost, Lima) and they already don't allow to
> >>>>> do that, I think we're good.
> >>>>
> >>>> So can I have an ack from Rob here or are there still questions that this
> >>>> might go boom?
> >>>>
> >>>> Dmitry, since you have a bunch of patches merged now I think would also be
> >>>> good to get commit rights so you can drive this more yourself. I've asked
> >>>> Daniel Stone to help you out with getting that.
> >>>
> >>> I *think* we'd be ok with this on msm, mostly just by dumb luck.
> >>> Because the dma-buf's we import will be self-import.  I'm less sure
> >>> about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
> >>> special path for imported dma-bufs either, and in that case they won't
> >>> be self-imports.. but I guess no one has tried to run android cts on
> >>> panfrost).
> >>
> >> The last time I tried to mmap dma-buf imported to Panfrost didn't work
> >> because Panfrost didn't implement something needed for that. I'll need
> >> to take a look again because can't recall what it was.
> >>
> >>> What about something less drastic to start, like (apologies for
> >>> hand-edited patch):
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> >>> index 86d670c71286..fc9ec42fa0ab 100644
> >>> --- a/drivers/gpu/drm/drm_gem.c
> >>> +++ b/drivers/gpu/drm/drm_gem.c
> >>> @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
> >>> *obj, unsigned long obj_size,
> >>>  {
> >>>         int ret;
> >>>
> >>> +       WARN_ON_ONCE(obj->import_attach);
> >>
> >> This will hang NVIDIA Tegra, which is what this patch fixed initially.
> >> If neither of upstream DRM drivers need to map imported dma-bufs and
> >> never needed, then why do we need this?
> >
> > oh, tegra isn't using shmem helpers?  I assumed it was.  Well my point
> > was to make a more targeted fail on tegra, and a WARN_ON for everyone
> > else to make it clear that what they are doing is undefined behavior.
> > Because so far existing userspace (or well, panfrost and freedreno at
> > least, those are the two I know or checked) don't make special cases
> > for mmap'ing against the dmabuf fd against the dmabuf fd instead of
> > the drm device fd.
>
> It's not clear to me what bad Android does form yours comments. Does it
> export dma-buf from GPU and then import it to GPU? If yes, then DRM core
> has a check for the self-importing [1].
>
> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_prime.c#L918
>
> If you're meaning something else, then please explain in a more details.

So, android/gralloc allocates buffers externally to the driver and
imports them into driver.  (And that seems to not just be window
surfaces, but in cases random textures, etc)

In the normal case these should be allocated from drm/msm so it should
hit [1].. this is the "dumb luck" I mentioned earlier.  But I'm not
confident enough to say that there is no other case.

>
> > I *think* it should work out that we don't hit this path with
> > freedreno but on android I can't really guarantee or prove it.  So
> > your patch would potentially break existing working userspace.  Maybe
> > it is userspace that isn't portable (but OTOH it isn't like you are
> > going to be using freedreno on tegra).  So why don't you go for a more
> > targeted fix that only returns an error on hw where this is
> > problematic?
>
> That's what the first versions of the patch did and Christian suggested
> that it's not a good approach. In fact it should be not only Tegra that
> has a broken dma-buf mapping, but apparently OMAP driver too.

Hmm, I guess I'm a bit more conservative when it comes to potentially
breaking userspace.

BR,
-R

>
> --
> Best regards,
> Dmitry
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs

[Rob Clark]

On Fri, Aug 12, 2022 at 4:26 AM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 8/11/22 02:19, Rob Clark wrote:
> > On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
> > <dmitry.osipenko@collabora.com> wrote:
> >>
> >> On 8/11/22 01:03, Rob Clark wrote:
> >>> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
> >>> <dmitry.osipenko@collabora.com> wrote:
> >>>>
> >>>> On 8/10/22 18:08, Rob Clark wrote:
> >>>>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>>>>>
> >>>>>> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
> >>>>>>> On 7/6/22 00:48, Rob Clark wrote:
> >>>>>>>> On Tue, Jul 5, 2022 at 4:51 AM Christian König <christian.koenig@amd.com> wrote:
> >>>>>>>>>
> >>>>>>>>> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> >>>>>>>>>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> >>>>>>>>>> handle imported dma-bufs properly, which results in mapping of something
> >>>>>>>>>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
> >>>>>>>>>> userspace writes to the memory mapping of a dma-buf that was imported into
> >>>>>>>>>> Tegra's DRM GEM.
> >>>>>>>>>>
> >>>>>>>>>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
> >>>>>>>>>> Mapping of imported GEMs require special care from userspace since it
> >>>>>>>>>> should sync dma-buf because mapping coherency of the exporter device may
> >>>>>>>>>> not match the DRM device. Let's prohibit the mapping for all DRM drivers
> >>>>>>>>>> for consistency.
> >>>>>>>>>>
> >>>>>>>>>> Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> >>>>>>>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> >>>>>>>>>
> >>>>>>>>> I'm pretty sure that this is the right approach, but it's certainly more
> >>>>>>>>> than possible that somebody abused this already.
> >>>>>>>>
> >>>>>>>> I suspect that this is abused if you run deqp cts on android.. ie. all
> >>>>>>>> winsys buffers are dma-buf imports from gralloc.  And then when you
> >>>>>>>> hit readpix...
> >>>>>>>>
> >>>>>>>> You might only hit this in scenarios with separate gpu and display (or
> >>>>>>>> dGPU+iGPU) because self-imports are handled differently in
> >>>>>>>> drm_gem_prime_import_dev().. and maybe not in cases where you end up
> >>>>>>>> with a blit from tiled/compressed to linear.. maybe that narrows the
> >>>>>>>> scope enough to just fix it in userspace?
> >>>>>>>
> >>>>>>> Given that that only drivers which use DRM-SHMEM potentially could've
> >>>>>>> map imported dma-bufs (Panfrost, Lima) and they already don't allow to
> >>>>>>> do that, I think we're good.
> >>>>>>
> >>>>>> So can I have an ack from Rob here or are there still questions that this
> >>>>>> might go boom?
> >>>>>>
> >>>>>> Dmitry, since you have a bunch of patches merged now I think would also be
> >>>>>> good to get commit rights so you can drive this more yourself. I've asked
> >>>>>> Daniel Stone to help you out with getting that.
> >>>>>
> >>>>> I *think* we'd be ok with this on msm, mostly just by dumb luck.
> >>>>> Because the dma-buf's we import will be self-import.  I'm less sure
> >>>>> about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
> >>>>> special path for imported dma-bufs either, and in that case they won't
> >>>>> be self-imports.. but I guess no one has tried to run android cts on
> >>>>> panfrost).
> >>>>
> >>>> The last time I tried to mmap dma-buf imported to Panfrost didn't work
> >>>> because Panfrost didn't implement something needed for that. I'll need
> >>>> to take a look again because can't recall what it was.
> Upd: I re-checked Panfrost using today's linux-next and mapping of
> imported dma-buf works, I mapped imported buf from video decoder.
> Apparently previously I had some local kernel change that broke the mapping.
>
> >>>>> What about something less drastic to start, like (apologies for
> >>>>> hand-edited patch):
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> >>>>> index 86d670c71286..fc9ec42fa0ab 100644
> >>>>> --- a/drivers/gpu/drm/drm_gem.c
> >>>>> +++ b/drivers/gpu/drm/drm_gem.c
> >>>>> @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
> >>>>> *obj, unsigned long obj_size,
> >>>>>  {
> >>>>>         int ret;
> >>>>>
> >>>>> +       WARN_ON_ONCE(obj->import_attach);
> >>>>
> >>>> This will hang NVIDIA Tegra, which is what this patch fixed initially.
> >>>> If neither of upstream DRM drivers need to map imported dma-bufs and
> >>>> never needed, then why do we need this?
> >>>
> >>> oh, tegra isn't using shmem helpers?  I assumed it was.  Well my point
> >>> was to make a more targeted fail on tegra, and a WARN_ON for everyone
> >>> else to make it clear that what they are doing is undefined behavior.
> >>> Because so far existing userspace (or well, panfrost and freedreno at
> >>> least, those are the two I know or checked) don't make special cases
> >>> for mmap'ing against the dmabuf fd against the dmabuf fd instead of
> >>> the drm device fd.
> >>
> >> It's not clear to me what bad Android does form yours comments. Does it
> >> export dma-buf from GPU and then import it to GPU? If yes, then DRM core
> >> has a check for the self-importing [1].
> >>
> >> [1]
> >> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_prime.c#L918
> >>
> >> If you're meaning something else, then please explain in a more details.
> >
> > So, android/gralloc allocates buffers externally to the driver and
> > imports them into driver.  (And that seems to not just be window
> > surfaces, but in cases random textures, etc)
> >
> > In the normal case these should be allocated from drm/msm so it should
> > hit [1].. this is the "dumb luck" I mentioned earlier.  But I'm not
> > confident enough to say that there is no other case.
> >
> >>
> >>> I *think* it should work out that we don't hit this path with
> >>> freedreno but on android I can't really guarantee or prove it.  So
> >>> your patch would potentially break existing working userspace.  Maybe
> >>> it is userspace that isn't portable (but OTOH it isn't like you are
> >>> going to be using freedreno on tegra).  So why don't you go for a more
> >>> targeted fix that only returns an error on hw where this is
> >>> problematic?
> >>
> >> That's what the first versions of the patch did and Christian suggested
> >> that it's not a good approach. In fact it should be not only Tegra that
> >> has a broken dma-buf mapping, but apparently OMAP driver too.
> >
> > Hmm, I guess I'm a bit more conservative when it comes to potentially
> > breaking userspace.
>
> If such userspace exists, then of course the mapping should continue to
> work. Still will be great to know what that userpsace is.

Definitely existing mesa does not have a special mmap path for
imported dma-bufs (both in the case of panfrost and freedreno, I
didn't check any others).  The only question is whether there is a
case where some app/test/etc imports a foreign dma-buf fd and then
does something that would trigger mmap'ing, like readpix.

BR,
-R

> Alright, let's keep the dma-buf mapping as-is for now. I'll fix just the
> Tegra driver then.
>
> --
> Best regards,
> Dmitry
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs

[Rob Clark]

On Fri, Aug 12, 2022 at 7:57 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Fri, Aug 12, 2022 at 4:26 AM Dmitry Osipenko
> <dmitry.osipenko@collabora.com> wrote:
> >
> > On 8/11/22 02:19, Rob Clark wrote:
> > > On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
> > > <dmitry.osipenko@collabora.com> wrote:
> > >>
> > >> On 8/11/22 01:03, Rob Clark wrote:
> > >>> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
> > >>> <dmitry.osipenko@collabora.com> wrote:
> > >>>>
> > >>>> On 8/10/22 18:08, Rob Clark wrote:
> > >>>>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >>>>>>
> > >>>>>> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
> > >>>>>>> On 7/6/22 00:48, Rob Clark wrote:
> > >>>>>>>> On Tue, Jul 5, 2022 at 4:51 AM Christian König <christian.koenig@amd.com> wrote:
> > >>>>>>>>>
> > >>>>>>>>> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> > >>>>>>>>>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> > >>>>>>>>>> handle imported dma-bufs properly, which results in mapping of something
> > >>>>>>>>>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
> > >>>>>>>>>> userspace writes to the memory mapping of a dma-buf that was imported into
> > >>>>>>>>>> Tegra's DRM GEM.
> > >>>>>>>>>>
> > >>>>>>>>>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
> > >>>>>>>>>> Mapping of imported GEMs require special care from userspace since it
> > >>>>>>>>>> should sync dma-buf because mapping coherency of the exporter device may
> > >>>>>>>>>> not match the DRM device. Let's prohibit the mapping for all DRM drivers
> > >>>>>>>>>> for consistency.
> > >>>>>>>>>>
> > >>>>>>>>>> Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > >>>>>>>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> > >>>>>>>>>
> > >>>>>>>>> I'm pretty sure that this is the right approach, but it's certainly more
> > >>>>>>>>> than possible that somebody abused this already.
> > >>>>>>>>
> > >>>>>>>> I suspect that this is abused if you run deqp cts on android.. ie. all
> > >>>>>>>> winsys buffers are dma-buf imports from gralloc.  And then when you
> > >>>>>>>> hit readpix...
> > >>>>>>>>
> > >>>>>>>> You might only hit this in scenarios with separate gpu and display (or
> > >>>>>>>> dGPU+iGPU) because self-imports are handled differently in
> > >>>>>>>> drm_gem_prime_import_dev().. and maybe not in cases where you end up
> > >>>>>>>> with a blit from tiled/compressed to linear.. maybe that narrows the
> > >>>>>>>> scope enough to just fix it in userspace?
> > >>>>>>>
> > >>>>>>> Given that that only drivers which use DRM-SHMEM potentially could've
> > >>>>>>> map imported dma-bufs (Panfrost, Lima) and they already don't allow to
> > >>>>>>> do that, I think we're good.
> > >>>>>>
> > >>>>>> So can I have an ack from Rob here or are there still questions that this
> > >>>>>> might go boom?
> > >>>>>>
> > >>>>>> Dmitry, since you have a bunch of patches merged now I think would also be
> > >>>>>> good to get commit rights so you can drive this more yourself. I've asked
> > >>>>>> Daniel Stone to help you out with getting that.
> > >>>>>
> > >>>>> I *think* we'd be ok with this on msm, mostly just by dumb luck.
> > >>>>> Because the dma-buf's we import will be self-import.  I'm less sure
> > >>>>> about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
> > >>>>> special path for imported dma-bufs either, and in that case they won't
> > >>>>> be self-imports.. but I guess no one has tried to run android cts on
> > >>>>> panfrost).
> > >>>>
> > >>>> The last time I tried to mmap dma-buf imported to Panfrost didn't work
> > >>>> because Panfrost didn't implement something needed for that. I'll need
> > >>>> to take a look again because can't recall what it was.
> > Upd: I re-checked Panfrost using today's linux-next and mapping of
> > imported dma-buf works, I mapped imported buf from video decoder.
> > Apparently previously I had some local kernel change that broke the mapping.
> >
> > >>>>> What about something less drastic to start, like (apologies for
> > >>>>> hand-edited patch):
> > >>>>>
> > >>>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > >>>>> index 86d670c71286..fc9ec42fa0ab 100644
> > >>>>> --- a/drivers/gpu/drm/drm_gem.c
> > >>>>> +++ b/drivers/gpu/drm/drm_gem.c
> > >>>>> @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
> > >>>>> *obj, unsigned long obj_size,
> > >>>>>  {
> > >>>>>         int ret;
> > >>>>>
> > >>>>> +       WARN_ON_ONCE(obj->import_attach);
> > >>>>
> > >>>> This will hang NVIDIA Tegra, which is what this patch fixed initially.
> > >>>> If neither of upstream DRM drivers need to map imported dma-bufs and
> > >>>> never needed, then why do we need this?
> > >>>
> > >>> oh, tegra isn't using shmem helpers?  I assumed it was.  Well my point
> > >>> was to make a more targeted fail on tegra, and a WARN_ON for everyone
> > >>> else to make it clear that what they are doing is undefined behavior.
> > >>> Because so far existing userspace (or well, panfrost and freedreno at
> > >>> least, those are the two I know or checked) don't make special cases
> > >>> for mmap'ing against the dmabuf fd against the dmabuf fd instead of
> > >>> the drm device fd.
> > >>
> > >> It's not clear to me what bad Android does form yours comments. Does it
> > >> export dma-buf from GPU and then import it to GPU? If yes, then DRM core
> > >> has a check for the self-importing [1].
> > >>
> > >> [1]
> > >> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_prime.c#L918
> > >>
> > >> If you're meaning something else, then please explain in a more details.
> > >
> > > So, android/gralloc allocates buffers externally to the driver and
> > > imports them into driver.  (And that seems to not just be window
> > > surfaces, but in cases random textures, etc)
> > >
> > > In the normal case these should be allocated from drm/msm so it should
> > > hit [1].. this is the "dumb luck" I mentioned earlier.  But I'm not
> > > confident enough to say that there is no other case.
> > >
> > >>
> > >>> I *think* it should work out that we don't hit this path with
> > >>> freedreno but on android I can't really guarantee or prove it.  So
> > >>> your patch would potentially break existing working userspace.  Maybe
> > >>> it is userspace that isn't portable (but OTOH it isn't like you are
> > >>> going to be using freedreno on tegra).  So why don't you go for a more
> > >>> targeted fix that only returns an error on hw where this is
> > >>> problematic?
> > >>
> > >> That's what the first versions of the patch did and Christian suggested
> > >> that it's not a good approach. In fact it should be not only Tegra that
> > >> has a broken dma-buf mapping, but apparently OMAP driver too.
> > >
> > > Hmm, I guess I'm a bit more conservative when it comes to potentially
> > > breaking userspace.
> >
> > If such userspace exists, then of course the mapping should continue to
> > work. Still will be great to know what that userpsace is.
>
> Definitely existing mesa does not have a special mmap path for
> imported dma-bufs (both in the case of panfrost and freedreno, I
> didn't check any others).  The only question is whether there is a
> case where some app/test/etc imports a foreign dma-buf fd and then
> does something that would trigger mmap'ing, like readpix.

The other complication I noticed is that we don't seem to keep around
the fd after importing to a GEM handle.  And I could imagine that
doing so could cause issues with too many fd's.  So I guess the best
thing is to keep the status quo and let drivers that cannot mmap
imported buffers just fail mmap?

BR,
-R

> BR,
> -R
>
> > Alright, let's keep the dma-buf mapping as-is for now. I'll fix just the
> > Tegra driver then.
> >
> > --
> > Best regards,
> > Dmitry
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs

[Christian König]

Am 16.08.22 um 13:44 schrieb Dmitry Osipenko:
> [SNIP]
>> The other complication I noticed is that we don't seem to keep around
>> the fd after importing to a GEM handle.  And I could imagine that
>> doing so could cause issues with too many fd's.  So I guess the best
>> thing is to keep the status quo and let drivers that cannot mmap
>> imported buffers just fail mmap?
> That actually should be all the drivers excluding those that use
> DRM-SHMEM because only DRM-SHMEM uses dma_buf_mmap(), that's why it
> works for Panfrost. I'm pretty sure mmaping of imported GEMs doesn't
> work for the MSM driver, isn't it?
>
> Intel and AMD drivers don't allow to map the imported dma-bufs. Both
> refuse to do the mapping.
>
> Although, AMDGPU "succeeds" to do the mapping using
> AMDGPU_GEM_DOMAIN_GTT, but then touching the mapping causes bus fault,
> hence mapping actually fails. I think it might be the AMDGPU
> driver/libdrm bug, haven't checked yet.

That's then certainly broken somehow. Amdgpu should nerve ever have 
allowed to mmap() imported DMA-bufs and the last time I check it didn't.

Regards,
Christian.

>
> So we're back to the point that neither of DRM drivers need to map
> imported dma-bufs and this was never tested. In this case this patch is
> valid, IMO.
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs

[Christian König]

Am 22.08.22 um 19:26 schrieb Dmitry Osipenko:
> On 8/16/22 22:55, Dmitry Osipenko wrote:
>> On 8/16/22 15:03, Christian König wrote:
>>> Am 16.08.22 um 13:44 schrieb Dmitry Osipenko:
>>>> [SNIP]
>>>>> The other complication I noticed is that we don't seem to keep around
>>>>> the fd after importing to a GEM handle.  And I could imagine that
>>>>> doing so could cause issues with too many fd's.  So I guess the best
>>>>> thing is to keep the status quo and let drivers that cannot mmap
>>>>> imported buffers just fail mmap?
>>>> That actually should be all the drivers excluding those that use
>>>> DRM-SHMEM because only DRM-SHMEM uses dma_buf_mmap(), that's why it
>>>> works for Panfrost. I'm pretty sure mmaping of imported GEMs doesn't
>>>> work for the MSM driver, isn't it?
>>>>
>>>> Intel and AMD drivers don't allow to map the imported dma-bufs. Both
>>>> refuse to do the mapping.
>>>>
>>>> Although, AMDGPU "succeeds" to do the mapping using
>>>> AMDGPU_GEM_DOMAIN_GTT, but then touching the mapping causes bus fault,
>>>> hence mapping actually fails. I think it might be the AMDGPU
>>>> driver/libdrm bug, haven't checked yet.
>>> That's then certainly broken somehow. Amdgpu should nerve ever have
>>> allowed to mmap() imported DMA-bufs and the last time I check it didn't.
>> I'll take a closer look. So far I can only tell that it's a kernel
>> driver issue because once I re-applied this "Don't map imported GEMs"
>> patch, AMDGPU began to refuse mapping AMDGPU_GEM_DOMAIN_GTT.
>>
>>>> So we're back to the point that neither of DRM drivers need to map
>>>> imported dma-bufs and this was never tested. In this case this patch is
>>>> valid, IMO.
>> Actually, I'm now looking at Etnaviv and Nouveau and seems they should
>> map imported dma-buf properly. I know that people ran Android on
>> Etnaviv. So maybe devices with a separated GPU/display need to map
>> imported display BO for Android support. Wish somebody who ran Android
>> on one of these devices using upstream drivers could give a definitive
>> answer. I may try to test Nouveau later on.
>>
> Nouveau+Intel combo doesn't work because of [1] that says:
>
> "Refuse to fault imported pages. This should be handled (if at all) by
> redirecting mmap to the exporter."
>
> [1]
> https://elixir.bootlin.com/linux/v5.19/source/drivers/gpu/drm/ttm/ttm_bo_vm.c#L154
>
> Interestingly, I noticed that there are IGT tests which check prime
> mmaping of Nouveau+Intel [2] (added 9 years ago), but they fail as well,
> as expected. The fact that IGT has such tests is interesting because it
> suggests that the mapping worked in the past. It's also surprising that
> nobody cared to fix the failing tests. For the reference, I checked
> v5.18 and today's linux-next.
>
> [2]
> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/prime_nv_test.c#L132
>
> Starting subtest: nv_write_i915_cpu_mmap_read
> Received signal SIGBUS.
> Stack trace:
>   #0 [fatal_sig_handler+0x163]
>   #1 [__sigaction+0x50]
>   #2 [__igt_unique____real_main354+0x406]
>   #3 [main+0x23]
>   #4 [__libc_start_call_main+0x80]
>   #5 [__libc_start_main+0x89]
>   #6 [_start+0x25]
> Subtest nv_write_i915_cpu_mmap_read: CRASH (0,005s)
>
> Starting subtest: nv_write_i915_gtt_mmap_read
> Received signal SIGBUS.
> Stack trace:
>   #0 [fatal_sig_handler+0x163]
>   #1 [__sigaction+0x50]
>   #2 [__igt_unique____real_main354+0x33d]
>   #3 [main+0x23]
>   #4 [__libc_start_call_main+0x80]
>   #5 [__libc_start_main+0x89]
>   #6 [_start+0x25]
> Subtest nv_write_i915_gtt_mmap_read: CRASH (0,004s)
>
> I'm curious about the Etnaviv driver because it uses own shmem
> implementation and maybe it has a working mmaping of imported GEMs since
> it imports the dma-buf pages into Entaviv BO. Although, it should be
> risking to map pages using a different caching attributes (WC) from the
> exporter, which is prohibited on ARM ad then one may try to map imported
> udmabuf.
>
> Apparently, the Intel DG TTM driver should be able to map imported
> dma-buf because it sets TTM_TT_FLAG_EXTERNAL_MAPPABLE.

Even with that flag set it is illegal to map the pages directly by an 
importer.

If that ever worked then the only real solution is to redirect mmap() 
calls on importer BOs to dma_buf_mmap().

Regards,
Christian.

>
> Overall, it still questionable to me whether it's worthwhile to allow
> the mmaping of imported GEMs since only Panfrost/Lima can do it out of
> all drivers and h/w that I tested. Feels like drivers that can do the
> mapping have it just because they can and not because they need.
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs

[Rob Clark]

On Tue, Aug 23, 2022 at 3:01 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 22.08.22 um 19:26 schrieb Dmitry Osipenko:
> > On 8/16/22 22:55, Dmitry Osipenko wrote:
> >> On 8/16/22 15:03, Christian König wrote:
> >>> Am 16.08.22 um 13:44 schrieb Dmitry Osipenko:
> >>>> [SNIP]
> >>>>> The other complication I noticed is that we don't seem to keep around
> >>>>> the fd after importing to a GEM handle.  And I could imagine that
> >>>>> doing so could cause issues with too many fd's.  So I guess the best
> >>>>> thing is to keep the status quo and let drivers that cannot mmap
> >>>>> imported buffers just fail mmap?
> >>>> That actually should be all the drivers excluding those that use
> >>>> DRM-SHMEM because only DRM-SHMEM uses dma_buf_mmap(), that's why it
> >>>> works for Panfrost. I'm pretty sure mmaping of imported GEMs doesn't
> >>>> work for the MSM driver, isn't it?
> >>>>
> >>>> Intel and AMD drivers don't allow to map the imported dma-bufs. Both
> >>>> refuse to do the mapping.
> >>>>
> >>>> Although, AMDGPU "succeeds" to do the mapping using
> >>>> AMDGPU_GEM_DOMAIN_GTT, but then touching the mapping causes bus fault,
> >>>> hence mapping actually fails. I think it might be the AMDGPU
> >>>> driver/libdrm bug, haven't checked yet.
> >>> That's then certainly broken somehow. Amdgpu should nerve ever have
> >>> allowed to mmap() imported DMA-bufs and the last time I check it didn't.
> >> I'll take a closer look. So far I can only tell that it's a kernel
> >> driver issue because once I re-applied this "Don't map imported GEMs"
> >> patch, AMDGPU began to refuse mapping AMDGPU_GEM_DOMAIN_GTT.
> >>
> >>>> So we're back to the point that neither of DRM drivers need to map
> >>>> imported dma-bufs and this was never tested. In this case this patch is
> >>>> valid, IMO.
> >> Actually, I'm now looking at Etnaviv and Nouveau and seems they should
> >> map imported dma-buf properly. I know that people ran Android on
> >> Etnaviv. So maybe devices with a separated GPU/display need to map
> >> imported display BO for Android support. Wish somebody who ran Android
> >> on one of these devices using upstream drivers could give a definitive
> >> answer. I may try to test Nouveau later on.
> >>
> > Nouveau+Intel combo doesn't work because of [1] that says:
> >
> > "Refuse to fault imported pages. This should be handled (if at all) by
> > redirecting mmap to the exporter."
> >
> > [1]
> > https://elixir.bootlin.com/linux/v5.19/source/drivers/gpu/drm/ttm/ttm_bo_vm.c#L154
> >
> > Interestingly, I noticed that there are IGT tests which check prime
> > mmaping of Nouveau+Intel [2] (added 9 years ago), but they fail as well,
> > as expected. The fact that IGT has such tests is interesting because it
> > suggests that the mapping worked in the past. It's also surprising that
> > nobody cared to fix the failing tests. For the reference, I checked
> > v5.18 and today's linux-next.
> >
> > [2]
> > https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/prime_nv_test.c#L132
> >
> > Starting subtest: nv_write_i915_cpu_mmap_read
> > Received signal SIGBUS.
> > Stack trace:
> >   #0 [fatal_sig_handler+0x163]
> >   #1 [__sigaction+0x50]
> >   #2 [__igt_unique____real_main354+0x406]
> >   #3 [main+0x23]
> >   #4 [__libc_start_call_main+0x80]
> >   #5 [__libc_start_main+0x89]
> >   #6 [_start+0x25]
> > Subtest nv_write_i915_cpu_mmap_read: CRASH (0,005s)
> >
> > Starting subtest: nv_write_i915_gtt_mmap_read
> > Received signal SIGBUS.
> > Stack trace:
> >   #0 [fatal_sig_handler+0x163]
> >   #1 [__sigaction+0x50]
> >   #2 [__igt_unique____real_main354+0x33d]
> >   #3 [main+0x23]
> >   #4 [__libc_start_call_main+0x80]
> >   #5 [__libc_start_main+0x89]
> >   #6 [_start+0x25]
> > Subtest nv_write_i915_gtt_mmap_read: CRASH (0,004s)
> >
> > I'm curious about the Etnaviv driver because it uses own shmem
> > implementation and maybe it has a working mmaping of imported GEMs since
> > it imports the dma-buf pages into Entaviv BO. Although, it should be
> > risking to map pages using a different caching attributes (WC) from the
> > exporter, which is prohibited on ARM ad then one may try to map imported
> > udmabuf.
> >
> > Apparently, the Intel DG TTM driver should be able to map imported
> > dma-buf because it sets TTM_TT_FLAG_EXTERNAL_MAPPABLE.
>
> Even with that flag set it is illegal to map the pages directly by an
> importer.
>
> If that ever worked then the only real solution is to redirect mmap()
> calls on importer BOs to dma_buf_mmap().

Yeah, I think this is the best option.  Forcing userspace to hang on
to the fd just in case someone calls readpix would be pretty harsh.

BR,
-R

> Regards,
> Christian.
>
> >
> > Overall, it still questionable to me whether it's worthwhile to allow
> > the mmaping of imported GEMs since only Panfrost/Lima can do it out of
> > all drivers and h/w that I tested. Feels like drivers that can do the
> > mapping have it just because they can and not because they need.
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs

[Rob Clark]

On Wed, Sep 7, 2022 at 3:25 AM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 8/23/22 19:47, Rob Clark wrote:
> > On Tue, Aug 23, 2022 at 3:01 AM Christian König
> > <ckoenig.leichtzumerken@gmail.com> wrote:
> >>
> >> Am 22.08.22 um 19:26 schrieb Dmitry Osipenko:
> >>> On 8/16/22 22:55, Dmitry Osipenko wrote:
> >>>> On 8/16/22 15:03, Christian König wrote:
> >>>>> Am 16.08.22 um 13:44 schrieb Dmitry Osipenko:
> >>>>>> [SNIP]
> >>>>>>> The other complication I noticed is that we don't seem to keep around
> >>>>>>> the fd after importing to a GEM handle.  And I could imagine that
> >>>>>>> doing so could cause issues with too many fd's.  So I guess the best
> >>>>>>> thing is to keep the status quo and let drivers that cannot mmap
> >>>>>>> imported buffers just fail mmap?
> >>>>>> That actually should be all the drivers excluding those that use
> >>>>>> DRM-SHMEM because only DRM-SHMEM uses dma_buf_mmap(), that's why it
> >>>>>> works for Panfrost. I'm pretty sure mmaping of imported GEMs doesn't
> >>>>>> work for the MSM driver, isn't it?
> >>>>>>
> >>>>>> Intel and AMD drivers don't allow to map the imported dma-bufs. Both
> >>>>>> refuse to do the mapping.
> >>>>>>
> >>>>>> Although, AMDGPU "succeeds" to do the mapping using
> >>>>>> AMDGPU_GEM_DOMAIN_GTT, but then touching the mapping causes bus fault,
> >>>>>> hence mapping actually fails. I think it might be the AMDGPU
> >>>>>> driver/libdrm bug, haven't checked yet.
> >>>>> That's then certainly broken somehow. Amdgpu should nerve ever have
> >>>>> allowed to mmap() imported DMA-bufs and the last time I check it didn't.
> >>>> I'll take a closer look. So far I can only tell that it's a kernel
> >>>> driver issue because once I re-applied this "Don't map imported GEMs"
> >>>> patch, AMDGPU began to refuse mapping AMDGPU_GEM_DOMAIN_GTT.
> >>>>
> >>>>>> So we're back to the point that neither of DRM drivers need to map
> >>>>>> imported dma-bufs and this was never tested. In this case this patch is
> >>>>>> valid, IMO.
> >>>> Actually, I'm now looking at Etnaviv and Nouveau and seems they should
> >>>> map imported dma-buf properly. I know that people ran Android on
> >>>> Etnaviv. So maybe devices with a separated GPU/display need to map
> >>>> imported display BO for Android support. Wish somebody who ran Android
> >>>> on one of these devices using upstream drivers could give a definitive
> >>>> answer. I may try to test Nouveau later on.
> >>>>
> >>> Nouveau+Intel combo doesn't work because of [1] that says:
> >>>
> >>> "Refuse to fault imported pages. This should be handled (if at all) by
> >>> redirecting mmap to the exporter."
> >>>
> >>> [1]
> >>> https://elixir.bootlin.com/linux/v5.19/source/drivers/gpu/drm/ttm/ttm_bo_vm.c#L154
> >>>
> >>> Interestingly, I noticed that there are IGT tests which check prime
> >>> mmaping of Nouveau+Intel [2] (added 9 years ago), but they fail as well,
> >>> as expected. The fact that IGT has such tests is interesting because it
> >>> suggests that the mapping worked in the past. It's also surprising that
> >>> nobody cared to fix the failing tests. For the reference, I checked
> >>> v5.18 and today's linux-next.
> >>>
> >>> [2]
> >>> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/prime_nv_test.c#L132
> >>>
> >>> Starting subtest: nv_write_i915_cpu_mmap_read
> >>> Received signal SIGBUS.
> >>> Stack trace:
> >>>   #0 [fatal_sig_handler+0x163]
> >>>   #1 [__sigaction+0x50]
> >>>   #2 [__igt_unique____real_main354+0x406]
> >>>   #3 [main+0x23]
> >>>   #4 [__libc_start_call_main+0x80]
> >>>   #5 [__libc_start_main+0x89]
> >>>   #6 [_start+0x25]
> >>> Subtest nv_write_i915_cpu_mmap_read: CRASH (0,005s)
> >>>
> >>> Starting subtest: nv_write_i915_gtt_mmap_read
> >>> Received signal SIGBUS.
> >>> Stack trace:
> >>>   #0 [fatal_sig_handler+0x163]
> >>>   #1 [__sigaction+0x50]
> >>>   #2 [__igt_unique____real_main354+0x33d]
> >>>   #3 [main+0x23]
> >>>   #4 [__libc_start_call_main+0x80]
> >>>   #5 [__libc_start_main+0x89]
> >>>   #6 [_start+0x25]
> >>> Subtest nv_write_i915_gtt_mmap_read: CRASH (0,004s)
> >>>
> >>> I'm curious about the Etnaviv driver because it uses own shmem
> >>> implementation and maybe it has a working mmaping of imported GEMs since
> >>> it imports the dma-buf pages into Entaviv BO. Although, it should be
> >>> risking to map pages using a different caching attributes (WC) from the
> >>> exporter, which is prohibited on ARM ad then one may try to map imported
> >>> udmabuf.
>
> I see now that Etnaviv uses dma_buf_mmap(), so it should be okay.
>
> >>> Apparently, the Intel DG TTM driver should be able to map imported
> >>> dma-buf because it sets TTM_TT_FLAG_EXTERNAL_MAPPABLE.
> >>
> >> Even with that flag set it is illegal to map the pages directly by an
> >> importer.
> >>
> >> If that ever worked then the only real solution is to redirect mmap()
> >> calls on importer BOs to dma_buf_mmap().
> >
> > Yeah, I think this is the best option.  Forcing userspace to hang on
> > to the fd just in case someone calls readpix would be pretty harsh.
>
> Actually, I proposed this couple months ago [1].
>
> [1] https://patchwork.freedesktop.org/patch/487481/
>
> What's not clear to me is how userspace is supposed to sync CPU accesses
> for imported GEMs. Either userspace need to use dma_buf_sync UAPI for
> dmabuf of imported GEM or importer driver should do that, or there is
> some other option?

For anything involving the GPU, userspace already needs to sync access
(ie. GPUs are asynchronous).. this is done using drm_gem_object::resv
(which comes from dma_buf::resv)..

Possibly we should require that
dma_buf_begin_cpu_access()/dma_buf_end_cpu_access().. but in practice
if the gl/vk driver needs CPU access it is to read the results of it's
own GPU rendering.  Disallow that and all of deqp/piglit/etc will
break on systems where "window" surfaces are dma-buf imports (like
android), which would be a rather unpopular outcome ;-)

BR,
-R
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs

[Rob Clark]

On Tue, Aug 16, 2022 at 4:45 AM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 8/12/22 18:01, Rob Clark wrote:
> > On Fri, Aug 12, 2022 at 7:57 AM Rob Clark <robdclark@gmail.com> wrote:
> >>
> >> On Fri, Aug 12, 2022 at 4:26 AM Dmitry Osipenko
> >> <dmitry.osipenko@collabora.com> wrote:
> >>>
> >>> On 8/11/22 02:19, Rob Clark wrote:
> >>>> On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
> >>>> <dmitry.osipenko@collabora.com> wrote:
> >>>>>
> >>>>> On 8/11/22 01:03, Rob Clark wrote:
> >>>>>> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
> >>>>>> <dmitry.osipenko@collabora.com> wrote:
> >>>>>>>
> >>>>>>> On 8/10/22 18:08, Rob Clark wrote:
> >>>>>>>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>>>>>>>>
> >>>>>>>>> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
> >>>>>>>>>> On 7/6/22 00:48, Rob Clark wrote:
> >>>>>>>>>>> On Tue, Jul 5, 2022 at 4:51 AM Christian König <christian.koenig@amd.com> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> >>>>>>>>>>>>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> >>>>>>>>>>>>> handle imported dma-bufs properly, which results in mapping of something
> >>>>>>>>>>>>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
> >>>>>>>>>>>>> userspace writes to the memory mapping of a dma-buf that was imported into
> >>>>>>>>>>>>> Tegra's DRM GEM.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
> >>>>>>>>>>>>> Mapping of imported GEMs require special care from userspace since it
> >>>>>>>>>>>>> should sync dma-buf because mapping coherency of the exporter device may
> >>>>>>>>>>>>> not match the DRM device. Let's prohibit the mapping for all DRM drivers
> >>>>>>>>>>>>> for consistency.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> >>>>>>>>>>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> >>>>>>>>>>>>
> >>>>>>>>>>>> I'm pretty sure that this is the right approach, but it's certainly more
> >>>>>>>>>>>> than possible that somebody abused this already.
> >>>>>>>>>>>
> >>>>>>>>>>> I suspect that this is abused if you run deqp cts on android.. ie. all
> >>>>>>>>>>> winsys buffers are dma-buf imports from gralloc.  And then when you
> >>>>>>>>>>> hit readpix...
> >>>>>>>>>>>
> >>>>>>>>>>> You might only hit this in scenarios with separate gpu and display (or
> >>>>>>>>>>> dGPU+iGPU) because self-imports are handled differently in
> >>>>>>>>>>> drm_gem_prime_import_dev().. and maybe not in cases where you end up
> >>>>>>>>>>> with a blit from tiled/compressed to linear.. maybe that narrows the
> >>>>>>>>>>> scope enough to just fix it in userspace?
> >>>>>>>>>>
> >>>>>>>>>> Given that that only drivers which use DRM-SHMEM potentially could've
> >>>>>>>>>> map imported dma-bufs (Panfrost, Lima) and they already don't allow to
> >>>>>>>>>> do that, I think we're good.
> >>>>>>>>>
> >>>>>>>>> So can I have an ack from Rob here or are there still questions that this
> >>>>>>>>> might go boom?
> >>>>>>>>>
> >>>>>>>>> Dmitry, since you have a bunch of patches merged now I think would also be
> >>>>>>>>> good to get commit rights so you can drive this more yourself. I've asked
> >>>>>>>>> Daniel Stone to help you out with getting that.
> >>>>>>>>
> >>>>>>>> I *think* we'd be ok with this on msm, mostly just by dumb luck.
> >>>>>>>> Because the dma-buf's we import will be self-import.  I'm less sure
> >>>>>>>> about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
> >>>>>>>> special path for imported dma-bufs either, and in that case they won't
> >>>>>>>> be self-imports.. but I guess no one has tried to run android cts on
> >>>>>>>> panfrost).
> >>>>>>>
> >>>>>>> The last time I tried to mmap dma-buf imported to Panfrost didn't work
> >>>>>>> because Panfrost didn't implement something needed for that. I'll need
> >>>>>>> to take a look again because can't recall what it was.
> >>> Upd: I re-checked Panfrost using today's linux-next and mapping of
> >>> imported dma-buf works, I mapped imported buf from video decoder.
> >>> Apparently previously I had some local kernel change that broke the mapping.
> >>>
> >>>>>>>> What about something less drastic to start, like (apologies for
> >>>>>>>> hand-edited patch):
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> >>>>>>>> index 86d670c71286..fc9ec42fa0ab 100644
> >>>>>>>> --- a/drivers/gpu/drm/drm_gem.c
> >>>>>>>> +++ b/drivers/gpu/drm/drm_gem.c
> >>>>>>>> @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
> >>>>>>>> *obj, unsigned long obj_size,
> >>>>>>>>  {
> >>>>>>>>         int ret;
> >>>>>>>>
> >>>>>>>> +       WARN_ON_ONCE(obj->import_attach);
> >>>>>>>
> >>>>>>> This will hang NVIDIA Tegra, which is what this patch fixed initially.
> >>>>>>> If neither of upstream DRM drivers need to map imported dma-bufs and
> >>>>>>> never needed, then why do we need this?
> >>>>>>
> >>>>>> oh, tegra isn't using shmem helpers?  I assumed it was.  Well my point
> >>>>>> was to make a more targeted fail on tegra, and a WARN_ON for everyone
> >>>>>> else to make it clear that what they are doing is undefined behavior.
> >>>>>> Because so far existing userspace (or well, panfrost and freedreno at
> >>>>>> least, those are the two I know or checked) don't make special cases
> >>>>>> for mmap'ing against the dmabuf fd against the dmabuf fd instead of
> >>>>>> the drm device fd.
> >>>>>
> >>>>> It's not clear to me what bad Android does form yours comments. Does it
> >>>>> export dma-buf from GPU and then import it to GPU? If yes, then DRM core
> >>>>> has a check for the self-importing [1].
> >>>>>
> >>>>> [1]
> >>>>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_prime.c#L918
> >>>>>
> >>>>> If you're meaning something else, then please explain in a more details.
> >>>>
> >>>> So, android/gralloc allocates buffers externally to the driver and
> >>>> imports them into driver.  (And that seems to not just be window
> >>>> surfaces, but in cases random textures, etc)
> >>>>
> >>>> In the normal case these should be allocated from drm/msm so it should
> >>>> hit [1].. this is the "dumb luck" I mentioned earlier.  But I'm not
> >>>> confident enough to say that there is no other case.
> >>>>
> >>>>>
> >>>>>> I *think* it should work out that we don't hit this path with
> >>>>>> freedreno but on android I can't really guarantee or prove it.  So
> >>>>>> your patch would potentially break existing working userspace.  Maybe
> >>>>>> it is userspace that isn't portable (but OTOH it isn't like you are
> >>>>>> going to be using freedreno on tegra).  So why don't you go for a more
> >>>>>> targeted fix that only returns an error on hw where this is
> >>>>>> problematic?
> >>>>>
> >>>>> That's what the first versions of the patch did and Christian suggested
> >>>>> that it's not a good approach. In fact it should be not only Tegra that
> >>>>> has a broken dma-buf mapping, but apparently OMAP driver too.
> >>>>
> >>>> Hmm, I guess I'm a bit more conservative when it comes to potentially
> >>>> breaking userspace.
> >>>
> >>> If such userspace exists, then of course the mapping should continue to
> >>> work. Still will be great to know what that userpsace is.
> >>
> >> Definitely existing mesa does not have a special mmap path for
> >> imported dma-bufs (both in the case of panfrost and freedreno, I
> >> didn't check any others).  The only question is whether there is a
> >> case where some app/test/etc imports a foreign dma-buf fd and then
> >> does something that would trigger mmap'ing, like readpix.
> >
> > The other complication I noticed is that we don't seem to keep around
> > the fd after importing to a GEM handle.  And I could imagine that
> > doing so could cause issues with too many fd's.  So I guess the best
> > thing is to keep the status quo and let drivers that cannot mmap
> > imported buffers just fail mmap?
>
> That actually should be all the drivers excluding those that use
> DRM-SHMEM because only DRM-SHMEM uses dma_buf_mmap(), that's why it
> works for Panfrost. I'm pretty sure mmaping of imported GEMs doesn't
> work for the MSM driver, isn't it?

Hmm, I would have expected it to work, at least prior to the mangling
debug thing danvet added.

Actually, I suspect with the mangling enabled, all sorts of things
would fall apart, I'm not seeing how things like sg_phys() continue to
work.  But not sure if we have that enabled in anything that is
getting android CTS run on it.

BR,
-R

> Intel and AMD drivers don't allow to map the imported dma-bufs. Both
> refuse to do the mapping.
>
> Although, AMDGPU "succeeds" to do the mapping using
> AMDGPU_GEM_DOMAIN_GTT, but then touching the mapping causes bus fault,
> hence mapping actually fails. I think it might be the AMDGPU
> driver/libdrm bug, haven't checked yet.
>
> So we're back to the point that neither of DRM drivers need to map
> imported dma-bufs and this was never tested. In this case this patch is
> valid, IMO.
>
> --
> Best regards,
> Dmitry
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization