drm/i915: prevent out of range pt in the PDE macros (take 2)

From: Paulo Zanoni <[email protected]>

We tried to fix this in the following commit:

commit fdc454c1484a20e1345cf4e4d7a9feaee814147f

Author: Michel Thierry <[email protected]>
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 <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

---
 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.

drivers/gpu/drm/i915/i915_gem_gtt.h +0 -0

index 0d46dd2..b202ca0 100644

Quoted 47 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

We tried to fix this in the following commit:

commit fdc454c1484a20e1345cf4e4d7a9feaee814147f

Author: Michel Thierry <[email protected]>
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 <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

---
 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.

drivers/gpu/drm/i915/i915_gem_gtt.h +0 -0

index 0d46dd2..b202ca0 100644

drivers/gpu/drm/i915/i915_gem_gtt.h struct i915_hw_ppgtt { · line 352

Quoted 1 line

drivers/gpu/drm/i915/i915_gem_gtt.h +0 -0

@@ -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

Quoted 58 lines · Chris Wilson

Quoted 47 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

We tried to fix this in the following commit:

commit fdc454c1484a20e1345cf4e4d7a9feaee814147f

Author: Michel Thierry <[email protected]>
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 <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

---
 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.

drivers/gpu/drm/i915/i915_gem_gtt.h +0 -0

index 0d46dd2..b202ca0 100644

drivers/gpu/drm/i915/i915_gem_gtt.h struct i915_hw_ppgtt { · line 352

Quoted 1 line

drivers/gpu/drm/i915/i915_gem_gtt.h +0 -0

@@ -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), \

Quoted 1 line

-Chris

This might trigger different warnings from some static analysers, as 'pt' doesn't get assigned at all if length == 0.

Quoted 61 lines · Dave Gordon

Quoted 59 lines · Chris Wilson

Quoted 47 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

We tried to fix this in the following commit:

commit fdc454c1484a20e1345cf4e4d7a9feaee814147f

Author: Michel Thierry <[email protected]>
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 <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

---
 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.

drivers/gpu/drm/i915/i915_gem_gtt.h +0 -0

index 0d46dd2..b202ca0 100644

drivers/gpu/drm/i915/i915_gem_gtt.h struct i915_hw_ppgtt { · line 352

Quoted 1 line

drivers/gpu/drm/i915/i915_gem_gtt.h +0 -0

@@ -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

Quoted 2 lines

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

Quoted 62 lines · Chris Wilson

Quoted 61 lines · Dave Gordon

Quoted 59 lines · Chris Wilson

Quoted 47 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

We tried to fix this in the following commit:

commit fdc454c1484a20e1345cf4e4d7a9feaee814147f

Author: Michel Thierry <[email protected]>
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 <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

---
 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.

drivers/gpu/drm/i915/i915_gem_gtt.h +0 -0

index 0d46dd2..b202ca0 100644

drivers/gpu/drm/i915/i915_gem_gtt.h struct i915_hw_ppgtt { · line 352

Quoted 1 line

drivers/gpu/drm/i915/i915_gem_gtt.h +0 -0

@@ -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

Quoted 8 lines

Quoted 2 lines

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.

From: Paulo Zanoni <[email protected]>

We tried to fix this in the following commit:

commit fdc454c1484a20e1345cf4e4d7a9feaee814147f

Author: Michel Thierry <[email protected]>
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 <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

---
 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.

drivers/gpu/drm/i915/i915_gem_gtt.h +0 -0

index 0d46dd2..b202ca0 100644

drivers/gpu/drm/i915/i915_gem_gtt.h +4 -2

 	     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),					\