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

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

On 16/03/17 05:12, Maninder Singh wrote:
> This patch adds memory barrier before updating the liblist next.
> 
> Issue Fix: race condition between add_name_to_object  & _dl_name_match_p.
> One threads calling dlopen which further calls add_name_to_object &
> other thread trying to resolve RTLD_LAZY symbols through _dl_runtime_resolve
> which further calls.
> 

i assume _dl_name_match_p is called from do_lookup_x which
is called from _dl_fixup at lazy resolution.

> _dl_name_match_p checks if libname->next is valid, then it assumes
> libname->next->name to be valid. Also add_name_to_object initialized name
> first and then sets valid next pointer.
> 
> This patch avoids any reorder of instruction when next is set before name
> to avoid any race.
> 
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
> ---
>  elf/dl-load.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 016a99c..c1e69b8 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -429,6 +429,7 @@ add_name_to_object (struct link_map *l, const char *name)
>    newname->name = memcpy (newname + 1, name, name_len);
>    newname->next = NULL;
>    newname->dont_free = 0;
> +  atomic_write_barrier ();
>    lastp->next = newname;
>  }

ideally lastp->next = newname should be a mo_release
store and wherever map->next is read should be a
mo_acquire load or mo_consume load (if it is followed
by a map->next->something load).

mo_consume is frowned upon because of some
specification and implementation issues so use
mo_acquire.

only adding a barrier on the write side is not
correct (it happens to work on most architectures
because the compiled code of the read side will
have dependent loads that are guaranteed to be
ordered even on some weakly ordered architectures
like arm and aarch64, but on the c language
level there is no such guarantee so the compiler
may break this)