WARNING in get_signal

So this is:

static void do_jobctl_trap(void)

{
	struct signal_struct *signal = current->signal;
	int signr = current->jobctl & JOBCTL_STOP_SIGMASK;

	if (current->ptrace & PT_SEIZED) {
		if (!signal->group_stop_count &&
		    !(signal->flags & SIGNAL_STOP_STOPPED))
			signr = SIGTRAP;
		WARN_ON_ONCE(!signr);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
		ptrace_do_notify(signr, signr | (PTRACE_EVENT_STOP << 8),
				 CLD_STOPPED);
	} else {
		WARN_ON_ONCE(!signr);
		ptrace_stop(signr, CLD_STOPPED, 0, NULL);
		current->exit_code = 0;
	}
}

I have the state of this paged out of my head at the moment.

Oleg or Tejun do you remember what is supposed to keep signr from being NULL?

It looks like this code was introduced in commit 73ddff2bee15 ("job
control: introduce JOBCTL_TRAP_STOP and use it for group stop trap").

Eric

This nearly killed me, but I seem to understand whats going on. ptrace_init_task() does task_set_jobctl_pending(JOBCTL_TRAP_STOP) while SIGNAL_STOP_STOPPED, child->jobctl & JOBCTL_STOP_SIGMASK == 0.

Yes, but I bet this was broken later, _may be_ by 924de3b8c9410c4.

I need to take a rest and read this code again. I too forgot how this all supposed to work.

Oleg.

No, it seems this bug is really old. I'll try to make the fix tomorrow.

Oleg.

I still do not see a good fix. I am crying ;)

For the moment, lets forget about this problem. 924de3b8c9410c4 was wrong anyway, task_join_group_stop() should be fixed:

- if current is traced, "jobctl & JOBCTL_STOP_PENDING" is not enough, we need to check SIGNAL_STOP_STOPPED/group_stop_count

- if the new thread is traced, task_join_group_stop() should do nothing, we should rely on ptrace_init_task()

Now lets return to this bug report. This (incomplete) test-case

	void *tf(void *arg)
	{
		return NULL;
	}

	int main(void)
	{
		int pid = fork();
		if (!pid) {
			setpgrp();
			kill(getpid(), SIGTSTP);

			pthread_t th;
			pthread_create(&th, NULL, tf, NULL);

			return 0;
		}

		waitpid(pid, NULL, WSTOPPED);

		ptrace(PTRACE_SEIZE, pid, 0, PTRACE_O_TRACECLONE);
		waitpid(pid, NULL, 0);

		ptrace(PTRACE_CONT, pid, 0,0);
		waitpid(pid, NULL, 0);

		int status;
		int thr = waitpid(-1, &status, 0);
		printf("pids: %d %d status: %x\n", pid, thr, status);

		return 0;
	}

triggers WARN_ON_ONCE(!signr) in do_jobctl_trap() and shows that the auto-attached sub-thread reports the wrong status.

This patch

	--- x/include/linux/ptrace.h
	+++ x/include/linux/ptrace.h
	@@ -218,7 +218,7 @@ static inline void ptrace_init_task(stru
			__ptrace_link(child, current->parent, current->ptracer_cred);
	 
			if (child->ptrace & PT_SEIZED)
	-			task_set_jobctl_pending(child, JOBCTL_TRAP_STOP);
	+			task_set_jobctl_pending(child, JOBCTL_TRAP_STOP|SIGTRAP);
			else
				sigaddset(&child->pending.signal, SIGSTOP);
		}

should fix the problem, but it is not enough even if we forget about task_join_group_stop().

- it is not clear to me if the new thread should join the group stop after (say) PTRACE_CONT. If yes, it is not clear how can we do this.

- in any case it should stop after ptrace_detach(), but in this case jobctl & JOBCTL_STOP_SIGMASK == SIGTRAP doesn't look right.

So perhaps we can change the patch above to use current->jobctl & JOBCTL_STOP_SIGMASK instead of SIGTRAP ?

This too doesn't look good, the 1st ptrace_stop() should probably always report SIGTRAP...

Oleg.

Sorry for delay... Finally I think I have a simple and clean fix. We can leave ptrace_init_task() alone and fix task_join_group_stop().

I need to test it a bit and write the changelog, do you see any problem in the patch below?

(TODO: SIGCONT should clear JOBCTL_STOP_SIGMASK, needs another patch)

Oleg.

@@ -391,16 +391,17 @@ static bool task_participate_group_stop(struct task_struct *task)
 
 void task_join_group_stop(struct task_struct *task)
 {
+	struct signal_struct *sig = current->signal;
+	unsigned long mask = current->jobctl & JOBCTL_STOP_SIGMASK;
+
+	if (sig->group_stop_count) {
+		sig->group_stop_count++;
+		mask |= JOBCTL_STOP_CONSUME;
+	} else if (!(sig->flags & SIGNAL_STOP_STOPPED))
+		return;
+
 	/* Have the new thread join an on-going signal group stop */
-	unsigned long jobctl = current->jobctl;
-	if (jobctl & JOBCTL_STOP_PENDING) {
-		struct signal_struct *sig = current->signal;
-		unsigned long signr = jobctl & JOBCTL_STOP_SIGMASK;
-		unsigned long gstop = JOBCTL_STOP_PENDING | JOBCTL_STOP_CONSUME;
-		if (task_set_jobctl_pending(task, signr | gstop)) {
-			sig->group_stop_count++;
-		}
-	}
+	task_set_jobctl_pending(task, mask | JOBCTL_STOP_PENDING);
 }
 
 /*

This testcase

	#include <stdio.h>
	#include <unistd.h>
	#include <signal.h>
	#include <sys/ptrace.h>
	#include <sys/wait.h>
	#include <pthread.h>
	#include <assert.h>

	void *tf(void *arg)
	{
		return NULL;
	}

	int main(void)
	{
		int pid = fork();
		if (!pid) {
			kill(getpid(), SIGSTOP);

			pthread_t th;
			pthread_create(&th, NULL, tf, NULL);

			return 0;
		}

		waitpid(pid, NULL, WSTOPPED);

		ptrace(PTRACE_SEIZE, pid, 0, PTRACE_O_TRACECLONE);
		waitpid(pid, NULL, 0);

		ptrace(PTRACE_CONT, pid, 0,0);
		waitpid(pid, NULL, 0);

		int status;
		int thread = waitpid(-1, &status, 0);
		assert(thread > 0 && thread != pid);
		assert(status == 0x80137f);

		return 0;
	}

fails and triggers WARN_ON_ONCE(!signr) in do_jobctl_trap().

This is because task_join_group_stop() has 2 problems when current is traced:

1. We can't rely on the "JOBCTL_STOP_PENDING" check, a stopped tracee can be woken up by debugger and it can clone another thread which should join the group-stop.

We need to check group_stop_count || SIGNAL_STOP_STOPPED.

2. If SIGNAL_STOP_STOPPED is already set, we should not increment sig->group_stop_count and add JOBCTL_STOP_CONSUME. The new thread should stop without another do_notify_parent_cldstop() report.

To clarify, the problem is very old and we should blame ptrace_init_task(). But now that we have task_join_group_stop() it makes more sense to fix this helper to avoid the code duplication.

---
 kernel/signal.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

index a38b3edc6851..ef8f2a28d37c 100644
@@ -391,16 +391,17 @@ static bool task_participate_group_stop(struct task_struct *task)
 
 void task_join_group_stop(struct task_struct *task)
 {
+	unsigned long mask = current->jobctl & JOBCTL_STOP_SIGMASK;
+	struct signal_struct *sig = current->signal;
+
+	if (sig->group_stop_count) {
+		sig->group_stop_count++;
+		mask |= JOBCTL_STOP_CONSUME;
+	} else if (!(sig->flags & SIGNAL_STOP_STOPPED))
+		return;
+
 	/* Have the new thread join an on-going signal group stop */
-	unsigned long jobctl = current->jobctl;
-	if (jobctl & JOBCTL_STOP_PENDING) {
-		struct signal_struct *sig = current->signal;
-		unsigned long signr = jobctl & JOBCTL_STOP_SIGMASK;
-		unsigned long gstop = JOBCTL_STOP_PENDING | JOBCTL_STOP_CONSUME;
-		if (task_set_jobctl_pending(task, signr | gstop)) {
-			sig->group_stop_count++;
-		}
-	}
+	task_set_jobctl_pending(task, mask | JOBCTL_STOP_PENDING);
 }
 
 /*

2.25.1.362.g51ebf55