[Intel-gfx] [PATCH v2] drm/i915: prevent out of range pt in the PDE macros (take 3)
Michel Thierry
michel.thierry at intel.com
Mon Oct 5 09:59:50 PDT 2015
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 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, 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.
More information about the Intel-gfx
mailing list