* [RFC] drm/i915: prevent out of range pt in the PDE macros (take 2)
@ 2015-06-12 21:30 Paulo Zanoni
2015-06-13 8:28 ` Chris Wilson
2015-06-15 10:31 ` Dave Gordon
0 siblings, 2 replies; 7+ messages in thread
From: Paulo Zanoni @ 2015-06-12 21:30 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
We tried to fix this in the following commit:
commit fdc454c1484a20e1345cf4e4d7a9feaee814147f
Author: Michel Thierry <michel.thierry@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@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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 {
*/
#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; \
+ pt = iter < I915_PDES ? (pd)->page_table[iter] : NULL, \
+ length > 0 && iter < I915_PDES; \
iter++, \
temp = ALIGN(start+1, 1 << GEN6_PDE_SHIFT) - start, \
temp = min_t(unsigned, temp, length), \
@@ -360,7 +361,8 @@ struct i915_hw_ppgtt {
#define gen6_for_all_pdes(pt, ppgtt, iter) \
for (iter = 0; \
- pt = ppgtt->pd.page_table[iter], iter < I915_PDES; \
+ pt = iter < I915_PDES ? ppgtt->pd.page_table[iter] : NULL, \
+ iter < I915_PDES; \
iter++)
static inline uint32_t i915_pte_index(uint64_t address, uint32_t pde_shift)
@@ -417,7 +419,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; \
+ pt = iter < I915_PDES ? (pd)->page_table[iter] : NULL, \
+ length > 0 && iter < I915_PDES; \
iter++, \
temp = ALIGN(start+1, 1 << GEN8_PDE_SHIFT) - start, \
temp = min(temp, length), \
@@ -425,7 +428,9 @@ 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 < GEN8_LEGACY_PDPES; \
+ pd = iter < GEN8_LEGACY_PDPES ? \
+ (pdp)->page_directory[iter] : NULL, \
+ length > 0 && iter < GEN8_LEGACY_PDPES; \
iter++, \
temp = ALIGN(start+1, 1 << GEN8_PDPE_SHIFT) - start, \
temp = min(temp, length), \
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC] drm/i915: prevent out of range pt in the PDE macros (take 2)
2015-06-12 21:30 [RFC] drm/i915: prevent out of range pt in the PDE macros (take 2) Paulo Zanoni
@ 2015-06-13 8:28 ` Chris Wilson
2015-06-15 10:33 ` Dave Gordon
2015-06-15 10:31 ` Dave Gordon
1 sibling, 1 reply; 7+ messages in thread
From: Chris Wilson @ 2015-06-13 8:28 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Fri, Jun 12, 2015 at 06:30:56PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> We tried to fix this in the following commit:
>
> commit fdc454c1484a20e1345cf4e4d7a9feaee814147f
> Author: Michel Thierry <michel.thierry@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@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] drm/i915: prevent out of range pt in the PDE macros (take 2)
2015-06-12 21:30 [RFC] drm/i915: prevent out of range pt in the PDE macros (take 2) Paulo Zanoni
2015-06-13 8:28 ` Chris Wilson
@ 2015-06-15 10:31 ` Dave Gordon
1 sibling, 0 replies; 7+ messages in thread
From: Dave Gordon @ 2015-06-15 10:31 UTC (permalink / raw)
To: Paulo Zanoni, intel-gfx; +Cc: Paulo Zanoni
On 12/06/15 22:30, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> We tried to fix this in the following commit:
>
> commit fdc454c1484a20e1345cf4e4d7a9feaee814147f
> Author: Michel Thierry <michel.thierry@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@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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 {
> */
> #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; \
> + pt = iter < I915_PDES ? (pd)->page_table[iter] : NULL, \
> + length > 0 && iter < I915_PDES; \
You don't need the repeated test on 'iter'; you can write the test
clause of the loop as:
(pt = iter < I915_PDES ? (pd)->page_table[iter] : NULL) &&
length > 0;
using the fact that pt will be NULL when iter >= I915_PDES to break from
the loop :)
This version will leave 'pt' NULL after the loop if the break was due to
the test on 'iter', but non-NULL if the test on 'length' triggered the
break -- is this a useful feature?
.Dave.
> temp = ALIGN(start+1, 1 << GEN6_PDE_SHIFT) - start, \
> temp = min_t(unsigned, temp, length), \
> @@ -360,7 +361,8 @@ struct i915_hw_ppgtt {
>
> #define gen6_for_all_pdes(pt, ppgtt, iter) \
> for (iter = 0; \
> - pt = ppgtt->pd.page_table[iter], iter < I915_PDES; \
> + pt = iter < I915_PDES ? ppgtt->pd.page_table[iter] : NULL, \
> + iter < I915_PDES; \
> iter++)
>
> static inline uint32_t i915_pte_index(uint64_t address, uint32_t pde_shift)
> @@ -417,7 +419,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; \
> + pt = iter < I915_PDES ? (pd)->page_table[iter] : NULL, \
> + length > 0 && iter < I915_PDES; \
> iter++, \
> temp = ALIGN(start+1, 1 << GEN8_PDE_SHIFT) - start, \
> temp = min(temp, length), \
> @@ -425,7 +428,9 @@ 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 < GEN8_LEGACY_PDPES; \
> + pd = iter < GEN8_LEGACY_PDPES ? \
> + (pdp)->page_directory[iter] : NULL, \
> + length > 0 && iter < GEN8_LEGACY_PDPES; \
> iter++, \
> temp = ALIGN(start+1, 1 << GEN8_PDPE_SHIFT) - start, \
> temp = min(temp, length), \
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] drm/i915: prevent out of range pt in the PDE macros (take 2)
2015-06-13 8:28 ` Chris Wilson
@ 2015-06-15 10:33 ` Dave Gordon
2015-06-15 10:53 ` Chris Wilson
0 siblings, 1 reply; 7+ messages in thread
From: Dave Gordon @ 2015-06-15 10:33 UTC (permalink / raw)
To: Chris Wilson, Paulo Zanoni, intel-gfx, Paulo Zanoni
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@intel.com>
>>
>> We tried to fix this in the following commit:
>>
>> commit fdc454c1484a20e1345cf4e4d7a9feaee814147f
>> Author: Michel Thierry <michel.thierry@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@intel.com>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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.
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] drm/i915: prevent out of range pt in the PDE macros (take 2)
2015-06-15 10:33 ` Dave Gordon
@ 2015-06-15 10:53 ` Chris Wilson
2015-06-16 13:45 ` Dave Gordon
0 siblings, 1 reply; 7+ messages in thread
From: Chris Wilson @ 2015-06-15 10:53 UTC (permalink / raw)
To: Dave Gordon; +Cc: intel-gfx, Paulo Zanoni
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@intel.com>
> >>
> >> We tried to fix this in the following commit:
> >>
> >> commit fdc454c1484a20e1345cf4e4d7a9feaee814147f
> >> Author: Michel Thierry <michel.thierry@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@intel.com>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] drm/i915: prevent out of range pt in the PDE macros (take 2)
2015-06-15 10:53 ` Chris Wilson
@ 2015-06-16 13:45 ` Dave Gordon
2015-06-16 14:04 ` Chris Wilson
0 siblings, 1 reply; 7+ messages in thread
From: Dave Gordon @ 2015-06-16 13:45 UTC (permalink / raw)
To: Chris Wilson, Paulo Zanoni, intel-gfx, Paulo Zanoni
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@intel.com>
>>>>
>>>> We tried to fix this in the following commit:
>>>>
>>>> commit fdc454c1484a20e1345cf4e4d7a9feaee814147f
>>>> Author: Michel Thierry <michel.thierry@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@intel.com>
>>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] drm/i915: prevent out of range pt in the PDE macros (take 2)
2015-06-16 13:45 ` Dave Gordon
@ 2015-06-16 14:04 ` Chris Wilson
0 siblings, 0 replies; 7+ messages in thread
From: Chris Wilson @ 2015-06-16 14:04 UTC (permalink / raw)
To: Dave Gordon; +Cc: intel-gfx, Paulo Zanoni
On Tue, Jun 16, 2015 at 02:45:39PM +0100, Dave Gordon wrote:
> 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@intel.com>
> >>>>
> >>>> We tried to fix this in the following commit:
> >>>>
> >>>> commit fdc454c1484a20e1345cf4e4d7a9feaee814147f
> >>>> Author: Michel Thierry <michel.thierry@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@intel.com>
> >>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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 :(
Good point.
> 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.
Just one level of parenthesis required to shut gcc up, but that indeed
does look the neatest way of writing it so far.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-06-16 14:05 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-12 21:30 [RFC] drm/i915: prevent out of range pt in the PDE macros (take 2) Paulo Zanoni
2015-06-13 8:28 ` Chris Wilson
2015-06-15 10:33 ` Dave Gordon
2015-06-15 10:53 ` Chris Wilson
2015-06-16 13:45 ` Dave Gordon
2015-06-16 14:04 ` Chris Wilson
2015-06-15 10:31 ` Dave Gordon
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.