Re: rseq notes from Cauldron

[Mathieu Desnoyers <mathieu dot desnoyers at efficios dot com>]

----- On Dec 20, 2019, at 2:31 PM, Florian Weimer fw@deneb.enyo.de wrote:

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

Actually, it only applies to the struct rseq_cs per se. The kernel
does not need to access the text region described by struct rseq_cs.
It only uses the start_ip and post_commit_offset fields to figure out
if it is indeed preempting a rseq critical section. It does not need
to access the memory contents at those addresses. So I don't think
any change to the UAPI comment is needed.

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

None of the current membarrier commands would be sufficient for this
unfortunately. But perhaps some extension of membarrier could eventually
do the trick.

> But I think we should simply require that userspace
> clears rseq_cs.  In that case, no dlclose change is needed.

Allright, so I won't change anything in dlclose.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com