[Intel-gfx] [PATCH] drm/i915: prevent out of range pt in the PDE macros (take 3)

[Chris Wilson]

On Thu, Oct 01, 2015 at 04:59:35PM +0100, Michel Thierry wrote:
> We tried to fix this in commit fdc454c1484a ("drm/i915: Prevent out of
> range pt in gen6_for_each_pde").
> 
> But the static analyzer still complains that, just before we break due
> to "iter < I915_PDES", we do "pt = (pd)->page_table[iter]" with an
> iter value that is bigger than I915_PDES. Of course, this isn't really
> a problem since no one uses pt outside the macro. Still, every single
> new usage of the macro will create a new issue for us to mark as a
> false positive.
> 
> Also, Paulo re-started the discussion a while ago [1], but didn't end up
> implemented.
> 
> In order to "solve" this "problem", this patch takes the ideas from
> Chris and Dave, but that check would change the desired behavior of the
> code, because the object (for example pdp->page_directory[iter]) can be
> null during init/alloc, and C would take this as false, breaking the for
> loop immediately.
> 
> This has been already verified with "static analysis tools".
> 
> [1]http://lists.freedesktop.org/archives/intel-gfx/2015-June/068548.html
> 
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Dave Gordon <david.s.gordon at intel.com>
> Signed-off-by: Michel Thierry <michel.thierry at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.h | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 9fbb07d..94f8344 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -394,7 +394,9 @@ struct i915_hw_ppgtt {
>   */
>  #define gen6_for_each_pde(pt, pd, start, length, temp, iter) \
>  	for (iter = gen6_pde_index(start); \
> -	     pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES; \
> +	     pt = (length > 0 && iter < I915_PDES) ? \
> +			(pd)->page_table[iter] : NULL, \
> +	     length > 0 && iter < I915_PDES; \

length > 0 && iter < I915_PDES ? (pt = (pd)->page_table[iter]) : 0,

as the compiler wouldn't be able to CSE it otherwise (I think).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre