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

[Dave Gordon <david.s.gordon@intel.com>]

On 02/10/15 14:16, 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
>
> v2: Make it a single statement, while preventing the common subexpression
> elimination (Chris)
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.h | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 9fbb07d..a216397 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -394,7 +394,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; \
> +	     length > 0 && iter < I915_PDES ? \
> +			(pt = (pd)->page_table[iter]), 1 : 0; \
>   	     iter++, \
>   	     temp = ALIGN(start+1, 1 << GEN6_PDE_SHIFT) - start, \
>   	     temp = min_t(unsigned, temp, length), \
> @@ -459,7 +460,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;	\
> +	     length > 0 && iter < I915_PDES ? \
> +			(pt = (pd)->page_table[iter]), 1 : 0; \
>   	     iter++,				\
>   	     temp = ALIGN(start+1, 1 << GEN8_PDE_SHIFT) - start,	\
>   	     temp = min(temp, length),					\
> @@ -467,8 +469,8 @@ 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 < I915_PDPES_PER_PDP(dev)); \
> +	     length > 0 && (iter < I915_PDPES_PER_PDP(dev)) ? \
> +			(pd = (pdp)->page_directory[iter]), 1 : 0; \
>   	     iter++,				\
>   	     temp = ALIGN(start+1, 1 << GEN8_PDPE_SHIFT) - start,	\
>   	     temp = min(temp, length),					\
> @@ -476,8 +478,8 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
>
>   #define gen8_for_each_pml4e(pdp, pml4, start, length, temp, iter)	\
>   	for (iter = gen8_pml4e_index(start);	\
> -	     pdp = (pml4)->pdps[iter], \
> -	     length > 0 && iter < GEN8_PML4ES_PER_PML4; \
> +	     length > 0 && iter < GEN8_PML4ES_PER_PML4 ? \
> +			(pdp = (pml4)->pdps[iter]), 1 : 0; \

this won't compile -- see below

>   	     iter++,				\
>   	     temp = ALIGN(start+1, 1ULL << GEN8_PML4E_SHIFT) - start,	\
>   	     temp = min(temp, length),					\

The man page for C operators tells us:

        Operator                             Associativity
        () [] -> .                           left to right
        ! ~ ++ -- + - (type) * & sizeof      right to left
        * / %                                left to right
        + -                                  left to right
        << >>                                left to right
        < <= > >=                            left to right
        == !=                                left to right
        &                                    left to right
        ^                                    left to right
        |                                    left to right
        &&                                   left to right
        ||                                   left to right
        ?:                                   right to left
        = += -= *= /= %= <<= >>= &= ^= |=    right to left
        ,                                    left to right

So there's a problem with the above code, because the comma operator is 
LOWER precedence than either assignment or ?: You'd need to put the 
parentheses around the (pdp = ... , 1) section, not just the assignment.

Or for yet another variation, how about:

#define gen8_for_each_pdpe(pd, pdp, start, length, temp, iter)         \
         for (iter = gen8_pdpe_index(start);                            \
              iter < I915_PDPES_PER_PDP(dev) &&                         \
                 (pd = (pdp)->page_directory[iter], length > 0);        \
              iter++,                                                   \
              temp = ALIGN(start+1, 1 << GEN8_PDPE_SHIFT) - start,      \
              temp = min(temp, length),                                 \
              start += temp, length -= temp)

#define gen8_for_each_pml4e(pdp, pml4, start, length, temp, iter)      \
         for (iter = gen8_pml4e_index(start);                           \
              iter < GEN8_PML4ES_PER_PML4 &&                            \
                 (pdp = (pml4)->pdps[iter], length > 0);                \
              iter++,                                                   \
              temp = ALIGN(start+1, 1ULL << GEN8_PML4E_SHIFT) - start,  \
              temp = min(temp, length),                                 \
              start += temp, length -= temp)

with no ugly ?: at all :)

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