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

[Torvald Riegel <triegel at redhat dot com>]

On Sat, 2017-03-18 at 09:12 +0000, Vaneet Narang wrote:
> >only adding a barrier on the write side is not
> 
> >correct 
> 
> 
> 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;

While that may be true for a particular HW memory model, this is not
guaranteed on the level of C code.  (It's unlikely that a compiler would
transform the code so that this wouldn't work in practice, and some
software such as the Linux kernel assumes that this won't ever happen.
Nonetheless, for valid C11 code, you must avoid the data race, and you
must tell the compiler that you need this particular ordering.)

As Szabolcs has mentioned, atomic loads with memory_order_consume as
memory order are intended to be the mechanism that allows a program to
state that it depends on such dependencies; however, there are
specification bugs in how memory_order_consume is currently specified.
Therefore, we'd just use an memory_order_acquire load instead to get the
value for runp in the code above.

> In Reader code if runp is not NULL only then it reads runp->name So i don't think instruction reordering 
> with conditional case is possible so barrier is required.  
> If there is any instruction reordering happens in writer side as lastp->next updated before newname->name 
> then we can face issue in reader side. So this is the reason why i added barrier only in writer side.
> 
> 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.

Generally, in glibc we want to reason about concurrent code based on the
C11 memory model.  This ensures that the code is portable across
architectures.