Re: [PATCH for 5.3 2/3] rseq: Fix: Unregister rseq for CLONE_SETTLS

----- On Sep 14, 2019, at 10:21 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

There is an ongoing discussion on the choice of flag we want to care
about here. Therefore, please don't pull this patch until we reach an
agreement.

Following discussion with Neel Natu (Google) and Paul Turner (Google),
I plan to modify this patch, and unregister RSEQ on clone CLONE_VM for the
following reasons:

1) CLONE_THREAD requires CLONE_SIGHAND, which requires CLONE_VM to be
   set. Therefore, just checking for CLONE_VM covers all CLONE_THREAD uses,

2) There is the possibility of an unlikely scenario where CLONE_SETTLS is used
   without CLONE_VM. In order to be an issue, it would require that the rseq
   TLS is in a shared memory area.

   I do not plan on adding CLONE_SETTLS to the set of clone flags which
   unregister RSEQ, because it would require that we also unregister RSEQ
   on set_thread_area(2) and arch_prctl(2) ARCH_SET_FS for completeness.
   So rather than doing a partial solution, it appears better to let user-space
   explicitly perform rseq unregistration across clone if needed in scenarios
   where CLONE_VM is not set.

Thoughts ?

Thanks,

Mathieu

Thanks,

Mathieu

----- On Sep 13, 2019, at 11:12 AM, Mathieu Desnoyers
mathieu.desnoyers@efficios.com wrote:

quoted

It has been reported by Google that rseq is not behaving properly
with respect to clone when CLONE_VM is used without CLONE_THREAD.
It keeps the prior thread's rseq TLS registered when the TLS of the
thread has moved, so the kernel deals with the wrong TLS.

The approach of clearing the per task-struct rseq registration
on clone with CLONE_THREAD flag is incomplete. It does not cover
the use-case of clone with CLONE_VM set, but without CLONE_THREAD.

Looking more closely at each of the clone flags:

- CLONE_THREAD,
- CLONE_VM,
- CLONE_SETTLS.

It appears that the flag we really want to track is CLONE_SETTLS, which
moves the location of the TLS for the child, making the rseq
registration point to the wrong TLS.

Suggested-by: "H . Peter Anvin" <hpa@zytor.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Thomas Gleixner <redacted>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: "Paul E. McKenney" <redacted>
Cc: Boqun Feng <redacted>
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Paul Turner <redacted>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: linux-api@vger.kernel.org
Cc: <redacted>
---
include/linux/sched.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 9f51932bd543..76bf55b5cccf 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h

@@ -1919,11 +1919,11 @@ static inline void rseq_migrate(struct task_struct *t)

/*
 * If parent process has a registered restartable sequences area, the
- * child inherits. Only applies when forking a process, not a thread.
+ * child inherits. Unregister rseq for a clone with CLONE_SETTLS set.
 */
static inline void rseq_fork(struct task_struct *t, unsigned long clone_flags)
{
-	if (clone_flags & CLONE_THREAD) {
+	if (clone_flags & CLONE_SETTLS) {
		t->rseq = NULL;
		t->rseq_sig = 0;
		t->rseq_event_mask = 0;
--
2.17.1

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

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