drm/i915: Reorder phys backing storage release

In commit a4f5ea64f0a8 ("drm/i915: Refactor object page API"), I
reordered the object->pages teardown to be more friendly wrt to a
separate obj->mm.lock. However, I overlooked the phys object and left it
with a dangling use-after-free of its phys_handle. Move the allocation
of the phys handle to get_pages and it release to put_pages to prevent
the invalid access and to improve symmetry.

v2: Add commentary about always aligning to page size.

Testcase: igt/drv_selftest/objects
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Fixes: a4f5ea64f0a8 ("drm/i915: Refactor object page API")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20161207133411.8028-1-chris@chris-wilson.co.uk

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 36183945e61aa893512da2b160507cc1f28c5f22..f86a71d9fe372c942235180c25ede629407bc132 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -176,21 +176,35 @@ static struct sg_table *
 i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
 {
        struct address_space *mapping = obj->base.filp->f_mapping;
-       char *vaddr = obj->phys_handle->vaddr;
+       drm_dma_handle_t *phys;
        struct sg_table *st;
        struct scatterlist *sg;
+       char *vaddr;
        int i;
 
        if (WARN_ON(i915_gem_object_needs_bit17_swizzle(obj)))
                return ERR_PTR(-EINVAL);
 
+       /* Always aligning to the object size, allows a single allocation
+        * to handle all possible callers, and given typical object sizes,
+        * the alignment of the buddy allocation will naturally match.
+        */
+       phys = drm_pci_alloc(obj->base.dev,
+                            obj->base.size,
+                            roundup_pow_of_two(obj->base.size));
+       if (!phys)
+               return ERR_PTR(-ENOMEM);
+
+       vaddr = phys->vaddr;
        for (i = 0; i < obj->base.size / PAGE_SIZE; i++) {
                struct page *page;
                char *src;
 
                page = shmem_read_mapping_page(mapping, i);
-               if (IS_ERR(page))
-                       return ERR_CAST(page);
+               if (IS_ERR(page)) {
+                       st = ERR_CAST(page);
+                       goto err_phys;
+               }
 
                src = kmap_atomic(page);
                memcpy(vaddr, src, PAGE_SIZE);
@@ -204,21 +218,29 @@ i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
        i915_gem_chipset_flush(to_i915(obj->base.dev));
 
        st = kmalloc(sizeof(*st), GFP_KERNEL);
-       if (st == NULL)
-               return ERR_PTR(-ENOMEM);
+       if (!st) {
+               st = ERR_PTR(-ENOMEM);
+               goto err_phys;
+       }
 
        if (sg_alloc_table(st, 1, GFP_KERNEL)) {
                kfree(st);
-               return ERR_PTR(-ENOMEM);
+               st = ERR_PTR(-ENOMEM);
+               goto err_phys;
        }
 
        sg = st->sgl;
        sg->offset = 0;
        sg->length = obj->base.size;
 
-       sg_dma_address(sg) = obj->phys_handle->busaddr;
+       sg_dma_address(sg) = phys->busaddr;
        sg_dma_len(sg) = obj->base.size;
 
+       obj->phys_handle = phys;
+       return st;
+
+err_phys:
+       drm_pci_free(obj->base.dev, phys);
        return st;
 }
 
@@ -274,12 +296,13 @@ i915_gem_object_put_pages_phys(struct drm_i915_gem_object *obj,
 
        sg_free_table(pages);
        kfree(pages);
+
+       drm_pci_free(obj->base.dev, obj->phys_handle);
 }
 
 static void
 i915_gem_object_release_phys(struct drm_i915_gem_object *obj)
 {
-       drm_pci_free(obj->base.dev, obj->phys_handle);
        i915_gem_object_unpin_pages(obj);
 }
 
@@ -540,15 +563,13 @@ int
 i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
                            int align)
 {
-       drm_dma_handle_t *phys;
        int ret;
 
-       if (obj->phys_handle) {
-               if ((unsigned long)obj->phys_handle->vaddr & (align -1))
-                       return -EBUSY;
+       if (align > obj->base.size)
+               return -EINVAL;
 
+       if (obj->ops == &i915_gem_phys_ops)
                return 0;
-       }
 
        if (obj->mm.madv != I915_MADV_WILLNEED)
                return -EFAULT;
@@ -564,12 +585,6 @@ i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
        if (obj->mm.pages)
                return -EBUSY;
 
-       /* create a new object */
-       phys = drm_pci_alloc(obj->base.dev, obj->base.size, align);
-       if (!phys)
-               return -ENOMEM;
-
-       obj->phys_handle = phys;
        obj->ops = &i915_gem_phys_ops;
 
        return i915_gem_object_pin_pages(obj);