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

[Iago Toral]

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(-)
>