Re: [PATCH 2/2] drm/etnaviv: properly implement mmaping of imported buffers

[Lucas Stach <l.stach@pengutronix.de>]

Am Dienstag, den 29.05.2018, 10:20 +0200 schrieb Daniel Vetter:
> On Fri, May 25, 2018 at 03:42:54PM +0200, Lucas Stach wrote:
> > The intention of the existing code was to deflect the actual work
> > of mmaping a dma-buf to the exporter, as that one probably knows best
> > how to handle the buffer. Unfortunately the call to drm_gem_mmap did
> > more than what etnaviv needs in this case by actually setting up the
> > mapping.
> > 
> > Move mapping setup to the shm buffer type mmap implementation so we
> > only need to look up the BO and call the buffer type mmap function
> > from the handler.
> > 
> > Fixes mmap behavior with dma-buf imported and userptr buffers.
> 
> You allow mmap on userptr buffers? That sounds really nasty ...

No, we don't because that's obviously crazy, even more so on ARM with
it's special rules about aliasing mappings. The current code is buggy
in that it first sets up the mapping and then tells userspace the mmap
failed. After this patch we properly reject userptr mmap outright.

> Also not really thrilled about dma-buf mmap forwarding either, since you
> don't seem to forward the begin/end_cpu_access stuff either.

Yeah, that's still missing, but IMHO more of a correctness fix (which
can be done in a follow on patch), distinct of the bugfix in this
patch.

Regards,
Lucas
> -Daniel
>  
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 30 ++++++++++++++++++++-------
> >  1 file changed, 23 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > index fcc969fa0e69..f38989960d7f 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > @@ -138,6 +138,13 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj,
> >  		struct vm_area_struct *vma)
> >  {
> >  	pgprot_t vm_page_prot;
> > +	int ret;
> > +
> > +	ret = drm_gem_mmap_obj(&etnaviv_obj->base,
> > +		drm_vma_node_size(&etnaviv_obj->base.vma_node) << PAGE_SHIFT,
> > +		vma);
> > +	if (ret)
> > +		return ret;
> >  
> >  	vma->vm_flags &= ~VM_PFNMAP;
> >  	vma->vm_flags |= VM_MIXEDMAP;
> > @@ -167,17 +174,26 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj,
> >  
> >  int etnaviv_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> >  {
> > -	struct etnaviv_gem_object *obj;
> > +	struct drm_file *priv = filp->private_data;
> > +	struct etnaviv_gem_object *etnaviv_obj;
> > +	struct drm_gem_object *obj;
> >  	int ret;
> >  
> > -	ret = drm_gem_mmap(filp, vma);
> > -	if (ret) {
> > -		DBG("mmap failed: %d", ret);
> > -		return ret;
> > +	obj = drm_gem_bo_vm_lookup(filp, vma);
> > +	if (!obj)
> > +		return -EINVAL;
> > +
> > +	if (!drm_vma_node_is_allowed(&obj->vma_node, priv)) {
> > +		drm_gem_object_put_unlocked(obj);
> > +		return -EACCES;
> >  	}
> >  
> > -	obj = to_etnaviv_bo(vma->vm_private_data);
> > -	return obj->ops->mmap(obj, vma);
> > +	etnaviv_obj = to_etnaviv_bo(obj);
> > +
> > +	ret = etnaviv_obj->ops->mmap(etnaviv_obj, vma);
> > +	drm_gem_object_put_unlocked(obj);
> > +
> > +	return ret;
> >  }
> >  
> >  int etnaviv_gem_fault(struct vm_fault *vmf)
> > -- 
> > 2.17.0
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel