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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Oct 05, 2015 at 05:59:50PM +0100, Michel Thierry wrote:
> 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.

Yeah, since ?: is a ternary operator parsing implicitly adds the () in the
middle and always parses it as a ? (b) : c. If lower-level operators in
the middle could split the ternary operator then it would result in
parsing fail (sinc ? without the : is just useless). So lgtm. Someone
willing to smack an r-b onto the patch?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx