[Intel-gfx] [PATCH] drm/i915: avoid leaking DMA mappings

[Imre Deak]

On ma, 2015-07-06 at 17:29 +0200, Daniel Vetter wrote:
> On Mon, Jul 06, 2015 at 03:57:44PM +0100, Chris Wilson wrote:
> > On Mon, Jul 06, 2015 at 05:50:37PM +0300, Imre Deak wrote:
> > > We have 3 types of DMA mappings for GEM objects:
> > > 1. physically contiguous for stolen and for objects needing contiguous
> > >    memory
> > > 2. DMA-buf mappings imported via a DMA-buf attach operation
> > > 3. SG DMA mappings for shmem backed and userptr objects
> > > 
> > > For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
> > > corresponding backing pages and so in practice we create/release the
> > > mapping in the object's get_pages/put_pages callback.
> > > 
> > > For 3. the lifetime of the mapping matches that of any existing GPU binding
> > > of the object, so we'll create the mapping when the object is bound to
> > > the first vma and release the mapping when the object is unbound from its
> > > last vma.
> > > 
> > > Since the object can be bound to multiple vmas, we can end up creating a
> > > new DMA mapping in the 3. case even if the object already had one. This
> > > is not allowed by the DMA API and can lead to leaked mapping data and
> > > IOMMU memory space starvation in certain cases. For example HW IOMMU
> > > drivers (intel_iommu) allocate a new range from their memory space
> > > whenever a mapping is created, silently overriding a pre-existing
> > > mapping.
> 
> How does this happen? Essentially list_empty(obj->vmas) ==
> !dma_mapping_exists should hold for objects of the 3rd type. I don't
> understand how this is broken in the current code. There was definitely
> versions of the ppgtt code where this wasn't working properly, but I
> thought we've fixed that up again.

When binding the object we don't check if it's already bound, just
create the mapping regardless. So if it was already bound (having a
mapping) we'll again create a new mapping overriding the old one.

> > > Fix this by adding new callbacks to create/release the DMA mapping. This
> > > way we can use the has_dma_mapping flag for objects of the 3. case also
> > > (so far the flag was only used for the 1. and 2. case) and skip creating
> > > a new mapping if one exists already.
> > > 
> > > Note that I also thought about simply creating/releasing the mapping
> > > when get_pages/put_pages is called. However since creating a DMA mapping
> > > may have associated resources (at least in case of HW IOMMU) it does
> > > make sense to release these resources as early as possible. We can
> > > release the DMA mapping as soon as the object is unbound from the last
> > > vma, before we drop the backing pages, hence it's worth keeping the two
> > > operations separate.
> > > 
> > > I noticed this issue by enabling DMA debugging, which got disabled after
> > > a while due to its internal mapping tables getting full. It also reported
> > > errors in connection to random other drivers that did a DMA mapping for
> > > an address that was previously mapped by i915 but was never released.
> > > Besides these diagnostic messages and the memory space starvation
> > > problem for IOMMUs, I'm not aware of this causing a real issue.
> > 
> > Nope, it is much much simpler. Since we only do the dma prepare/finish
> > from inside get_pages/put_pages, we can put the calls there. The only
> > caveat there is userptr worker, but that can be easily fixed up.
> 
> I do kinda like the distinction between just grabbing the backing storage
> and making it accessible to the hw. Small one, but I think it does help if
> we keep these two maps separate. Now the function names otoh are
> super-confusing, that I agree with.

Well, please convince Chris :)


> -Daniel