Re: [PATCH v6 02/22] drm/gem: Move mapping of imported dma-bufs to drm_gem_mmap_obj()

[Thomas Hellström (Intel)]

On 6/29/22 10:22, Dmitry Osipenko wrote:
> On 6/29/22 09:40, Thomas Hellström (Intel) wrote:
> > On 5/27/22 01:50, Dmitry Osipenko wrote:
> > > 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. For example, 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.
> > >
> > > To fix this bug, move mapping of imported dma-bufs to drm_gem_mmap_obj().
> > > Now mmaping of imported dma-bufs works properly for all DRM drivers.
> > Same comment about Fixes: as in patch 1,
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>
> > > ---
> > >    drivers/gpu/drm/drm_gem.c              | 3 +++
> > >    drivers/gpu/drm/drm_gem_shmem_helper.c | 9 ---------
> > >    drivers/gpu/drm/tegra/gem.c            | 4 ++++
> > >    3 files changed, 7 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > index 86d670c71286..7c0b025508e4 100644
> > > --- a/drivers/gpu/drm/drm_gem.c
> > > +++ b/drivers/gpu/drm/drm_gem.c
> > > @@ -1038,6 +1038,9 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj,
> > > unsigned long obj_size,
> > >        if (obj_size < vma->vm_end - vma->vm_start)
> > >            return -EINVAL;
> > >    +    if (obj->import_attach)
> > > +        return dma_buf_mmap(obj->dma_buf, vma, 0);
> > If we start enabling mmaping of imported dma-bufs on a majority of
> > drivers in this way, how do we ensure that user-space is not blindly
> > using the object mmap without calling the needed DMA_BUF_IOCTL_SYNC
> > which is needed before and after cpu access of mmap'ed dma-bufs?
> >
> > I was under the impression (admittedly without looking) that the few
> > drivers that actually called into dma_buf_mmap() had some private
> > user-mode driver code in place that ensured this happened.
> Since it's a userspace who does the mapping, then it should be a
> responsibility of userspace to do all the necessary syncing.

Sure, but nothing prohibits user-space to ignore the syncing thinking 
"It works anyway", testing those drivers where the syncing is a NOP. And 
when a driver that finally needs syncing is tested it's too late to fix 
all broken user-space.

>   I'm not
> sure whether anyone in userspace really needs to map imported dma-bufs
> in practice. Nevertheless, this use-case is broken and should be fixed
> by either allowing to do the mapping or prohibiting it.
Then I'd vote for prohibiting it, at least for now. And for the future 
moving forward we could perhaps revisit the dma-buf need for syncing, 
requiring those drivers that actually need it to implement emulated 
coherent memory which can be done not too inefficiently (vmwgfx being 
one example).

/Thomas