[Mesa-dev] i965: Invalid memory accesses after resizing brw_codegen's store table

[Samuel Iglesias Gonsálvez]

There is no opinions about this issue or reviews of the proposed patch
after one week.

This is just a reminder in case you have missed it :-)

Sam

On 21/10/15 12:23, Iago Toral wrote:
> Hi,
> 
> The problem is with code like this (see brw_send_indirect_message):
> 
> setup = brw_OR(p, addr, desc, brw_imm_ud(0));
> send = next_insn(p, BRW_OPCODE_SEND);
> ...
> return setup;
> 
> If next_insn triggers a realloc of the instruction store, then the setup
> instruction pointer is no longer valid. Notice that this can happen
> anywhere where we keep pointers to previous instructions before creating
> new ones (!)
> 
> The patch from Samuel fixes this by special-casing this for SEND
> instructions only (since we know that the indirect versions can hit
> this, maybe there are more situations though). It does so by trying to
> make sure that we never realloc the store with a SEND instruction. For
> this, we realloc before we reach the end of the current store (32
> instructions before the limit) as long as the instruction is not a SEND
> (so that if it is a SEND we still have up to 32 opportunities to do the
> realloc without a different instruction before running out of space in
> the store).
> 
> Iago
> 
> On Wed, 2015-10-21 at 09:02 +0200, Samuel Iglesias Gonsalvez wrote:
>> Hello,
>>
>> I have found several invalid memory accesses when running
>> dEQP-GLES31.functional.ssbo.* tests on i965 driver (and gen7+). That
>> invalid memory accesses were unluckily happening when generating the
>> assembly instructions for SSBO stores for different compute shaders.
>>
>> However it looks like this problem could happen to other shaders and
>> situations. Because of that, I am going to explain the problem here:
>>
>> When generating a untyped surface write/read, i965 driver will end up
>> calling brw_send_indirect_message() through
>> brw_send_indirect_surface_message(). At brw_send_indirect_message()'s
>> 'else' branch, the code generates a load of the indirect descriptor to
>> an address register using an OR instruction and it also generates a new
>> SEND instruction; if this case happens, the OR instruction is returned.
>> brw_send_indirect_surface_message() uses that OR instruction to set mlen
>> and rlen's descriptor bits later.
>>
>> Just to give more context, when generating instructions in fs/vec4
>> generators, i965 driver uses pointers to elements in the 'store' table
>> inside struct brw_codegen. That table has an initial size of 1024 but,
>> when it's full, it is resized (doubling its size each time,
>> see brw_next_insn()). This resize operation ends up calling
>> realloc(). However the returned pointer by realloc() could be different
>> and the old allocated memory would be free'd as part of the process.
>>
>> Back to the issue, if the p->store's resize happens when we get the
>> pointer to the SEND instruction at brw_send_indirect_message()'s 'else'
>> branch, we could have the following problem:
>>
>> The realloc() returns a new pointer and *free'd* the old allocation, so
>> the pointer we previously saved for the OR instruction at
>> brw_send_indirect_surface_message() becomes invalid (because it is a
>> pointer of the old allocation). Then, we access to that invalid pointer
>> when setting up rlen/mlen bits at the end of
>> brw_send_indirect_surface_message() and we would have undefined results.
>>
>> This issue is quite unlikely to happen but it is reproducible on
>> ~120 dEQP-GLES31.functional.ssbo.* tests, basically because they have
>> the same shaders except the buffer variable's data type. Those tests
>> were failing intermittently at different rates but valgrind helped to
>> find what was happening.
>>
>> I would like to expose publicly the problem and analyse possible
>> solutions for it along with the community. For the time being, a patch
>> is proposed to mitigate this issue, which is sent as a reply to this
>> one.
>>
>> What this work-around patch does is: book enough room for one or more
>> consecutive SEND* instructions at the end of the p->store table in order
>> to avoid reallocating p->store in the aforementioned case. The 32 value
>> was chosen arbitrary because it has low impact in p->store
>> (its initial size is 1024) and makes this issue much less likely to
>> happen. We could tune this number to a less conservative value if
>> needed. If you want to test it, that patch should be applied on top of
>> this Curro's patch [0] as it fixes a lot of compute shader compilation
>> errors (~700 dEQP-GLES31.functional.ssbo.* tests). I have setup a branch
>> with both patches in [1].
>>
>> Feel free to comment about other solutions, ideas, opinions, etc.
>>
>> Thanks,
>>
>> Sam
>>
>> [0] See attachment at
>> http://lists.freedesktop.org/archives/mesa-dev/2015-October/097183.html
>> [1] $ git clone -b dEQP-functional-ssbo-fixes-v1 \
>> https://github.com/Igalia/mesa.git
>>
>> Samuel Iglesias Gonsalvez (1):
>>   i965: book space at the end of p->store for SEND opcodes to avoid
>>     invalid memory access
>>
>>  src/mesa/drivers/dri/i965/brw_eu_emit.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
> 
> 
>