[PATCH for 5.5 1/2] rseq: Fix: Clarify rseq.h UAPI rseq_cs memory reclaim requirements

[PATCH for 5.5 1/2] rseq: Fix: Clarify rseq.h UAPI rseq_cs memory reclaim requirements

[Mathieu Desnoyers]

The rseq.h UAPI documents that the rseq_cs field must be cleared
before reclaiming memory that contains the targeted struct rseq_cs.

We should extend this comment to also dictate that the rseq_cs field
must be cleared before reclaiming memory of the code pointed to by
the rseq_cs start_ip and post_commit_offset fields.

While we can expect that use of dlclose(3) will typically unmap
both struct rseq_cs and its associated code at once, nothing would
theoretically prevent a JIT from reclaiming the code without
reclaiming the struct rseq_cs, which would erroneously allow the
kernel to consider new code which is not a rseq critical section
as a rseq critical section following a code reclaim.

Suggested-by: Florian Weimer <fw@deneb.enyo.de>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Florian Weimer <fw@deneb.enyo.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Paul Turner <pjt@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Neel Natu <neelnatu@google.com>
Cc: linux-api@vger.kernel.org
---
 include/uapi/linux/rseq.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
index 9a402fdb60e9..6f26b0b148a6 100644
--- a/include/uapi/linux/rseq.h
+++ b/include/uapi/linux/rseq.h
@@ -100,7 +100,9 @@ struct rseq {
 	 * instruction sequence block, as well as when the kernel detects that
 	 * it is preempting or delivering a signal outside of the range
 	 * targeted by the rseq_cs. Also needs to be set to NULL by user-space
-	 * before reclaiming memory that contains the targeted struct rseq_cs.
+	 * before reclaiming memory that contains the targeted struct rseq_cs
+	 * or reclaiming memory that contains the code refered to by the
+	 * start_ip and post_commit_offset fields of struct rseq_cs.
 	 *
 	 * Read and set by the kernel. Set by user-space with single-copy
 	 * atomicity semantics. This field should only be updated by the
-- 
2.17.1

[PATCH for 5.5 2/2] rseq/selftests: Clarify rseq_prepare_unload() helper requirements

[Mathieu Desnoyers]

The rseq.h UAPI now documents that the rseq_cs field must be cleared
before reclaiming memory that contains the targeted struct rseq_cs, but
also that the rseq_cs field must be cleared before reclaiming memory of
the code pointed to by the rseq_cs start_ip and post_commit_offset
fields.

While we can expect that use of dlclose(3) will typically unmap
both struct rseq_cs and its associated code at once, nothing would
theoretically prevent a JIT from reclaiming the code without
reclaiming the struct rseq_cs, which would erroneously allow the
kernel to consider new code which is not a rseq critical section
as a rseq critical section following a code reclaim.

Suggested-by: Florian Weimer <fw@deneb.enyo.de>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: Florian Weimer <fw@deneb.enyo.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Paul Turner <pjt@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
---
 tools/testing/selftests/rseq/rseq.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/rseq/rseq.h b/tools/testing/selftests/rseq/rseq.h
index d40d60e7499e..15cbd51d0818 100644
--- a/tools/testing/selftests/rseq/rseq.h
+++ b/tools/testing/selftests/rseq/rseq.h
@@ -149,11 +149,13 @@ static inline void rseq_clear_rseq_cs(void)
 /*
  * rseq_prepare_unload() should be invoked by each thread executing a rseq
  * critical section at least once between their last critical section and
- * library unload of the library defining the rseq critical section
- * (struct rseq_cs). This also applies to use of rseq in code generated by
- * JIT: rseq_prepare_unload() should be invoked at least once by each
- * thread executing a rseq critical section before reclaim of the memory
- * holding the struct rseq_cs.
+ * library unload of the library defining the rseq critical section (struct
+ * rseq_cs) or the code refered to by the struct rseq_cs start_ip and
+ * post_commit_offset fields. This also applies to use of rseq in code
+ * generated by JIT: rseq_prepare_unload() should be invoked at least once by
+ * each thread executing a rseq critical section before reclaim of the memory
+ * holding the struct rseq_cs or reclaim of the code pointed to by struct
+ * rseq_cs start_ip and post_commit_offset fields.
  */
 static inline void rseq_prepare_unload(void)
 {
-- 
2.17.1

Re: [PATCH for 5.5 2/2] rseq/selftests: Clarify rseq_prepare_unload() helper requirements

[Shuah Khan]

Hi Mathieu,

On 12/20/19 1:12 PM, Mathieu Desnoyers wrote:
> The rseq.h UAPI now documents that the rseq_cs field must be cleared
> before reclaiming memory that contains the targeted struct rseq_cs, but
> also that the rseq_cs field must be cleared before reclaiming memory of
> the code pointed to by the rseq_cs start_ip and post_commit_offset
> fields.
> 
> While we can expect that use of dlclose(3) will typically unmap
> both struct rseq_cs and its associated code at once, nothing would
> theoretically prevent a JIT from reclaiming the code without
> reclaiming the struct rseq_cs, which would erroneously allow the
> kernel to consider new code which is not a rseq critical section
> as a rseq critical section following a code reclaim.
> 
> Suggested-by: Florian Weimer <fw@deneb.enyo.de>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Shuah Khan <skhan@linuxfoundation.org>
> Cc: Florian Weimer <fw@deneb.enyo.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: "H . Peter Anvin" <hpa@zytor.com>
> Cc: Paul Turner <pjt@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> ---
>   tools/testing/selftests/rseq/rseq.h | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/rseq/rseq.h b/tools/testing/selftests/rseq/rseq.h
> index d40d60e7499e..15cbd51d0818 100644
> --- a/tools/testing/selftests/rseq/rseq.h
> +++ b/tools/testing/selftests/rseq/rseq.h
> @@ -149,11 +149,13 @@ static inline void rseq_clear_rseq_cs(void)
>   /*
>    * rseq_prepare_unload() should be invoked by each thread executing a rseq
>    * critical section at least once between their last critical section and
> - * library unload of the library defining the rseq critical section
> - * (struct rseq_cs). This also applies to use of rseq in code generated by
> - * JIT: rseq_prepare_unload() should be invoked at least once by each
> - * thread executing a rseq critical section before reclaim of the memory
> - * holding the struct rseq_cs.
> + * library unload of the library defining the rseq critical section (struct
> + * rseq_cs) or the code refered to by the struct rseq_cs start_ip and

Nit: referred instead of refered

> + * post_commit_offset fields. This also applies to use of rseq in code
> + * generated by JIT: rseq_prepare_unload() should be invoked at least once by
> + * each thread executing a rseq critical section before reclaim of the memory
> + * holding the struct rseq_cs or reclaim of the code pointed to by struct
> + * rseq_cs start_ip and post_commit_offset fields.
>    */
>   static inline void rseq_prepare_unload(void)
>   {
> 

thanks,
-- Shuah

Re: [PATCH for 5.5 2/2] rseq/selftests: Clarify rseq_prepare_unload() helper requirements

[Mathieu Desnoyers]

----- On Dec 20, 2019, at 3:27 PM, Shuah Khan skhan@linuxfoundation.org wrote:

> Hi Mathieu,
> 
> On 12/20/19 1:12 PM, Mathieu Desnoyers wrote:
>> The rseq.h UAPI now documents that the rseq_cs field must be cleared
>> before reclaiming memory that contains the targeted struct rseq_cs, but
>> also that the rseq_cs field must be cleared before reclaiming memory of
>> the code pointed to by the rseq_cs start_ip and post_commit_offset
>> fields.
>> 
>> While we can expect that use of dlclose(3) will typically unmap
>> both struct rseq_cs and its associated code at once, nothing would
>> theoretically prevent a JIT from reclaiming the code without
>> reclaiming the struct rseq_cs, which would erroneously allow the
>> kernel to consider new code which is not a rseq critical section
>> as a rseq critical section following a code reclaim.
>> 
>> Suggested-by: Florian Weimer <fw@deneb.enyo.de>
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> Cc: Shuah Khan <skhan@linuxfoundation.org>
>> Cc: Florian Weimer <fw@deneb.enyo.de>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
>> Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
>> Cc: Boqun Feng <boqun.feng@gmail.com>
>> Cc: "H . Peter Anvin" <hpa@zytor.com>
>> Cc: Paul Turner <pjt@google.com>
>> Cc: Dmitry Vyukov <dvyukov@google.com>
>> ---
>>   tools/testing/selftests/rseq/rseq.h | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/rseq/rseq.h
>> b/tools/testing/selftests/rseq/rseq.h
>> index d40d60e7499e..15cbd51d0818 100644
>> --- a/tools/testing/selftests/rseq/rseq.h
>> +++ b/tools/testing/selftests/rseq/rseq.h
>> @@ -149,11 +149,13 @@ static inline void rseq_clear_rseq_cs(void)
>>   /*
>>    * rseq_prepare_unload() should be invoked by each thread executing a rseq
>>    * critical section at least once between their last critical section and
>> - * library unload of the library defining the rseq critical section
>> - * (struct rseq_cs). This also applies to use of rseq in code generated by
>> - * JIT: rseq_prepare_unload() should be invoked at least once by each
>> - * thread executing a rseq critical section before reclaim of the memory
>> - * holding the struct rseq_cs.
>> + * library unload of the library defining the rseq critical section (struct
>> + * rseq_cs) or the code refered to by the struct rseq_cs start_ip and
> 
> Nit: referred instead of refered

Good catch. I've done the same error in patch 1/2. I'll update both and
resend.

Thanks!

Mathieu

> 
>> + * post_commit_offset fields. This also applies to use of rseq in code
>> + * generated by JIT: rseq_prepare_unload() should be invoked at least once by
>> + * each thread executing a rseq critical section before reclaim of the memory
>> + * holding the struct rseq_cs or reclaim of the code pointed to by struct
>> + * rseq_cs start_ip and post_commit_offset fields.
>>    */
>>   static inline void rseq_prepare_unload(void)
>>   {
>> 
> 
> thanks,
> -- Shuah

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

Re: [PATCH for 5.5 1/2] rseq: Fix: Clarify rseq.h UAPI rseq_cs memory reclaim requirements

[Florian Weimer]

* Mathieu Desnoyers:

> diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
> index 9a402fdb60e9..6f26b0b148a6 100644
> --- a/include/uapi/linux/rseq.h
> +++ b/include/uapi/linux/rseq.h
> @@ -100,7 +100,9 @@ struct rseq {
>  	 * instruction sequence block, as well as when the kernel detects that
>  	 * it is preempting or delivering a signal outside of the range
>  	 * targeted by the rseq_cs. Also needs to be set to NULL by user-space
> -	 * before reclaiming memory that contains the targeted struct rseq_cs.
> +	 * before reclaiming memory that contains the targeted struct rseq_cs
> +	 * or reclaiming memory that contains the code refered to by the
> +	 * start_ip and post_commit_offset fields of struct rseq_cs.

Maybe mention that it's good practice to clear rseq_cs before
returning from a function that contains a restartable sequence?

That will deal with the dlclose issue because even if the function
calls dlclose itself, unmapping something on call stack for dlclose is
already undefined.

Re: [PATCH for 5.5 1/2] rseq: Fix: Clarify rseq.h UAPI rseq_cs memory reclaim requirements

[Mathieu Desnoyers]

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

> * Mathieu Desnoyers:
> 
>> diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
>> index 9a402fdb60e9..6f26b0b148a6 100644
>> --- a/include/uapi/linux/rseq.h
>> +++ b/include/uapi/linux/rseq.h
>> @@ -100,7 +100,9 @@ struct rseq {
>>  	 * instruction sequence block, as well as when the kernel detects that
>>  	 * it is preempting or delivering a signal outside of the range
>>  	 * targeted by the rseq_cs. Also needs to be set to NULL by user-space
>> -	 * before reclaiming memory that contains the targeted struct rseq_cs.
>> +	 * before reclaiming memory that contains the targeted struct rseq_cs
>> +	 * or reclaiming memory that contains the code refered to by the
>> +	 * start_ip and post_commit_offset fields of struct rseq_cs.
> 
> Maybe mention that it's good practice to clear rseq_cs before
> returning from a function that contains a restartable sequence?

Unfortunately, clearing it is not free. Considering that rseq is meant to
be used in very hot code paths, it would be preferable that applications
clear it in the very infrequent case where the rseq_cs or code will
vanish (e.g. dlclose or JIT reclaim), and not require it to be cleared
after each critical section. I am therefore reluctant to document the
behavior you describe as a "good practice" for rseq.

> That will deal with the dlclose issue because even if the function
> calls dlclose itself, unmapping something on call stack for dlclose is
> already undefined.

It would, but at the cost of adding an extra NULL store on pretty much
every use of rseq (think memory allocator fast-path for instance).

Thanks,

Mathieu

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

Re: [PATCH for 5.5 1/2] rseq: Fix: Clarify rseq.h UAPI rseq_cs memory reclaim requirements

[Florian Weimer]

* Mathieu Desnoyers:

> ----- On Dec 20, 2019, at 3:37 PM, Florian Weimer fw@deneb.enyo.de wrote:
>
>> * Mathieu Desnoyers:
>> 
>>> diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
>>> index 9a402fdb60e9..6f26b0b148a6 100644
>>> --- a/include/uapi/linux/rseq.h
>>> +++ b/include/uapi/linux/rseq.h
>>> @@ -100,7 +100,9 @@ struct rseq {
>>>  	 * instruction sequence block, as well as when the kernel detects that
>>>  	 * it is preempting or delivering a signal outside of the range
>>>  	 * targeted by the rseq_cs. Also needs to be set to NULL by user-space
>>> -	 * before reclaiming memory that contains the targeted struct rseq_cs.
>>> +	 * before reclaiming memory that contains the targeted struct rseq_cs
>>> +	 * or reclaiming memory that contains the code refered to by the
>>> +	 * start_ip and post_commit_offset fields of struct rseq_cs.
>> 
>> Maybe mention that it's good practice to clear rseq_cs before
>> returning from a function that contains a restartable sequence?
>
> Unfortunately, clearing it is not free. Considering that rseq is meant to
> be used in very hot code paths, it would be preferable that applications
> clear it in the very infrequent case where the rseq_cs or code will
> vanish (e.g. dlclose or JIT reclaim), and not require it to be cleared
> after each critical section. I am therefore reluctant to document the
> behavior you describe as a "good practice" for rseq.

You already have to write to rseq_cs before entering the critical
section, right?  Then you've already determined the address, and the
cache line is already hot, so it really should be close to zero cost.

I mean, you can still discard the advice, but you do so ad your own
peril …

Re: [PATCH for 5.5 1/2] rseq: Fix: Clarify rseq.h UAPI rseq_cs memory reclaim requirements

[Mathieu Desnoyers]

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

> * Mathieu Desnoyers:
> 
>> ----- On Dec 20, 2019, at 3:37 PM, Florian Weimer fw@deneb.enyo.de wrote:
>>
>>> * Mathieu Desnoyers:
>>> 
>>>> diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
>>>> index 9a402fdb60e9..6f26b0b148a6 100644
>>>> --- a/include/uapi/linux/rseq.h
>>>> +++ b/include/uapi/linux/rseq.h
>>>> @@ -100,7 +100,9 @@ struct rseq {
>>>>  	 * instruction sequence block, as well as when the kernel detects that
>>>>  	 * it is preempting or delivering a signal outside of the range
>>>>  	 * targeted by the rseq_cs. Also needs to be set to NULL by user-space
>>>> -	 * before reclaiming memory that contains the targeted struct rseq_cs.
>>>> +	 * before reclaiming memory that contains the targeted struct rseq_cs
>>>> +	 * or reclaiming memory that contains the code refered to by the
>>>> +	 * start_ip and post_commit_offset fields of struct rseq_cs.
>>> 
>>> Maybe mention that it's good practice to clear rseq_cs before
>>> returning from a function that contains a restartable sequence?
>>
>> Unfortunately, clearing it is not free. Considering that rseq is meant to
>> be used in very hot code paths, it would be preferable that applications
>> clear it in the very infrequent case where the rseq_cs or code will
>> vanish (e.g. dlclose or JIT reclaim), and not require it to be cleared
>> after each critical section. I am therefore reluctant to document the
>> behavior you describe as a "good practice" for rseq.
> 
> You already have to write to rseq_cs before entering the critical
> section, right?  Then you've already determined the address, and the
> cache line is already hot, so it really should be close to zero cost.

Considering that overall rseq executes in fraction of nanoseconds on
some architectures, adding an extra store is perhaps close to zero,
but still significantly degrades performance.

> 
> I mean, you can still discard the advice, but you do so ad your own
> peril …

I am also uncomfortable leaving this to the end user. One possibility
would be to extend rseq or membarrier to add a kind of "rseq-clear"
barrier, which would ensure that the kernel will have cleared the
rseq_cs field for each thread belonging to the current process. glibc
could then call this barrier before dlclose.

This is slightly different from another rseq-barrier that has been
requested by Paul Turner: a way to ensure that all previously
running rseq critical sections have completed or aborted.

AFAIU, the desiderata for each of the 2 use-cases is as follows:

rseq-barrier: guarantee that all prior rseq critical sections have
completed or aborted for the current process or for a set of registered
processes. Allows doing RCU-like algorithms within rseq critical sections.

rseq-clear: guarantee that the rseq_cs field is cleared for each thread
belonging to the current process before the barrier system call returns
to the caller. Aborts currently running rseq critical sections for all
threads belonging to the current process. The use-case is to allow
dlclose and JIT reclaim to clear any leftover reference to struct
rseq_cs or code which are going to be reclaimed.

Thoughts ?

Thanks,

Mathieu

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

Re: [PATCH for 5.5 1/2] rseq: Fix: Clarify rseq.h UAPI rseq_cs memory reclaim requirements

[Mathieu Desnoyers]

----- On Dec 20, 2019, at 4:15 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> ----- On Dec 20, 2019, at 3:57 PM, Florian Weimer fw@deneb.enyo.de wrote:
> 
>> * Mathieu Desnoyers:
>> 
>>> ----- On Dec 20, 2019, at 3:37 PM, Florian Weimer fw@deneb.enyo.de wrote:
>>>
>>>> * Mathieu Desnoyers:
>>>> 
>>>>> diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
>>>>> index 9a402fdb60e9..6f26b0b148a6 100644
>>>>> --- a/include/uapi/linux/rseq.h
>>>>> +++ b/include/uapi/linux/rseq.h
>>>>> @@ -100,7 +100,9 @@ struct rseq {
>>>>>  	 * instruction sequence block, as well as when the kernel detects that
>>>>>  	 * it is preempting or delivering a signal outside of the range
>>>>>  	 * targeted by the rseq_cs. Also needs to be set to NULL by user-space
>>>>> -	 * before reclaiming memory that contains the targeted struct rseq_cs.
>>>>> +	 * before reclaiming memory that contains the targeted struct rseq_cs
>>>>> +	 * or reclaiming memory that contains the code refered to by the
>>>>> +	 * start_ip and post_commit_offset fields of struct rseq_cs.
>>>> 
>>>> Maybe mention that it's good practice to clear rseq_cs before
>>>> returning from a function that contains a restartable sequence?
>>>
>>> Unfortunately, clearing it is not free. Considering that rseq is meant to
>>> be used in very hot code paths, it would be preferable that applications
>>> clear it in the very infrequent case where the rseq_cs or code will
>>> vanish (e.g. dlclose or JIT reclaim), and not require it to be cleared
>>> after each critical section. I am therefore reluctant to document the
>>> behavior you describe as a "good practice" for rseq.
>> 
>> You already have to write to rseq_cs before entering the critical
>> section, right?  Then you've already determined the address, and the
>> cache line is already hot, so it really should be close to zero cost.
> 
> Considering that overall rseq executes in fraction of nanoseconds on
> some architectures, adding an extra store is perhaps close to zero,
> but still significantly degrades performance.
> 
>> 
>> I mean, you can still discard the advice, but you do so ad your own
>> peril …
> 
> I am also uncomfortable leaving this to the end user. One possibility
> would be to extend rseq or membarrier to add a kind of "rseq-clear"
> barrier, which would ensure that the kernel will have cleared the
> rseq_cs field for each thread belonging to the current process. glibc
> could then call this barrier before dlclose.
> 
> This is slightly different from another rseq-barrier that has been
> requested by Paul Turner: a way to ensure that all previously
> running rseq critical sections have completed or aborted.
> 
> AFAIU, the desiderata for each of the 2 use-cases is as follows:
> 
> rseq-barrier: guarantee that all prior rseq critical sections have
> completed or aborted for the current process or for a set of registered
> processes. Allows doing RCU-like algorithms within rseq critical sections.
> 
> rseq-clear: guarantee that the rseq_cs field is cleared for each thread
> belonging to the current process before the barrier system call returns
> to the caller. Aborts currently running rseq critical sections for all
> threads belonging to the current process. The use-case is to allow
> dlclose and JIT reclaim to clear any leftover reference to struct
> rseq_cs or code which are going to be reclaimed.

Just to clarify: should the discussion here prevent the UAPI documentation
change from being merged into the Linux kernel ? Our discussion seems to be
related to integration of rseq into glibc, rather than the kernel UAPI per se.

Thanks,

Mathieu


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

Re: [PATCH for 5.5 1/2] rseq: Fix: Clarify rseq.h UAPI rseq_cs memory reclaim requirements

[Florian Weimer]

* Mathieu Desnoyers:

> Just to clarify: should the discussion here prevent the UAPI
> documentation change from being merged into the Linux kernel ? Our
> discussion seems to be related to integration of rseq into glibc,
> rather than the kernel UAPI per se.

I still think that clearing rseq_cs upon exit from the function that
contains the sequence is good practice, and the UAPI header should
mention that.

For glibc, if I recall correctly, we decided against doing anything in
dlclose to deal with this issue (remapping new code in an existing
rseq area) because it would need updating all threads, not just the
thread calling dlclose.  That's why we're punting this to
applications and why I think the UAPI header should mention this.

Re: [PATCH for 5.5 1/2] rseq: Fix: Clarify rseq.h UAPI rseq_cs memory reclaim requirements

[Mathieu Desnoyers]

----- On Jan 6, 2020, at 2:30 PM, Florian Weimer fw@deneb.enyo.de wrote:

> * Mathieu Desnoyers:
> 
>> Just to clarify: should the discussion here prevent the UAPI
>> documentation change from being merged into the Linux kernel ? Our
>> discussion seems to be related to integration of rseq into glibc,
>> rather than the kernel UAPI per se.
> 
> I still think that clearing rseq_cs upon exit from the function that
> contains the sequence is good practice, and the UAPI header should
> mention that.

My understanding is that a UAPI header should document what is strictly
required (here, clearing rseq_cs before unmapping the memory area
containing the rseq_cs structure or the code). Documenting a "best
practice" would AFAIU belong to a man page and not a UAPI header.

I'm adding Michael Kerrisk in CC in case he has an opinion on this
matter.

> For glibc, if I recall correctly, we decided against doing anything in
> dlclose to deal with this issue (remapping new code in an existing
> rseq area) because it would need updating all threads, not just the
> thread calling dlclose.  That's why we're punting this to
> applications and why I think the UAPI header should mention this.

Nothing prevents us from implementing a clever scheme in the future,
e.g. as a new membarrier command, that could be invoked from dlclose()
when it becomes available.

By documenting only the basic requirement in the UAPI header (do not
use-after-free) and not providing a "best practice" (which is not so good
performance-wise), we can then let the man page state the best practices,
and update them as new system call commands are implemented.

Thanks,

Mathieu

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