[Mesa-dev] [PATCH 1/3] cso: don't release sampler states that are bound

[Nicolai Hähnle]

On 05.12.2016 10:05, Michel Dänzer wrote:
> On 03/12/16 05:38 AM, Marek Olšák wrote:
>> From: Marek Olšák <marek.olsak at amd.com>
>>
>> This fixes random radeonsi GPU hangs in Batman Arkham: Origins (Wine) and
>> probably many other games too.
>>
>> cso_cache deletes sampler states when the cache size is too big and doesn't
>> check which sampler states are bound, causing use-after-free in drivers.
>> Because of that, radeonsi uploaded garbage sampler states and the hardware
>> went bananas. Other drivers may have experienced similar issues.
>>
>> Cc: 13.0 12.0 <mesa-stable at lists.freedesktop.org>
>> ---
>>  src/gallium/auxiliary/cso_cache/cso_cache.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/gallium/auxiliary/cso_cache/cso_cache.c b/src/gallium/auxiliary/cso_cache/cso_cache.c
>> index b240c93..1f3be4b 100644
>> --- a/src/gallium/auxiliary/cso_cache/cso_cache.c
>> +++ b/src/gallium/auxiliary/cso_cache/cso_cache.c
>> @@ -181,21 +181,23 @@ static inline void sanitize_cb(struct cso_hash *hash, enum cso_cache_type type,
>>        --to_remove;
>>     }
>>  }
>>
>>  struct cso_hash_iter
>>  cso_insert_state(struct cso_cache *sc,
>>                   unsigned hash_key, enum cso_cache_type type,
>>                   void *state)
>>  {
>>     struct cso_hash *hash = _cso_hash_for_type(sc, type);
>> -   sanitize_hash(sc, hash, type, sc->max_size);
>> +
>> +   if (type != CSO_SAMPLER)
>> +      sanitize_hash(sc, hash, type, sc->max_size);
>>
>>     return cso_hash_insert(hash, hash_key, state);
>>  }
>>
>>  struct cso_hash_iter
>>  cso_find_state(struct cso_cache *sc,
>>                 unsigned hash_key, enum cso_cache_type type)
>>  {
>>     struct cso_hash *hash = _cso_hash_for_type(sc, type);
>>
>>
>
> If I understand correctly, this means that the maximum cache size
> effectively isn't enforced for sampler states? Wouldn't it be better
> instead to add code which prevents currently bound states from being
> deleted from here?

This is a really good catch, but I agree. Looks like sanitize_cb should 
scan the array of currently bound samplers. The alternative is 
ref-counting; it's a trade-off between making the fast-path slightly 
slower and a slow-path somewhat more slower.

Patches 2 & 3 are:

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

>
>