| Message ID | 1450071971-30321-10-git-send-email-ankitprasad.r.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 14, 2015 at 11:16:11AM +0530, ankitprasad.r.sharma@intel.com wrote: > From: Ankitprasad Sharma <ankitprasad.r.sharma@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. Nope. Let's fix it properly. -Chris
On 14/12/15 05:46, ankitprasad.r.sharma@intel.com wrote: > From: Ankitprasad Sharma <ankitprasad.r.sharma@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@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 */ .Dave. > + else if (use_cpu_reloc(obj)) > ret = relocate_entry_cpu(obj, reloc, target_offset); > else if (obj->map_and_fenceable) > ret = relocate_entry_gtt(obj, reloc, target_offset); >
On Tue, Dec 15, 2015 at 02:41:47PM +0000, Dave Gordon wrote: > On 14/12/15 05:46, ankitprasad.r.sharma@intel.com wrote: > >From: Ankitprasad Sharma <ankitprasad.r.sharma@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@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
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@intel.com wrote: >>> From: Ankitprasad Sharma <ankitprasad.r.sharma@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@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. And I thought we couldn't kmap stolen anyway? .Dave.
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@intel.com wrote: > >>>From: Ankitprasad Sharma <ankitprasad.r.sharma@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@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
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; + else if (use_cpu_reloc(obj)) ret = relocate_entry_cpu(obj, reloc, target_offset); else if (obj->map_and_fenceable) ret = relocate_entry_gtt(obj, reloc, target_offset);