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

[Michel Thierry <michel.thierry@intel.com>]

On 10/5/2015 5:36 PM, Dave Gordon wrote:
> 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

Hmm, it compiled (also got rid of of the "analysis tool error" and 
didn't see any behavior change).

>
>>            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 :)

This also works.

Since it's a change for not a real issue, I don't have a preference.
I can send either.

Thanks,

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