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

[Dave Gordon]

On 15/06/15 11:53, Chris Wilson wrote:
> 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

If you overallocate as suggested then you can keep the assignment to
'pt' first (i.e. unconditional, before the length test) so even a dumb
analyser won't get confused. OTOH, page_table[] is currently an array of
512 pointers which is (or can be) nicely page-aligned, whereas
increasing it to 513 will make them not fit so nicely :(

Perhaps the simplest way to write the test is:

    for ((iter) = gen6_pde_index(start);             \
         (pt) = (length) > 0 && (iter) < I915_PDES ? \
                  (pd)->page_table[iter] : NULL;     \
         (iter)++, ...

which always assigns 'pt', and always leaves it NULL on loop exit.

.Dave.