Re: [RFC] drm/i915: prevent out of range pt in the PDE macros (take 2)

From: Dave Gordon <david.s.gordon@intel.com>
To: Paulo Zanoni <przanoni@gmail.com>, intel-gfx@lists.freedesktop.org
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Subject: Re: [RFC] drm/i915: prevent out of range pt in the PDE macros (take 2)
Date: Mon, 15 Jun 2015 11:31:59 +0100	[thread overview]
Message-ID: <557EA99F.2080500@intel.com> (raw)
In-Reply-To: <1434144656-1557-1-git-send-email-przanoni@gmail.com>

On 12/06/15 22:30, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We tried to fix this in the following commit:
> 
> commit fdc454c1484a20e1345cf4e4d7a9feaee814147f
> Author: Michel Thierry <michel.thierry@intel.com>
> Date:   Tue Mar 24 15:46:19 2015 +0000
>     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 possitive.
> 
> After the commit mentioned above we also created some new versions of
> the macros, so they carry the same "problem".
> 
> In order to "solve" this "problem", let's leave the macro with a NULL
> value for pt. So if somebody uses it, we're more likely to get a big
> error message instead of some silent failure. I hope the static
> analyzer won't complain about the new solution (I don't have a way to
> check this!).
> 
> I know, the solution looks really ugly. I am hoping the reviewers will
> help us decide if we prefer this patch or if we prefer to keep marking
> things as false positives.
> 
> Cc: Michel Thierry <michel.thierry@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.h | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> I sent this as an RFC because I really don't know if complicating the
> macro even more will help us in any way. I won't really be surprised
> if I see NACKs on this patch, so don't hesitate if you want to.
> 
> Also, all I did was boot a Kernel with this patch and make sure it
> shows the desktop. So consider this as untested, possibly broken.
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 0d46dd2..b202ca0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -352,7 +352,8 @@ 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 = iter < I915_PDES ? (pd)->page_table[iter] : NULL, \
> +	     length > 0 && iter < I915_PDES; \

You don't need the repeated test on 'iter'; you can write the test
clause of the loop as:

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

using the fact that pt will be NULL when iter >= I915_PDES to break from
the loop :)

This version will leave 'pt' NULL after the loop if the break was due to
the test on 'iter', but non-NULL if the test on 'length' triggered the
break -- is this a useful feature?

.Dave.

>  	     temp = ALIGN(start+1, 1 << GEN6_PDE_SHIFT) - start, \
>  	     temp = min_t(unsigned, temp, length), \
> @@ -360,7 +361,8 @@ struct i915_hw_ppgtt {
>  
>  #define gen6_for_all_pdes(pt, ppgtt, iter)  \
>  	for (iter = 0;		\
> -	     pt = ppgtt->pd.page_table[iter], iter < I915_PDES;	\
> +	     pt = iter < I915_PDES ? ppgtt->pd.page_table[iter] : NULL, \
> +	     iter < I915_PDES;	\
>  	     iter++)
>  
>  static inline uint32_t i915_pte_index(uint64_t address, uint32_t pde_shift)
> @@ -417,7 +419,8 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
>   */
>  #define gen8_for_each_pde(pt, pd, start, length, temp, iter)		\
>  	for (iter = gen8_pde_index(start); \
> -	     pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES;	\
> +	     pt = iter < I915_PDES ? (pd)->page_table[iter] : NULL,	\
> +	     length > 0 && iter < I915_PDES;	\
>  	     iter++,				\
>  	     temp = ALIGN(start+1, 1 << GEN8_PDE_SHIFT) - start,	\
>  	     temp = min(temp, length),					\
> @@ -425,7 +428,9 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
>  
>  #define gen8_for_each_pdpe(pd, pdp, start, length, temp, iter)		\
>  	for (iter = gen8_pdpe_index(start);	\
> -	     pd = (pdp)->page_directory[iter], length > 0 && iter < GEN8_LEGACY_PDPES;	\
> +	     pd = iter < GEN8_LEGACY_PDPES ?				\
> +		  (pdp)->page_directory[iter] : NULL,			\
> +	     length > 0 && iter < GEN8_LEGACY_PDPES;			\
>  	     iter++,				\
>  	     temp = ALIGN(start+1, 1 << GEN8_PDPE_SHIFT) - start,	\
>  	     temp = min(temp, length),					\
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx