Re: [PATCH v4] dl-load: add memory barrier before updating the next

Re: [PATCH v4] dl-load: add memory barrier before updating the next

[Maninder Singh]

Hi Szabolcs,

Thanks for the comment,



>>>[BZ #21349]: race condition between dl_open and rtld lazy symbol resolve.
>>>
>>>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.
>>>
> 
>this patch needs updated comments and description to
>follow the new concurrency documentation requirements.
> 
>the code looks acceptable to me.
> 
>i can try to provide comments that are acceptable and
>resubmit the patch for you if you want me to, or you
>can look at the concurrency wiki and try to come up
>with better documentation.
 
Can you please suggest the coment, I will update that one
and send the new patch, because we have updated the comment as
suggested by you in last mail.

https://sourceware.org/ml/libc-alpha/2017-03/msg00383.html

Or it would be great if you can submit the patch with acceptable comments :) .

We have updated testcase at below BugZilla link
https://sourceware.org/bugzilla/show_bug.cgi?id=21349

 
--------------------
Thanks and Regards,
Maninder Singh

Re: [PATCH v4] dl-load: add memory barrier before updating the next

[Szabolcs Nagy]

On 10/04/17 09:20, Maninder Singh wrote:
> [BZ #21349]: race condition between dl_open and rtld lazy symbol resolve.
> 
> 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.
> 
> so as suggested by Torvald Riegel <triegel@redhat.com>:
> "Add C11 memory barrier before updating the liblist next and add comments
> for barriers."
> 
> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> ---
> v1 -> v2: use C11 atomics rather than direct memory barriers.
> v2 -> v3: use comments for barriers and enter Bugzilla ID.
> v3 -> v4: remove extra parens.
> 

what happened with this patch?
this fixes a known data race.

>  elf/dl-load.c |    5 ++++-
>  elf/dl-misc.c |    5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index a5318f9..03c6afb 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -418,7 +418,10 @@ 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;
> +  /* We need release memory order here because we need to synchronize
> +     with other thread doing _dl_runtime_resolve which calls _dl_name_match_p
> +     to traverse all names added to libname_list */
> +  atomic_store_release (&lastp->next, newname);
>  }
>  
>  /* Standard search directories.  */
> diff --git a/elf/dl-misc.c b/elf/dl-misc.c
> index 1e9a6ee..a26d6f6 100644
> --- a/elf/dl-misc.c
> +++ b/elf/dl-misc.c
> @@ -295,7 +295,10 @@ _dl_name_match_p (const char *name, const struct link_map *map)
>      if (strcmp (name, runp->name) == 0)
>        return 1;
>      else
> -      runp = runp->next;
> +      /* We need to acquire memory order here because we need to synchronize
> +         with other thread calling dlopen and adding new name to libname_list
> +         through add_name_to_object */
> +      runp = atomic_load_acquire (&runp->next);
>  
>    return 0;
>  }
> 

[PATCH v4] dl-load: add memory barrier before updating the next

[Maninder Singh]

[BZ #21349]: race condition between dl_open and rtld lazy symbol resolve.

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.

so as suggested by Torvald Riegel <triegel@redhat.com>:
"Add C11 memory barrier before updating the liblist next and add comments
for barriers."

Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
v1 -> v2: use C11 atomics rather than direct memory barriers.
v2 -> v3: use comments for barriers and enter Bugzilla ID.
v3 -> v4: remove extra parens.

 elf/dl-load.c |    5 ++++-
 elf/dl-misc.c |    5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/elf/dl-load.c b/elf/dl-load.c
index a5318f9..03c6afb 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -418,7 +418,10 @@ 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;
+  /* We need release memory order here because we need to synchronize
+     with other thread doing _dl_runtime_resolve which calls _dl_name_match_p
+     to traverse all names added to libname_list */
+  atomic_store_release (&lastp->next, newname);
 }
 
 /* Standard search directories.  */
diff --git a/elf/dl-misc.c b/elf/dl-misc.c
index 1e9a6ee..a26d6f6 100644
--- a/elf/dl-misc.c
+++ b/elf/dl-misc.c
@@ -295,7 +295,10 @@ _dl_name_match_p (const char *name, const struct link_map *map)
     if (strcmp (name, runp->name) == 0)
       return 1;
     else
-      runp = runp->next;
+      /* We need to acquire memory order here because we need to synchronize
+         with other thread calling dlopen and adding new name to libname_list
+         through add_name_to_object */
+      runp = atomic_load_acquire (&runp->next);
 
   return 0;
 }
-- 
1.7.1