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

[Chris Wilson]

On Thu, Jul 09, 2015 at 12:59:05PM +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.
> 
> Fix this by moving the creation/removal of DMA mappings to the object's
> get_pages/put_pages callbacks. These callbacks already check for and do
> an early return in case of any nested calls. This way objects of the 3.
> case also become more like the other object types.
> 
> 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.
> 
> The fix is based on a patch from Chris.
> 
> v2:
> - move the DMA mapping create/remove calls to the get_pages/put_pages
>   callbacks instead of adding new callbacks for these (Chris)
> v3:
> - also fix the get_page cache logic on the userptr async path (Chris)
> 
> Signed-off-by: Imre Deak <imre.deak at intel.com>
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: stable at vger.kernel.org

Note to future self, I still like adding __i915_gem_object_init_pages()
so that we can get that bit of incestrous knowledge out of userptr.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre