[PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Daniel Vetter]

On Tue, Aug 29, 2017 at 05:03:19PM +0100, Brian Starkey wrote:
> On Fri, Aug 25, 2017 at 03:34:33PM +0200, Daniel Vetter wrote:
> > On Thu, Aug 24, 2017 at 11:04:01AM +0100, Brian Starkey wrote:
> > > Hi,
> > > 
> > > Thanks for the CC.
> > > 
> > > On Fri, Aug 18, 2017 at 06:13:14PM +0200, Noralf Tr??nnes wrote:
> > > > (cc affected parties)
> > > >
> > > >
> > > > Den 18.08.2017 09.46, skrev Daniel Vetter:
> > > > > On Thu, Aug 17, 2017 at 06:21:30PM +0200, Noralf Tr??nnes wrote:
> > > > > > Reject mapping an imported dma-buf since is's an invalid use-case.
> > > > > >
> > > > > > Cc: Philipp Zabel <p.zabel at pengutronix.de>
> > > > > > Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > > > Cc: Sean Paul <seanpaul at chromium.org>
> > > > > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > > > > Signed-off-by: Noralf Tr??nnes <noralf at tronnes.org>
> > > > > I think acks from someone using mali would be good too. amdgpu already has
> > > > > such checks, so I think on the desktop side we're ok.
> > > > >
> > > 
> > > This looks like it would break anyone running the Mali-4xx series GPUs
> > > with the Arm graphics stack (e.g. Hikey board).
> > > 
> > > I don't know where that sits in terms of policy.
> > 
> > Why would this break this use-case? Is the mali blob somehow using dumb
> > mmap for it's own buffers it shares with display? That would be rather
> > backwards ...
> 
> If it's running with a DRM display driver (which may or may not be
> Mali-DP, they are separate IPs), then its window system code always
> uses dumb mmap if it needs a pointer. That might be a native DRM
> buffer that the display driver allocated, or a dma-buf which it PRIME
> imported (which would now fail).

Yeah, that's pretty much the kind of uabi abuse I want to prevent. Dumb
mmap is for dumb buffers, if you have a egl platform then you need to mmap
through that one anyway. If a egl stack uses dumb mmap, something went
wrong somewhere.
-Daniel

> 
> -Brian
> 
> > 
> > Either way, blob -> meh, not a concern really.
> > -Daniel
> > 
> > > 
> > > Cheers,
> > > -Brian
> > > 
> > > > > Acked-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > > >
> > > > > But I think this one here definitely needs a few more acks. I could break
> > > > > uabi if we're unlucky, so let's not rush it.
> > > >
> > > > Ok, I've CC'ed the affected parties to increase the odds that they look
> > > > at this. These are the drivers using drm_gem_dumb_map_offset()
> > > > (hopefully I got the list right):
> > > >
> > > > arc
> > > > atmel-hlcdc
> > > > cirrus
> > > > exynos
> > > > fsl-dcu
> > > > gma500
> > > > hdlcd
> > > > imx
> > > > kirin
> > > > mali-dp
> > > > mediatek
> > > > meson
> > > > mxsfb
> > > > pl111
> > > > rcar-du
> > > > rockchip
> > > > shmobile
> > > > sti
> > > > stm
> > > > sun4i
> > > > tegra
> > > > tilcd
> > > > vc4
> > > > zte
> > > >
> > > >
> > > > Noralf.
> > > >
> > > > > -Daniel
> > > > >
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_gem.c | 6 ++++++
> > > > > >  1 file changed, 6 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > > > > index ad4e9cf..8da5801 100644
> > > > > > --- a/drivers/gpu/drm/drm_gem.c
> > > > > > +++ b/drivers/gpu/drm/drm_gem.c
> > > > > > @@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
> > > > > >  	if (!obj)
> > > > > >  		return -ENOENT;
> > > > > > +	/* Don't allow imported objects to be mapped */
> > > > > > +	if (obj->import_attach) {
> > > > > > +		ret = -EINVAL;
> > > > > > +		goto out;
> > > > > > +	}
> > > > > > +
> > > > > >  	ret = drm_gem_create_mmap_offset(obj);
> > > > > >  	if (ret)
> > > > > >  		goto out;
> > > > > > --
> > > > > > 2.7.4
> > > > > >
> > > >
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch