Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1659071 > unrolled thread

[PATCH 00/26] Fixing wait, exit, ptrace, exec, and CLONE_THREAD

Started byebiederm@xmission.com (Eric W. Biederman)
First post2017-06-06 21:10 +0200
Last post2017-06-07 18:10 +0200
Articles 18 on this page of 38 — 6 participants

Back to article view | Back to linux.kernel


Contents

  [PATCH 00/26] Fixing wait, exit, ptrace, exec, and CLONE_THREAD ebiederm@xmission.com (Eric W. Biederman) - 2017-06-06 21:10 +0200
    [PATCH 01/26] alpha: Remove unused TASK_GROUP_LEADER "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 12/26] wait: Directly test for the two cases where wait_task_zombie is called "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 15/26] wait: Don't delay !ptrace_reparented leaders "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 13/26] wait: Remove unused delay_group_leader "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 17/26] exit: Rework the exit states for ptracees "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 20/26] wait: Don't pass the list to wait_consider_task "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 26/26] pidns: Ensure zap_pid_ns_processes always terminates "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 19/26] wait: Simpler code for clearing notask_error in wait_consider_task "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 24/26] signal: In ptrace_stop improve identical signal detection "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 09/26] signal: Don't allow sending SIGKILL or SIGSTOP to init "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 03/26] signal: Do not perform permission checks when sending pdeath_signal "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
        Re: [PATCH 03/26] signal: Do not perform permission checks when  sending pdeath_signal Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-06 22:10 +0200
          Re: [PATCH 03/26] signal: Do not perform permission checks when sending pdeath_signal ebiederm@xmission.com (Eric W. Biederman) - 2017-06-07 13:40 +0200
        Re: [PATCH 03/26] signal: Do not perform permission checks when  sending pdeath_signal Richard Weinberger <richard.weinberger@gmail.com> - 2017-06-06 23:50 +0200
      [PATCH 07/26] pidns: Improve the error handling in alloc_pid "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 22/26] exit: Fix auto-wait of ptraced children "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 02/26] cgroup: Don't open code tasklist_empty() "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 04/26] signal: Make group_send_sig_info static "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 23/26] signal:  Fix SIGCONT before group stop completes. "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 18/26] wait: Fix WSTOPPED on a ptraced child "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 10/26] ptrace: Simplify ptrace_detach & exit_ptrace "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 21/26] wait: Optmize waitpid "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 14/26] wait: Move changing of ptrace from wait_consider_task into wait_task_stopped "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 08/26] exit: Make the runqueue rcu safe "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
        Re: [PATCH 08/26] exit: Make the runqueue rcu safe Oleg Nesterov <oleg@redhat.com> - 2017-06-07 15:20 +0200
      [PATCH 06/26] rlimit: Remove unnecessary grab of tasklist_lock "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
        Re: [PATCH 06/26] rlimit: Remove unnecessary grab of tasklist_lock Oleg Nesterov <oleg@redhat.com> - 2017-06-07 14:40 +0200
          Re: [PATCH 06/26] rlimit: Remove unnecessary grab of tasklist_lock ebiederm@xmission.com (Eric W. Biederman) - 2017-06-07 16:20 +0200
      [PATCH 16/26] exit: Fix reporting a ptraced !reparented leader has exited "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 11/26] wait: Properly implement __WCLONE handling in the presence of exec and ptrace "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 05/26] exit:  Remove the pointless clearing of SIGPENDING in __exit_signal "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:20 +0200
      [PATCH 25/26] signal: In ptrace_stop use CLD_TRAPPED in all ptrace signals "Eric W. Biederman" <ebiederm@xmission.com> - 2017-06-06 21:30 +0200
    Re: [PATCH 00/26] Fixing wait, exit, ptrace, exec, and CLONE_THREAD Aleksa Sarai <asarai@suse.de> - 2017-06-06 21:50 +0200
      Re: [PATCH 00/26] Fixing wait, exit, ptrace, exec, and CLONE_THREAD ebiederm@xmission.com (Eric W. Biederman) - 2017-06-07 13:50 +0200
        Re: [PATCH 00/26] Fixing wait, exit, ptrace, exec, and CLONE_THREAD Aleksa Sarai <asarai@suse.de> - 2017-06-07 14:30 +0200
    Re: [PATCH 00/26] Fixing wait, exit, ptrace, exec, and CLONE_THREAD Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-06 22:10 +0200
      Re: [PATCH 00/26] Fixing wait, exit, ptrace, exec, and CLONE_THREAD ebiederm@xmission.com (Eric W. Biederman) - 2017-06-07 18:10 +0200

Page 2 of 2 — ← Prev page 1 [2]


#1659104 — [PATCH 18/26] wait: Fix WSTOPPED on a ptraced child

From"Eric W. Biederman" <ebiederm@xmission.com>
Date2017-06-06 21:20 +0200
Subject[PATCH 18/26] wait: Fix WSTOPPED on a ptraced child
Message-ID<tPsbi-4vm-83@gated-at.bofh.it>
In reply to#1659075
When ptracing waitpid(pid, WUNTRACED) has two possible meanings.
- Wait for ptrace stops from the task with tid == pid
- Wait for ordinary stops from the process with tgid == pid

The only sensible behavior and the Linux behavior in 2.2 and
2.4 has been to consume both ptrace stops and group stops
in this case.  It looks like when Oleg disentangled thread
stops and group stops in 2.6.30 fixing a lot of other issues
the case when we want to reap both was overlooked.

Consume both the group and the ptrace stop state when
waitpid(pid, WUNTRACED) could be asking for both, and
the wait status for both is idenitical.  This keeps
us from double reporting the stop and causing confusion.

This is very slight user visible change and is only visible
in the unlikely case a ptracer specifies WUNTRACED aka
WSTOPPED.

Write this code in such a way that it doesn't matter which
list we are traversing when we find a child whose stop states
we care about.

Fixes: 90bc8d8b1a38 ("do_wait: fix waiting for the group stop with the dead leader")
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/exit.c | 82 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 40 insertions(+), 42 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index ff2ed1d60a8c..4e2d2b6f5581 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1151,22 +1151,23 @@ static int wait_task_zombie(struct wait_opts *wo, int old_state, struct task_str
 	return retval;
 }
 
-static int *task_stopped_code(struct task_struct *p, bool ptrace)
+static int *task_trace_stopped_code(struct task_struct *p)
 {
-	if (ptrace) {
-		if (task_is_traced(p) && !(p->jobctl & JOBCTL_LISTENING))
-			return &p->exit_code;
-	} else {
-		if (p->signal->flags & SIGNAL_STOP_STOPPED)
-			return &p->signal->group_exit_code;
-	}
+	if (task_is_traced(p) && !(p->jobctl & JOBCTL_LISTENING))
+		return &p->exit_code;
+	return NULL;
+}
+
+static int *task_group_stopped_code(struct task_struct *p)
+{
+	if (p->signal->flags & SIGNAL_STOP_STOPPED)
+		return &p->signal->group_exit_code;
 	return NULL;
 }
 
 /**
  * wait_task_stopped - Wait for %TASK_STOPPED or %TASK_TRACED
  * @wo: wait options
- * @ptrace: is the wait for ptrace
  * @p: task to wait for
  *
  * Handle sys_wait4() work for %p in state %TASK_STOPPED or %TASK_TRACED.
@@ -1181,49 +1182,47 @@ static int *task_stopped_code(struct task_struct *p, bool ptrace)
  * success, implies that tasklist_lock is released and wait condition
  * search should terminate.
  */
-static int wait_task_stopped(struct wait_opts *wo,
-				int ptrace, struct task_struct *p)
+static int wait_task_stopped(struct wait_opts *wo, struct task_struct *p)
 {
 	struct siginfo __user *infop;
-	int retval, exit_code, *p_code, why;
-	uid_t uid = 0; /* unneeded, required by compiler */
+	int retval, exit_code, *p_code, *g_code, why;
+	bool group, gstop, pstop;
+	uid_t uid;
 	pid_t pid;
 
 	/*
-	 * Hide group stop state from real parent; otherwise a single
-	 * stop can be reported twice as group and ptrace stop.  If a
-	 * ptracer wants to distinguish these two events for its own
-	 * children it should create a separate process which takes the
-	 * role of real parent.
-	 */
-	if (!ptrace && p->ptrace && !ptrace_reparented(p))
-		ptrace = 1;
-
-	/*
 	 * Traditionally we see ptrace'd stopped tasks regardless of options.
 	 */
-	if (!ptrace && !(wo->wo_flags & WUNTRACED))
+	group = thread_group_leader(p) && !ptrace_reparented(p);
+	pstop = same_thread_group(current, p->parent);
+	gstop = group && (wo->wo_flags & WUNTRACED);
+	if (!pstop && !gstop)
 		return 0;
 
-	if (!task_stopped_code(p, ptrace))
+	if ((!pstop || !task_trace_stopped_code(p)) &&
+	    (!gstop || !task_group_stopped_code(p)))
 		return 0;
 
 	exit_code = 0;
 	spin_lock_irq(&p->sighand->siglock);
 
-	p_code = task_stopped_code(p, ptrace);
-	if (unlikely(!p_code))
-		goto unlock_sig;
-
-	exit_code = *p_code;
-	if (!exit_code)
-		goto unlock_sig;
-
-	if (!unlikely(wo->wo_flags & WNOWAIT))
-		*p_code = 0;
-
-	uid = from_kuid_munged(current_user_ns(), task_uid(p));
-unlock_sig:
+	p_code = g_code = NULL;
+	if (pstop)
+		p_code = task_trace_stopped_code(p);
+	if (gstop)
+		g_code = task_group_stopped_code(p);
+	if (p_code) {
+		exit_code = *p_code;
+		why = CLD_TRAPPED;
+		if (!(wo->wo_flags & WNOWAIT))
+			*p_code = 0;
+	}
+	if (g_code && (!exit_code || (*g_code == exit_code))) {
+		exit_code = *g_code;
+		why = CLD_STOPPED;
+		if (!(wo->wo_flags & WNOWAIT))
+			*g_code = 0;
+	}
 	spin_unlock_irq(&p->sighand->siglock);
 	if (!exit_code)
 		return 0;
@@ -1236,8 +1235,8 @@ static int wait_task_stopped(struct wait_opts *wo,
 	 * possibly take page faults for user memory.
 	 */
 	get_task_struct(p);
+	uid = from_kuid_munged(current_user_ns(), task_uid(p));
 	pid = task_pid_vnr(p);
-	why = ptrace ? CLD_TRAPPED : CLD_STOPPED;
 	read_unlock(&tasklist_lock);
 	sched_annotate_sleep();
 
@@ -1403,10 +1402,9 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,
 	}
 
 	/*
-	 * Wait for stopped.  Depending on @ptrace, different stopped state
-	 * is used and the two don't interact with each other.
+	 * Wait for stopped.
 	 */
-	ret = wait_task_stopped(wo, ptrace, p);
+	ret = wait_task_stopped(wo, p);
 	if (ret)
 		return ret;
 
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]


#1659105 — [PATCH 10/26] ptrace: Simplify ptrace_detach & exit_ptrace

From"Eric W. Biederman" <ebiederm@xmission.com>
Date2017-06-06 21:20 +0200
Subject[PATCH 10/26] ptrace: Simplify ptrace_detach & exit_ptrace
Message-ID<tPsbi-4vm-77@gated-at.bofh.it>
In reply to#1659075
Call __ptrace_unlink not __ptrace_detach.  As it is guaranteed that
ptrace_detach will never be called on a process that has or may exit.

Rename __ptrace_detach __exit_ptrace as exit_ptrace is now it's only
caller and the corrected name is less confusing.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/ptrace.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 266ddcc1d8bb..490333db9e21 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -495,7 +495,7 @@ static int ignoring_children(struct sighand_struct *sigh)
  * reap it now, in that case we must also wake up sub-threads sleeping in
  * do_wait().
  */
-static bool __ptrace_detach(struct task_struct *tracer, struct task_struct *p)
+static bool __exit_ptrace(struct task_struct *tracer, struct task_struct *p)
 {
 	bool dead;
 
@@ -539,7 +539,7 @@ static int ptrace_detach(struct task_struct *child, unsigned int data)
 	 * the comment in ptrace_resume().
 	 */
 	child->exit_code = data;
-	__ptrace_detach(current, child);
+	__ptrace_unlink(child);
 	write_unlock_irq(&tasklist_lock);
 
 	proc_ptrace_connector(child, PTRACE_DETACH);
@@ -559,7 +559,7 @@ void exit_ptrace(struct task_struct *tracer, struct list_head *dead)
 		if (unlikely(p->ptrace & PT_EXITKILL))
 			send_sig_info(SIGKILL, SEND_SIG_FORCED, p);
 
-		if (__ptrace_detach(tracer, p))
+		if (__exit_ptrace(tracer, p))
 			list_add(&p->ptrace_entry, dead);
 	}
 }
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]


#1659106 — [PATCH 21/26] wait: Optmize waitpid

From"Eric W. Biederman" <ebiederm@xmission.com>
Date2017-06-06 21:20 +0200
Subject[PATCH 21/26] wait: Optmize waitpid
Message-ID<tPsbi-4vm-81@gated-at.bofh.it>
In reply to#1659075
Now that which list a task is found on does not matter there is no
reason to walk the child lists for waitpid when task_pid can directly
find the child.

Add a new helper do_wait_pid that finds the target task via pid_task
and verifies it is on one of the lists for the thread we are
reaping.

This is more efficient in two ways.  It skips the list traversal
so uninteresting tasks don't slow things down.  It guarantees
a task will only be visited once if p->parent == p->real_parent.

Except for the increase in efficiency this results in no user
visible behavioral differences.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/exit.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index c783d5fb5ab3..2f01b75e3b2e 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1438,6 +1438,18 @@ static int ptrace_do_wait(struct wait_opts *wo, struct task_struct *tsk)
 	return 0;
 }
 
+static int do_wait_pid(struct wait_opts *wo, struct task_struct *tsk)
+{
+	struct task_struct *p = pid_task(wo->wo_pid, wo->wo_type);
+
+	/* Not on one of this tasks child lists? */
+	if ((tsk != p->parent) &&
+	    ((tsk != p->real_parent) || !thread_group_leader(p)))
+		return 0;
+
+	return wait_consider_task(wo, p);
+}
+
 static int child_wait_callback(wait_queue_t *wait, unsigned mode,
 				int sync, void *key)
 {
@@ -1486,11 +1498,15 @@ static long do_wait(struct wait_opts *wo)
 	read_lock(&tasklist_lock);
 	tsk = current;
 	do {
-		retval = do_wait_thread(wo, tsk);
-		if (retval)
-			goto end;
+		if (wo->wo_type == PIDTYPE_PID) {
+			retval = do_wait_pid(wo, tsk);
+		} else {
+			retval = do_wait_thread(wo, tsk);
+			if (retval)
+				goto end;
 
-		retval = ptrace_do_wait(wo, tsk);
+			retval = ptrace_do_wait(wo, tsk);
+		}
 		if (retval)
 			goto end;
 
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]


#1659108 — [PATCH 14/26] wait: Move changing of ptrace from wait_consider_task into wait_task_stopped

From"Eric W. Biederman" <ebiederm@xmission.com>
Date2017-06-06 21:20 +0200
Subject[PATCH 14/26] wait: Move changing of ptrace from wait_consider_task into wait_task_stopped
Message-ID<tPsbi-4vm-87@gated-at.bofh.it>
In reply to#1659075
With the freeing and slaying of zombies moved earlier in wait_consider_task
changing of the ptrace value only effects the clearing of notask_error and
wait_task_stopped.  Move the changing of ptrace into wait_task_stopped.
The value of ptrace coming into the code clearing notask_error is left
at it's original value.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/exit.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 9b70e21c960d..dbf3fce00a1f 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1194,6 +1194,16 @@ static int wait_task_stopped(struct wait_opts *wo,
 	pid_t pid;
 
 	/*
+	 * Hide group stop state from real parent; otherwise a single
+	 * stop can be reported twice as group and ptrace stop.  If a
+	 * ptracer wants to distinguish these two events for its own
+	 * children it should create a separate process which takes the
+	 * role of real parent.
+	 */
+	if (!ptrace && p->ptrace && !ptrace_reparented(p))
+		ptrace = 1;
+
+	/*
 	 * Traditionally we see ptrace'd stopped tasks regardless of options.
 	 */
 	if (!ptrace && !(wo->wo_flags & WUNTRACED))
@@ -1370,18 +1380,6 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,
 		return 0;
 	}
 
-	if (likely(!ptrace) && unlikely(p->ptrace)) {
-		/*
-		 * Hide group stop state from real parent; otherwise a single
-		 * stop can be reported twice as group and ptrace stop.  If a
-		 * ptracer wants to distinguish these two events for its own
-		 * children it should create a separate process which takes the
-		 * role of real parent.
-		 */
-		if (!ptrace_reparented(p))
-			ptrace = 1;
-	}
-
 	if (exit_state == EXIT_ZOMBIE) {
 		/*
 		 * Allow access to stopped/continued state via zombie by
@@ -1403,7 +1401,8 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,
 		 * target task dies.  Only continued and exited can happen.
 		 * Clear notask_error if WCONTINUED | WEXITED.
 		 */
-		if (likely(!ptrace) || (wo->wo_flags & (WCONTINUED | WEXITED)))
+		if ((!ptrace && (!p->ptrace || ptrace_reparented(p))) ||
+		    (wo->wo_flags & (WCONTINUED | WEXITED)))
 			wo->notask_error = 0;
 	} else {
 		/*
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]


#1659109 — [PATCH 08/26] exit: Make the runqueue rcu safe

From"Eric W. Biederman" <ebiederm@xmission.com>
Date2017-06-06 21:20 +0200
Subject[PATCH 08/26] exit: Make the runqueue rcu safe
Message-ID<tPsbi-4vm-93@gated-at.bofh.it>
In reply to#1659075
Add a rcu_usage to task_struct and use it to reuse the delayed rcu put
logic from release_task in finish_task_switch.  This guarantees that
there will be an rcu interval before usage drops to zero for any task
on the run queue.  Making it safe to unconditionally call
get_task_struct in a rcu critical section for any task on the run
queue.

This guarantee in turn allows the fair scheduluer to use ordinary rcu
primitives to access tasks on the run queue and makes the magic functions
task_rcu_dereference and try_get_task_struct completely unnecessary.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 include/linux/sched.h      |  1 +
 include/linux/sched/task.h |  4 +--
 kernel/exit.c              | 83 ++++------------------------------------------
 kernel/fork.c              |  3 +-
 kernel/sched/core.c        |  2 +-
 kernel/sched/fair.c        |  2 +-
 6 files changed, 12 insertions(+), 83 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 2b69fc650201..461ecd20731c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -492,6 +492,7 @@ struct task_struct {
 	volatile long			state;
 	void				*stack;
 	atomic_t			usage;
+	atomic_t			rcu_usage;
 	/* Per task flags (PF_*), defined further below: */
 	unsigned int			flags;
 	unsigned int			ptrace;
diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
index a978d7189cfd..dc4a4f4c566b 100644
--- a/include/linux/sched/task.h
+++ b/include/linux/sched/task.h
@@ -94,9 +94,7 @@ static inline void put_task_struct(struct task_struct *t)
 		__put_task_struct(t);
 }
 
-struct task_struct *task_rcu_dereference(struct task_struct **ptask);
-struct task_struct *try_get_task_struct(struct task_struct **ptask);
-
+extern void rcu_put_task_struct(struct task_struct *tsk);
 
 #ifdef CONFIG_ARCH_WANTS_DYNAMIC_TASK_STRUCT
 extern int arch_task_struct_size __read_mostly;
diff --git a/kernel/exit.c b/kernel/exit.c
index c3de7ace243c..625e57f1bb5c 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -179,6 +179,11 @@ static void delayed_put_task_struct(struct rcu_head *rhp)
 	put_task_struct(tsk);
 }
 
+void rcu_put_task_struct(struct task_struct *tsk)
+{
+	if (atomic_dec_and_test(&tsk->rcu_usage))
+		call_rcu(&tsk->rcu, delayed_put_task_struct);
+}
 
 void release_task(struct task_struct *p)
 {
@@ -218,76 +223,13 @@ void release_task(struct task_struct *p)
 
 	write_unlock_irq(&tasklist_lock);
 	release_thread(p);
-	call_rcu(&p->rcu, delayed_put_task_struct);
+	rcu_put_task_struct(p);
 
 	p = leader;
 	if (unlikely(zap_leader))
 		goto repeat;
 }
 
-/*
- * Note that if this function returns a valid task_struct pointer (!NULL)
- * task->usage must remain >0 for the duration of the RCU critical section.
- */
-struct task_struct *task_rcu_dereference(struct task_struct **ptask)
-{
-	struct sighand_struct *sighand;
-	struct task_struct *task;
-
-	/*
-	 * We need to verify that release_task() was not called and thus
-	 * delayed_put_task_struct() can't run and drop the last reference
-	 * before rcu_read_unlock(). We check task->sighand != NULL,
-	 * but we can read the already freed and reused memory.
-	 */
-retry:
-	task = rcu_dereference(*ptask);
-	if (!task)
-		return NULL;
-
-	probe_kernel_address(&task->sighand, sighand);
-
-	/*
-	 * Pairs with atomic_dec_and_test() in put_task_struct(). If this task
-	 * was already freed we can not miss the preceding update of this
-	 * pointer.
-	 */
-	smp_rmb();
-	if (unlikely(task != READ_ONCE(*ptask)))
-		goto retry;
-
-	/*
-	 * We've re-checked that "task == *ptask", now we have two different
-	 * cases:
-	 *
-	 * 1. This is actually the same task/task_struct. In this case
-	 *    sighand != NULL tells us it is still alive.
-	 *
-	 * 2. This is another task which got the same memory for task_struct.
-	 *    We can't know this of course, and we can not trust
-	 *    sighand != NULL.
-	 *
-	 *    In this case we actually return a random value, but this is
-	 *    correct.
-	 *
-	 *    If we return NULL - we can pretend that we actually noticed that
-	 *    *ptask was updated when the previous task has exited. Or pretend
-	 *    that probe_slab_address(&sighand) reads NULL.
-	 *
-	 *    If we return the new task (because sighand is not NULL for any
-	 *    reason) - this is fine too. This (new) task can't go away before
-	 *    another gp pass.
-	 *
-	 *    And note: We could even eliminate the false positive if re-read
-	 *    task->sighand once again to avoid the falsely NULL. But this case
-	 *    is very unlikely so we don't care.
-	 */
-	if (!sighand)
-		return NULL;
-
-	return task;
-}
-
 void rcuwait_wake_up(struct rcuwait *w)
 {
 	struct task_struct *task;
@@ -317,19 +259,6 @@ void rcuwait_wake_up(struct rcuwait *w)
 	rcu_read_unlock();
 }
 
-struct task_struct *try_get_task_struct(struct task_struct **ptask)
-{
-	struct task_struct *task;
-
-	rcu_read_lock();
-	task = task_rcu_dereference(ptask);
-	if (task)
-		get_task_struct(task);
-	rcu_read_unlock();
-
-	return task;
-}
-
 /*
  * Determine if a process group is "orphaned", according to the POSIX
  * definition in 2.2.2.52.  Orphaned process groups are not to be affected
diff --git a/kernel/fork.c b/kernel/fork.c
index aa1076c5e4a9..1fe837e8c38e 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -567,7 +567,8 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node)
 	 * One for us, one for whoever does the "release_task()" (usually
 	 * parent)
 	 */
-	atomic_set(&tsk->usage, 2);
+	atomic_set(&tsk->rcu_usage, 2);
+	atomic_set(&tsk->usage, 1); /* For rcu_usage */
 #ifdef CONFIG_BLK_DEV_IO_TRACE
 	tsk->btrace_seq = 0;
 #endif
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 803c3bc274c4..1fccfd397cab 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2762,7 +2762,7 @@ static struct rq *finish_task_switch(struct task_struct *prev)
 		/* Task is done with its stack. */
 		put_task_stack(prev);
 
-		put_task_struct(prev);
+		rcu_put_task_struct(prev);
 	}
 
 	tick_nohz_task_switch();
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index d71109321841..5c0a1e1cc0f6 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1527,7 +1527,7 @@ static void task_numa_compare(struct task_numa_env *env,
 	int dist = env->dist;
 
 	rcu_read_lock();
-	cur = task_rcu_dereference(&dst_rq->curr);
+	cur = rcu_dereference(dst_rq->curr);
 	if (cur && ((cur->flags & PF_EXITING) || is_idle_task(cur)))
 		cur = NULL;
 
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]


#1659781 — Re: [PATCH 08/26] exit: Make the runqueue rcu safe

FromOleg Nesterov <oleg@redhat.com>
Date2017-06-07 15:20 +0200
SubjectRe: [PATCH 08/26] exit: Make the runqueue rcu safe
Message-ID<tPJ2p-77i-1@gated-at.bofh.it>
In reply to#1659109
On 06/06, Eric W. Biederman wrote:
>
> Add a rcu_usage to task_struct and use it to reuse the delayed rcu put
> logic from release_task in finish_task_switch.

I didn't really read this patch yet, but it first glance it should work and
you can also remove the ->exit_state check/limitation in rcuwait_wait_event().

Oleg.

[toc] | [prev] | [next] | [standalone]


#1659110 — [PATCH 06/26] rlimit: Remove unnecessary grab of tasklist_lock

From"Eric W. Biederman" <ebiederm@xmission.com>
Date2017-06-06 21:20 +0200
Subject[PATCH 06/26] rlimit: Remove unnecessary grab of tasklist_lock
Message-ID<tPsbi-4vm-91@gated-at.bofh.it>
In reply to#1659075
There is no reason to take the tasklist lock here.  The sighand
structure is never referenced and and tsk->signal is guaranteed
to stick around until tsk is freed. Further update_rlimit_cpu
does not need the tasklist_lock.  And the rlim_lock is used
to guarantee mutual exclusion.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/sys.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index 8a94b4eabcaa..705f14b28134 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1380,13 +1380,6 @@ int do_prlimit(struct task_struct *tsk, unsigned int resource,
 			return -EPERM;
 	}
 
-	/* protect tsk->signal and tsk->sighand from disappearing */
-	read_lock(&tasklist_lock);
-	if (!tsk->sighand) {
-		retval = -ESRCH;
-		goto out;
-	}
-
 	rlim = tsk->signal->rlim + resource;
 	task_lock(tsk->group_leader);
 	if (new_rlim) {
@@ -1425,8 +1418,7 @@ int do_prlimit(struct task_struct *tsk, unsigned int resource,
 	     new_rlim->rlim_cur != RLIM_INFINITY &&
 	     IS_ENABLED(CONFIG_POSIX_TIMERS))
 		update_rlimit_cpu(tsk, new_rlim->rlim_cur);
-out:
-	read_unlock(&tasklist_lock);
+
 	return retval;
 }
 
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]


#1659753 — Re: [PATCH 06/26] rlimit: Remove unnecessary grab of tasklist_lock

FromOleg Nesterov <oleg@redhat.com>
Date2017-06-07 14:40 +0200
SubjectRe: [PATCH 06/26] rlimit: Remove unnecessary grab of tasklist_lock
Message-ID<tPIpH-6D4-11@gated-at.bofh.it>
In reply to#1659110
Hi Eric,

I'll try very much to read this series tomorrow, can't do this today...

On 06/06, Eric W. Biederman wrote:
>
> @@ -1380,13 +1380,6 @@ int do_prlimit(struct task_struct *tsk, unsigned int resource,
>  			return -EPERM;
>  	}
>  
> -	/* protect tsk->signal and tsk->sighand from disappearing */
> -	read_lock(&tasklist_lock);
> -	if (!tsk->sighand) {
> -		retval = -ESRCH;
> -		goto out;
> -	}

Yes, the comment is wrong.

However we do need read_lock(tasklist_lock) to access ->group_leader. And the
->sighand != NULL check ensures that ->group_leader is the valid pointer.

Also, update_rlimit_cpu() is not safe without tasklist / sighand-check.

We can probably change this code to rely on rcu.

Oleg.

[toc] | [prev] | [next] | [standalone]


#1659846 — Re: [PATCH 06/26] rlimit: Remove unnecessary grab of tasklist_lock

Fromebiederm@xmission.com (Eric W. Biederman)
Date2017-06-07 16:20 +0200
SubjectRe: [PATCH 06/26] rlimit: Remove unnecessary grab of tasklist_lock
Message-ID<tPJYu-7IG-23@gated-at.bofh.it>
In reply to#1659753
Oleg Nesterov <oleg@redhat.com> writes:

> Hi Eric,
>
> I'll try very much to read this series tomorrow, can't do this today...
>
> On 06/06, Eric W. Biederman wrote:
>>
>> @@ -1380,13 +1380,6 @@ int do_prlimit(struct task_struct *tsk, unsigned int resource,
>>  			return -EPERM;
>>  	}
>>  
>> -	/* protect tsk->signal and tsk->sighand from disappearing */
>> -	read_lock(&tasklist_lock);
>> -	if (!tsk->sighand) {
>> -		retval = -ESRCH;
>> -		goto out;
>> -	}
>
> Yes, the comment is wrong.
>
> However we do need read_lock(tasklist_lock) to access ->group_leader. And the
> ->sighand != NULL check ensures that ->group_leader is the valid
> pointer.

As of 4.12-rc1 The code does not access group_leader anymore.

> Also, update_rlimit_cpu() is not safe without tasklist / sighand-check.
>
> We can probably change this code to rely on rcu.

Good point a NULL sighand will cause update_rlimit_cpu to OOPS.

Grr.  There is a point in my tree where this is perfectly safe.  But not
at this point.  Consider this patch dropped for the moment.

Eric

[toc] | [prev] | [next] | [standalone]


#1659111 — [PATCH 16/26] exit: Fix reporting a ptraced !reparented leader has exited

From"Eric W. Biederman" <ebiederm@xmission.com>
Date2017-06-06 21:20 +0200
Subject[PATCH 16/26] exit: Fix reporting a ptraced !reparented leader has exited
Message-ID<tPsbi-4vm-89@gated-at.bofh.it>
In reply to#1659075
When a task exits and uses do_notify_parent to send tsk->exit_signal
or SIGCHLD this has one of two possible meanings.
- The ptraced task has exited
- The thread group has exited

Linux resolves this ambiguity by preferring the thread group exit
interpretation if it is possible.  As the exit of a thread group
containing a task is a superset of the exit of a task.

The code attempts to implement this in it's selection of signal
before calling do_notify_parent for a ptraced task.  Unfortunately
it fails to properly handle the case when the thread_group is still
running (but it's leader has exited).

Fix this for a ptraced child leader by skipping the notification
instead of changing the exit signal.  If we skip sending the signal in
exit_noitfy, when the last of the other threads are reaped
release_task will send the signal for us.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/exit.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 85b34eff8807..72591eb5e361 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -639,7 +639,7 @@ static void forget_original_parent(struct task_struct *father,
  */
 static void exit_notify(struct task_struct *tsk, int group_dead)
 {
-	bool autoreap;
+	bool autoreap = true;
 	struct task_struct *p, *n;
 	LIST_HEAD(dead);
 
@@ -649,17 +649,12 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 	if (group_dead)
 		kill_orphaned_pgrp(tsk->group_leader, NULL);
 
-	if (unlikely(tsk->ptrace)) {
-		int sig = thread_group_leader(tsk) &&
-				thread_group_empty(tsk) &&
-				!ptrace_reparented(tsk) ?
-			tsk->exit_signal : SIGCHLD;
-		autoreap = do_notify_parent(tsk, sig);
-	} else if (thread_group_leader(tsk)) {
+	if (thread_group_leader(tsk) && !ptrace_reparented(tsk)) {
 		autoreap = thread_group_empty(tsk) &&
 			do_notify_parent(tsk, tsk->exit_signal);
-	} else {
-		autoreap = true;
+	}
+	else if (unlikely(tsk->ptrace)) {
+		autoreap = do_notify_parent(tsk, SIGCHLD);
 	}
 
 	tsk->exit_state = autoreap ? EXIT_DEAD : EXIT_ZOMBIE;
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]


#1659113 — [PATCH 11/26] wait: Properly implement __WCLONE handling in the presence of exec and ptrace

From"Eric W. Biederman" <ebiederm@xmission.com>
Date2017-06-06 21:20 +0200
Subject[PATCH 11/26] wait: Properly implement __WCLONE handling in the presence of exec and ptrace
Message-ID<tPsbj-4vm-97@gated-at.bofh.it>
In reply to#1659075
Rewrite the condition for what contitues a clone child.  AKA a child
that reports to it's parent using something other than SIGCHLD.

If the parent has called exec since the child has started that child
will alwasy report to it's parent with SIGCHLD.

If the parent is only ptracing the child the child will always
report to the parent with SIGCHLD.

This implements the documented semantics and subsumes the fix Oleg
made in 4.7 to make __WCLONE unnecessary when ptracing a child.  It was
just a bug in the check for __WCLONE support that made that necessary
the ``semantic'' change necessary.

Around v2.3.23 notify_parent was fixed to send SIGCHLD if the parent
had exec'd.  Fixing half a bug but wait was not fixed to see children
in that case.

Not handling either the ptrace or the parent exec case appears to
go all of the way back to 1.0.

Fixes: bf959931ddb8 ("wait/ptrace: assume __WALL if the child is traced")
Fixes: v1.0
Fixes: v2.3.23
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/exit.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 625e57f1bb5c..306e526f4c5e 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -950,31 +950,26 @@ static int eligible_pid(struct wait_opts *wo, struct task_struct *p)
 		task_pid_type(p, wo->wo_type) == wo->wo_pid;
 }
 
-static int
-eligible_child(struct wait_opts *wo, bool ptrace, struct task_struct *p)
+static int eligible_child(struct wait_opts *wo, struct task_struct *p)
 {
 	if (!eligible_pid(wo, p))
 		return 0;
 
 	/*
-	 * Wait for all children (clone and not) if __WALL is set or
-	 * if it is traced by us.
+	 * Wait for all children (clone and not) if __WALL is set.
 	 */
-	if (ptrace || (wo->wo_flags & __WALL))
+	if (wo->wo_flags & __WALL)
 		return 1;
 
 	/*
-	 * Otherwise, wait for clone children *only* if __WCLONE is set;
-	 * otherwise, wait for non-clone children *only*.
-	 *
-	 * Note: a "clone" child here is one that reports to its parent
-	 * using a signal other than SIGCHLD, or a non-leader thread which
-	 * we can only see if it is traced by us.
+	 * Otherwise wait for either children that report to their
+	 * parent via SIGCHLD (when __WCLONE is not set) or use
+	 * another signal (when __WCLONE is set).
 	 */
-	if ((p->exit_signal != SIGCHLD) ^ !!(wo->wo_flags & __WCLONE))
-		return 0;
-
-	return 1;
+	return (((p->exit_signal == SIGCHLD) ||
+		 (p->parent_exec_id == p->real_parent->self_exec_id))
+		 && thread_group_leader(p) && !ptrace_reparented(p)) ^
+		!!(wo->wo_flags & __WCLONE);
 }
 
 static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
@@ -1343,7 +1338,7 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,
 	if (unlikely(exit_state == EXIT_DEAD))
 		return 0;
 
-	ret = eligible_child(wo, ptrace, p);
+	ret = eligible_child(wo, p);
 	if (!ret)
 		return ret;
 
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]


#1659114 — [PATCH 05/26] exit: Remove the pointless clearing of SIGPENDING in __exit_signal

From"Eric W. Biederman" <ebiederm@xmission.com>
Date2017-06-06 21:20 +0200
Subject[PATCH 05/26] exit: Remove the pointless clearing of SIGPENDING in __exit_signal
Message-ID<tPsbj-4vm-101@gated-at.bofh.it>
In reply to#1659075
Handling of signals does not happen once do_exit is called as a
process never again exits from the kernel.  So remove this ancient hold
over from something else.

Furthermore this is TIF_SIGPENDING on a zombie.  Which will never
be scheduled on a cpu again.

Setting sigpending=0 was silly in 2.4 when exit_sighand did it and was
called from do_exit.  When exit_sighand started being called from
release_task the code went from silly to completely pointless.

History  tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/
Fixes: 6dfc88977e42 ("[PATCH] shared thread signals")
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 kernel/exit.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 8926cdd132f2..c3de7ace243c 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -164,7 +164,6 @@ static void __exit_signal(struct task_struct *tsk)
 	spin_unlock(&sighand->siglock);
 
 	__cleanup_sighand(sighand);
-	clear_tsk_thread_flag(tsk, TIF_SIGPENDING);
 	if (group_dead) {
 		flush_sigqueue(&sig->shared_pending);
 		tty_kref_put(tty);
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]


#1659141 — [PATCH 25/26] signal: In ptrace_stop use CLD_TRAPPED in all ptrace signals

From"Eric W. Biederman" <ebiederm@xmission.com>
Date2017-06-06 21:30 +0200
Subject[PATCH 25/26] signal: In ptrace_stop use CLD_TRAPPED in all ptrace signals
Message-ID<tPskY-4zc-59@gated-at.bofh.it>
In reply to#1659075
If the only job of the signal is to report a ptrace level event set
si_code to CLD_TRAPPED instead of possibly CLD_STOPPED.

This causes the siginfo of the signals that are sent to match the
signinfo of the signals returned by waitid.

This is a user visible difference but I don't expect anything will
care.

In fact this is a return to historical linux behavior.  In linux 2.4.0
all ptrace stops were reported through do_notify_parent with
CLD_TRAPPED.  When do_notify_parent_cldstop was added the CLD_TRAPPED
logic was not included and CLD_TRAPPED for ptrace stops was lost.  As
nothing was said about this case I assume it was an oversight.

When waitid was added a little earlier all stops were being
reported with do_notify_parent and all ptrace stops were setting
CLD_TRAPPED.  So initially signals and waitid were in sync with
respect to setting CLD_TRAPPED.

It is also worth knowing that posix uses documents CLD_TRAPPED
as "Traced child has trapped."

History Tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
Ref: ca3f74aa7baa ("[PATCH] waitid system call")
Fixes: Fixes: ebf5ebe31d2c ("[PATCH] signal-fixes-2.5.59-A4")
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/signal.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index ff41e6ee2b5e..0d4ca87f1fee 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1886,9 +1886,9 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
 		 * separately unless they are going to be identical.
 		 */
 		if (!identical)
-			do_notify_parent_cldstop(current, true, why);
+			do_notify_parent_cldstop(current, true, CLD_TRAPPED);
 		if (gstop_done)
-			do_notify_parent_cldstop(current, false, why);
+			do_notify_parent_cldstop(current, false, CLD_STOPPED);
 
 		/*
 		 * Don't want to allow preemption here, because
@@ -1912,7 +1912,7 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
 		 * the real parent of the group stop completion is enough.
 		 */
 		if (gstop_done)
-			do_notify_parent_cldstop(current, false, why);
+			do_notify_parent_cldstop(current, false, CLD_STOPPED);
 
 		/* tasklist protects us from ptrace_freeze_traced() */
 		__set_current_state(TASK_RUNNING);
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]


#1659156

FromAleksa Sarai <asarai@suse.de>
Date2017-06-06 21:50 +0200
Message-ID<tPsEi-4Hz-11@gated-at.bofh.it>
In reply to#1659071
> Another easy entry point is to see that a multi-threaded setuid won't
> change the credentials on a zombie thread group leader.  Which can allow
> sending signals to a process that the credential change should forbid.
> This is in violation of posix and the semantics we attempt to enforce in
> linux.

I might be completely wrong on this point (and I haven't looked at the 
patches), but I was under the impression that multi-threaded set[ug]id 
was implemented in userspace (by glibc's nptl(7) library that uses RT 
signals internally to get each thread to update their credentials). And 
given that, I wouldn't be surprised (as a user) that zombie threads will 
have stale credentials (glibc isn't running in those threads anymore).

Am I mistaken in that belief?

</off-topic>

-- 
Aleksa Sarai
Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/

[toc] | [prev] | [next] | [standalone]


#1659699

Fromebiederm@xmission.com (Eric W. Biederman)
Date2017-06-07 13:50 +0200
Message-ID<tPHDj-64E-1@gated-at.bofh.it>
In reply to#1659156
Aleksa Sarai <asarai@suse.de> writes:

>> Another easy entry point is to see that a multi-threaded setuid won't
>> change the credentials on a zombie thread group leader.  Which can allow
>> sending signals to a process that the credential change should forbid.
>> This is in violation of posix and the semantics we attempt to enforce in
>> linux.
>
> I might be completely wrong on this point (and I haven't looked at the patches),
> but I was under the impression that multi-threaded set[ug]id was implemented in
> userspace (by glibc's nptl(7) library that uses RT signals internally to get
> each thread to update their credentials). And given that, I wouldn't be
> surprised (as a user) that zombie threads will have stale credentials (glibc
> isn't running in those threads anymore).
>
> Am I mistaken in that belief?

Would you be surprised if you learned that if your first thread
exits, it will become a zombie and persist for the lifetime of your
process?

Furthermore all non-thread specific signals will permission check
against that first zombie thread.

Which I think makes this surprising even if you know that setuid is
implemented in userspace.

Eric

[toc] | [prev] | [next] | [standalone]


#1659747

FromAleksa Sarai <asarai@suse.de>
Date2017-06-07 14:30 +0200
Message-ID<tPIg2-6zL-25@gated-at.bofh.it>
In reply to#1659699
On 06/07/2017 09:36 PM, Eric W. Biederman wrote:
>>> Another easy entry point is to see that a multi-threaded setuid won't
>>> change the credentials on a zombie thread group leader.  Which can allow
>>> sending signals to a process that the credential change should forbid.
>>> This is in violation of posix and the semantics we attempt to enforce in
>>> linux.
>>
>> I might be completely wrong on this point (and I haven't looked at the patches),
>> but I was under the impression that multi-threaded set[ug]id was implemented in
>> userspace (by glibc's nptl(7) library that uses RT signals internally to get
>> each thread to update their credentials). And given that, I wouldn't be
>> surprised (as a user) that zombie threads will have stale credentials (glibc
>> isn't running in those threads anymore).
>>
>> Am I mistaken in that belief?
> 
> Would you be surprised if you learned that if your first thread
> exits, it will become a zombie and persist for the lifetime of your
> process?
> 
> Furthermore all non-thread specific signals will permission check
> against that first zombie thread.

Ah okay, so it really is a matter of Linux's threadgroup semantics just 
not being "right" on a more fundamental level than nptl.

> Which I think makes this surprising even if you know that setuid is
> implemented in userspace.

Quite surprising, thanks for the explanation.

-- 
Aleksa Sarai
Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/

[toc] | [prev] | [next] | [standalone]


#1659167

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2017-06-06 22:10 +0200
Message-ID<tPsXE-54F-21@gated-at.bofh.it>
In reply to#1659071
On Tue, Jun 6, 2017 at 12:01 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> I am posting this patches in the hope of some review of the strategy I
> am taking and to let the individual patches be reviewed.

I'm trying to look through these, and finding (as usual) that the
signal handling and exit code is extremely scary from a correctness
and security standpoint.

I really want Oleg to review/ack these. Oleg?

I also would really really want to see the stuff that actually changes
semantics split out.

For example, I feel much less nervous about things like making the
tasklist RCU-safe. So I'd like to see changes like that be separated
out from the much scarier ones. Would that be possible? Hint hint..

                Linus

[toc] | [prev] | [next] | [standalone]


#1659964

Fromebiederm@xmission.com (Eric W. Biederman)
Date2017-06-07 18:10 +0200
Message-ID<tPLGW-rD-31@gated-at.bofh.it>
In reply to#1659167
Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Tue, Jun 6, 2017 at 12:01 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>>
>> I am posting this patches in the hope of some review of the strategy I
>> am taking and to let the individual patches be reviewed.
>
> I'm trying to look through these, and finding (as usual) that the
> signal handling and exit code is extremely scary from a correctness
> and security standpoint.
>
> I really want Oleg to review/ack these. Oleg?

Definitely.  The more review I can get the better. 

> I also would really really want to see the stuff that actually changes
> semantics split out.
>
> For example, I feel much less nervous about things like making the
> tasklist RCU-safe. So I'd like to see changes like that be separated
> out from the much scarier ones. Would that be possible? Hint hint..

The patches that I posted are the ones that I would really like to have
ready for the 4.13 merge window.  They are supposed to be the least
scary patches that don't really depend on anything else, and the semantic changes.

After a first brush with code review the non-scary patches wind up just
being these.

[PATCH 01/26] alpha: Remove unused TASK_GROUP_LEADER
[PATCH 02/26] cgroup: Don't open code tasklist_empty()
[PATCH 05/26] exit:  Remove the pointless clearing of SIGPENDING in __exit_signal
[PATCH 07/26] pidns: Improve the error handling in alloc_pid
[PATCH 08/26] exit: Make the runqueue rcu safe

This turns out to have a dependency I overlooked so I am dropping it for now.
[PATCH 06/26] rlimit: Remove unnecessary grab of tasklist_lock

There is a small pair of a semantic change and it's dependency:
[PATCH 03/26] signal: Do not perform permission checks when sending pdeath_signal
[PATCH 04/26] signal: Make group_send_sig_info static

There is a teeny tiny semantic change:
[PATCH 09/26] signal: Don't allow sending SIGKILL or SIGSTOP to init

The deeply related scarier changes (+ indicates it includes a semantic change):
 [PATCH 10/26] ptrace: Simplify ptrace_detach & exit_ptrace
+[PATCH 11/26] wait: Properly implement __WCLONE handling in the presence of exec and ptrace
 [PATCH 12/26] wait: Directly test for the two cases where wait_task_zombie is called
 [PATCH 13/26] wait: Remove unused delay_group_leader
+[PATCH 14/26] wait: Move changing of ptrace from wait_consider_task into wait_task_stopped
+[PATCH 15/26] wait: Don't delay !ptrace_reparented leaders
+[PATCH 16/26] exit: Fix reporting a ptraced !reparented leader has exited
 [PATCH 17/26] exit: Rework the exit states for ptracees
+[PATCH 18/26] wait: Fix WSTOPPED on a ptraced child
 [PATCH 19/26] wait: Simpler code for clearing notask_error in wait_consider_task
 [PATCH 20/26] wait: Don't pass the list to wait_consider_task
 [PATCH 21/26] wait: Optmize waitpid
+[PATCH 22/26] exit: Fix auto-wait of ptraced children
+[PATCH 23/26] signal: Fix SIGCONT before group stop completes.
+[PATCH 24/26] signal: In ptrace_stop improve identical signal detection
+[PATCH 25/26] signal: In ptrace_stop use CLD_TRAPPED in all ptrace signals
+[PATCH 26/26] pidns: Ensure zap_pid_ns_processes always terminates

Out of my 170 or so total changes those are the bulk of the semantic
changes and definitely the scariest.  Even those above are very
conservative and are really just about sorting out the weirdness in the
semantics when we are ptracing our own child which causes wait and the
siginfo of SIGCHLD to have two sets of meanings.

I am hoping we can just get 1,2,5,7, and 8 reviewed and I can just apply
them to my for-next branch.

If I need to repost I will respect the split-out I have described above.

At this point I am happy to see that people are not scared when I
suggest killing the concept of the thread group leader.

Eric

[toc] | [prev] | [standalone]


Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel


csiph-web