ptrace,signal: sane interaction between ptrace and job control signals, take#2

Hello,

This is the second attempt at cleaning up ptrace and signal behaviors, especially the interaction between ptrace and group stop. There are quite some number of problems in the area and the current behavior is often racy, indeterministic and sometimes outright buggy. This patchset aims to clean up the muddy stuff, and define and implement a clear interaction between ptrace and job control.

Most changes from the last take[L] are to address the problems pointed out by Oleg. Let's hope things are less swiss-cheesy this time. I considered adding a separate task state tracking which encompasses ptrace related transitions but it quickly became too redundant and uglier than the current approach of augmenting the default state machine with flags for special cases. So, at least for now, I think it's better to continue with the current approach.

* Group stop helpers restructured: task_clear_group_stop[_trapping]() added and consume_group_stop() replaced with task_participate_group_stop().

* 0002-freezer-fix-a-race-during-freezing-of-TASK_STOPPED-t.patch from the last posting was taken by Rafael and dropped from this series.

* 0002-signal-fix-CLD_CONTINUED-notification-target.patch added.

* 0004-signal-don-t-notify-parent-if-not-stopping-after-tra.patch replaced with 0004-ptrace-kill-tracehook_notify_jctl.patch to streamline the conversion. Whether something similar can or should be factored out after conversion is to be determined but I'm quite doubtful.

* 0005-ptrace-add-why-to-ptrace_stop.patch repositioned from 0007 to 0005.

* 0007-signal-use-GROUP_STOP_PENDING-to-stop-once-for-a-sin.patch:
Fixed the problems pointed out by Oleg.

* 0010-ptrace-don-t-consume-group-count-from-ptrace_stop.patch replaced by 0008-ptrace-participate-in-group-stop-from-ptrace_stop-if.patch. This is to ensure that GROUP_STOP_PENDING consumption and transition into TASK_TRACED is atomic so that a task doesn't trap for group stop with GROUP_STOP_PENDING set.

* 0010-ptrace-clean-transitions-between-TASK_STOPPED-and-TR.patch. Fixed group stop bookkeeping bug on retry. TASK_STOPPED -> TASK_TRACED transition is now hidden from userland with the help of GROUP_STOP_TRAPPING flag.

* Notification reliability patches restructured so that the determination logics now reside in do_notify_parent_cldstop() and are executed while holding both tasklist_lock and siglock.

* New patches 0011 and 0015 added.

This patchset contains the following sixteen patches.

0001-signal-fix-SIGCONT-notification-code.patch 0002-signal-fix-CLD_CONTINUED-notification-target.patch 0003-signal-remove-superflous-try_to_freeze-loop-in-do_si.patch 0004-ptrace-kill-tracehook_notify_jctl.patch 0005-ptrace-add-why-to-ptrace_stop.patch 0006-signal-fix-premature-completion-of-group-stop-when-i.patch 0007-signal-use-GROUP_STOP_PENDING-to-stop-once-for-a-sin.patch 0008-ptrace-participate-in-group-stop-from-ptrace_stop-if.patch 0009-ptrace-make-do_signal_stop-use-ptrace_stop-if-the-ta.patch 0010-ptrace-clean-transitions-between-TASK_STOPPED-and-TR.patch 0011-signal-prepare-for-CLD_-notification-changes.patch 0012-ptrace-make-group-stop-notification-reliable-against.patch 0013-ptrace-reorganize-__ptrace_unlink-and-ptrace_untrace.patch 0014-ptrace-make-SIGCONT-notification-reliable-against-pt.patch 0015-ptrace-make-sure-SIGNAL_NOTIFY_CONT-is-checked-after.patch 0016-ptrace-remove-the-extra-wake_up_process-from-ptrace_.patch

0001-0002 are fixes. 0003-0005 are preparation patches.

0006 prevents a ptraced task from being stopped multiple times for the same group stop instance.

0007-0010 update the code such that a ptracee always enters and leaves TASK_TRACED on its own. ptracer no longer changes tracee's state underneath it; instead, it tells the tracee to enter the target state. A TASK_TRACED task is guaranteed to be stopped inside ptrace_stop() after executing the arch hooks while TASK_STOPPED task is guaranteed to be stopped in do_signal_stop().

0011-0015 make CLD_STOPPED/CONTINUED notification reliable with intervening ptrace. Whether a ptracee owes an notification to its parent is tracked and the real parent is notified accordingly on detach.

0016 kills the unnecessary wake_up_process() which is wrong in so many ways including the possibility of abruptly waking up a task in an uninterruptible sleep. Now that ptrace / job control interaction is cleaned up, this really should go.

After the patchset, the interaction between ptrace and job control is defined as,

* Regardless of ptrace, job control signals control the process-wide stopped state. A ptracee receives and handles job control signals the same as before but no matter what ptrace does the global state isn't affected.

* On ptrace detach, the task is put into the state which matches the process-wide stopped state. If necessary, notification to the real parent is reinstated.

Due to the implicit sending of SIGSTOP on PTRACE_ATTACH, ptrace attach/detach are still not transparent w.r.t. job control but these changes lay the base for (almost) transparent ptracing.

The branch is on top of 2.6.37-rc4 and available in the following git branch,

git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git ptrace

and contains the following changes.

 fs/exec.c                 |    1 
 include/linux/sched.h     |   14 +
 include/linux/tracehook.h |   27 ---
 kernel/ptrace.c           |  139 ++++++++++++++----
 kernel/signal.c           |  345 +++++++++++++++++++++++++++++++++++-----------
 5 files changed, 391 insertions(+), 135 deletions(-)

Thanks.

--
tejun

[L] http://thread.gmane.org/gmane.linux.kernel/1068420

After a task receives SIGCONT, its parent is notified via SIGCHLD with its siginfo describing what the notified event is. If SIGCONT is received while the child process is stopped, the code should be CLD_CONTINUED. If SIGCONT is recieved while the child process is in the process of being stopped, it should be CLD_STOPPED. Which code to use is determined in prepare_signal() and recorded in signal->flags using SIGNAL_CLD_CONTINUED|STOP flags.

get_signal_deliver() should test these flags and then notify accoringly; however, it incorrectly tested SIGNAL_STOP_CONTINUED instead of SIGNAL_CLD_CONTINUED, thus incorrectly notifying CLD_CONTINUED if the signal is delivered before the task is wait(2)ed and CLD_STOPPED if the state was fetched already.

Fix it by testing SIGNAL_CLD_CONTINUED. While at it, uncompress the ?: test into if/else clause for better readability.

---
 kernel/signal.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

index 4e3cff1..fe004b5 100644
@@ -1853,8 +1853,13 @@ relock:
 	 * the CLD_ si_code into SIGNAL_CLD_MASK bits.
 	 */
 	if (unlikely(signal->flags & SIGNAL_CLD_MASK)) {
-		int why = (signal->flags & SIGNAL_STOP_CONTINUED)
-				? CLD_CONTINUED : CLD_STOPPED;
+		int why;
+
+		if (signal->flags & SIGNAL_CLD_CONTINUED)
+			why = CLD_CONTINUED;
+		else
+			why = CLD_STOPPED;
+
 		signal->flags &= ~SIGNAL_CLD_MASK;
 
 		why = tracehook_notify_jctl(why, CLD_CONTINUED);

1.7.1

CLD_CONTINUED notification code calls do_notify_parent_cldstop() with its group_leader; however, do_notify_parent_cldstop() already uses the group_leader for non-ptraced notifications. The duplicate ->group_leader dereferencing is unnecessary and leads to incorrectly notifying the group_leader's ptracer of CLD_CONTINUED from a different task in the group. Fix it.

---
 kernel/signal.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index fe004b5..0411131 100644
@@ -1867,7 +1867,7 @@ relock:
 
 		if (why) {
 			read_lock(&tasklist_lock);
-			do_notify_parent_cldstop(current->group_leader, why);
+			do_notify_parent_cldstop(current, why);
 			read_unlock(&tasklist_lock);
 		}
 		goto relock;

1.7.1

Yes,

I do not really agree this is wrong, group_leader was used intentionally for ptrace case.

There is no "correct" thread who should report CLD_CONTINUED, a random thread wins and notifies its ->real_parent or debugger. If we always choose ->group_leader, we always knows what happens. With this patch we can't predict where does this notification go.

Hello, Oleg.

Ah, okay, I thought it was an unintentional bug. The reason why the change is necessary is because of later changes which re-instates the notification on ptrace detach. The code is modified such that notification pending is cleared iff the notification is delivered to an actual parent. The double dereference makes it difficult to tell whether the notification is beling delivered to an actual parent or not.

If this wasn't a bug, I think the proper thing to do is to move this later where the change is necessary, after the code learns how to buffer the notification until ptrace detach and needs to determine whether it's being eaten by the debugger or not.

Thanks.

tejun

do_signal_stop() is used only by get_signal_to_deliver() and after a successful signal stop, it always calls try_to_freeze(), so the try_to_freeze() loop around schedule() in do_signal_stop() is superflous and confusing. Remove it.

---
 kernel/signal.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

index 0411131..7211c9f 100644
@@ -1781,9 +1781,7 @@ static int do_signal_stop(int signr)
 	}
 
 	/* Now we don't run again until woken by SIGCONT or SIGKILL */
-	do {
-		schedule();
-	} while (try_to_freeze());
+	schedule();
 
 	tracehook_finish_jctl();
 	current->exit_code = 0;

1.7.1

I think the patch is obvioulsy fine.

But I am a bit confused, a couple of off-topic questions to Rafael.

tracehook_notify_jctl() aids in determining whether and what to report to the parent when a task is stopped or continued. The function also adds an extra requirement that siglock may be released across it, which is currently unused and quite difficult to satisfy in well-defined manner.

As job control and the notifications are about to receive major overhaul, remove the tracehook and open code it. If ever necessary, let's factor it out after the overhaul.

---
 include/linux/tracehook.h |   27 ---------------------------
 kernel/signal.c           |   37 ++++++++++++++++---------------------
 2 files changed, 16 insertions(+), 48 deletions(-)

index 3a2e66d..b073f3c 100644
@@ -469,33 +469,6 @@ static inline int tracehook_get_signal(struct task_struct *task,
 }
 
 /**
- * tracehook_notify_jctl - report about job control stop/continue
- * @notify:		zero, %CLD_STOPPED or %CLD_CONTINUED
- * @why:		%CLD_STOPPED or %CLD_CONTINUED
- *
- * This is called when we might call do_notify_parent_cldstop().
- *
- * @notify is zero if we would not ordinarily send a %SIGCHLD,
- * or is the %CLD_STOPPED or %CLD_CONTINUED .si_code for %SIGCHLD.
- *
- * @why is %CLD_STOPPED when about to stop for job control;
- * we are already in %TASK_STOPPED state, about to call schedule().
- * It might also be that we have just exited (check %PF_EXITING),
- * but need to report that a group-wide stop is complete.
- *
- * @why is %CLD_CONTINUED when waking up after job control stop and
- * ready to make a delayed @notify report.
- *
- * Return the %CLD_* value for %SIGCHLD, or zero to generate no signal.
- *
- * Called with the siglock held.
- */
-static inline int tracehook_notify_jctl(int notify, int why)
-{
-	return notify ?: (current->ptrace & PT_PTRACED) ? why : 0;
-}
-
-/**
  * tracehook_finish_jctl - report about return from job control stop
  *
  * This is called by do_signal_stop() after wakeup.

index 7211c9f..5c334ec 100644
@@ -1727,7 +1727,7 @@ void ptrace_notify(int exit_code)
 static int do_signal_stop(int signr)
 {
 	struct signal_struct *sig = current->signal;
-	int notify;
+	int notify = 0;
 
 	if (!sig->group_stop_count) {
 		struct task_struct *t;

@@ -1759,19 +1759,16 @@ static int do_signal_stop(int signr)
 	 * a group stop in progress and we are the last to stop, report
 	 * to the parent.  When ptraced, every thread reports itself.
 	 */
-	notify = sig->group_stop_count == 1 ? CLD_STOPPED : 0;
-	notify = tracehook_notify_jctl(notify, CLD_STOPPED);
-	/*
-	 * tracehook_notify_jctl() can drop and reacquire siglock, so
-	 * we keep ->group_stop_count != 0 before the call. If SIGCONT
-	 * or SIGKILL comes in between ->group_stop_count == 0.
-	 */
-	if (sig->group_stop_count) {
-		if (!--sig->group_stop_count)
-			sig->flags = SIGNAL_STOP_STOPPED;
-		current->exit_code = sig->group_exit_code;
-		__set_current_state(TASK_STOPPED);
+	if (!--sig->group_stop_count) {
+		sig->flags = SIGNAL_STOP_STOPPED;
+		notify = CLD_STOPPED;
 	}
+	if (task_ptrace(current))
+		notify = CLD_STOPPED;
+
+	current->exit_code = sig->group_exit_code;
+	__set_current_state(TASK_STOPPED);
+
 	spin_unlock_irq(&current->sighand->siglock);
 
 	if (notify) {

@@ -1853,21 +1850,19 @@ relock:
 	if (unlikely(signal->flags & SIGNAL_CLD_MASK)) {
 		int why;
 
-		if (signal->flags & SIGNAL_CLD_CONTINUED)
+		if (task_ptrace(current) ||
+		    (signal->flags & SIGNAL_CLD_CONTINUED))
 			why = CLD_CONTINUED;
 		else
 			why = CLD_STOPPED;
 
 		signal->flags &= ~SIGNAL_CLD_MASK;
 
-		why = tracehook_notify_jctl(why, CLD_CONTINUED);
 		spin_unlock_irq(&sighand->siglock);
 
-		if (why) {
-			read_lock(&tasklist_lock);
-			do_notify_parent_cldstop(current, why);
-			read_unlock(&tasklist_lock);
-		}
+		read_lock(&tasklist_lock);
+		do_notify_parent_cldstop(current, why);
+		read_unlock(&tasklist_lock);
 		goto relock;
 	}
 

@@ -2034,7 +2029,7 @@ void exit_signals(struct task_struct *tsk)
 	if (unlikely(tsk->signal->group_stop_count) &&
 			!--tsk->signal->group_stop_count) {
 		tsk->signal->flags = SIGNAL_STOP_STOPPED;
-		group_stop = tracehook_notify_jctl(CLD_STOPPED, CLD_STOPPED);
+		group_stop = CLD_STOPPED;
 	}
 out:
 	spin_unlock_irq(&tsk->sighand->siglock);

1.7.1

OK. I agree, tracehook_notify_jctl() looks very unobvious, especially because it is not really used currently.

The patch looks correct, except

That's the logic from tracehook_notify_jctl() or I think it is incorrectly. Yes, the latter. I got confused the two parameters. I thought tracehook_notify_jctl() always returned CLD_CONTINUED when traced. The @why is @notified and CLD_CONTINUED is @why. :-)

I'll drop the above chunk. Thanks.

tejun

To prepare for cleanup of the interaction between group stop and ptrace, add @why to ptrace_stop(). Existing users are updateda such that there is no behavior change.

---
 kernel/signal.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

index 5c334ec..2964642 100644
@@ -1617,7 +1617,7 @@ static int sigkill_pending(struct task_struct *tsk)
  * If we actually decide not to stop at all because the tracer
  * is gone, we keep current->exit_code unless clear_code.
  */
-static void ptrace_stop(int exit_code, int clear_code, siginfo_t *info)
+static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
 	__releases(&current->sighand->siglock)
 	__acquires(&current->sighand->siglock)
 {

@@ -1655,7 +1655,7 @@ static void ptrace_stop(int exit_code, int clear_code, siginfo_t *info)
 	spin_unlock_irq(&current->sighand->siglock);
 	read_lock(&tasklist_lock);
 	if (may_ptrace_stop()) {
-		do_notify_parent_cldstop(current, CLD_TRAPPED);
+		do_notify_parent_cldstop(current, why);
 		/*
 		 * Don't want to allow preemption here, because
 		 * sys_ptrace() needs this task to be inactive.

@@ -1714,7 +1714,7 @@ void ptrace_notify(int exit_code)
 
 	/* Let the debugger run.  */
 	spin_lock_irq(&current->sighand->siglock);
-	ptrace_stop(exit_code, 1, &info);
+	ptrace_stop(exit_code, CLD_TRAPPED, 1, &info);
 	spin_unlock_irq(&current->sighand->siglock);
 }
 

@@ -1795,7 +1795,7 @@ static int ptrace_signal(int signr, siginfo_t *info,
 	ptrace_signal_deliver(regs, cookie);
 
 	/* Let the debugger run.  */
-	ptrace_stop(signr, 0, info);
+	ptrace_stop(signr, CLD_TRAPPED, 0, info);
 
 	/* We're back.  Did the debugger cancel the sig?  */
 	signr = current->exit_code;

1.7.1

task->signal->group_stop_count is used to tracke the progress of group stop. It's initialized to the number of tasks which need to stop for group stop to finish and each stopping or trapping task decrements. However, each task doesn't keep track of whether it decremented the counter or not and if woken up before the group stop is complete and stops again, it can decrement the counter multiple times.

Please consider the following example code.

static void *worker(void *arg)

 {
	 while (1) ;
	 return NULL;

}

int main(void)

 {
	 pthread_t thread;
	 pid_t pid;
	 int i;

	 pid = fork();
	 if (!pid) {
		 for (i = 0; i < 5; i++)
			 pthread_create(&thread, NULL, worker, NULL);
		 while (1) ;
		 return 0;
	 }

	 ptrace(PTRACE_ATTACH, pid, NULL, NULL);
	 while (1) {
		 waitid(P_PID, pid, NULL, WSTOPPED);
		 ptrace(PTRACE_SINGLESTEP, pid, NULL, (void *)(long)SIGSTOP);
	 }
	 return 0;

}

The child creates five threads and the parent continuously traps the first thread and whenever the child gets a signal, SIGSTOP is delivered. If an external process sends SIGSTOP to the child, all other threads in the process should reliably stop. However, due to the above bug, the first thread will often end up consuming group_stop_count multiple times and SIGSTOP often ends up stopping none or part of the other four threads.

This patch adds a new field task->group_stop which is protected by siglock and uses GROUP_STOP_CONSUME flag to track which task is still to consume group_stop_count to fix this bug.

task_clear_group_stop() and task_participate_group_stop() are added to help manipulating group stop states. As ptrace_stop() now also uses task_participate_group_stop(), it will set SIGNAL_STOP_STOPPED if it completes a group stop.

There still are many issues regarding the interaction between group stop and ptrace. Patches to address them will follow.

---
 include/linux/sched.h |    6 ++++
 kernel/signal.c       |   64 ++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 62 insertions(+), 8 deletions(-)

index 2c79e92..93157a4 100644
@@ -1245,6 +1245,7 @@ struct task_struct {
 	int exit_state;
 	int exit_code, exit_signal;
 	int pdeath_signal;  /*  The signal sent when the parent dies  */
+	unsigned int group_stop;	/* GROUP_STOP_*, siglock protected */
 	/* ??? */
 	unsigned int personality;
 	unsigned did_exec:1;

@@ -1756,6 +1757,11 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *
 #define tsk_used_math(p) ((p)->flags & PF_USED_MATH)
 #define used_math() tsk_used_math(current)
 
+/*
+ * task->group_stop flags
+ */
+#define GROUP_STOP_CONSUME	(1 << 17) /* consume group stop count */
+
 #ifdef CONFIG_PREEMPT_RCU
 
 #define RCU_READ_UNLOCK_BLOCKED (1 << 0) /* blocked while in RCU read-side. */

1.7.1

Thanks. Dropped.

tejun

Currently task->signal->group_stop_count is used to decide whether to stop for group stop. However, if there is a task in the group which is taking a long time to stop, other tasks which are continued by ptrace would repeatedly stop for the same group stop until the group stop is complete.

Conversely, if a ptraced task is in TASK_TRACED state, the debugger won't get notified of group stops which is inconsistent compared to the ptraced task in any other state.

This patch introduces GROUP_STOP_PENDING which tracks whether a task is yet to stop for the group stop in progress. The flag is set when a group stop starts and cleared when the task stops the first time for the group stop, and consulted whenever whether the task should participate in a group stop needs to be determined. Note that now tasks in TASK_TRACED also participate in group stop.

This results in the following behavior changes.

* For a single group stop, a ptracer would see at most one stop reported.

* A ptracee in TASK_TRACED now also participates in group stop and the tracer would get the notification. However, as a ptraced task could be in TASK_STOPPED state or any ptrace trap could consume group stop, the notification may still be missing. These will be addressed with further patches.

* A ptracee may start a group stop while one is still in progress if the tracer let it continue with stop signal delivery. Group stop code handles this correctly.

Oleg:

* Spotted that a task might skip signal check even when its GROUP_STOP_PENDING is set. Fixed by updating recalc_sigpending_tsk() to check GROUP_STOP_PENDING instead of group_stop_count.

* Pointed out that task->group_stop should be cleared whenever task->signal->group_stop_count is cleared. Fixed accordingly.

* Pointed out the behavior inconsistency between TASK_TRACED and RUNNING and the last behavior change.

---
 fs/exec.c             |    1 +
 include/linux/sched.h |    3 +++
 kernel/signal.c       |   36 +++++++++++++++++++++---------------
 3 files changed, 25 insertions(+), 15 deletions(-)

index d68c378..8d4142c 100644
@@ -1648,6 +1648,7 @@ static int zap_process(struct task_struct *start, int exit_code)
 
 	t = start;
 	do {
+		task_clear_group_stop(t);
 		if (t != current && t->mm) {
 			sigaddset(&t->pending.signal, SIGKILL);
 			signal_wake_up(t, 1);

index 93157a4..c2538dd 100644
@@ -1760,8 +1760,11 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *
 /*
  * task->group_stop flags
  */
+#define GROUP_STOP_PENDING	(1 << 16) /* task should stop for group stop */
 #define GROUP_STOP_CONSUME	(1 << 17) /* consume group stop count */
 
+extern void task_clear_group_stop(struct task_struct *task);
+
 #ifdef CONFIG_PREEMPT_RCU
 
 #define RCU_READ_UNLOCK_BLOCKED (1 << 0) /* blocked while in RCU read-side. */

index bac959e..bff46ed 100644
@@ -124,7 +124,7 @@ static inline int has_pending_signals(sigset_t *signal, sigset_t *blocked)
 
 static int recalc_sigpending_tsk(struct task_struct *t)
 {
-	if (t->signal->group_stop_count > 0 ||
+	if ((t->group_stop & GROUP_STOP_PENDING) ||
 	    PENDING(&t->pending, &t->blocked) ||
 	    PENDING(&t->signal->shared_pending, &t->blocked)) {
 		set_tsk_thread_flag(t, TIF_SIGPENDING);

@@ -232,19 +232,19 @@ static inline void print_dropped_signal(int sig)
  * CONTEXT:
  * Must be called with @task->sighand->siglock held.
  */
-static void task_clear_group_stop(struct task_struct *task)
+void task_clear_group_stop(struct task_struct *task)
 {
-	task->group_stop &= ~GROUP_STOP_CONSUME;
+	task->group_stop &= ~(GROUP_STOP_PENDING | GROUP_STOP_CONSUME);
 }
 
 /**
  * task_participate_group_stop - participate in a group stop
  * @task: task participating in a group stop
  *
- * @task is participating in a group stop.  Group stop states are cleared
- * and the group stop count is consumed if %GROUP_STOP_CONSUME was set.  If
- * the consumption completes the group stop, the appropriate %SIGNAL_*
- * flags are set.
+ * @task has GROUP_STOP_PENDING set and is participating in a group stop.
+ * Group stop states are cleared and the group stop count is consumed if
+ * %GROUP_STOP_CONSUME was set.  If the consumption completes the group
+ * stop, the appropriate %SIGNAL_* flags are set.
  *
  * CONTEXT:
  * Must be called with @task->sighand->siglock held.

@@ -254,6 +254,8 @@ static bool task_participate_group_stop(struct task_struct *task)
 	struct signal_struct *sig = task->signal;
 	bool consume = task->group_stop & GROUP_STOP_CONSUME;
 
+	WARN_ON_ONCE(!(task->group_stop & GROUP_STOP_PENDING));
+
 	task_clear_group_stop(task);
 
 	if (!consume)

@@ -767,6 +769,9 @@ static int prepare_signal(int sig, struct task_struct *p, int from_ancestor_ns)
 		t = p;
 		do {
 			unsigned int state;
+
+			task_clear_group_stop(t);
+
 			rm_from_queue(SIG_KERNEL_STOP_MASK, &t->pending);
 			/*
 			 * If there is a handler for SIGCONT, we must make

@@ -908,6 +913,7 @@ static void complete_signal(int sig, struct task_struct *p, int group)
 			signal->group_stop_count = 0;
 			t = p;
 			do {
+				task_clear_group_stop(t);
 				sigaddset(&t->pending.signal, SIGKILL);
 				signal_wake_up(t, 1);
 			} while_each_thread(p, t);

@@ -1141,6 +1147,7 @@ int zap_other_threads(struct task_struct *p)
 	p->signal->group_stop_count = 0;
 
 	while_each_thread(p, t) {
+		task_clear_group_stop(t);
 		count++;
 
 		/* Don't bother with already dead threads */

@@ -1692,7 +1699,7 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
 	 * If there is a group stop in progress,
 	 * we must participate in the bookkeeping.
 	 */
-	if (current->signal->group_stop_count > 0)
+	if (current->group_stop & GROUP_STOP_PENDING)
 		task_participate_group_stop(current);
 
 	current->last_siginfo = info;

@@ -1777,8 +1784,8 @@ static int do_signal_stop(int signr)
 	struct signal_struct *sig = current->signal;
 	int notify = 0;
 
-	if (!sig->group_stop_count) {
-		unsigned int gstop = GROUP_STOP_CONSUME;
+	if (!(current->group_stop & GROUP_STOP_PENDING)) {
+		unsigned int gstop = GROUP_STOP_PENDING | GROUP_STOP_CONSUME;
 		struct task_struct *t;
 
 		if (!likely(sig->flags & SIGNAL_STOP_DEQUEUED) ||

@@ -1798,8 +1805,7 @@ static int do_signal_stop(int signr)
 			 * stop is always done with the siglock held,
 			 * so this check has no races.
 			 */
-			if (!(t->flags & PF_EXITING) &&
-			    !task_is_stopped_or_traced(t)) {
+			if (!(t->flags & PF_EXITING) && !task_is_stopped(t)) {
 				t->group_stop = gstop;
 				sig->group_stop_count++;
 				signal_wake_up(t, 0);

@@ -1929,8 +1935,8 @@ relock:
 		if (unlikely(signr != 0))
 			ka = return_ka;
 		else {
-			if (unlikely(signal->group_stop_count > 0) &&
-			    do_signal_stop(0))
+			if (unlikely(current->group_stop &
+				     GROUP_STOP_PENDING) && do_signal_stop(0))
 				goto relock;
 
 			signr = dequeue_signal(current, &current->blocked,

@@ -2076,7 +2082,7 @@ void exit_signals(struct task_struct *tsk)
 		if (!signal_pending(t) && !(t->flags & PF_EXITING))
 			recalc_sigpending_and_wake(t);
 
-	if (unlikely(tsk->signal->group_stop_count) &&
+	if (unlikely(tsk->group_stop & GROUP_STOP_PENDING) &&
 	    task_participate_group_stop(tsk))
 		group_stop = CLD_STOPPED;
 out:

1.7.1

Currently, ptrace_stop() unconditionally participates in group stop bookkeeping. This is unnecessary and inaccurate. Make it only participate if the task is trapping for group stop - ie. if @why is CLD_STOPPED. As ptrace_stop() currently is not used when trapping for group stop, this equals to disabling group stop participation from ptrace_stop().

A visible behavior change is increased likelihood of delayed group stop completion if the thread group contains one or more ptraced tasks.

This is to preapre for further cleanup of the interaction between group stop and ptrace.

---
 kernel/signal.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

index bff46ed..faf218b 100644
@@ -1696,10 +1696,13 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
 	}
 
 	/*
-	 * If there is a group stop in progress,
-	 * we must participate in the bookkeeping.
+	 * If @why is CLD_STOPPED, we're trapping to participate in a group
+	 * stop.  Do the bookkeeping.  Note that if SIGCONT was delievered
+	 * while siglock was released for the arch hook, PENDING could be
+	 * clear now.  We act as if SIGCONT is received after TASK_TRACED
+	 * is entered - ignore it.
 	 */
-	if (current->group_stop & GROUP_STOP_PENDING)
+	if (why == CLD_STOPPED && (current->group_stop & GROUP_STOP_PENDING))
 		task_participate_group_stop(current);
 
 	current->last_siginfo = info;

1.7.1

A ptraced task would still stop at do_signal_stop() when it's stopping for stop signals and do_signal_stop() behaves the same whether the task is ptraced or not. However, in addition to stopping, ptrace_stop() also does ptrace specific stuff like calling architecture specific callbacks, so this behavior makes the code more fragile and difficult to understand.

This patch makes do_signal_stop() test whether the task is ptraced and use ptrace_stop() if so. This renders tracehook_notify_jctl() rather pointless as the ptrace notification is now handled by ptrace_stop() regardless of the return value from the tracehook. It probably is a good idea to update it.

This doesn't solve the whole problem as tasks already in stopped state would stay in the regular stop when ptrace attached. That part will be handled by the next patch.

Oleg spotted a minor userland visible change. In some cases, the ptracee's state would now be TASK_TRACED where it used to be TASK_STOPPED, which is visible via fs/proc.

---
 kernel/signal.c |   43 +++++++++++++++++++++++++------------------
 1 files changed, 25 insertions(+), 18 deletions(-)

index faf218b..a6bc4cf 100644
@@ -1785,7 +1785,6 @@ void ptrace_notify(int exit_code)
 static int do_signal_stop(int signr)
 {
 	struct signal_struct *sig = current->signal;
-	int notify = 0;
 
 	if (!(current->group_stop & GROUP_STOP_PENDING)) {
 		unsigned int gstop = GROUP_STOP_PENDING | GROUP_STOP_CONSUME;

@@ -1815,29 +1814,37 @@ static int do_signal_stop(int signr)
 			} else
 				task_clear_group_stop(t);
 	}
-	/*
-	 * If there are no other threads in the group, or if there is
-	 * a group stop in progress and we are the last to stop, report
-	 * to the parent.  When ptraced, every thread reports itself.
-	 */
-	if (task_participate_group_stop(current))
-		notify = CLD_STOPPED;
-	if (task_ptrace(current))
-		notify = CLD_STOPPED;
 
 	current->exit_code = sig->group_exit_code;
 	__set_current_state(TASK_STOPPED);
 
-	spin_unlock_irq(&current->sighand->siglock);
+	if (likely(!task_ptrace(current))) {
+		int notify = 0;
 
-	if (notify) {
-		read_lock(&tasklist_lock);
-		do_notify_parent_cldstop(current, notify);
-		read_unlock(&tasklist_lock);
-	}
+		/*
+		 * If there are no other threads in the group, or if there
+		 * is a group stop in progress and we are the last to stop,
+		 * report to the parent.
+		 */
+		if (task_participate_group_stop(current))
+			notify = CLD_STOPPED;
 
-	/* Now we don't run again until woken by SIGCONT or SIGKILL */
-	schedule();
+		spin_unlock_irq(&current->sighand->siglock);
+
+		if (notify) {
+			read_lock(&tasklist_lock);
+			do_notify_parent_cldstop(current, notify);
+			read_unlock(&tasklist_lock);
+		}
+
+		/* Now we don't run again until woken by SIGCONT or SIGKILL */
+		schedule();
+
+		spin_lock_irq(&current->sighand->siglock);
+	} else
+		ptrace_stop(current->exit_code, CLD_STOPPED, 0, NULL);
+
+	spin_unlock_irq(&current->sighand->siglock);
 
 	tracehook_finish_jctl();
 	current->exit_code = 0;

1.7.1

Jan, Roland, this change needs your review.

As it was already discussed, this is the user-visible change, and I am starting to worry we can underestimate it.

Again, I am not saying this can break something, I simply do not know. However, I think there is something non-consistent in the new behaviour, please see below.

In short: suppose that the tracee recieves a signal, reports it to debugger, and the debugger does ptrace(PTRACE_CONT, pid, SIGSTOP).

Before the patch the tracee sleeps in TASK_STOPPED, after the patch it becomes TASK_TRACED.

I missed this part of the changelog. "visible via fs/proc" is not the only change. Another change is how the tracee reacts to SIGCONT after that. With this patch it can't wake the tracee up.

Consider the simplest example. The tracee is single-threaded and debugger == parent. Something like

	int main(void)
	{
		int child, status;

		child = fork();
		if (!child) {
			ptrace(PTRACE_TRACEME);

			kill(getpid(), SIGSTOP);

			return 0;
		}

		wait(&status)
		// the tracee reports the signal
		assert(WIFSTOPPED() && WSTOPSIG() == SIGSTOP);
		// it should stop after that
		ptrace(PTRACE_CONT, child, SIGSTOP);

		wait(&status);
		// now it is stopped
		assert(WIFSTOPPED() && WSTOPSIG() == SIGSTOP);

		kill(child, SIGCONT);

		wait(&status);
		assert(WIFSTOPPED() && WSTOPSIG() == SIGCONT);

This won't work with this patch. the last do_wait() will hang forever. Probably this is fine, I do not know. Please take a look and ack/nack explicitly.

However. There is something I missed previously, and this small detail doesn't look good to me: the behaviour of SIGCONT becomes a bit unpredictable. Suppose it races with do_signal_stop() and clears GROUP_STOP_PENDING or SIGNAL_STOP_DEQUEUED before, in this case in can "wakeup" the tracee.

IOW. Let's remove the 2nd wait() in the code above, the parent does

		wait(&status)
		// the tracee reports the signal
		assert(WIFSTOPPED() && WSTOPSIG() == SIGSTOP);
		// it should stop after that
		ptrace(PTRACE_CONT, child, SIGSTOP);

		kill(child, SIGCONT);

Now we can't know id this SIGCONT works or not. If the tracee is already parked in ptrace_stop() - it doesn't. If the parent wins - the tracee doesn't stop.

OTOH. Looking at this patch, I can no longer understand why ptrace_check_attach() can silently do s/STOPPED/TRACED/. Indeed, as Tejun pointed out, if ptrace_stop() needs arch_ptrace_stop(), then ptrace_check_attach() should be arch-friendly as well.

So, the patch looks like the bugfix, but I do not understand this ia64/sparc magic and thus I do not know how important this fix. Nobody complained so far, though.

Roland, could you comment this part?

Oleg.

Hello,

Yes, before the change, the task would respond to SIGCONT before the first ptrace request succeeds after attach. To me, this doesn't seem to be anything intentional tho. It seems a lot of ptrace and group stop interactions is in the grey area with only the current (quirky, I'm afraid) behavior drawing almost arbitrary lines across different behaviors.

We can try to preserve all those behaviors but I don't think that will be very beneficial. I think the best way to proceed would be identifying the sensible and depended upon assumptions and then draw clear lines from there stating what's guaranteed and what's undefined. We'll have to keep some of quirkiness for sure.

Anyways, pondering and verifying all the possibly visible changes definitely is necessary, but that said, we fortunately have rather limited number of ptrace users and their usages don't seem to be too wild (at least on my cursory investigation), so I think it to be doable without breaking anything noticeably. But yeap we definitely need to be careful.

We can change ptrace_stop() to use TASK_STOPPED if it's stopping for group stop to preserve the original behavior but if it doesn't disturb current users (and I doubt it would), I think it would be far cleaner to state that the behavior is undefined. The current behavior - it works if there hasn't been whichever ptrace operation inbetween - is quite unexpected anyway, IMHO.

And, for longer term, I think it would be a good idea to separate group stop and ptrace trap mechanisms, so that ptrace trap works properly on per-task level and properly transparent from group stop handling. The intertwining between the two across different domains of threads inhfferently involves a lot of grey areas where there is no good intuitive behavior.

IIUC, it dumps the register window to userland memory. ia64 has this stacked windows of registers which gets wound up and unwound according to function calls and those need to be dumped to userland memory so that the debugger can PEEK and POKE them. Not really sure why skipping it didn't cause any problem until now tho.

Thanks.

tejun

Not exactly. But perhaps you meant that even without this change, any ptrace() request after ptrace(PTRACE_CONT, SIGSTOP) will change child->state = TASK_TRACED, and kill(SIGCONT) won't work after that.

Agreed.

However. Strangely, I didn't think about this before. With this change, it is not possible to trace/debug the application so that it can properly react to SIGCONT. Yes, currently we have a lot more problems here, including do_wait, so probably this doesn't matter.

Still I'd like to know what Jan and Roland think. I am paranoid, yes ;)

Yes, at least I think it makes sense to document this change in the changelog. This can simplify the life if we have a bug report blaiming this patch.

Agreed.

Yes, that was my question.

Oleg.

Yeap, exactly.

Definitely.

Hmm... the mention of SIGCONT behavior reminds me of your mention of notification behavior on the other message. I might write a more detailed reply there but, anyways, I don't think we'll be able to find one good behavior with the current set of operations. If we hide the group stop handling and notification from the ptracer, it would allow, for example, transparent strace, but it will at the same time restrict what a debugger can do behind the tracee's back in a regressive way (ie. the current users would notice).

I think it would be better to introduce a few new ptrace operations and give them more control in clearly defined and hopefully intutive way. For example, a ptracer is notified when a task is stopping for group stop and it should be able to decide and tell the kernel whether to participate in the group stop or not. For SIGCONT, likewise, the kernel can notify all ptracers in the group and can notify the real parent if all ptracers decide to participate in the continuation.

That way, debuggers can still work behind the real parent's back (they often want to) while audit type tools can sniff transparently. I think the trickier part would be adapting the existing operations so that they have clearly defined (mostly) compatible behavior while not hindering too much with addition of new capabilities.

Thanks.

tejun

I am not immediately comfortable with this. What really matters is the userland-visible behavior (ptrace, wait, and signals). The /proc/pid/status difference of "T (stopped)" vs "T (traced)" is technically userland-visible, but I don't think that really matters to userland.

This seems like a problem to me, though there are caveats to that. My rationale has always been that there should be some way to get into normal job-control stop state, both meaning that group-stop happens normally and that SIGCONT works normally.

However, it is not entirely clear how useful that is to userland given the rest of ptrace behavior as it stands. That is, the real parent can't see the job-control stop anyway because its wait reports are preempted by ptrace, so what is it for?

It would be more clear if we had a cleaner interface available in which tracing did not prevent normal job-control stops from being reported to the real parent. I'm not really sure how much we should try to change this stuff before adding something like that.

This sounds to me exactly like the normal race between a SIGSTOP and a SIGCONT when ptrace is not involved. It's supposed to be that way, i.e. generating SIGCONT clears all stop signals whether pending or in the midst of delivery or already fully delivered. So, if the meaning of PTRACE_CONT,SIGSTOP is to behave like a normal SIGSTOP delivery, then that is what's expected.

The purpose of that is so that when ptrace is in use, SIGCONT can't resume the tracee. If the tracee was in TASK_STOPPED either because it was already in job-control stop before PTRACE_ATTACH, or because PTRACE_CONT,SIGSTOP put it there, then we need it to be in TASK_TRACED while ptrace does its work in case an external SIGCONT comes along.

Yes, perhaps so. It's certainly the case that the lack of complaint from ia64/sparc users doesn't mean it's correct now, just that this case is subtle, difficult to notice in practice, and subject to other external conditions (other load causing flushes or not, etc.).

But the way the code is structured, arch_ptrace_stop can only be used on current, meaning it requires waking it up from TASK_STOPPED long enough to run the ptrace_stop logic. IMHO it would be wrong to introduce a new window visible to userland wherein a process previously in job-control stop is seen to be running before it gets into its ptrace stop. It might not matter if /proc/pid/status can be read to see "R (running)" for such a window. But certainly it should not have CLD_CONTINUED behavior (SIGCHLD and waitid visibility), nor report a CLD_TRAPPED stop via SIGCHLD or wait after it was already stopped.

[tj:]

That is largely true. But userland programs like gdb and strace have gone to lots of contortions to work with the behavior as it is, and we should not break what works in practice for them.

I agree with this overall. But we must consider whatever weirdness comes up in ptrace.

There are more ptrace users than you might think, though indeed there are few of them that are widely-known and free code (strace and gdb). Most every ptrace user out there has worked with the manifest behavior seen on existing kernels rather than trying to understand any sound principles of behavior, because it's been to hard to figure out.

I quite agree. The best way to fix this is with sane semantics for normal job-control operations when traced. But ptrace has never had that before and it's just not possible both to be sane and to match the expected behavior of the traditional ptrace semantics. Hence I lean towards leaving things as much as possible as they are today when only the ptrace operations we have today are used.

It's not necessarily that it didn't cause any problem. It's more likely that it's hard to notice, because the problems are so arcane. It's also entirely possible that the remaining problematic cases just happen to be vanishingly rare because of other factors--e.g., between when something entered job-control stop and when ptrace was used, the system is likely to have had enough other pressure on its magical register caches that they got flushed anyway. Moreover, the current method with arch_ptrace_stop was only added relatively recently--on the time scale for the actual users of these machines (in 2.6.28 for sparc, in 2.6.25 for ia64). Before that, ia64's PTRACE_PEEKDATA et al used terrible arch-specific kludges to read the unflushed register data rather than the real user memory (I don't know if sparc had something similar).

I certainly agree that we should have some new operations or modes of tracing and have them behave sanely. I don't think the details you've described here are exactly what would make sense, however.

IMHO there should be no discretion about whether a thread participates in a group stop. If a group stop is happening, every thread must be stopped ASAP and the debugger should not be able to interfere with that. (The time for the debugger to interfere is when it gets the chance to intercept the stop signal before it's delivered.) If a thread is stop for tracing when a group stop happens, then the right way to think about its state is "both stopped for tracing and stopped for job-control". When the tracer says it can resume, it goes to simply "stopped for job-control". Likewise, if before that a SIGCONT is generated, then it goes to simply "stopped for tracing" and does not prevent the normal CLD_CONTINUED notification from happening. That is, to the non-debugger parts of the system, it's no different from if the debugger stopped that thread for tracing immediately after SIGCONT resumed it and before it had a chance to do anything else. (This is exactly the model we tried to implement in utrace.)

I've read over the rest of the long thread from December and will reply further to the later iterations of the patches shortly. I'm sorry if I've replied to stale issues or overlooked questions still needing answers. I hope we can all get caught up to discussing the same things this week.

Thanks,
Roland

Hello, Roland.

I think we're getting a bit off course here. The CONT problem in this context isn't generic at all. In principle, CONT doesn't (immediately) work on a ptraced task. We basically just had this window where STOPPED -> TRACED transition was delayed in which CONT worked. It's more of a behavioral inconsistency than anything else.

If a task was RUNNING when PTRACE_ATTACH happens, when the tracer wait(2) completes, the task is TRACED and won't immediately respond to CONT. If a task was STOPPED, CONT can still wake up between the completion of wait(2) and the first ptrace command on the tracee. It's a subtle inconsistency which I don't believe anyone could have taken advantag of.

As for the generic discussion of group stop vs. ptrace, yeah, there's a lot to discuss and as I already wrote before I don't think there will be a single behavior which would satisfy all the requirements. I think it would be best to iron out the existing inconsistencies in a way which doesn't break the current users and then gradually introduce a few more ptrace operations which have well defined and more flexible interaction with group stop.

As I wrote in another reply, the visibility is masked from the tracer and only visible if the user does a pretty convoluted thing. Unless there's such current user, I think we can clearly define what's guaranteed regarding ptrace/wait interaction and leave cases outside of it as undefined.

Sure, I definitely agree but at the same time that doesn't mean we shouldn't improve ptrace at all. It just means it's delicate and we should proceed carefully, which we shall.

Yeap.

Yeap, definitely. I want to make things just clean enough so that it doesn't hinder introduction of improvements while not breaking the existing users, but things like silent STOPPED -> TRACED transition are outright buggy and have to be fixed one way or the other. We can't ignore them.

There are two extremes. You can put ptrace under group stop or completely the other way around. I think there are valid use cases for both of them. I'll try to summarize it later.

Heh, I was the late one this time. I'll soon repost the refreshed STOPPED <-> TRACED patchset. I think we're mostly in agreement regarding that part.

Thank you.

tejun

I was thinking out loud to share the rationale behind my reactions.

SIGCONT works just fine on a process that is being traced. It only resumes on a process that is in job control stop rather than tracing stop, but then it works normally. The other effects of SIGCONT (to clear all pending stop signals at generation time, and to make a SIGCONT signal pending for later delivery if it's not ignored or default-ignored) always work. It is a further wrinkle that any ptrace operation on a thread that is in job control stop morphs that into tracing stop. But it's just not true to say that "in principle" SIGCONT doesn't work.

I think you underestimate the fiddliness of userland ptrace users.

I agree with that overall sentiment, as I think I said before.

I don't really buy the "masked" claim.

And so we are. I'm just pushing on the degree of caution.

I don't agree with "outright buggy". It's a well-defined situation that has been well understood for quite some time, by possibility as many as three people.

I look forward to that elucidation. I can't really imagine a rationale I'd accept for anything contrary to what I said about the supremacy of group stops.

Mostly. Mostly. ;-)

Thanks,
Roland

Hello, Roland.

I see.

I'm afraid it's not that clear. I wish it were but it simply isn't. There's no clear distinction between group stopped and trapped state as group stopped tasks silently transit to trapped state in racy, non-obvious and buggy (in that it skips arch hooks) manner.

The ambiguity in completion and notification of group stop also contributes to non-deterministic behavior. Before the patchset, a ptrace task would happily report that it participated in group stop triggering group stop notification (which also is buggy and wouldn't reach the parent reliably BTW) even when it's stopping for ptrace trap. SIGCONT may or may not work depending _both_ on where the task has stopped and whether the tracee has received another ptrace call after that or not.

There's nothing even resembling consistency in that direction and it's no wonder userland had so many problems and needed to grow hacks to work around quirky behaviors.

The ironic upper side of all those inconsistencies is that we can flip the behavior to one side and be relatively safe as the userland must have been dealing with non-deterministic behavior until now.

So, I really can't agree that the current behavior regarding group stop is "just fine". It's utterly incoherent. It simply is that the userland learned to cope with it and we can and better make it consistent within the limits of the current inconsistent behavior.

I really don't think "it may work depending on where it stops and whether there's another ptrace call afterwards" can be called working in any principle. It is broken.

Give me one realistic scenario where the above would actually affect userland, or better, an actually used program which breaks due to the above change. I might look like coming off too aggressively but to me it seems like the don't-change-the-current-behavior principle is being taken too far. I do agree that we shouldn't be breaking existing users but at the same time there should be a balance - a trade-off at the right point. Almost all ptrace users follow up PTRACE_ATTACH, wait sequence with another PTRACE request, and the above change would increase behavioral consistency.

If there actually is an user, we can change the behavior such that SIGCONT is followed until the second PTRACE request, which should probably be implemented by making the first ptrace_check_attach() call, instead of PTRACE_ATTACH, put the tracee into TRACED state. I think it should be doable but that behavior is extremely subtle and thus likely to cause more confusion for its users. Its description would go like the following,

PTRACE_ATTACH stops the tracee and the tracer can wait for it with wait(2); however, regardless of wait(2) success, SIGCONT is allowed to wake up the tracee until the next ptrace request and if SIGCONT is received before the next ptrace request, the ptrace request might fail with -ESRCH. Once the next ptrace request has succeeded, further SIGCONT are ignored until the task participates in another group stop which can't be determined reliably by the tracer.

With the proposed patchset, it becomes,

PTRACE_ATTACH stops the tracee and the tracer can wait for it with wait(2). Once attached, whenever wait(2) indicates task stop, it is guaranteed that all ptrace requests don't fail with -ESRCH and the tracee stays stopped until continued or detached by the tracer or killed with SIGKILL.

To me, arguing that the first description is better sounds quite far fetched. If we _absolutely_ have to, well, we'll have to, but let's not mold ourselves into that mess if at all possible.

Awesome. :-)

Hah? It's not a claim that I'm trying to sell out of thin air. It's actually masked. There's no way for the tracer to see it (outside the /proc state, that is). It's properly and completely masked from the tracer.

Yeah, your reviews are much appreciated. Please don't take the heated arguments as attacks. :-)

We'll have to agree to disagree on that. To me, the overall behavior seems extremely inconsistent and filled with implementation details leaking out to userland without clearly defined intentions. As I wrote in another reply, the only upside seems to be that we probably have quite a leeway in changing the behavior thanks to the inconsistencies as userland already has had to deal with them. In many cases, we should be able to choose one mode of operation among the various inconsistent behaviors and stick with it without breaking userland.

But, with or without future use cases, currently group stop is already inferior to ptrace and that is very visible to userland. There is no way we can change that without breaking the current users. It doesn't even fall inside the range of inconsistent behaviors.

Thank you. :-)

tejun

Currently, if the task is STOPPED on ptrace attach, it's left alone and the state is silently changed to TRACED on the next ptrace call. The behavior breaks the assumption that arch_ptrace_stop() is called before any task is poked by ptrace and is ugly in that a task manipulates the state of another task directly.

With GROUP_STOP_PENDING, the transitions between TASK_STOPPED and TRACED can be made clean. The tracer can use the flag to tell the tracee to retry stop on attach and detach. On retry, the tracee will enter the desired state in the correct way. The lower 16bits of task->group_stop is used to remember the signal number which caused the last group stop. This is used while retrying for ptrace attach as the original group_exit_code could have been consumed with wait(2) by then.

As the real parent may wait(2) and consume the group_exit_code anytime, the group_exit_code needs to be saved separately so that it can be used when switching from regular sleep to ptrace_stop(). This is recorded in the lower 16bits of task->group_stop.

If a task is already stopped and there's no intervening SIGCONT, a ptrace request immediately following a successful PTRACE_ATTACH should always succeed even if the tracer doesn't wait(2) for attach completion; however, with this change, the tracee might still be TASK_RUNNING trying to enter TASK_TRACED which would cause the following request to fail with -ESRCH.

This intermediate state is hidden from userland by setting GROUP_STOP_TRAPPING on attach and making ptrace_check_attach() wait for it to clear. Completing the transition or any event which clears the group stop states of the task clears the bit and wakes up the ptracer if waiting.

Oleg:

* Spotted a race condition where a task may retry group stop without proper bookkeeping. Fixed by redoing bookkeeping on retry.

* Pointed out the userland visible intermediate state. Fixed with GROUP_STOP_TRAPPING.

---
 include/linux/sched.h |    2 +
 kernel/ptrace.c       |   63 ++++++++++++++++++++++++++++++++++++++++++------
 kernel/signal.c       |   62 ++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 112 insertions(+), 15 deletions(-)

index c2538dd..7045c34 100644
@@ -1760,8 +1760,10 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *
 /*
  * task->group_stop flags
  */
+#define GROUP_STOP_SIGMASK	0xffff    /* signr of the last group stop */
 #define GROUP_STOP_PENDING	(1 << 16) /* task should stop for group stop */
 #define GROUP_STOP_CONSUME	(1 << 17) /* consume group stop count */
+#define GROUP_STOP_TRAPPING	(1 << 18) /* switching from STOPPED to TRACED */
 
 extern void task_clear_group_stop(struct task_struct *task);
 

1.7.1

Oops, left over from a previous waitqueue based implementation.

Hmmmm, I actually think that would be cleaner. I just didn't know it was there. Will convert over to it.

Sure thing.

Updated.

Ah, thanks for spotting it. I missed that. We should be able to convert it to call ptrace_stop(), right?

I see. I can move the transition wait logic into PTRACE_ATTACH. Would that be good enough?

This is also related to how to wait for attach completion for a new more transparent attach. Would it be better for such a request to make sure the operation to complete before returning or is it preferable to keep using wait(2) for that? We'll probably be able to share the transition wait logic with it. I think it would be better to return after the attach is actually complete but is there any reason that I'm missing which makes using wait(2) preferrable?

I'm feeling a bit too dense to process the above right now. I'll respond to the above next morning after a strong cup of coffee. :-)

The GROUP_STOP_SIGMASK part of t->group_stop is supposed to track the signr of the latest stop attempt. Hmmm... but yeah, not changing the value there would result in more consistent behavior. Updating...

I see. Removed.

Hmmm... and dropping current->exit_code clearing from the do_signal_stop(), right? I'm a bit confused about the use of current->exit_code tho. Why aren't we clearing it from ptrace_stop()?

Thanks.

tejun

Ah, never mind. It's used as the signr return from ptrace signal trap.

Thanks.

tejun

Hello,

Ah, right, the lock drop across arch_ptrace_stop(). Yeah, I agree calling ptrace_stop() with TASK_RUNNING would solve it. I'll think about it a bit more.

Thank you.

tejun

__wake_up_parent() needs tasklist to pin ->parent. But probably in this particular case we can rely on rcu, or even ->siglock (given that attach/detach take this lock too).

Perhaps... But then we should wakeup the new child. Perhaps we can just kill that code, CLONE_STOPPED is deprecated and triggers the warning since bdff746a (Feb 4 2008).

Yes, I thought about this too. But ptrace's semantics is really strange, even if we move wait_on_bit() into ptrace_attach() we still have a user-visible change.

sys_ptrace() only works for the single thread who did PTRACE_ATTACH, but do_wait() should work for its sub-threads.

1. the tracer knows that the tracee is stopped

2. the tracer does ptrace(ATTACH)

3. the tracer's sub-thread does do_wait()

Note! Personally I think we can ignore this "problem", I do not think it can break anything except some specialized test-case.

Oh, I do not know. This is the main problem with ptrace. You can always understand what the code does, but you can never know what was the supposed behaviour ;)

That is why I am asking Jan and Roland who understand the userland needs.

Personally, I _think_ it makes sense to keep do_wait() working after ptrace_attach(), if it is called by the thread which did attach. But perhaps even this is not really important.

OK ;)

But look. Even if the race doesn't exist. ptrace_stop() can drop ->siglock and call arch_ptrace_stop() which can fault/sleep/whatever. I think this doesn't really matter, but otoh it would be more clean to do this in TASK_RUNNING state anyway. At least, in anny case arch_ptrace_stop() can return in TASK_RUNNING.

Oh, the right answer is: ptrace shouldn't use ->exit_code at all ;) And its usage is very confusing.

ptrace_report_syscall() and ptrace_signal() check ->exit_code after return from ptrace_stop(), otherwise we ignore the "data" argument of ptrace_resume/ptrace_detach.

Oleg.

I see. Added a patch to kill CLONE_STOPPED.

But if ptrace(ATTACH) doesn't return until the transition is complete when the task is already stopped, the tracer's sub-thread's do_wait() will behave exactly the same. The only difference would be that ptrace(ATTACH) may now block and/or is failed by a signal delivery.

How would #3 behave differently if STOPPED -> TRACED transition is guaranteed to be complete by the end of #2?

Hmmm... I see. After this fix / cleanup rounds are complete, I'll just write up something. It would be much easier to decide which way to go with a working implementation and switching between wait(2) based one and with implicit wait shouldn't be too difficult anyway.

On resume, T is in TASK_RUNNING and the lock is only dropped in ptrace_stop() if arch_ptrace_stop_needed() is true.

So, yes, the temporary lock dropping can definitely confuse ptrace_check_attach().

I agree. I'll update toward that direction.

Thanks.

tejun

Ahhh, sorry. I meant, two threads can do 2. and 3. at the same time.

But let me repeat, it is not that I think we should worry. I mentioned this only because I think it is better to discuss everything we can, even the really minor things.

Oleg.

Hello,

Ah, okay, now I see, so WNOHANG wait(2) from a different thread may fail if it races against ptrace(ATTACH), although it would have succeeded regardless of the timing between #2 and #3 before the change.

It's definitely worth documenting. We can close off the condition by tweaking wait(2) so that it blocks if the target thread is in transition, but that might be reaching into the realm of unnecessary over-engineering. IMHO, it would be wiser to hold off implementing it until we know it's actually necessary. Roland, Jan, do you guys think this can actually cause a problem?

Thank you.

tejun

Add docbook comment for do_notify_parent_cldstop(), fix a comment typo and add boilerplat code to prepare for moving notification determination logic into do_notify_parent_cldstop().

This patch doesn't cause any visible behavior change.

---
 kernel/signal.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 49 insertions(+), 4 deletions(-)

index 6d93a3f..7dfbba9 100644
@@ -1597,13 +1597,57 @@ int do_notify_parent(struct task_struct *tsk, int sig)
 	return ret;
 }
 
+/**
+ * do_notify_parent_cldstop - notify parent of CLD_CONTINUED, STOPPED or TRAPPED
+ * @tsk: task which has been continued or is about to stop
+ * @why: CLD_{CONTINUED|STOPPED|TRAPPED}
+ *
+ * Notifies the parent that @tsk has been continued or is about to stop.
+ *
+ * The notify target changes depending on whether @tsk is being ptraced or
+ * not.  If @tsk is being ptraced, it's always the ptracer; otherwise, it's
+ * the task group's real parent.
+ *
+ * CONTEXT:
+ * Must be called with tasklist_lock held.  Grabs and releases the siglocks
+ * of @tsk and the notify target.
+ */
 static void do_notify_parent_cldstop(struct task_struct *tsk, int why)
 {
 	struct siginfo info;
 	unsigned long flags;
 	struct task_struct *parent;
 	struct sighand_struct *sighand;
+	struct signal_struct *sig;
+	int notify = 0;
+
+	/*
+	 * Determine whether and what to notify.  This should be done under
+	 * @tsk's siglock.  As tasklist_lock is already held,
+	 * task_ptrace(@tsk) won't change beneath us.
+	 */
+	sighand = tsk->sighand;
+	sig = tsk->signal;
+	spin_lock_irqsave(&sighand->siglock, flags);
 
+	switch (why) {
+	case CLD_CONTINUED:
+	case CLD_STOPPED:
+	case CLD_TRAPPED:
+		notify = why;
+		break;
+	}
+
+	spin_unlock_irqrestore(&sighand->siglock, flags);
+
+	if (!notify)
+		return;
+
+	/*
+	 * Determine notify target and who should be notifying.  If being
+	 * ptraced, @tsk notifies to its current parent; otherwise, the
+	 * group leader reports to its real parent.
+	 */
 	if (task_ptrace(tsk))
 		parent = tsk->parent;
 	else {

@@ -1614,7 +1658,7 @@ static void do_notify_parent_cldstop(struct task_struct *tsk, int why)
 	info.si_signo = SIGCHLD;
 	info.si_errno = 0;
 	/*
-	 * see comment in do_notify_parent() abot the following 3 lines
+	 * see comment in do_notify_parent() about the following 3 lines
 	 */
 	rcu_read_lock();
 	info.si_pid = task_pid_nr_ns(tsk, parent->nsproxy->pid_ns);

@@ -1624,13 +1668,13 @@ static void do_notify_parent_cldstop(struct task_struct *tsk, int why)
 	info.si_utime = cputime_to_clock_t(tsk->utime);
 	info.si_stime = cputime_to_clock_t(tsk->stime);
 
- 	info.si_code = why;
- 	switch (why) {
+	info.si_code = notify;
+	switch (notify) {
  	case CLD_CONTINUED:
  		info.si_status = SIGCONT;
  		break;
  	case CLD_STOPPED:
- 		info.si_status = tsk->signal->group_exit_code & 0x7f;
+		info.si_status = sig->group_exit_code & 0x7f;
  		break;
  	case CLD_TRAPPED:
  		info.si_status = tsk->exit_code & 0x7f;

@@ -1640,6 +1684,7 @@ static void do_notify_parent_cldstop(struct task_struct *tsk, int why)
  	}
 
 	sighand = parent->sighand;
+	sig = parent->signal;
 	spin_lock_irqsave(&sighand->siglock, flags);
 	if (sighand->action[SIGCHLD-1].sa.sa_handler != SIG_IGN &&
 	    !(sighand->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDSTOP))

1.7.1

Ah, I didn't notice we are going to change sig->flags here, please ignore.

Oleg.

Yeah, it's unneeded but sighand and sig are used earlier in the function to point to the respective structures of the child while in the latter part for the parent. I was afraid a later change to the parent part might incorrectly use sig not realizing that it's still pointing to the child->signal, so I think it would be better to keep the unnecessary chunk for now. Or we can introduce separate variables for child and parent. Hmmm... yeah, that actually would be better, I think.

Thanks.

tejun

Group stop notifications are unreliable if one or more tasks of the task group are being ptraced. If a ptraced task ends up finishing a group stop, the notification is sent to the ptracer and the real parent never gets notified.

This patch adds a new signal flag SIGNAL_NOTIFY_STOP which is set on group stop completion and cleared on notification to the real parent or together with other stopped flags on SIGCONT/KILL. This guarantees that the real parent is notified correctly regardless of ptrace. If a ptraced task is the last task to stop, the notification is postponed till ptrace detach or canceled if SIGCONT/KILL is received inbetween.

Oleg spotted race against ptrace attach/detach in the initial implementation. This is fixed by moving notification determiniation into do_notify_parent_cldstop() and performing it while holding both tasklist_lock and siglock.

---
 include/linux/sched.h |    2 +
 kernel/signal.c       |   65 +++++++++++++++++++++++++++++-------------------
 2 files changed, 41 insertions(+), 26 deletions(-)

index 7045c34..7a26e7d 100644
@@ -653,6 +653,8 @@ struct signal_struct {
 
 #define SIGNAL_UNKILLABLE	0x00000040 /* for init: ignore fatal signals */
 
+#define SIGNAL_NOTIFY_STOP	0x00000100 /* notify parent of group stop */
+
 /* If true, all threads except ->group_exit_task have pending SIGKILL */
 static inline int signal_group_exit(const struct signal_struct *sig)
 {

1.7.1

OK, I am a bit confused. I do not understand exactly what this "correctly" actually means.

OK. But what if the last task to stop is not ptraced? In this case ->real_parent gets the notification.

Of course, the current behaviour is not better, it is obviously wrong. But if we want to fix things, perhaps we should invite the new and clear rules. Isn't it better to always notify ->real_parent when the last thread stops in STOPPED/TRACED state? Otherwise the behaviour is not predictable, it depends on task_ptrace() state of the last thead which sees SIGNAL_NOTIFY_STOP.

Actually, I think it would be even better to never notify ->real_parent until debugger detaches all threads, but this is not simple to implement.

It means that the ptracer wouldn't eat the notification. The notification is buffered and delivered when ptrace detaches.

I see. My focus was to make ptrace attach/detach transparent. IOW, minimizing the effect of a debugger (or gcore or whatever) attaching and then leaving. So, this patch just makes sure that the notification isn't absorbed by a ptracer.

But, yes, this is gonna change the behavior in subtle ways anyway so it would be better to take care of that too.

Hmmm? ptrace_attach() can't happen while tasklist_lock is held.

Thanks.

tejun

Yes, I understand. I am a bit worried it is not easy to describe the new behaviour exactly.

Agreed. And the code itself certainly becomes correct/consistent, contrary to "everything is broken" we currently have.

Tejun, I'll try to summarize my (very foggy) concerns in a separate email. Don't get me wrong, I think this series rightly addresses the numerous problems we have. My only question is, can't we go a bit further and create the new (and simple) rules. Probably not.

Sure, but is is not held after we drop ->siglock. And ptrace_attach() can happen in the window before we take it for do_notify_parent_cldstop().

Oleg.

Hello,

Yeah, definitely, if we're gonna make some userland visible changes, let's get it right once and for all (at least in terms of the intended behaviors, that is). It's sure gonna take more work but I think it'll be manageable both in terms of the required effort and implementation complexity. Well, actually, in terms of the latter, I think we're likely to improve the situation by making the rules and intentions clear.

I thought the code snippet was from inside do_notify_parent_cldstop() for some reason. Okay, so the debugger can attach there and... hmmm... right. Yeah, the debugger gets the extra notification. I don't think the previous code fared any better tho. Anyways, I'll think about how to fix this.

Thanks.

tejun

Heh, the previous code is much worse, sure ;)

Oleg.

* Collapse ptrace_untrace() into __ptrace_unlink().

* Always do the whole unlinking inside siglock.

* Untracing is done before unlinking.

This is to prepare for further changes. As the whole unlinking/tracing is done inside both tasklist lock and siglock, the reordering doesn't cause any visible behavior difference.

---
 kernel/ptrace.c |   40 ++++++++++++++++++----------------------
 1 files changed, 18 insertions(+), 22 deletions(-)

index 5191301..6ac12f4 100644
@@ -38,45 +38,41 @@ void __ptrace_link(struct task_struct *child, struct task_struct *new_parent)
 }
 
 /*
- * Turn a tracing stop into a normal stop now, since with no tracer there
- * would be no way to wake it up with SIGCONT or SIGKILL.  If there was a
- * signal sent that would resume the child, but didn't because it was in
- * TASK_TRACED, resume it now.
- * Requires that irqs be disabled.
+ * unptrace a task: move it back to its original parent and remove it
+ * from the ptrace list.
+ *
+ * Turn a tracing stop into a normal stop now, since with no tracer
+ * there would be no way to wake it up with SIGCONT or SIGKILL.  If
+ * there was a signal sent that would resume the child, but didn't
+ * because it was in TASK_TRACED, resume it now.  Requires that irqs
+ * be disabled.
+ *
+ * Must be called with the tasklist lock write-held.
  */
-static void ptrace_untrace(struct task_struct *child)
+void __ptrace_unlink(struct task_struct *child)
 {
+	struct signal_struct *sig = child->signal;
+
+	BUG_ON(!child->ptrace);
+
 	spin_lock(&child->sighand->siglock);
+
 	if (task_is_traced(child)) {
 		/*
 		 * If group stop is completed or in progress, it should
 		 * participate in the group stop.  Set GROUP_STOP_PENDING
 		 * before kicking it.
 		 */
-		if (child->signal->flags & SIGNAL_STOP_STOPPED ||
-		    child->signal->group_stop_count)
+		if (sig->flags & SIGNAL_STOP_STOPPED || sig->group_stop_count)
 			child->group_stop |= GROUP_STOP_PENDING;
 		signal_wake_up(child, 1);
 	}
-	spin_unlock(&child->sighand->siglock);
-}
-
-/*
- * unptrace a task: move it back to its original parent and
- * remove it from the ptrace list.
- *
- * Must be called with the tasklist lock write-held.
- */
-void __ptrace_unlink(struct task_struct *child)
-{
-	BUG_ON(!child->ptrace);
 
 	child->ptrace = 0;
 	child->parent = child->real_parent;
 	list_del_init(&child->ptrace_entry);
 
-	if (task_is_traced(child))
-		ptrace_untrace(child);
+	spin_unlock(&child->sighand->siglock);
 }
 
 static int ptrace_wait_trap(void *flags)

1.7.1

A bit off-topic note,

Hello,

Yes, that would be the more consistent behavior.

Aside from the user-visible change part, I don't think the implemnentation would be difficult.

You haven't written this yet, right? (I reconfigured / migrated my mail setup during past few days so things are still a bit shaky.)

Thank you.

tejun

Currently, SIGCONT notifications which are pending on ptrace attach or occur while ptraced are reported to the tracer and never make it to the real parent.

This patch adds a new signal flag SIGNAL_NOTIFY_CONT which is set when a task is woken up by SIGCONT and cleared once the event is notified to the parent. SIGNAL_CLD_MASK bits are no longer cleared after notification. Combined with clearing SIGNAL_CLD_MASK if !SIGNAL_NOTIFY_CONT on ptrace attach, these bits are set on ptrace detach iff the tracee owes a notification to the real parent. __ptrace_unlink() is updated to check these bits and reschedule SIGCONT notification if necessary.

As notification delivery should consider both the child's signal flags and ptraced state, it should be done while holding both siglock and tasklist_lock. The delivery logic is moved into do_notify_parent_cldstop() and performed while holding both tasklist_lock and siglock.

This change puts the initial SIGNAL_NOTIFY_CONT test out of siglock. This is safe as the bit set by the waker are guaranteed to be visible to the woken up task.

Oleg spotted the above race condition caused by ptrace state and signal state protected by different locks.

---
 include/linux/sched.h |    1 +
 kernel/ptrace.c       |   36 ++++++++++++++++++++++++++++++++++++
 kernel/signal.c       |   49 ++++++++++++++++++++++++++++---------------------
 3 files changed, 65 insertions(+), 21 deletions(-)

index 7a26e7d..ed7725b 100644
@@ -654,6 +654,7 @@ struct signal_struct {
 #define SIGNAL_UNKILLABLE	0x00000040 /* for init: ignore fatal signals */
 
 #define SIGNAL_NOTIFY_STOP	0x00000100 /* notify parent of group stop */
+#define SIGNAL_NOTIFY_CONT	0x00000200 /* notify parent of continuation */
 
 /* If true, all threads except ->group_exit_task have pending SIGKILL */
 static inline int signal_group_exit(const struct signal_struct *sig)

index 6ac12f4..dba6aeb 100644
@@ -52,6 +52,7 @@ void __ptrace_link(struct task_struct *child, struct task_struct *new_parent)
 void __ptrace_unlink(struct task_struct *child)
 {
 	struct signal_struct *sig = child->signal;
+	bool woken_up = false;
 
 	BUG_ON(!child->ptrace);
 

@@ -66,6 +67,33 @@ void __ptrace_unlink(struct task_struct *child)
 		if (sig->flags & SIGNAL_STOP_STOPPED || sig->group_stop_count)
 			child->group_stop |= GROUP_STOP_PENDING;
 		signal_wake_up(child, 1);
+		woken_up = true;
+	}
+
+	/*
+	 * SIGNAL_CLD_MASK is cleared only on a stop signal or, if
+	 * notification isn't pending, ptrace attach.  If any bit is
+	 * set,
+	 *
+	 * - SIGCONT notification was pending before attach or there
+	 *   was one or more SIGCONT notifications while tracing.
+	 *
+	 * - And, there hasn't been any stop signal since the last
+	 *   pending SIGCONT notification.
+	 *
+	 * Combined, it means that the tracee owes a SIGCONT
+	 * notification to the real parent.
+	 */
+	if (sig->flags & SIGNAL_CLD_MASK) {
+		sig->flags |= SIGNAL_NOTIFY_CONT;
+		/*
+		 * Force the tracee into signal delivery path so that
+		 * the notification is delievered ASAP.  This wakeup
+		 * is unintrusive as SIGCONT delivery would have
+		 * caused the same effect.
+		 */
+		if (!woken_up)
+			signal_wake_up(child, 0);
 	}
 
 	child->ptrace = 0;

@@ -245,6 +273,14 @@ int ptrace_attach(struct task_struct *task)
 		signal_wake_up(task, 1);
 	}
 
+	/*
+	 * Clear SIGNAL_CLD_MASK if NOTIFY_CONT is not set.  This is
+	 * used to preserve SIGCONT notification across ptrace
+	 * attach/detach.  Read the comment in __ptrace_unlink().
+	 */
+	if (!(task->signal->flags & SIGNAL_NOTIFY_CONT))
+		task->signal->flags &= ~SIGNAL_CLD_MASK;
+
 	spin_unlock(&task->sighand->siglock);
 
 	retval = 0;

1.7.1

It turns out I can no longer read the patches today ;) Will continue tomorrow with the fresh head.

I must admit, I have some concerns. But most probably I just need to actually understand what this patch does.

Oleg.

Hello, Oleg.

Hey, join the club.

The bigger question is about the direction tho. The patchset definitely still has enough holes but do you in general agree with where it's headed? If so, maybe it would be a good idea to set up an incremental devel branch somewhere? We definitely need more review rounds but given the fragility of the whole thing I think it would be wiser to keep the spotted problems and fixes in changelog after the general direction is agreed on.

Thanks.

tejun

Absolutely.

Except. I am not sure we understand the desired behaviour for CLD_CONTINUED. To the point, perhaps the debugger does not need it at all?

Again, I'll try to summarize my questions later today.

Oleg.

But we can't know this. The notification and SIGNAL_NOTIFY_CONT are per-process, while attach/detach is per-thread.

Hello, Oleg.

You're right. Any thread which notifies the real parent should clear the pending status.

Okay, hmmm... yeah, this is problematic. Man, I suck.

Combined with the previously pointed out issue with indeterministic behavior regarding stop notification, maybe it's better to reimplement something which blocks both stop and cont notifications while any thread in the group is ptraced and reissues them when all detach? So that we can get both transparent and more predictable behavior?

Indeed.

Will update.

This probably should be resolved together with the above recalc_sigpending() issue. I'll think about it.

Thank you.

tejun

ptrace_signal() releases siglock and signal delivery may continue afterwards. SIGNAL_NOTIFY_CONT can be set inbetween and should be checked after returning from the function.

* Restart from the top if ptrace_signal() returns 0.

* Factor out CLD_CONTINUED check code into notify_parent_cont() and check before returning from get_signal_to_deliver() too.

With the latter, the former isn't strictly necessary but it's still better to do it to document what's going on if for nothing else.

---
 kernel/signal.c |   38 +++++++++++++++++++++++++++-----------
 1 files changed, 27 insertions(+), 11 deletions(-)

index 7b6f972..5eddda6 100644
@@ -2019,6 +2019,19 @@ static int ptrace_signal(int signr, siginfo_t *info,
 	return signr;
 }
 
+static inline void notify_parent_cont(void)
+{
+	/*
+	 * Every stopped thread should go through this function after
+	 * waking up.  Check to see if we should notify the parent.
+	 */
+	if (unlikely(current->signal->flags & SIGNAL_NOTIFY_CONT)) {
+		read_lock(&tasklist_lock);
+		do_notify_parent_cldstop(current->group_leader, CLD_CONTINUED);
+		read_unlock(&tasklist_lock);
+	}
+}
+
 int get_signal_to_deliver(siginfo_t *info, struct k_sigaction *return_ka,
 			  struct pt_regs *regs, void *cookie)
 {

@@ -2035,15 +2048,7 @@ relock:
 	 */
 	try_to_freeze();
 
-	/*
-	 * Every stopped thread should go through this function after
-	 * waking up.  Check to see if we should notify the parent.
-	 */
-	if (unlikely(current->signal->flags & SIGNAL_NOTIFY_CONT)) {
-		read_lock(&tasklist_lock);
-		do_notify_parent_cldstop(current, CLD_CONTINUED);
-		read_unlock(&tasklist_lock);
-	}
+	notify_parent_cont();
 
 	spin_lock_irq(&sighand->siglock);
 

@@ -2073,8 +2078,11 @@ relock:
 			if (signr != SIGKILL) {
 				signr = ptrace_signal(signr, info,
 						      regs, cookie);
-				if (!signr)
-					continue;
+				if (!signr) {
+					/* siglock was released, restart */
+					spin_unlock_irq(&sighand->siglock);
+					goto relock;
+				}
 			}
 
 			ka = &sighand->action[signr-1];

@@ -2177,6 +2185,14 @@ relock:
 		/* NOTREACHED */
 	}
 	spin_unlock_irq(&sighand->siglock);
+
+	/*
+	 * If ptrace_signal() returned a non-zero signal, control can reach
+	 * here without other pending signals or going through relocking
+	 * and a CONT notification may be left pending.  Check it.
+	 */
+	notify_parent_cont();
+
 	return signr;
 }
 

1.7.1

This wake_up_process() has a turbulent history. This is a remnant from ancient ptrace implementation and patently wrong. Commit 95a3540d (ptrace_detach: the wrong wakeup breaks the ERESTARTxxx logic) removed it but the change was reverted later by commit edaba2c5 (ptrace: revert "ptrace_detach: the wrong wakeup breaks the ERESTARTxxx logic ") citing compatibility breakage and general brokeness of the whole group stop / ptrace interaction.

Digging through the mailing archives, the compatibility breakage doesn't seem to be critical in the sense that the behavior isn't well defined or reliable to begin with and it seems to have been agreed to remove the wakeup with proper cleanup of the whole thing.

Now that the group stop and its interaction with ptrace are cleaned up and well defined, it's high time to finally kill this silliness.

---
 kernel/ptrace.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

index dba6aeb..575bf8f 100644
@@ -391,8 +391,6 @@ int ptrace_detach(struct task_struct *child, unsigned int data)
 	if (child->ptrace) {
 		child->exit_code = data;
 		dead = __ptrace_detach(current, child);
-		if (!child->exit_state)
-			wake_up_process(child);
 	}
 	write_unlock_irq(&tasklist_lock);
 

1.7.1

The plain wake_up_process was certainly wrong from the beginning.

We were conservative about changing it because of the difficulty of chasing all the corners where userland debuggers' behavior might be made to regress when it had been reliable in practice before (even if not always in theory, such as possible races that didn't bite in reality). The userland code has gone to many contortions to cope with how the kernel behaved in the past, whether or not that behavior ever made any good sense.

For that sort of reason, none of this stuff should change at all in a -stable kernel, nor late in a release cycle.

For new kernels, I think changing the behavior in the direction of something that can actually be described is OK as long as userland debugger maintainers like Jan agree to the new behavior and that the behavior really and truly does follow an articulated set of rules that the kernel and userland sides agree to.

Thanks,
Roland

Hello, Roland.

Sure, definitely. All these changes are at the earliest for the next merge window.

Yeap, that sounds good to me.

Thanks.

tejun

I'll send out the acked ones as a separate patchset so that they can be merged.

Thanks.

tejun

Tejun, I am really, really sorry for the huge delay.

I will _try_ very much to read this on Friday. If nothing else, I am very interested to understand these changes in details.

Oleg.

Hello,

No need to be sorry at all. It's not like this patchset is an urgent fix. This part of ptrace has been broken for a very long time and we've been more or less fine with it. At least I would be able to enjoy the sweet delusion that this take is okay for a bit longer. :-)

tejun

I am moving this to 0/16 to get more attention from everyone.

First of all, I'd like to clarify that I am not arguing with these changes. Quite contrary, I think this is the good step in the right direction imho. In this email, I do not try to comment this series, I am going to ask the questions.

My concern is: we never tried to discuss the desired behaviour as it seen by the user-space.

To simplify the discussion, let's assume that debugger != real_parent. Now, what should we actually do if the tracee starts/completes the group stop?

To me, the only obvious thing is that each thread should report CLD_STOPPED to the debugger. Everything else is not clear to me. How and when we should notify real_parent? What should we do if tracee is multithreaded and some threads are not traced? (in the latter case we can't know which thread completes the group stop and sends the final notification).

Probably we can delay this notification until the debugger detaches all threads. This makes sense because the debugger can resume the stopped thread and confuse its real_parent (say, /bin/sh) who has all rights to assume the child can't run without the subsequent CLD_CONTINUED. However, this doesn't look very good. This doesn't allow to write the "really transparent" strace, if the tracee was stopped by SIGSTOP this should be visible to its real_parent who probably owns this application and should react (again, sh/fg/bg).

So. I think that probably we need some very simple and predictable behaviour, even if this implies the user-visible changes. If nothing else, any fix in this area is visible to user-space. To me, the best behaviour is

- each thread notifies the debugger (if it is traced)

- when the last thread stops, it also notifies its real_parent. IOW, it can send two notifications if it is traced.

(This differs from the logic in 12/16)

But, again, this means we are trying to fool the poor real_parent who does do_wait() and doesn't expect the child can suddenly run because of PTRACE_CONT/etc which does the unconditional wakeup.

A bit off-topic, but can't resist. I like very much what utrace does in this case. Since it doesn't use these notifications (in fact it doesn't use signals/reparenting at all) we do not have any problems with parent/real_parent mess. And, utrace does _not_ resume the stopped tracee. If the debugger wants to resume a thread in the SIGNAL_STOP_STOPPED group, it should send SIGCONT and this is visible to the real_parent. But of course, we can't change ptrace this way.

However. Any chance we can change ptrace_resume() so that it won't break SIGNAL_STOP_STOPPED contract? Roughly, instead of unconditional wake_up_process(child) ptrace_resume() should do

	if (child->signal->flags & SIGNAL_STOP_STOPPED)
		prepare_signal(SIGCONT);
	wake_up_state(child, __TASK_TRACED);

(of course, we should not literally use prepare_signal(), only to
explain what I mean).

IOW, if we are going to resume the tracee and its thread group is stopped, we notify the real_parent and wakeup all TASK_STOPPED (or non-ptraced) sub-threads.

Sure, this is the serious change. But otherwise, imho whatever we do the end result is not sane.

Thoughts?

As for CLD_CONTINUED, basically the same questions (in particular, I think that real_parent should be notified unconditionally). Except, perhaps the debugger doesn't need it at all?

Oleg.