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

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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?

> 
> __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.