Reinitialize dl_load_write_lock on fork (bug 19282)

  * sysdeps/nptl/fork.c (__libc_fork): Reinitialize
	GL(dl_load_write_lock).
---
 sysdeps/nptl/fork.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
  

On 10/05/2017 05:17 PM, Andreas Schwab wrote:
> -      /* Reset the lock the dynamic loader uses to protect its data.  */
> +      /* Reset the locks the dynamic loader uses to protect its data.  */
>         __rtld_lock_initialize (GL(dl_load_lock));
> +      __rtld_lock_initialize (GL(dl_load_write_lock));

I'm pretty sure that the child now sees corrupted a dynamic linker state 
instead of hanging.  If not, this needs a comment why this does not happen.

Thanks,
Florian
  

On Okt 05 2017, Florian Weimer <fweimer@redhat.com> wrote:

> On 10/05/2017 05:17 PM, Andreas Schwab wrote:
>> -      /* Reset the lock the dynamic loader uses to protect its data.  */
>> +      /* Reset the locks the dynamic loader uses to protect its data.  */
>>         __rtld_lock_initialize (GL(dl_load_lock));
>> +      __rtld_lock_initialize (GL(dl_load_write_lock));
>
> I'm pretty sure that the child now sees corrupted a dynamic linker state
> instead of hanging.

That's a good point, though this problem already existed before the
introduction of dl_load_write_lock.

Andreas.
  

On 10/05/2017 05:39 PM, Andreas Schwab wrote:
> On Okt 05 2017, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> On 10/05/2017 05:17 PM, Andreas Schwab wrote:
>>> -      /* Reset the lock the dynamic loader uses to protect its data.  */
>>> +      /* Reset the locks the dynamic loader uses to protect its data.  */
>>>          __rtld_lock_initialize (GL(dl_load_lock));
>>> +      __rtld_lock_initialize (GL(dl_load_write_lock));
>>
>> I'm pretty sure that the child now sees corrupted a dynamic linker state
>> instead of hanging.
> 
> That's a good point, though this problem already existed before the
> introduction of dl_load_write_lock.

Why does this make a difference?

Thanks,
Florian
  

On Okt 05 2017, Florian Weimer <fweimer@redhat.com> wrote:

> On 10/05/2017 05:39 PM, Andreas Schwab wrote:
>> On Okt 05 2017, Florian Weimer <fweimer@redhat.com> wrote:
>>
>>> On 10/05/2017 05:17 PM, Andreas Schwab wrote:
>>>> -      /* Reset the lock the dynamic loader uses to protect its data.  */
>>>> +      /* Reset the locks the dynamic loader uses to protect its data.  */
>>>>          __rtld_lock_initialize (GL(dl_load_lock));
>>>> +      __rtld_lock_initialize (GL(dl_load_write_lock));
>>>
>>> I'm pretty sure that the child now sees corrupted a dynamic linker state
>>> instead of hanging.
>>
>> That's a good point, though this problem already existed before the
>> introduction of dl_load_write_lock.
>
> Why does this make a difference?

Difference to what?

Andreas.
  

On 10/05/2017 05:50 PM, Andreas Schwab wrote:
> On Okt 05 2017, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> On 10/05/2017 05:39 PM, Andreas Schwab wrote:
>>> On Okt 05 2017, Florian Weimer <fweimer@redhat.com> wrote:
>>>
>>>> On 10/05/2017 05:17 PM, Andreas Schwab wrote:
>>>>> -      /* Reset the lock the dynamic loader uses to protect its data.  */
>>>>> +      /* Reset the locks the dynamic loader uses to protect its data.  */
>>>>>           __rtld_lock_initialize (GL(dl_load_lock));
>>>>> +      __rtld_lock_initialize (GL(dl_load_write_lock));
>>>>
>>>> I'm pretty sure that the child now sees corrupted a dynamic linker state
>>>> instead of hanging.
>>>
>>> That's a good point, though this problem already existed before the
>>> introduction of dl_load_write_lock.
>>
>> Why does this make a difference?
> 
> Difference to what?

I assumed you were arguing the patch was somehow okay because we used to 
have the corruption bug.  If not and you are retracting your patch, 
that's fine, too.

Thanks,
Florian
  

On Okt 05 2017, Florian Weimer <fweimer@redhat.com> wrote:

> I assumed you were arguing the patch was somehow okay because we used to
> have the corruption bug.

I was just stating a fact.  Both problems are bad enough.

Andreas.
  

On 10/05/2017 09:02 AM, Andreas Schwab wrote:
> On Okt 05 2017, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> I assumed you were arguing the patch was somehow okay because we used to
>> have the corruption bug.
> 
> I was just stating a fact.  Both problems are bad enough.

Either the user coordinated the threads to avoid the situation of having
a thread in dlopen while another thread calls fork, *or* it did not avoid
this problem and you end up with a deadlock.

Either way there is no reason to apply your patch? Why do you want to clear
the lock on fork? Just so you can access potentially corrupt data?
  

So this bug is actually INVALID but for a different reason: in a
multi-threaded program the forked child may only call async-signal-safe
functions.

Andreas.
  

On 10/09/2017 09:58 AM, Andreas Schwab wrote:
> So this bug is actually INVALID but for a different reason: in a
> multi-threaded program the forked child may only call async-signal-safe
> functions.

I think we really need to support dlopen-after-fork in multi-threaded 
processes as an extension, in the same we way need to support 
malloc-after-fork.  I expect that there are quite a few programs which 
do this.

Florian
  

On 10/09/2017 01:06 AM, Florian Weimer wrote:
> On 10/09/2017 09:58 AM, Andreas Schwab wrote:
>> So this bug is actually INVALID but for a different reason: in a 
>> multi-threaded program the forked child may only call
>> async-signal-safe functions.
> 
> I think we really need to support dlopen-after-fork in multi-threaded
> processes as an extension, in the same we way need to support
> malloc-after-fork.  I expect that there are quite a few programs
> which do this.

I agree with your sentiment, some of this needs to be supported, but
it will also need a new safety classification in the manual to describe
which interfaces are safe. Until we do this work, I agree with Andreas
that these uses are in theory INVALID. The same issues apply to SR-safety
which we discussed a year or two ago i.e. when is it safe for a function
to re-enter libc e.g. dlopen->malloc->dlopen (happens mostly with interposed
malloc).
  

On Mon, Oct 9, 2017 at 11:25 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>
> I agree with your sentiment, some of this needs to be supported, but
> it will also need a new safety classification in the manual to describe
> which interfaces are safe. Until we do this work, I agree with Andreas
> that these uses are in theory INVALID. The same issues apply to SR-safety
> which we discussed a year or two ago i.e. when is it safe for a function
> to re-enter libc e.g. dlopen->malloc->dlopen (happens mostly with interposed
> malloc).

Every time this stuff comes up I wonder whether it would be feasible
to reduce the scope of the locks held by dlopen, especially to avoid
holding locks while user-controlled code is running.

zw
  

On 10/09/2017 10:39 AM, Zack Weinberg wrote:
> On Mon, Oct 9, 2017 at 11:25 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>>
>> I agree with your sentiment, some of this needs to be supported, but
>> it will also need a new safety classification in the manual to describe
>> which interfaces are safe. Until we do this work, I agree with Andreas
>> that these uses are in theory INVALID. The same issues apply to SR-safety
>> which we discussed a year or two ago i.e. when is it safe for a function
>> to re-enter libc e.g. dlopen->malloc->dlopen (happens mostly with interposed
>> malloc).
> 
> Every time this stuff comes up I wonder whether it would be feasible
> to reduce the scope of the locks held by dlopen, especially to avoid
> holding locks while user-controlled code is running.

It is feasible, and it's exactly what we want to do IMO. I keep thinking
we want something like RCU in the dynamic loader, because that might allow
us to drop locks when calling foreign functions. The slow case is dlopen,
as it should be, though we could make startup just as fast as we are now,
in the single-threaded case.

We've been circling this drain for a while, and the only impediment
is fixing all the other broken stuff that is slightly higher priority.
As I may have mentioned already, I have plans to work on more dynamic
loader internals to try solve the TLS access problems we have. That won't
be until early next year though. So anyone else who wants to look at it is
more than welcome.