[Intel-gfx] [PATCH 9/9] drm/i915: Fail the execbuff using stolen objects as batchbuffers

[Chris Wilson]

On Tue, Dec 15, 2015 at 05:50:36PM +0000, Dave Gordon wrote:
> On 15/12/15 14:54, Chris Wilson wrote:
> >On Tue, Dec 15, 2015 at 02:41:47PM +0000, Dave Gordon wrote:
> >>On 14/12/15 05:46, ankitprasad.r.sharma at intel.com wrote:
> >>>From: Ankitprasad Sharma <ankitprasad.r.sharma at intel.com>
> >>>
> >>>Using stolen backed objects as a batchbuffer may result into a kernel
> >>>panic during relocation. Added a check to prevent the panic and fail
> >>>the execbuffer call. It is not recommended to use stolen object as
> >>>a batchbuffer.
> >>>
> >>>Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma at intel.com>
> >>>---
> >>>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>>index 48ec484..d342f10 100644
> >>>--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>>+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>>@@ -462,7 +462,9 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
> >>>  	if (obj->active && pagefault_disabled())
> >>>  		return -EFAULT;
> >>>
> >>>-	if (use_cpu_reloc(obj))
> >>>+	if (obj->stolen)
> >>>+		ret = -EINVAL;
> >>
> >>I'd rather reject ALL "weird" gem objects at the first opportunity,
> >>so that none of the execbuffer code has to worry about stolen, phys,
> >>dmabuf, etc ...
> >>
> >>	if (obj->ops != &i915_gem_object_ops))
> >>		ret = -EINVAL;		/* No exotica please */
> >
> >No. All GEM objects are supposed to be first-class so that they are
> >interchangeable through all aspects of the API (that becomes even more
> >important with dma-buf interoperation). We have had to relax that for a
> >couple of special categories (basically CPU mmapping) for certain clases
> >that are not struct file backed. Though in principle, a gemfs would work
> >just fine.
> >
> >The only restrictions we should ideally impose are those determined by
> >hardware.
> >-Chris
> 
> I don't think it's reasonable to place objects that the kernel
> driver cares about -- i.e. understands and decodes -- in memory
> areas that it does not manage, and which may be subject to arbitrary
> uncontrolled access by external hardware and/or processes.

We don't though. As for these objects, they are exposed no matter what
since the user can access them concurrently and remap them to other
devices without our intervention if they should so chose. The reloc
patching depends on a userspace handshake, we have no idea if they are
lying - let alone changing the contents on the fly.

> And I thought we couldn't kmap stolen anyway?

We can on gen2-5, but that isn't the point. The point is that the API is
consistent and not piecemeal.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre