Re: rseq notes from Cauldron

[Florian Weimer <fw at deneb dot enyo dot de>]

* Mathieu Desnoyers:

> I am currently going through the list of action items for rseq.
>
> One question arises (see below):
>
> ----- On Sep 23, 2019, at 11:34 AM, Mathieu Desnoyers
> mathieu.desnoyers@efficios.com wrote:
>
>> ----- On Sep 23, 2019, at 4:45 AM, Florian Weimer fw@deneb.enyo.de wrote:
>>[...]
>>> dlclose needs to reset the cs pointer before unmapping code.  I think
>>> this should be part of the rseq patch.
>> 
>> Yes, action item on my end.
>
> I have a design issue with this one.
>
> Consider the following scenario where we have 2 threads. One is
> running through a rseq critical section which references a rseq_cs
> located within a library which is about to be dlclosed:
>
> Thread A                                             Thread B
>
> - rseq critical section
> - __rseq_abi->rseq_cs = unmap_lib_addr
> - rseq critical section ends, but leaves the
>   __rseq_abi->rseq_cs pointer to its unmap_lib_addr
>   value, expecting the kernel to lazily clear it.
>                                                      - wait on semaphore
> - post semaphore telling Thread B that it can
>   dlclose the library
>                                                      - __abi_abi->rseq_cs = NULL
>                                                        _only for current TLS_
>                                                      - dlclose library
>                                                        - unmap or reuse memory
> - kernel preempts Thread A, try to access
>   __rseq_abi->rseq_cs which still points to
>   the dlclosed library, which is incorrect.
>
> The issue here is that dlclose should ensure that none of the
> process' threads TLS __rseq_abi->rseq_cs contain pointers to the
> library which is about to be dlclosed.

Right, this is a problem.

> Clearing the field in the TLS of the thread which is about to call
> dlclose is unfortunately not sufficient.

rseq.h already says this:

         * […] Also needs to be set to NULL by user-space
         * before reclaiming memory that contains the targeted struct rseq_cs.

With dlclose, the struct rseq_cs will likely be gone, not just the
code, so I think in practice, it's already necessary to clear rseq_cs
in userspace.  For completeness, the UAPI header should mention that
this applies to the text region described by struct rseq_cs as well.

> One possibility around this would be to add a signal-based
> synchronization with all other threads to ensure that their
> __rseq_abi->rseq_cs field is cleared by the kernel when delivering
> the signal.

I'm not sure we can do that on every dlclose.  Some sort of membarrier
might be enough.  But I think we should simply require that userspace
clears rseq_cs.  In that case, no dlclose change is needed.