Commit dbb4351b authored by Chris Wilson's avatar Chris Wilson

drm/i915: Reorder phys backing storage release

In commit a4f5ea64 ("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: default avatarVille Syrjälä <ville.syrjala@linux.intel.com>
Fixes: a4f5ea64 ("drm/i915: Refactor object page API")
Signed-off-by: default avatarChris 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: default avatarJoonas Lahtinen <joonas.lahtinen@linux.intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20161207133411.8028-1-chris@chris-wilson.co.uk
parent 7e3eb599
...@@ -176,21 +176,35 @@ static struct sg_table * ...@@ -176,21 +176,35 @@ static struct sg_table *
i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj) i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
{ {
struct address_space *mapping = obj->base.filp->f_mapping; 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 sg_table *st;
struct scatterlist *sg; struct scatterlist *sg;
char *vaddr;
int i; int i;
if (WARN_ON(i915_gem_object_needs_bit17_swizzle(obj))) if (WARN_ON(i915_gem_object_needs_bit17_swizzle(obj)))
return ERR_PTR(-EINVAL); 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++) { for (i = 0; i < obj->base.size / PAGE_SIZE; i++) {
struct page *page; struct page *page;
char *src; char *src;
page = shmem_read_mapping_page(mapping, i); page = shmem_read_mapping_page(mapping, i);
if (IS_ERR(page)) if (IS_ERR(page)) {
return ERR_CAST(page); st = ERR_CAST(page);
goto err_phys;
}
src = kmap_atomic(page); src = kmap_atomic(page);
memcpy(vaddr, src, PAGE_SIZE); memcpy(vaddr, src, PAGE_SIZE);
...@@ -204,21 +218,29 @@ i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj) ...@@ -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)); i915_gem_chipset_flush(to_i915(obj->base.dev));
st = kmalloc(sizeof(*st), GFP_KERNEL); st = kmalloc(sizeof(*st), GFP_KERNEL);
if (st == NULL) if (!st) {
return ERR_PTR(-ENOMEM); st = ERR_PTR(-ENOMEM);
goto err_phys;
}
if (sg_alloc_table(st, 1, GFP_KERNEL)) { if (sg_alloc_table(st, 1, GFP_KERNEL)) {
kfree(st); kfree(st);
return ERR_PTR(-ENOMEM); st = ERR_PTR(-ENOMEM);
goto err_phys;
} }
sg = st->sgl; sg = st->sgl;
sg->offset = 0; sg->offset = 0;
sg->length = obj->base.size; 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; sg_dma_len(sg) = obj->base.size;
obj->phys_handle = phys;
return st;
err_phys:
drm_pci_free(obj->base.dev, phys);
return st; return st;
} }
...@@ -274,12 +296,13 @@ i915_gem_object_put_pages_phys(struct drm_i915_gem_object *obj, ...@@ -274,12 +296,13 @@ i915_gem_object_put_pages_phys(struct drm_i915_gem_object *obj,
sg_free_table(pages); sg_free_table(pages);
kfree(pages); kfree(pages);
drm_pci_free(obj->base.dev, obj->phys_handle);
} }
static void static void
i915_gem_object_release_phys(struct drm_i915_gem_object *obj) 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); i915_gem_object_unpin_pages(obj);
} }
...@@ -540,15 +563,13 @@ int ...@@ -540,15 +563,13 @@ int
i915_gem_object_attach_phys(struct drm_i915_gem_object *obj, i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
int align) int align)
{ {
drm_dma_handle_t *phys;
int ret; int ret;
if (obj->phys_handle) { if (align > obj->base.size)
if ((unsigned long)obj->phys_handle->vaddr & (align -1)) return -EINVAL;
return -EBUSY;
if (obj->ops == &i915_gem_phys_ops)
return 0; return 0;
}
if (obj->mm.madv != I915_MADV_WILLNEED) if (obj->mm.madv != I915_MADV_WILLNEED)
return -EFAULT; return -EFAULT;
...@@ -564,12 +585,6 @@ i915_gem_object_attach_phys(struct drm_i915_gem_object *obj, ...@@ -564,12 +585,6 @@ i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
if (obj->mm.pages) if (obj->mm.pages)
return -EBUSY; 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; obj->ops = &i915_gem_phys_ops;
return i915_gem_object_pin_pages(obj); return i915_gem_object_pin_pages(obj);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment