Re: [PATCH 1/1] dl-load: add memory barrier before updating the next.

[Szabolcs Nagy <szabolcs dot nagy at arm dot com>]

On 18/03/17 09:12, Vaneet Narang wrote:
> Reason for adding barrier only in writer path is because reader path has conditional
> check. Since instructions has dependency so Instruction reordering is not possible.
> 
> Writer Code:  add_name_to_object()   |           Reader Code: _dl_name_match_p()
>                                      |
> newname->name = memcpy( ...)         |        struct libname_list *runp = map->l_libname;
> newname->next = NULL;                |        while (runp != NULL)
> newname->dont_free = 0;              |           if (strcmp (name, runp->name) == 0)
> lastp->next = newname;               |             return 1;  
>                                      |          else  
>                                      |             runp = runp->next;
> 
>   
...
> We have been facing issue in reader side where we get runp->next as valid but runp->next->name is NULL
> which results in NULL pointer access in strcmp but when we check coredump then we see runp->next->name
> so we are suspecting race condition.
> I don't see any issue with code as name is updated before next so only reason we can suspect for race 
> is instruction reordering at writer side.
> 
...
> We are facing issue on ARM based target. I have verified there
> is no compile time reordering, only reason which i can suspect now is
> runtime reordering because of cache miss on first store instruction. 
> So next store instruction gets executed first. 
> 

this is a data race in the c memory model (runp->next is
read and written concurrently without synchronization),
so the behaviour is undefined.

on arm, a write side barrier or release atomic store
should work in practice (but it's still incorrect e.g.
the compiled code can theoretically read runp->next->name
speculatively before runp->next by guessing the likely
value of runp->next and check the guess later), however
even if it was correct on arm, an arm only solution won't
be accepted in generic c code: the fix has to be correct
in the c memory model as Torvald already noted, see

https://sourceware.org/glibc/wiki/Concurrency

> Please suggest, if there is any other reason possible for issue i explained.
> Also please suggest if synchronization has already been taken care 
> between following threads executing loader code.
> 
>    Thread 1: dlopen  ---->  add_name_to_object
>    Thread 2: _dl_runtime_resolve  ---> _dl_name_match_p
> 

i think the write side needs atomic_store_release
and the read side an atomic_load_acquire and ideally
it should be checked where else this list is accessed
and document the synchronization in comments.

you should at least file a bug report against the
glibc dynamic-linker and in case you provide a patch
reference the bug number.