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

From: Paulo Zanoni <przanoni@gmail.com>
To: intel-gfx@lists.freedesktop.org
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Subject: [RFC] drm/i915: prevent out of range pt in the PDE macros (take 2)
Date: Fri, 12 Jun 2015 18:30:56 -0300	[thread overview]
Message-ID: <1434144656-1557-1-git-send-email-przanoni@gmail.com> (raw)

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
