Re: [OE-core] [PATCH] glibc: add ld.so locks in _libc_fork

2017-09-01 Thread Burton, Ross 
Can you rebase this to current master, which has glibc 2.26?

Ross

On 11 August 2017 at 04:16, Zhixiong Chi  wrote:

> The patch in this Bugzilla entry was requested by a customer:
>   https://sourceware.org/bugzilla/show_bug.cgi?id=4578
>   https://www.sourceware.org/bugzilla/show_bug.cgi?id=19282
>
> If a thread happens to hold dl_load_lock and have r_state set to RT_ADD or
> RT_DELETE at the time another thread calls fork(), then the child exit code
> from fork (in nptl/sysdeps/unix/sysv/linux/fork.c in our case)
> re-initializes
> dl_load_lock but does not restore r_state to RT_CONSISTENT. If the child
> subsequently requires ld.so functionality before calling exec(), then the
> assertion will fire.
>
> The patch acquires dl_load_lock on entry to fork() and releases it on exit
> from the parent path.  The child path is initialized as currently done.
> This is essentially pthreads_atfork, but forced to be first because the
> acquisition of dl_load_lock must happen before malloc_atfork is active
> to avoid a deadlock.
>
> The __libc_fork() code reset dl_load_lock, but it also needed to reset
> dl_load_write_lock.
>
> Signed-off-by: Zhixiong Chi 
> ---
>  .../0001-Bug-4578-add-ld.so-lock-while-fork.patch  | 57
> ++
>  ...bc-reset-dl-load-write-lock-after-forking.patch | 37 ++
>  meta/recipes-core/glibc/glibc_2.25.90.bb   |  2 +
>  3 files changed, 96 insertions(+)
>  create mode 100644 meta/recipes-core/glibc/glibc/
> 0001-Bug-4578-add-ld.so-lock-while-fork.patch
>  create mode 100644 meta/recipes-core/glibc/glibc/
> 0001-glibc-reset-dl-load-write-lock-after-forking.patch
>
> diff --git 
> a/meta/recipes-core/glibc/glibc/0001-Bug-4578-add-ld.so-lock-while-fork.patch
> b/meta/recipes-core/glibc/glibc/0001-Bug-4578-add-ld.so-
> lock-while-fork.patch
> new file mode 100644
> index 000..ab82866
> --- /dev/null
> +++ b/meta/recipes-core/glibc/glibc/0001-Bug-4578-add-ld.so-
> lock-while-fork.patch
> @@ -0,0 +1,57 @@
> +The patch in this Bugzilla entry was requested by a customer:
> +  https://sourceware.org/bugzilla/show_bug.cgi?id=4578
> +
> +If a thread happens to hold dl_load_lock and have r_state set to RT_ADD or
> +RT_DELETE at the time another thread calls fork(), then the child exit
> code
> +from fork (in nptl/sysdeps/unix/sysv/linux/fork.c in our case)
> re-initializes
> +dl_load_lock but does not restore r_state to RT_CONSISTENT. If the child
> +subsequently requires ld.so functionality before calling exec(), then the
> +assertion will fire.
> +
> +The patch acquires dl_load_lock on entry to fork() and releases it on exit
> +from the parent path.  The child path is initialized as currently done.
> +This is essentially pthreads_atfork, but forced to be first because the
> +acquisition of dl_load_lock must happen before malloc_atfork is active
> +to avoid a deadlock.
> +The patch has not yet been integrated upstream.
> +
> +Upstream-Status: Pending [ Not Author See bugzilla]
> +
> +Signed-off-by: Raghunath Lolur 
> +Signed-off-by: Yuanjie Huang 
> +Signed-off-by: Zhixiong Chi 
> +
> +Index: git/sysdeps/nptl/fork.c
> +===
> +--- git.orig/sysdeps/nptl/fork.c   2017-08-03 16:02:15.674704080
> +0800
>  git/sysdeps/nptl/fork.c2017-08-04 18:15:02.463362015 +0800
> +@@ -25,6 +25,7 @@
> + #include 
> + #include 
> + #include 
> ++#include 
> + #include 
> + #include 
> + #include 
> +@@ -60,6 +61,10 @@
> +  but our current fork implementation is not.  */
> +   bool multiple_threads = THREAD_GETMEM (THREAD_SELF,
> header.multiple_threads);
> +
> ++  /* grab ld.so lock BEFORE switching to malloc_atfork */
> ++  __rtld_lock_lock_recursive (GL(dl_load_lock));
> ++  __rtld_lock_lock_recursive (GL(dl_load_write_lock));
> ++
> +   /* Run all the registered preparation handlers.  In reverse order.
> +  While doing this we build up a list of all the entries.  */
> +   struct fork_handler *runp;
> +@@ -258,6 +263,10 @@
> +
> + allp = allp->next;
> +   }
> ++
> ++  /* unlock ld.so last, because we locked it first */
> ++  __rtld_lock_unlock_recursive (GL(dl_load_write_lock));
> ++  __rtld_lock_unlock_recursive (GL(dl_load_lock));
> + }
> +
> +   return pid;
> diff --git a/meta/recipes-core/glibc/glibc/0001-glibc-reset-dl-
> load-write-lock-after-forking.patch b/meta/recipes-core/glibc/
> glibc/0001-glibc-reset-dl-load-write-lock-after-forking.patch
> new file mode 100644
> index 000..777b253
> --- /dev/null
> +++ b/meta/recipes-core/glibc/glibc/0001-glibc-reset-dl-
> load-write-lock-after-forking.patch
> @@ -0,0 +1,37 @@
> +From a6bb73d1cfd20a73fbbe6076008376fb87879d1b Mon Sep 17 00:00:00 2001
> +From: Yuanjie Huang 
> +Date: Thu, 18 Aug 2016 17:59:13 +0800
> +Subject: [PATCH] reset

Re: [OE-core] [PATCH] glibc: add ld.so locks in _libc_fork

2017-08-13 Thread Khem Raj 
On Sun, Aug 13, 2017 at 8:04 PM, Zhixiong Chi
 wrote:
>
>
> On 2017年08月14日 00:35, Khem Raj wrote:
>>
>> On Sun, Aug 13, 2017 at 1:36 AM, Richard Purdie
>>  wrote:
>>>
>>> On Fri, 2017-08-11 at 11:16 +0800, Zhixiong Chi wrote:

 The patch in this Bugzilla entry was requested by a customer:
https://sourceware.org/bugzilla/show_bug.cgi?id=4578
https://www.sourceware.org/bugzilla/show_bug.cgi?id=19282
>>>
>>> I'm a little nervous about accepting a patch which has been sitting in
>>> the glibc bugzilla for around 10 years. Any idea why upstream haven't
>>> taken this?
>>>
>> patches look sane to me. I would like to see if we can reproduce the
>> issue on x86 using the
>> testcases from bugzilla
>
>
> I don't know why the upstream don't take care of it. I guess the reason
> may be that the testcases is out of scope and we usually don't use the
> unasync-signal-safe function after the forking.
>
> Yes, the testcases from bugzilla can be reproduced easily on every
> qemu bsp. At the same time we have done the glibc regression testing,
> The patches works well.

Please test it on hardware as well. I think the patch is safe. I will bring this
up in glibc community.

>
> Thanks.
>
>>> Cheers,
>>>
>>> Richard
>>> --
>>> ___
>>> Openembedded-core mailing list
>>> Openembedded-core@lists.openembedded.org
>>> http://lists.openembedded.org/mailman/listinfo/openembedded-core
>
>
> --
> -
> Thanks,
> Zhixiong Chi
> Tel: +86-10-8477-7036
>
-- 
___
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-core

Re: [OE-core] [PATCH] glibc: add ld.so locks in _libc_fork

2017-08-13 Thread Zhixiong Chi 



On 2017年08月14日 00:35, Khem Raj wrote:

On Sun, Aug 13, 2017 at 1:36 AM, Richard Purdie
 wrote:

On Fri, 2017-08-11 at 11:16 +0800, Zhixiong Chi wrote:

The patch in this Bugzilla entry was requested by a customer:
   https://sourceware.org/bugzilla/show_bug.cgi?id=4578
   https://www.sourceware.org/bugzilla/show_bug.cgi?id=19282

I'm a little nervous about accepting a patch which has been sitting in
the glibc bugzilla for around 10 years. Any idea why upstream haven't
taken this?


patches look sane to me. I would like to see if we can reproduce the
issue on x86 using the
testcases from bugzilla


I don't know why the upstream don't take care of it. I guess the reason
may be that the testcases is out of scope and we usually don't use the
unasync-signal-safe function after the forking.

Yes, the testcases from bugzilla can be reproduced easily on every
qemu bsp. At the same time we have done the glibc regression testing,
The patches works well.

Thanks.

Cheers,

Richard
--
___
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-core


--
-
Thanks,
Zhixiong Chi
Tel: +86-10-8477-7036

--
___
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-core

Re: [OE-core] [PATCH] glibc: add ld.so locks in _libc_fork

2017-08-13 Thread Khem Raj 
On Sun, Aug 13, 2017 at 1:36 AM, Richard Purdie
 wrote:
> On Fri, 2017-08-11 at 11:16 +0800, Zhixiong Chi wrote:
>> The patch in this Bugzilla entry was requested by a customer:
>>   https://sourceware.org/bugzilla/show_bug.cgi?id=4578
>>   https://www.sourceware.org/bugzilla/show_bug.cgi?id=19282
>
> I'm a little nervous about accepting a patch which has been sitting in
> the glibc bugzilla for around 10 years. Any idea why upstream haven't
> taken this?
>

patches look sane to me. I would like to see if we can reproduce the
issue on x86 using the
testcases from bugzilla

> Cheers,
>
> Richard
> --
> ___
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core
-- 
___
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-core

Re: [OE-core] [PATCH] glibc: add ld.so locks in _libc_fork

2017-08-13 Thread Richard Purdie 
On Fri, 2017-08-11 at 11:16 +0800, Zhixiong Chi wrote:
> The patch in this Bugzilla entry was requested by a customer:
>   https://sourceware.org/bugzilla/show_bug.cgi?id=4578
>   https://www.sourceware.org/bugzilla/show_bug.cgi?id=19282

I'm a little nervous about accepting a patch which has been sitting in
the glibc bugzilla for around 10 years. Any idea why upstream haven't
taken this?

Cheers,

Richard
-- 
___
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-core

[OE-core] [PATCH] glibc: add ld.so locks in _libc_fork

2017-08-10 Thread Zhixiong Chi 
The patch in this Bugzilla entry was requested by a customer:
  https://sourceware.org/bugzilla/show_bug.cgi?id=4578
  https://www.sourceware.org/bugzilla/show_bug.cgi?id=19282

If a thread happens to hold dl_load_lock and have r_state set to RT_ADD or
RT_DELETE at the time another thread calls fork(), then the child exit code
from fork (in nptl/sysdeps/unix/sysv/linux/fork.c in our case) re-initializes
dl_load_lock but does not restore r_state to RT_CONSISTENT. If the child
subsequently requires ld.so functionality before calling exec(), then the
assertion will fire.

The patch acquires dl_load_lock on entry to fork() and releases it on exit
from the parent path.  The child path is initialized as currently done.
This is essentially pthreads_atfork, but forced to be first because the
acquisition of dl_load_lock must happen before malloc_atfork is active
to avoid a deadlock.

The __libc_fork() code reset dl_load_lock, but it also needed to reset
dl_load_write_lock.

Signed-off-by: Zhixiong Chi 
---
 .../0001-Bug-4578-add-ld.so-lock-while-fork.patch  | 57 ++
 ...bc-reset-dl-load-write-lock-after-forking.patch | 37 ++
 meta/recipes-core/glibc/glibc_2.25.90.bb   |  2 +
 3 files changed, 96 insertions(+)
 create mode 100644 
meta/recipes-core/glibc/glibc/0001-Bug-4578-add-ld.so-lock-while-fork.patch
 create mode 100644 
meta/recipes-core/glibc/glibc/0001-glibc-reset-dl-load-write-lock-after-forking.patch

diff --git 
a/meta/recipes-core/glibc/glibc/0001-Bug-4578-add-ld.so-lock-while-fork.patch 
b/meta/recipes-core/glibc/glibc/0001-Bug-4578-add-ld.so-lock-while-fork.patch
new file mode 100644
index 000..ab82866
--- /dev/null
+++ 
b/meta/recipes-core/glibc/glibc/0001-Bug-4578-add-ld.so-lock-while-fork.patch
@@ -0,0 +1,57 @@
+The patch in this Bugzilla entry was requested by a customer:
+  https://sourceware.org/bugzilla/show_bug.cgi?id=4578
+
+If a thread happens to hold dl_load_lock and have r_state set to RT_ADD or
+RT_DELETE at the time another thread calls fork(), then the child exit code
+from fork (in nptl/sysdeps/unix/sysv/linux/fork.c in our case) re-initializes
+dl_load_lock but does not restore r_state to RT_CONSISTENT. If the child
+subsequently requires ld.so functionality before calling exec(), then the
+assertion will fire.
+
+The patch acquires dl_load_lock on entry to fork() and releases it on exit
+from the parent path.  The child path is initialized as currently done.
+This is essentially pthreads_atfork, but forced to be first because the
+acquisition of dl_load_lock must happen before malloc_atfork is active
+to avoid a deadlock.
+The patch has not yet been integrated upstream.
+
+Upstream-Status: Pending [ Not Author See bugzilla]
+
+Signed-off-by: Raghunath Lolur 
+Signed-off-by: Yuanjie Huang 
+Signed-off-by: Zhixiong Chi 
+
+Index: git/sysdeps/nptl/fork.c
+===
+--- git.orig/sysdeps/nptl/fork.c   2017-08-03 16:02:15.674704080 +0800
 git/sysdeps/nptl/fork.c2017-08-04 18:15:02.463362015 +0800
+@@ -25,6 +25,7 @@
+ #include 
+ #include 
+ #include 
++#include 
+ #include 
+ #include 
+ #include 
+@@ -60,6 +61,10 @@
+  but our current fork implementation is not.  */
+   bool multiple_threads = THREAD_GETMEM (THREAD_SELF, 
header.multiple_threads);
+ 
++  /* grab ld.so lock BEFORE switching to malloc_atfork */
++  __rtld_lock_lock_recursive (GL(dl_load_lock));
++  __rtld_lock_lock_recursive (GL(dl_load_write_lock));
++
+   /* Run all the registered preparation handlers.  In reverse order.
+  While doing this we build up a list of all the entries.  */
+   struct fork_handler *runp;
+@@ -258,6 +263,10 @@
+ 
+ allp = allp->next;
+   }
++
++  /* unlock ld.so last, because we locked it first */
++  __rtld_lock_unlock_recursive (GL(dl_load_write_lock));
++  __rtld_lock_unlock_recursive (GL(dl_load_lock));
+ }
+ 
+   return pid;
diff --git 
a/meta/recipes-core/glibc/glibc/0001-glibc-reset-dl-load-write-lock-after-forking.patch
 
b/meta/recipes-core/glibc/glibc/0001-glibc-reset-dl-load-write-lock-after-forking.patch
new file mode 100644
index 000..777b253
--- /dev/null
+++ 
b/meta/recipes-core/glibc/glibc/0001-glibc-reset-dl-load-write-lock-after-forking.patch
@@ -0,0 +1,37 @@
+From a6bb73d1cfd20a73fbbe6076008376fb87879d1b Mon Sep 17 00:00:00 2001
+From: Yuanjie Huang 
+Date: Thu, 18 Aug 2016 17:59:13 +0800
+Subject: [PATCH] reset dl_load_write_lock after forking
+
+The patch in this Bugzilla entry was requested by a customer:
+
+  https://www.sourceware.org/bugzilla/show_bug.cgi?id=19282
+
+The __libc_fork() code reset dl_load_lock, but it also needed to reset
+dl_load_write_lock.  The patch has not yet been integrated upstream.
+
+Upstream-Status: Pending [ Not Author See bugzilla]
+
+Signed-off-by: Damodar