ptrace,signal: cleanups and fixes

Hello,

This patchset contains the following four reviewed and updated patches from the "ptrace,signal: sane interaction between ptrace and job control signals, take#2"[1] patchset.

0001-signal-fix-SIGCONT-notification-code.patch 0002-ptrace-remove-the-extra-wake_up_process-from-ptrace_.patch 0003-signal-remove-superflous-try_to_freeze-loop-in-do_si.patch 0004-ptrace-kill-tracehook_notify_jctl.patch

The incorrect @why determination logic is dropped from 0004.

The git branch is located at,

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

Oleg, Roland, I'd like to base further patches on top of these, so that we don't end up constantly reshuffling patches. If there's a better place to host the above patches, please feel free to pull the branch or apply the patches. Just let me know where to base further patches.

Thank you.

--
tejun

[1] http://thread.gmane.org/gmane.linux.kernel/1072474

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

If a process does

	kill (PID, SIGSTOP);
	<varying delay, possibly even from a different process>
	kill (PID, SIGCONT);

does it mean the PID's parent may get different waitid() results? Or even that PID will finally remain still `T (stopped)'?

I do not see it has any userland impact, the PTRACE_ATTACH-to-T(stopped)-process is already racy for different reasons.

Thanks,
Jan

Hello, Jan.

Yeah, the @why part could be different with the fix. Before the fix the result was indeterministic.

I agree that it wouldn't have any userland impact especially given that the previous behavior was rather indeterministic. It just fixes an obvious bug which tested the wrong flag.

Thanks.

tejun

No. The only problem is that SIGCHLD can come with the wrong info.si_code/si_status

No.

Yes, the problem is minor.

Still, this is the clear bug due to SIGNAL_CLD_CONTINUED/SIGNAL_STOP_CONTINUED typo. And damn, it was introduced by me ;)

Please note that this bug affects !ptrace case too.

Oleg.

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 99bbaa3..a8c9f26 100644
@@ -312,8 +312,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

Of course I agree that the current code is wrong here. But I'm still not at all clear on what practical compatibility problems it introduces. This change is OK if and only if we are really making the only area clean and well-defined in the same release cycle. It doesn't really matter that the old behavior was ill-defined and unreliable in absolute terms, because the practical userland experience of it in real-world cases is what userland has grown to expect. We can't break any such expectations until we have a clear answer about how to solve the problems correctly.

Thanks,
Roland

Hello,

I don't follow what you meant by the above sentence. Can you please clarify a bit?

It's slightly more serious than that. Calling wake_up_process() unconditionally can cause a quite serious failure. It wakes up a process in any state and there are code paths which aren't prepared to be woken up unless certain conditions are met. A safer choice can be changing it to wake up only tasks in interruptible sleep.

I have very difficult time imagining what real user visible implication it would have in negative and noticeable manner, so while I do agree that we should proceed carefully, I think it would be better to take the chance and remove the erratic behavior. We have release cycles and testing at multiple layers after all. If we find out that it breaks something in serious manner, we can always revert.

Thank you.

tejun

I think I meant to write "the whole area" rather than "the only area". The point I meant to make is that removing this (wrong) wake-up entirely should only be done when we are also thoroughly cleaning up and verifying everything about the behavior of stopping/resuming in ptrace. When we get to a clear statement of what all the rules are and we really believe that the new code matches those rules, then fine. When we are still in the quagmire, no change is better than an incremental change whose subtle ramifications we can't clearly articulate.

I understand that. But I don't recall having seen anyone cite an actual scenario in the real world where it does something problematic.

For ptrace-related purposes, wake_up_state(child, TASK_TRACED|TASK_STOPPED) ought to cover anything that it is doing now that anything could reasonably be relying on.

"Difficult time imagining" and ptrace subtleties unfortunately often go together. I want to be sure we are doing something right, rather than just being thoroughly unsure that we aren't doing something wrong.

Thanks,
Roland

Hello, Roland.

Hmm... I'm not sure I quite agree. We sure should proceed with the big picture in mind but procedding gradually is usually the better and sometimes the only way to get there. Unconditional wake up doesn't belong anywhere no matter which direction we end up taking. It's a natural step to take.

I don't think it will be easily seen during regular use. Debugging is often pretty slow paced and tasks are usually stopped in well known places. But it does make a pretty decent attack vector for malicious users. I don't think it will take too much effort to come up with a proof-of-concept DOS attack exploiting this. It's plain dangerous to do this no matter how unlikely regular users might hit it.

That probably is something we need to do for -stable, I agree.

But that's the only way forward. Ptrace sure is complicated, some part due to its inherent nature but more because things were continuously piled on top of it without proper clean up, but other parts of kernel are complicated too and we make changes all the time. We have the whole release cycle structure and testing hierarchies throughout the ecosystem to enable changes like this.

But this is something which is apparently wrong and highly unlikely to cause any problem especially because with the patchset an attached task is always in traced state on detach and thus always gets signal_wake_up(). The only difference is removal of an extra spurious unconditional wakeup which may or may not disturb the group stop depending on timing. If we can't change this, I don't know what we can.

Thank you.

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 fe004b5..0a6816a 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

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.

= Oleg spotted incorrect CLD_CONTINUED/STOPPED selection when ptraced.
Fixed.

---
 include/linux/tracehook.h |   27 ---------------------------
 kernel/signal.c           |   34 ++++++++++++++--------------------
 2 files changed, 14 insertions(+), 47 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 0a6816a..7dc0ca2 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) {

@@ -1860,14 +1857,11 @@ relock:
 
 		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->group_leader, why);
-			read_unlock(&tasklist_lock);
-		}
+		read_lock(&tasklist_lock);
+		do_notify_parent_cldstop(current->group_leader, why);
+		read_unlock(&tasklist_lock);
 		goto relock;
 	}
 

@@ -2034,7 +2028,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

This causes problems for our unmerged patch-sets, but I'll leave it Oleg to decide if it's OK to remove this hook now.

Thanks,
Roland