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

[Samuel Iglesias Gonsalvez]

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

-- 
2.1.4