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

Commit Message

Dmitry Osipenko July 1, 2022, 9:02 a.m. UTC

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 <[e-mail]>
Signed-off-by: Dmitry Osipenko <[e-mail]>
---
 drivers/gpu/drm/drm_gem.c              | 4 ++++
 drivers/gpu/drm/drm_gem_shmem_helper.c | 9 ---------
 2 files changed, 4 insertions(+), 9 deletions(-)

Comments

Christian König July 5, 2022, 11:39 a.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Dmitry Osipenko <[e-mail]>

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 <[e-mail]> 
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);

Rob Clark July 5, 2022, 9:48 p.m. UTC | #2

On Tue, Jul 5, 2022 at 4:51 AM Christian König <[e-mail]> 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 <[e-mail]>
> > Signed-off-by: Dmitry Osipenko <[e-mail]>
>
> 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 <[e-mail]>
> 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);
>

Dmitry Osipenko July 6, 2022, 7:02 a.m. UTC | #3

On 7/6/22 00:48, Rob Clark wrote:
> On Tue, Jul 5, 2022 at 4:51 AM Christian König <[e-mail]> 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 <[e-mail]>
>>> Signed-off-by: Dmitry Osipenko <[e-mail]>
>>
>> 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.

Daniel Vetter Aug. 10, 2022, 11:47 a.m. UTC | #4

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 <[e-mail]> 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 <[e-mail]>
> >>> Signed-off-by: Dmitry Osipenko <[e-mail]>
> >>
> >> 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

Rob Clark Aug. 10, 2022, 3:08 p.m. UTC | #5

On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> > >>> Signed-off-by: Dmitry Osipenko <[e-mail]>
> > >>
> > >> 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

Dmitry Osipenko Aug. 10, 2022, 7:26 p.m. UTC | #6

On 8/10/22 18:08, Rob Clark wrote:
> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
>>>>>> Signed-off-by: Dmitry Osipenko <[e-mail]>
>>>>>
>>>>> 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?

Dmitry Osipenko Aug. 10, 2022, 7:27 p.m. UTC | #7

On 8/10/22 14:47, Daniel Vetter wrote:
> 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

Thank you!

Rob Clark Aug. 10, 2022, 10:03 p.m. UTC | #8

On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
<[e-mail]> wrote:
>
> On 8/10/22 18:08, Rob Clark wrote:
> > On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> >>>>>> Signed-off-by: Dmitry Osipenko <[e-mail]>
> >>>>>
> >>>>> 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

Dmitry Osipenko Aug. 10, 2022, 10:23 p.m. UTC | #9

On 8/11/22 01:03, Rob Clark wrote:
> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
> <[e-mail]> wrote:
>>
>> On 8/10/22 18:08, Rob Clark wrote:
>>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
>>>>>>>> Signed-off-by: Dmitry Osipenko <[e-mail]>
>>>>>>>
>>>>>>> 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.

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

Rob Clark Aug. 10, 2022, 11:19 p.m. UTC | #10

On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
<[e-mail]> wrote:
>
> On 8/11/22 01:03, Rob Clark wrote:
> > On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
> > <[e-mail]> wrote:
> >>
> >> On 8/10/22 18:08, Rob Clark wrote:
> >>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> >>>>>>>> Signed-off-by: Dmitry Osipenko <[e-mail]>
> >>>>>>>
> >>>>>>> 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

Dmitry Osipenko Aug. 12, 2022, 11:26 a.m. UTC | #11

On 8/11/22 02:19, Rob Clark wrote:
> On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
> <[e-mail]> wrote:
>>
>> On 8/11/22 01:03, Rob Clark wrote:
>>> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
>>> <[e-mail]> wrote:
>>>>
>>>> On 8/10/22 18:08, Rob Clark wrote:
>>>>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
>>>>>>>>>> Signed-off-by: Dmitry Osipenko <[e-mail]>
>>>>>>>>>
>>>>>>>>> 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.

Alright, let's keep the dma-buf mapping as-is for now. I'll fix just the
Tegra driver then.

Rob Clark Aug. 12, 2022, 2:57 p.m. UTC | #12

On Fri, Aug 12, 2022 at 4:26 AM Dmitry Osipenko
<[e-mail]> wrote:
>
> On 8/11/22 02:19, Rob Clark wrote:
> > On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
> > <[e-mail]> wrote:
> >>
> >> On 8/11/22 01:03, Rob Clark wrote:
> >>> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
> >>> <[e-mail]> wrote:
> >>>>
> >>>> On 8/10/22 18:08, Rob Clark wrote:
> >>>>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> >>>>>>>>>> Signed-off-by: Dmitry Osipenko <[e-mail]>
> >>>>>>>>>
> >>>>>>>>> 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

Rob Clark Aug. 12, 2022, 3:01 p.m. UTC | #13

On Fri, Aug 12, 2022 at 7:57 AM Rob Clark <[e-mail]> wrote:
>
> On Fri, Aug 12, 2022 at 4:26 AM Dmitry Osipenko
> <[e-mail]> wrote:
> >
> > On 8/11/22 02:19, Rob Clark wrote:
> > > On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
> > > <[e-mail]> wrote:
> > >>
> > >> On 8/11/22 01:03, Rob Clark wrote:
> > >>> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
> > >>> <[e-mail]> wrote:
> > >>>>
> > >>>> On 8/10/22 18:08, Rob Clark wrote:
> > >>>>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> > >>>>>>>>>> Signed-off-by: Dmitry Osipenko <[e-mail]>
> > >>>>>>>>>
> > >>>>>>>>> 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

Dmitry Osipenko Aug. 16, 2022, 11:44 a.m. UTC | #14

On 8/12/22 18:01, Rob Clark wrote:
> On Fri, Aug 12, 2022 at 7:57 AM Rob Clark <[e-mail]> wrote:
>>
>> On Fri, Aug 12, 2022 at 4:26 AM Dmitry Osipenko
>> <[e-mail]> wrote:
>>>
>>> On 8/11/22 02:19, Rob Clark wrote:
>>>> On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
>>>> <[e-mail]> wrote:
>>>>>
>>>>> On 8/11/22 01:03, Rob Clark wrote:
>>>>>> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
>>>>>> <[e-mail]> wrote:
>>>>>>>
>>>>>>> On 8/10/22 18:08, Rob Clark wrote:
>>>>>>>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
>>>>>>>>>>>>> Signed-off-by: Dmitry Osipenko <[e-mail]>
>>>>>>>>>>>>
>>>>>>>>>>>> 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?

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.

Dmitry Osipenko Aug. 16, 2022, 11:52 a.m. UTC | #15

On 8/16/22 14:44, Dmitry Osipenko wrote:
> On 8/12/22 18:01, Rob Clark wrote:
>> On Fri, Aug 12, 2022 at 7:57 AM Rob Clark <[e-mail]> wrote:
>>>
>>> On Fri, Aug 12, 2022 at 4:26 AM Dmitry Osipenko
>>> <[e-mail]> wrote:
>>>>
>>>> On 8/11/22 02:19, Rob Clark wrote:
>>>>> On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
>>>>> <[e-mail]> wrote:
>>>>>>
>>>>>> On 8/11/22 01:03, Rob Clark wrote:
>>>>>>> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
>>>>>>> <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> On 8/10/22 18:08, Rob Clark wrote:
>>>>>>>>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
>>>>>>>>>>>>>> Signed-off-by: Dmitry Osipenko <[e-mail]>
>>>>>>>>>>>>>
>>>>>>>>>>>>> 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?
> 
> 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.

To make it more clear, I imported AMDGPU dma-buf to Intel iGPU and vice
versa.

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

Christian König Aug. 16, 2022, 12:03 p.m. UTC | #16

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

Dmitry Osipenko Aug. 16, 2022, 7:55 p.m. UTC | #17

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.

Rob Clark Aug. 16, 2022, 10:18 p.m. UTC | #18

On Tue, Aug 16, 2022 at 4:45 AM Dmitry Osipenko
<[e-mail]> wrote:
>
> On 8/12/22 18:01, Rob Clark wrote:
> > On Fri, Aug 12, 2022 at 7:57 AM Rob Clark <[e-mail]> wrote:
> >>
> >> On Fri, Aug 12, 2022 at 4:26 AM Dmitry Osipenko
> >> <[e-mail]> wrote:
> >>>
> >>> On 8/11/22 02:19, Rob Clark wrote:
> >>>> On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
> >>>> <[e-mail]> wrote:
> >>>>>
> >>>>> On 8/11/22 01:03, Rob Clark wrote:
> >>>>>> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
> >>>>>> <[e-mail]> wrote:
> >>>>>>>
> >>>>>>> On 8/10/22 18:08, Rob Clark wrote:
> >>>>>>>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> >>>>>>>>>>>>> Signed-off-by: Dmitry Osipenko <[e-mail]>
> >>>>>>>>>>>>
> >>>>>>>>>>>> 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

Dmitry Osipenko Aug. 22, 2022, 5:26 p.m. UTC | #19

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.

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.

Christian König Aug. 23, 2022, 10:01 a.m. UTC | #20

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

Rob Clark Aug. 23, 2022, 4:47 p.m. UTC | #21

On Tue, Aug 23, 2022 at 3:01 AM Christian König
<[e-mail]> 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.
> >
>

Dmitry Osipenko Sept. 7, 2022, 10:25 a.m. UTC | #22

On 8/23/22 19:47, Rob Clark wrote:
> On Tue, Aug 23, 2022 at 3:01 AM Christian König
> <[e-mail]> 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?

Rob Clark Sept. 11, 2022, 6 p.m. UTC | #23

On Wed, Sep 7, 2022 at 3:25 AM Dmitry Osipenko
<[e-mail]> wrote:
>
> On 8/23/22 19:47, Rob Clark wrote:
> > On Tue, Aug 23, 2022 at 3:01 AM Christian König
> > <[e-mail]> 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

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;
 
+	/* 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);