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

[Chris Wilson]

On Mon, Jun 15, 2015 at 11:33:37AM +0100, Dave Gordon wrote:
> On 13/06/15 09:28, Chris Wilson wrote:
> > On Fri, Jun 12, 2015 at 06:30:56PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
> >>
> >> We tried to fix this in the following commit:
> >>
> >> commit fdc454c1484a20e1345cf4e4d7a9feaee814147f
> >> Author: Michel Thierry <michel.thierry at 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 at intel.com>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at 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 {
> >>   */
> > 
> > Overallocate page_table etc by one and put a NULL sentinel in it.
> > 
> > for ((iter) = gen6_pde_index(start); \
> >      (length) > 0 && (pt = (pd)->page_table[iter]); \
> >      (iter)++, \
> >      temp = ALIGN(start+1, 1 << GEN6_PDE_SHIFT) - start, \
> >      temp = min_t(unsigned, temp, length), \
> > 
> > -Chris
> 
> This might trigger different warnings from some static analysers, as
> 'pt' doesn't get assigned at all if length == 0.

And? If pt is used when length==0 then I would agree with the analyzer
that pt should be invalid. If the analyzer can't tell that length is
non-zero in the use case and gives false positives, then the analyzer is
likely missing genuinine bugs in other cases.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre