Re: [PATCH] drm/i915: add sanity check for partial view creation

From: "Auld, Matthew" <matthew.auld@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/i915: add sanity check for partial view creation
Date: Thu, 3 Mar 2016 11:27:47 +0000	[thread overview]
Message-ID: <85D4726C536E7146A9C15DB4A2CA69E5129CD89E@IRSMSX102.ger.corp.intel.com> (raw)
In-Reply-To: <20160302144205.GH30782@nuc-i3427.alporthouse.com>

> Handle overflow?

Okay, good idea.

> Why do it here and not at creation?

We could, given that we currently only exercise partial views in the gem fault handler code, but as Joonas mentioned we are expecting further use of partial views, so it makes sense to have the check in only one place.

> What bug are you fixing?

afaik this doesn't fix a bug, but it does seem like a reasonable sanity check to add, given more widespread use of partial views.

> Is this a user error? Or just an internal programming bug.

I think if we were to ever hit this it would be indicative of an internal programming bug.
________________________________________
From: Chris Wilson [chris@chris-wilson.co.uk]
Sent: 02 March 2016 14:42
To: Auld, Matthew
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: add sanity check for partial view creation

On Wed, Mar 02, 2016 at 02:33:29PM +0000, Matthew Auld wrote:
> When binding pages for a partial view we should check that the offset +
> size is valid relative to the size of the gem object.
>
> v2: Don't use pages->nents to determine the page count (Tvrtko Ursulin)
>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 7b8de85..2c49d043 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3493,6 +3493,10 @@ intel_partial_pages(const struct i915_ggtt_view *view,
>       struct sg_page_iter obj_sg_iter;
>       int ret = -ENOMEM;
>
> +     if (view->params.partial.offset + view->params.partial.size >

Handle overflow?

Why do it here and not at creation?

What bug are you fixing?

> +         obj->base.size >> PAGE_SHIFT)
> +             return ERR_PTR(-EINVAL);

Is this a user error? Or just an internal programming bug.
-Chris

--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx