Re: [PATCH] nptl: Fix deadlock on atfork handler which calls dlclose (BZ#24595)

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella:

> On 24/05/2019 08:06, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> On 23/05/2019 16:50, Florian Weimer wrote:
>>>> * Adhemerval Zanella:
>>>>
>>>>> The solution sounds correct, but I don't have a strong opinion if this
>>>>> is really an improvement over a recursive lock plus a linked list.  It 
>>>>> potentially adds 'free' calls in fork for multithread mode if list needs 
>>>>> to be deallocated. Also, since the locks is internal to register-atfork.c
>>>>> we might have a better control to make the exported interfaces not 
>>>>> deadlock.
>>>>
>>>> Recursive locks do not prevent deadlocks if the callback launches
>>>> threads.  I'm not sure if this can be considered a stretch.  If
>>>> there's dlclose involved, their might also be dlopen, and threads
>>>> launched from an ELF constructor and so on.
>>>
>>> That's why I said the core issue is atfork handlers are really
>>> underspecified, and maybe it would be better if we document more
>>> thoughtfully what we really aim to support by atfork handlers.
>> 
>> That's also true.
>> 
>>>> Furthermore, I'm not sure if the implementation of __unregister_atfork
>>>> is correct.  I think you have a potential use-after-free issue if the
>>>> callback deregisters a fork handler.
>> 
>>> The possible way I can think of is if an atfork handler from a module
>>> try to dlclose itself and I am not sure how valid it is.
>> 
>> dlclose'ing itself is not really valid, I think.  It can only work if
>> the dlclose call is a tail call.
>
> I would work towards not make dlclose itself valid, do you see any
> reason to allow to do so?

Sorry, what do you mean?  Inspect the caller address and reject the
dlclose call if it's within the DSO being closed?  This just avoids a
crash, instead resulting in a dlclose error return.  Not sure if this is
particularly relevant.

>> __unregister_atfork still adds a use-after-free issue, even with the
>> copy-on-write list: If a dlclose unregisters a fork handler, we will
>> still attempt to call it subsequently if it was on the list before.  No
>> data race is involved if the dlclose call happens itself in a fork
>> handler (so we are back to the territory of the bug).
>> 
>> So the simple reference counting scheme I proposed is probably not the
>> right solution, and we need a different data structure
>
> My understanding is if the idea of dlclose on atfork handler is to
> not just unregister the library but also prevent further arfork handler
> to been called (since __cxa_finalize will call __unregister_atfork), 
> I still think that the recursive mutex plus double-linked list is
> the best a suitable solution if we assume two premisses:
>
>   - It is invalid to dlclose itself.
>   - Atfork handlers should be local to modules.
>
> Both should avoid imho the use-after-free issue since the callback
> won't try to deregister an already used entry in atfork handler list
> walking.

I'm not sure about that.  The dlclose could remove a fork handler that
has yet to be called.  The handlers may not have been registered in load
order.

Thanks,
Florian