[PATCH] signal: Preserve the ptrace stop code for SIGCHLD notification

[PATCH] signal: Preserve the ptrace stop code for SIGCHLD notification

[Zhu Jun]

After ptrace_stop() drops siglock, the tracer can consume the stop with
waitpid() and clear the tracee's exit_code. do_notify_parent_cldstop()
then rereads that field without siglock and can report CLD_TRAPPED with
a zero si_status instead of the stop signal.

Pass the stop code saved in ptrace_stop() to the notification helper.
The other notification reasons continue to obtain their status from
the existing group-stop and continuation state.

Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2")
Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>
---
 kernel/signal.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index dd770e68faee..d3afa9b846f6 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2346,6 +2346,7 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
  * @tsk: task reporting the state change
  * @for_ptracer: the notification is for ptracer
  * @why: CLD_{CONTINUED|STOPPED|TRAPPED} to report
+ * @exit_code: saved ptrace stop code when @why is CLD_TRAPPED
  *
  * Notify @tsk's parent that the stopped/continued state has changed.  If
  * @for_ptracer is %false, @tsk's group leader notifies to its real parent.
@@ -2355,7 +2356,7 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
  * Must be called with tasklist_lock at least read locked.
  */
 static void do_notify_parent_cldstop(struct task_struct *tsk,
-				     bool for_ptracer, int why)
+				     bool for_ptracer, int why, int exit_code)
 {
 	struct kernel_siginfo info;
 	unsigned long flags;
@@ -2394,7 +2395,7 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
  		info.si_status = tsk->signal->group_exit_code & 0x7f;
  		break;
  	case CLD_TRAPPED:
- 		info.si_status = tsk->exit_code & 0x7f;
+		info.si_status = exit_code & 0x7f;
  		break;
  	default:
  		BUG();
@@ -2512,9 +2513,9 @@ static int ptrace_stop(int exit_code, int why, unsigned long message,
 	 * separately unless they're gonna be duplicates.
 	 */
 	if (current->ptrace)
-		do_notify_parent_cldstop(current, true, why);
+		do_notify_parent_cldstop(current, true, why, exit_code);
 	if (gstop_done && (!current->ptrace || ptrace_reparented(current)))
-		do_notify_parent_cldstop(current, false, why);
+		do_notify_parent_cldstop(current, false, why, 0);
 
 	/*
 	 * The previous do_notify_parent_cldstop() invocation woke ptracer.
@@ -2710,7 +2711,7 @@ static bool do_signal_stop(int signr)
 		 */
 		if (notify) {
 			read_lock(&tasklist_lock);
-			do_notify_parent_cldstop(current, false, notify);
+			do_notify_parent_cldstop(current, false, notify, 0);
 			read_unlock(&tasklist_lock);
 		}
 
@@ -2927,11 +2928,11 @@ bool get_signal(struct ksignal *ksig)
 		 * a duplicate.
 		 */
 		read_lock(&tasklist_lock);
-		do_notify_parent_cldstop(current, false, why);
+		do_notify_parent_cldstop(current, false, why, 0);
 
 		if (ptrace_reparented(current->group_leader))
 			do_notify_parent_cldstop(current->group_leader,
-						true, why);
+						true, why, 0);
 		read_unlock(&tasklist_lock);
 
 		goto relock;
@@ -3235,7 +3236,7 @@ void exit_signals(struct task_struct *tsk)
 	 */
 	if (unlikely(group_stop)) {
 		read_lock(&tasklist_lock);
-		do_notify_parent_cldstop(tsk, false, group_stop);
+		do_notify_parent_cldstop(tsk, false, group_stop, 0);
 		read_unlock(&tasklist_lock);
 	}
 }
-- 
2.51.0

Re: [PATCH] signal: Preserve the ptrace stop code for SIGCHLD notification

[Oleg Nesterov]

On 11/11, Zhu Jun wrote:
>
> After ptrace_stop() drops siglock, the tracer can consume the stop with
> waitpid() and clear the tracee's exit_code. do_notify_parent_cldstop()
> then rereads that field without siglock and can report CLD_TRAPPED with
> a zero si_status instead of the stop signal.
>
> Pass the stop code saved in ptrace_stop() to the notification helper.
> The other notification reasons continue to obtain their status from
> the existing group-stop and continuation state.

But how the "correct" .si_status can actually help debugger if it has
already consumed tsk->exit_code? Why do we want this change?

> Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2")

Yes, this logic is very old. And unlikely useful.

I don't think userspace actually uses the siginfo which comes with SIGCHLD.
If nothing else, SIGCHLD doesn't queue, so si_code/si_status is not necessary
uptodate even if parent/debugger has a single child/tracee.

>  static void do_notify_parent_cldstop(struct task_struct *tsk,
> -				     bool for_ptracer, int why)
> +				     bool for_ptracer, int why, int exit_code)
>  {
>  	struct kernel_siginfo info;
>  	unsigned long flags;
> @@ -2394,7 +2395,7 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
>   		info.si_status = tsk->signal->group_exit_code & 0x7f;
>   		break;
>   	case CLD_TRAPPED:
> - 		info.si_status = tsk->exit_code & 0x7f;
> +		info.si_status = exit_code & 0x7f;

Note that tsk->signal->group_exit_code can be cleared by waitpid() the same way,
even if tsk is not traced, so this change is incomplete.

Again, I personally think we don't care.. Perhaps something like this patch makes
sense just to make this code look more correct, but then please consider the
CLD_STOPPED case as well.

Oleg.