[Mesa-dev] [PATCH] i965/fs: Don't allow OOB array access of images

[Francisco Jerez]

Francisco Jerez <currojerez at riseup.net> writes:

> Jason Ekstrand <jason at jlekstrand.net> writes:
>
>> We have had a guard against OOB array access of images on IVB for a long
>> time, but it can actually cause hangs on any GPU generation.  This can
>> happen due to getting an untyped SURFACE_STATE for a typed message or
>> vice-versa.
>
> Are you sure about that?  AFAIK the
> "arb_shader_image_load_store/invalid/format mismatch" subtest already
> leads to a typed/untyped mismatch on HSW and I'm not aware of it causing
> hangs -- Not saying that the fix below is wrong, the code looks
> reasonable but I'm skeptical that the reason for the hang is simply a
> surface format mismatch.
>
With our off-line discussion in mind, if you s/or vice-versa// above
and s/push/pull/ below this patch is:

Reviewed-by: Francisco Jerez <currojerez at riseup.net>

>> We didn't used to hit this with the piglit test on anything
>> other than IVB because the OOB in the test would cause us to go past the
>> top of the push constant UBO and we would get a surface index of 0 which is
>
> You probably meant the pull constant buffer?
>
>> was always a valid surface.  Now that we're pushing small arrays, we can
>> end up grabbing garbage from the GRF and going to some random index which
>> causes a hang.  The solution is to just do the bounds check on all
>> hardware.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94944
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 26 +++++++++++---------------
>>  1 file changed, 11 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> index c16f1ed..e775049 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> @@ -1272,21 +1272,17 @@ fs_visitor::get_nir_image_deref(const nir_deref_var *deref)
>>        if (deref_array->deref_array_type == nir_deref_array_type_indirect) {
>>           fs_reg tmp = vgrf(glsl_type::uint_type);
>>  
>> -         if (devinfo->gen == 7 && !devinfo->is_haswell) {
>> -            /* IVB hangs when trying to access an invalid surface index with
>> -             * the dataport.  According to the spec "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" -- which is one
>> -             * of the possible outcomes of the hang.  Clamp the index to
>> -             * prevent access outside of the array bounds.
>> -             */
>> -            bld.emit_minmax(tmp, retype(get_nir_src(deref_array->indirect),
>> -                                        BRW_REGISTER_TYPE_UD),
>> -                            brw_imm_ud(size - base - 1), BRW_CONDITIONAL_L);
>> -         } else {
>> -            bld.MOV(tmp, get_nir_src(deref_array->indirect));
>> -         }
>> +         /* Accessing an invalid surface index with the dataport can result
>> +          * in a hang.  According to the spec "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" -- which is one
>> +          * of the possible outcomes of the hang.  Clamp the index to
>> +          * prevent access outside of the array bounds.
>> +          */
>> +         bld.emit_minmax(tmp, retype(get_nir_src(deref_array->indirect),
>> +                                     BRW_REGISTER_TYPE_UD),
>> +                         brw_imm_ud(size - base - 1), BRW_CONDITIONAL_L);
>>  
>>           indirect_max += element_size * (tail->type->length - 1);
>>  
>> -- 
>> 2.5.0.400.gff86faf
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160415/f5727dd5/attachment.sig>