On 27/02/17 09:20 PM, Marek Olšák wrote: > On Mon, Feb 27, 2017 at 10:57 AM, Michel Dänzer <[email protected]> wrote: >> On 25/02/17 01:56 PM, Timothy Arceri wrote: >>> On 24/02/17 21:02, Marek Olšák wrote: >>>> On Fri, Feb 24, 2017 at 3:18 AM, Timothy Arceri >>>> <[email protected]> wrote: >>>>> On 24/02/17 08:49, Timothy Arceri wrote: >>>>>> On 24/02/17 05:12, Marek Olšák wrote: >>>>>>> On Thu, Feb 23, 2017 at 3:09 AM, Timothy Arceri >>>>>>> <[email protected]> wrote: >>>>>>>> >>>>>>>> From: kdj0c <[email protected]> >>>>>>>> >>>>>>>> V2 (Timothy Arceri): >>>>>>>> - when loading from disk cache also binary insert into memory cache. >>>>>>>> - check that the binary loaded from disk is the correct size. If not >>>>>>>> delete the cache item and skip loading from cache. >>>>>>>> --- >>>>>>>> src/gallium/drivers/radeonsi/si_state_shaders.c | 69 >>>>>>>> ++++++++++++++++++++++--- >>>>>>>> 1 file changed, 62 insertions(+), 7 deletions(-) >>>>>>>> >>>>>>>> diff --git a/src/gallium/drivers/radeonsi/si_state_shaders.c >>>>>>>> b/src/gallium/drivers/radeonsi/si_state_shaders.c >>>>>>>> index f615aa8..71556f9 100644 >>>>>>>> --- a/src/gallium/drivers/radeonsi/si_state_shaders.c >>>>>>>> +++ b/src/gallium/drivers/radeonsi/si_state_shaders.c >>>>>>>> @@ -36,6 +36,9 @@ >>>>>>>> #include "util/u_memory.h" >>>>>>>> #include "util/u_prim.h" >>>>>>>> >>>>>>>> +#include "util/disk_cache.h" >>>>>>>> +#include "util/mesa-sha1.h" >>>>>>>> + >>>>>>>> /* SHADER_CACHE */ >>>>>>>> >>>>>>>> /** >>>>>>>> @@ -182,10 +185,12 @@ static bool si_load_shader_binary(struct >>>>>>>> si_shader *shader, void *binary) >>>>>>>> */ >>>>>>>> static bool si_shader_cache_insert_shader(struct si_screen *sscreen, >>>>>>>> void *tgsi_binary, >>>>>>>> - struct si_shader *shader) >>>>>>>> + struct si_shader *shader, >>>>>>>> + bool >>>>>>>> insert_into_disk_cache) >>>>>>>> { >>>>>>>> void *hw_binary; >>>>>>>> struct hash_entry *entry; >>>>>>>> + uint8_t key[CACHE_KEY_SIZE]; >>>>>>>> >>>>>>>> entry = _mesa_hash_table_search(sscreen->shader_cache, >>>>>>>> tgsi_binary); >>>>>>>> if (entry) >>>>>>>> @@ -201,6 +206,12 @@ static bool si_shader_cache_insert_shader(struct >>>>>>>> si_screen *sscreen, >>>>>>>> return false; >>>>>>>> } >>>>>>>> >>>>>>>> + if (sscreen->b.disk_shader_cache && insert_into_disk_cache) { >>>>>>>> + _mesa_sha1_compute(tgsi_binary, *((uint32_t >>>>>>>> *)tgsi_binary), key); >>>>>>> >>>>>>> >>>>>>> What happens if we randomly get a sha1 collision? >>>>>> >>>>>> >>>>>> You should stop playing your game which will be rendering incorrectly >>>>>> and by a lotto ticket. >>>>>> >>>>>>> Shouldn't we store the whole key as well? >>>>>> >>>>>> >>>>>> Sure I can add that, its cheap to check here anyway. Although the other >>>>>> cache stages rely on a collision being improbable. >>>>> >>>>> >>>>> >>>>> For some reason I thought the key was simpler than it is. It seems >>>>> excessive >>>>> to store and compare the tgsi again. I don't think git even worries >>>>> about >>>>> the possibility of a collision and we will be dealing with much smaller >>>>> amounts of cache items then commits in a git repository. >>>>> >>>>> Thoughts? >>>> >>>> I'll let others comment on this. If nobody comments, checking only the >>>> key can stay. >>> >>> Seems SVN didn't used to worry about collisions either. >>> >>> https://arstechnica.com/security/2017/02/watershed-sha1-collision-just-broke-the-webkit-repository-others-may-follow/ >> >> What scenario are we concerned about here? Getting a genuine SHA1 >> collision between two shader objects is still as unlikely as it ever was >> (virtually impossible?), and someone with access to the shader cache >> files can wreak havoc much more easily than via a hash collision. > > I'm concerned about the random scenario where 2 shaders can have the > same hash. That is, if the user doesn't do anything wrong and nobody > else corrupts the cache, can we say for sure that a collision will > never ever happen?
We cannot, of course — if we could, it would be possible to compress any file to any smaller size without loss. :) (Reminds me of the hilarious TV series Silicon Valley) If there are any other parameters like the size that can easily be double-checked, that's probably a good idea just in case, but other than that I think Timothy's followup is quite convincing that this is extremely unlikely to ever happen in practice. It might also be worth looking at what (if anything) ccache does to deal with this though. (Has anyone run into or heard of a ccache issue due to a hash collision? A quick web search only turns up https://sourceforge.net/p/free-cad/mailman/message/33250553/ for me, but it lacks details about the suspected collision) -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
