* Re: [PATCH v2] dl-load: add memory barrier before updating the next
2017-03-29 7:00 ` [PATCH v2] dl-load: add memory barrier before updating the next Maninder Singh
@ 2017-03-29 8:58 ` Torvald Riegel
2017-03-30 9:58 ` Szabolcs Nagy
1 sibling, 0 replies; 3+ messages in thread
From: Torvald Riegel @ 2017-03-29 8:58 UTC (permalink / raw)
To: Maninder Singh
Cc: libc-alpha, szabolcs.nagy, pankaj.m, ajeet.y, a.sahrawat,
lalit.mohan, akhilesh.k, hakbong5.lee, Vaneet Narang
On Wed, 2017-03-29 at 12:29 +0530, Maninder Singh wrote:
> This patch adds C11 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.
>
> _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 is better, but it is still incomplete:
We want to document concurrent code in sufficient detail so that other
developers can understand the synchronization scheme without having to
review all potentially interacting code first. (I'm aware that this
isn't the case in several parts of glibc code, but we're trying to
improve that.) This means documenting, in comments in the code, the
synchronization scheme that is intended. In this particular case, this
is this libname_list:
* What concurrent operations can run on it? Apparently, search in
_dl_name_match_p can be concurrent with insertion. Can insertion be
concurrent with other insertion or not (if so, why)?
* What are the important happens-before relationships we need to ensure?
(For example, in this case, we need to ensure that if we see a list
element in search, then all stores to the fields of list element happen
before the search operation looks at the fields.)
* What release operations and acquire operations are supposed to
synchronize with each other. In your patch, it's these two. If we
don't document this, it will be much harder for other developers to
figure out what the necessary pairings are. You analyzed that already,
so tell others about it.
Documenting the code in this way is a good test to see if you have
really analyzed the synchronization problem completely. (If one can't
clearly explain how something is supposed to work, perhaps one doesn't
yet fully understand it.)
As noted on the Concurrency wiki page, we don't use atomic types, but
use the atomic_* functions consistently as if they'd be required for
atomic types. That is, the next field would be an atomic type,
conceptually, and you should use atomic_load_relaxed for accesses to the
next field in the list search of the insertion operation, for example.
The only exception we make is for initializing stores.
> 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>
> ---
> v1 -> v2 use C11 atomics rather than direct memory barriers
>
> elf/dl-load.c | 2 +-
> elf/dl-misc.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index a5318f9..52df931 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -418,7 +418,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;
> - lastp->next = newname;
> + atomic_store_release (&(lastp->next), newname);
You can drop the inner parentheses.
Space between name of the function that you call and its arguments (see
the memcpy above).
> }
>
> /* Standard search directories. */
> diff --git a/elf/dl-misc.c b/elf/dl-misc.c
> index 1e9a6ee..8f48751 100644
> --- a/elf/dl-misc.c
> +++ b/elf/dl-misc.c
> @@ -295,7 +295,7 @@ _dl_name_match_p (const char *name, const struct link_map *map)
> if (strcmp (name, runp->name) == 0)
> return 1;
> else
> - runp = runp->next;
> + runp = atomic_load_acquire(&(runp->next));
Likewise.
>
> return 0;
> }
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] dl-load: add memory barrier before updating the next
2017-03-29 7:00 ` [PATCH v2] dl-load: add memory barrier before updating the next Maninder Singh
2017-03-29 8:58 ` Torvald Riegel
@ 2017-03-30 9:58 ` Szabolcs Nagy
1 sibling, 0 replies; 3+ messages in thread
From: Szabolcs Nagy @ 2017-03-30 9:58 UTC (permalink / raw)
To: Maninder Singh, libc-alpha, triegel
Cc: nd, pankaj.m, ajeet.y, a.sahrawat, lalit.mohan, akhilesh.k,
hakbong5.lee, Vaneet Narang
On 29/03/17 07:59, Maninder Singh wrote:
> This patch adds C11 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.
>
> _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>
> ---
> v1 -> v2 use C11 atomics rather than direct memory barriers
>
> elf/dl-load.c | 2 +-
> elf/dl-misc.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
please report this bug in bugzilla and add the
bug number reference to the changelog according to
https://sourceware.org/glibc/wiki/Contribution%20checklist#Properly_Formatted_GNU_ChangeLog
^ permalink raw reply [flat|nested] 3+ messages in thread