Re: [Mesa-dev] [PATCH] radeonsi: fix out-of-bounds indexing of shader images

[Michel Dänzer]

On 22.03.2016 05:41, Nicolai Hähnle wrote:
> From: Nicolai Hähnle 
> 
> Results are undefined but may not crash. Without this change, out-of-bounds
> indexing can lead to VM faults and GPU hangs.
> 
> Constant buffers, samplers, and possibly others will eventually need similar
> treatment to support GL_ARB_robust_buffer_access_behavior.

Reviewed-and-Tested-by: Michel Dänzer 


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radeonsi: fix out-of-bounds indexing of shader images

[Marek Olšák]

Reviewed-by: Marek Olšák 

Marek

On Mon, Mar 21, 2016 at 9:41 PM, Nicolai Hähnle  wrote:
> From: Nicolai Hähnle 
>
> Results are undefined but may not crash. Without this change, out-of-bounds
> indexing can lead to VM faults and GPU hangs.
>
> Constant buffers, samplers, and possibly others will eventually need similar
> treatment to support GL_ARB_robust_buffer_access_behavior.
> ---
>  src/gallium/drivers/radeonsi/si_shader.c | 44 
> +++-
>  1 file changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/drivers/radeonsi/si_shader.c 
> b/src/gallium/drivers/radeonsi/si_shader.c
> index 9ad2290..1e4bf82 100644
> --- a/src/gallium/drivers/radeonsi/si_shader.c
> +++ b/src/gallium/drivers/radeonsi/si_shader.c
> @@ -532,6 +532,37 @@ static LLVMValueRef get_indirect_index(struct 
> si_shader_context *ctx,
>  }
>
>  /**
> + * Like get_indirect_index, but restricts the return value to a (possibly
> + * undefined) value inside [0..num).
> + */
> +static LLVMValueRef get_bounded_indirect_index(struct si_shader_context *ctx,
> +  const struct tgsi_ind_register 
> *ind,
> +  int rel_index, unsigned num)
> +{
> +   struct gallivm_state *gallivm = >radeon_bld.gallivm;
> +   LLVMBuilderRef builder = gallivm->builder;
> +   LLVMValueRef result = get_indirect_index(ctx, ind, rel_index);
> +   LLVMValueRef c_max = LLVMConstInt(ctx->i32, num - 1, 0);
> +   LLVMValueRef cc;
> +
> +   if (util_is_power_of_two(num)) {
> +   result = LLVMBuildAnd(builder, result, c_max, "");
> +   } else {
> +   /* In theory, this MAX pattern should result in code that is
> +* as good as the bit-wise AND above.
> +*
> +* In practice, LLVM generates worse code (at the time of
> +* writing), because its value tracking is not strong enough.
> +*/
> +   cc = LLVMBuildICmp(builder, LLVMIntULE, result, c_max, "");
> +   result = LLVMBuildSelect(builder, cc, result, c_max, "");
> +   }
> +
> +   return result;
> +}
> +
> +
> +/**
>   * Calculate a dword address given an input or output register and a stride.
>   */
>  static LLVMValueRef get_dw_address(struct si_shader_context *ctx,
> @@ -2814,7 +2845,18 @@ image_fetch_rsrc(
> LLVMValueRef rsrc_ptr;
> LLVMValueRef tmp;
>
> -   ind_index = get_indirect_index(ctx, >Indirect, 
> image->Register.Index);
> +   /* From the GL_ARB_shader_image_load_store extension spec:
> +*
> +*If a shader performs an image load, store, or atomic
> +*operation using an image variable declared as an array,
> +*and if the index used to select an individual element is
> +*negative or greater than or equal to the size of the
> +*array, the results of the operation are undefined but 
> may
> +*not lead to termination.
> +*/
> +   ind_index = get_bounded_indirect_index(ctx, >Indirect,
> +  image->Register.Index,
> +  SI_NUM_IMAGES);
>
> rsrc_ptr = LLVMGetParam(ctx->radeon_bld.main_fn, 
> SI_PARAM_IMAGES);
> tmp = build_indexed_load_const(ctx, rsrc_ptr, ind_index);
> --
> 2.5.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radeonsi: fix out-of-bounds indexing of shader images

[eocallaghan]

Reviewed-by: Edward O'Callaghan 

On 2016-03-22 07:41, Nicolai Hähnle wrote:

From: Nicolai Hähnle 

Results are undefined but may not crash. Without this change, 
out-of-bounds

indexing can lead to VM faults and GPU hangs.

Constant buffers, samplers, and possibly others will eventually need 
similar

treatment to support GL_ARB_robust_buffer_access_behavior.
---
 src/gallium/drivers/radeonsi/si_shader.c | 44 
+++-

 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/src/gallium/drivers/radeonsi/si_shader.c
b/src/gallium/drivers/radeonsi/si_shader.c
index 9ad2290..1e4bf82 100644
--- a/src/gallium/drivers/radeonsi/si_shader.c
+++ b/src/gallium/drivers/radeonsi/si_shader.c
@@ -532,6 +532,37 @@ static LLVMValueRef get_indirect_index(struct
si_shader_context *ctx,
 }

 /**
+ * Like get_indirect_index, but restricts the return value to a 
(possibly

+ * undefined) value inside [0..num).
+ */
+static LLVMValueRef get_bounded_indirect_index(struct 
si_shader_context *ctx,

+  const struct tgsi_ind_register 
*ind,
+  int rel_index, unsigned num)
+{
+   struct gallivm_state *gallivm = >radeon_bld.gallivm;
+   LLVMBuilderRef builder = gallivm->builder;
+   LLVMValueRef result = get_indirect_index(ctx, ind, rel_index);
+   LLVMValueRef c_max = LLVMConstInt(ctx->i32, num - 1, 0);
+   LLVMValueRef cc;
+
+   if (util_is_power_of_two(num)) {
+   result = LLVMBuildAnd(builder, result, c_max, "");
+   } else {
+   /* In theory, this MAX pattern should result in code that is
+* as good as the bit-wise AND above.
+*
+* In practice, LLVM generates worse code (at the time of
+* writing), because its value tracking is not strong enough.
+*/
+   cc = LLVMBuildICmp(builder, LLVMIntULE, result, c_max, "");
+   result = LLVMBuildSelect(builder, cc, result, c_max, "");
+   }
+
+   return result;
+}
+
+
+/**
  * Calculate a dword address given an input or output register and a 
stride.

  */
 static LLVMValueRef get_dw_address(struct si_shader_context *ctx,
@@ -2814,7 +2845,18 @@ image_fetch_rsrc(
LLVMValueRef rsrc_ptr;
LLVMValueRef tmp;

-		ind_index = get_indirect_index(ctx, >Indirect, 
image->Register.Index);

+   /* From the GL_ARB_shader_image_load_store extension spec:
+*
+*If a shader performs an image load, store, or atomic
+*operation using an image variable declared as an array,
+*and if the index used to select an individual element is
+*negative or greater than or equal to the size of the
+*array, the results of the operation are undefined but may
+*not lead to termination.
+*/
+   ind_index = get_bounded_indirect_index(ctx, >Indirect,
+  image->Register.Index,
+  SI_NUM_IMAGES);

rsrc_ptr = LLVMGetParam(ctx->radeon_bld.main_fn, 
SI_PARAM_IMAGES);
tmp = build_indexed_load_const(ctx, rsrc_ptr, ind_index);


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radeonsi: fix out-of-bounds indexing of shader images

[Nicolai Hähnle]

On 21.03.2016 15:41, Nicolai Hähnle wrote:

From: Nicolai Hähnle 

Results are undefined but may not crash. Without this change, out-of-bounds
indexing can lead to VM faults and GPU hangs.

Constant buffers, samplers, and possibly others will eventually need similar
treatment to support GL_ARB_robust_buffer_access_behavior.


FWIW, I read the relevant specs again, and we do already need to apply 
the same change to the other buffers as well for ARB_robustness. I'll 
follow up with a patch soon.


Amusingly enough, this ends up producing cleaner code. The reason is 
simple: previously, LLVM produced code that works with a full 32 bit 
index, hence manipulating 64 bit pointer values all the way. By masking 
down to the low bits, LLVM understands that some of the intermediate 
computations (i.e., multiplying by the stride of the descriptor array) 
can be done using only 32 bit registers.


Cheers,
Nicolai


---
  src/gallium/drivers/radeonsi/si_shader.c | 44 +++-
  1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/src/gallium/drivers/radeonsi/si_shader.c 
b/src/gallium/drivers/radeonsi/si_shader.c
index 9ad2290..1e4bf82 100644
--- a/src/gallium/drivers/radeonsi/si_shader.c
+++ b/src/gallium/drivers/radeonsi/si_shader.c
@@ -532,6 +532,37 @@ static LLVMValueRef get_indirect_index(struct 
si_shader_context *ctx,
  }

  /**
+ * Like get_indirect_index, but restricts the return value to a (possibly
+ * undefined) value inside [0..num).
+ */
+static LLVMValueRef get_bounded_indirect_index(struct si_shader_context *ctx,
+  const struct tgsi_ind_register 
*ind,
+  int rel_index, unsigned num)
+{
+   struct gallivm_state *gallivm = >radeon_bld.gallivm;
+   LLVMBuilderRef builder = gallivm->builder;
+   LLVMValueRef result = get_indirect_index(ctx, ind, rel_index);
+   LLVMValueRef c_max = LLVMConstInt(ctx->i32, num - 1, 0);
+   LLVMValueRef cc;
+
+   if (util_is_power_of_two(num)) {
+   result = LLVMBuildAnd(builder, result, c_max, "");
+   } else {
+   /* In theory, this MAX pattern should result in code that is
+* as good as the bit-wise AND above.
+*
+* In practice, LLVM generates worse code (at the time of
+* writing), because its value tracking is not strong enough.
+*/
+   cc = LLVMBuildICmp(builder, LLVMIntULE, result, c_max, "");
+   result = LLVMBuildSelect(builder, cc, result, c_max, "");
+   }
+
+   return result;
+}
+
+
+/**
   * Calculate a dword address given an input or output register and a stride.
   */
  static LLVMValueRef get_dw_address(struct si_shader_context *ctx,
@@ -2814,7 +2845,18 @@ image_fetch_rsrc(
LLVMValueRef rsrc_ptr;
LLVMValueRef tmp;

-   ind_index = get_indirect_index(ctx, >Indirect, 
image->Register.Index);
+   /* From the GL_ARB_shader_image_load_store extension spec:
+*
+*If a shader performs an image load, store, or atomic
+*operation using an image variable declared as an array,
+*and if the index used to select an individual element is
+*negative or greater than or equal to the size of the
+*array, the results of the operation are undefined but may
+*not lead to termination.
+*/
+   ind_index = get_bounded_indirect_index(ctx, >Indirect,
+  image->Register.Index,
+  SI_NUM_IMAGES);

rsrc_ptr = LLVMGetParam(ctx->radeon_bld.main_fn, 
SI_PARAM_IMAGES);
tmp = build_indexed_load_const(ctx, rsrc_ptr, ind_index);


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] radeonsi: fix out-of-bounds indexing of shader images

[Nicolai Hähnle]

From: Nicolai Hähnle 

Results are undefined but may not crash. Without this change, out-of-bounds
indexing can lead to VM faults and GPU hangs.

Constant buffers, samplers, and possibly others will eventually need similar
treatment to support GL_ARB_robust_buffer_access_behavior.
---
 src/gallium/drivers/radeonsi/si_shader.c | 44 +++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/src/gallium/drivers/radeonsi/si_shader.c 
b/src/gallium/drivers/radeonsi/si_shader.c
index 9ad2290..1e4bf82 100644
--- a/src/gallium/drivers/radeonsi/si_shader.c
+++ b/src/gallium/drivers/radeonsi/si_shader.c
@@ -532,6 +532,37 @@ static LLVMValueRef get_indirect_index(struct 
si_shader_context *ctx,
 }
 
 /**
+ * Like get_indirect_index, but restricts the return value to a (possibly
+ * undefined) value inside [0..num).
+ */
+static LLVMValueRef get_bounded_indirect_index(struct si_shader_context *ctx,
+  const struct tgsi_ind_register 
*ind,
+  int rel_index, unsigned num)
+{
+   struct gallivm_state *gallivm = >radeon_bld.gallivm;
+   LLVMBuilderRef builder = gallivm->builder;
+   LLVMValueRef result = get_indirect_index(ctx, ind, rel_index);
+   LLVMValueRef c_max = LLVMConstInt(ctx->i32, num - 1, 0);
+   LLVMValueRef cc;
+
+   if (util_is_power_of_two(num)) {
+   result = LLVMBuildAnd(builder, result, c_max, "");
+   } else {
+   /* In theory, this MAX pattern should result in code that is
+* as good as the bit-wise AND above.
+*
+* In practice, LLVM generates worse code (at the time of
+* writing), because its value tracking is not strong enough.
+*/
+   cc = LLVMBuildICmp(builder, LLVMIntULE, result, c_max, "");
+   result = LLVMBuildSelect(builder, cc, result, c_max, "");
+   }
+
+   return result;
+}
+
+
+/**
  * Calculate a dword address given an input or output register and a stride.
  */
 static LLVMValueRef get_dw_address(struct si_shader_context *ctx,
@@ -2814,7 +2845,18 @@ image_fetch_rsrc(
LLVMValueRef rsrc_ptr;
LLVMValueRef tmp;
 
-   ind_index = get_indirect_index(ctx, >Indirect, 
image->Register.Index);
+   /* From the GL_ARB_shader_image_load_store extension spec:
+*
+*If a shader performs an image load, store, or atomic
+*operation using an image variable declared as an array,
+*and if the index used to select an individual element is
+*negative or greater than or equal to the size of the
+*array, the results of the operation are undefined but may
+*not lead to termination.
+*/
+   ind_index = get_bounded_indirect_index(ctx, >Indirect,
+  image->Register.Index,
+  SI_NUM_IMAGES);
 
rsrc_ptr = LLVMGetParam(ctx->radeon_bld.main_fn, 
SI_PARAM_IMAGES);
tmp = build_indexed_load_const(ctx, rsrc_ptr, ind_index);
-- 
2.5.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev