ptrace: implement PTRACE_SEIZE/INTERRUPT and group stop notification

Hello,

This patchset implements new ptrace requests SEIZE and INTERRUPT and also add group stop notification mechanism for ptracer. Combined, this implements "P4. PTRACE_SEIZE" and "P5. ^Z and fg for tracees" of the ptrace job control improvements proposal[1].

Please note that there are some deviations from the proposal.

* As suggested by Oleg, PTRACE_SEIZE only serves as ATTACH without signal/job control side-effects. After attached, PTRACE_INTERRUPT should be used to trap tracee without side effect.

* Group stop notification is implemented as sticky INTERRUPT trap which gets cleared on PTRACE_GETSIGINFO and notifies both start and end of group stops.

All the arch changse are for adding siginfo.si_pt_flags. It's tedious and likely to take some time to be available to userland but I think it's better this way than adding some hacky flag to si_code or other already used fields.

PTRACE_SEIZE/INTERRUPT and group stop notification all use INTERRUPT trap. The trap doesn't affect signal or job control states and is the job control mechanism for ptracer in the sense that all it does is just controlling the execution of tracee.

SEIZE/INTERRUPT behaviors are fairly straight-forward. For notification, making group stop state visible to userland via PTRACE_GETSIGINFO was easy; however, notifying ptracer of the event was somewhat more involved. I ended up choosing the followings.

* The trap condition is sticky until GETSIGINFO. This is necessary because generation of the event may race with CONT and ptracer may miss the trap.

* If tracee is running, simple trapping is enough. If tracee is already group stop or INTERRUPT trapped, tracee is re-trapped to INTERRUPT thus notifying ptracer. If tracee is in other traps, notification won't happen until the trap is finished. This simplifies both implementation and usage of the interface and doesn't lose any capability as tracer can always put tracee into INTERRUPT trap if it's already in a trap without allowing it to return to userland.

* If group stop is pending, it has higher priority than INTERRUPT. This doesn't really affect correctness but avoids an extra notification trap if tracee is already going for group stop.

Each patch implementing new feature includes test program showing its functionality. Notification would probably need a bit more polishing but all the needed functionalities are there.

This patchset contains the following 11 patches.

0001-job-control-rename-signal-group_stop-and-flags-to-jo.patch 0002-ptrace-implement-PTRACE_SEIZE.patch 0003-ptrace-ptrace_check_attach-rename-kill-to-ignore_sta.patch 0004-ptrace-implement-PTRACE_INTERRUPT.patch 0005-ptrace-restructure-ptrace_getsiginfo.patch 0006-ptrace-make-group-stop-state-visible-via-PTRACE_GETS.patch 0007-ptrace-add-JOBCTL_TRAPPED.patch 0008-ptrace-move-fallback-JOBCTL_TRAPPING-clearing-to-get.patch 0009-job-control-reorganize-wait_task_stopped.patch 0010-ptrace-move-JOBCTL_TRAPPING-wait-to-wait-2-and-ptrac.patch 0011-ptrace-implement-group-stop-notification-for-ptracer.patch

and on top of

Oleg's signals-review b013c39924 (signal: cleanup sys_sigprocmask())
+ [2] ptrace: fix signal->wait_chldexit usage in task_clear_group_stop_trapping()
+ [3] ptrace: use GROUP_STOP_TRAPPING for PTRACE_DETACH too

The combined patchset is available in the following git branch.

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

HEAD should be 74b094e53f38691c98ab73499e59eb7d5771dd4c. If not, git.korg is tasking some time to sync so please wait a while and try again, or you can pull from master directly.

ssh://master.kernel.org/pub/scm/linux/kernel/git/tj/misc.git review-ptrace-seize

diffstat follows.

 arch/ia64/include/asm/siginfo.h       |    7 +
 arch/ia64/kernel/signal.c             |    5 
 arch/mips/include/asm/compat-signal.h |    7 +
 arch/mips/include/asm/siginfo.h       |    7 +
 arch/mips/kernel/signal32.c           |    5 
 arch/parisc/kernel/signal32.c         |    5 
 arch/parisc/kernel/signal32.h         |    7 +
 arch/powerpc/kernel/ppc32.h           |    7 +
 arch/powerpc/kernel/signal_32.c       |    5 
 arch/s390/kernel/compat_linux.h       |    7 +
 arch/s390/kernel/compat_signal.c      |    5 
 arch/sparc/kernel/signal32.c          |   12 +
 arch/tile/kernel/compat_signal.c      |   11 +
 arch/x86/ia32/ia32_signal.c           |    4 
 arch/x86/include/asm/ia32.h           |    7 +
 fs/exec.c                             |    2 
 include/asm-generic/siginfo.h         |   10 +
 include/linux/ptrace.h                |   14 ++
 include/linux/sched.h                 |   26 ++--
 kernel/exit.c                         |   49 ++++++-
 kernel/ptrace.c                       |  193 ++++++++++++++++++++++++++----
 kernel/signal.c                       |  213 +++++++++++++++++++++++++---------
 22 files changed, 506 insertions(+), 102 deletions(-)

Thank you.

--
tejun

[1] http://thread.gmane.org/gmane.linux.kernel/1107045
[2] http://thread.gmane.org/gmane.linux.kernel/1136303
[3] http://thread.gmane.org/gmane.linux.kernel/1136915

signal->group_stop currently hosts mostly group stop related flags; however, it's gonna be used for wider purposes and the GROUP_STOP_ flag prefix becomes confusing. Rename signal->group_stop to signal->jobctl and rename all GROUP_STOP_* flags to JOBCTL_*.

Also, reassign JOBCTL_TRAPPING to bit 22 to better accomodate future additions.

This doesn't cause any functional change.

---
 fs/exec.c             |    2 +-
 include/linux/sched.h |   16 ++++----
 kernel/ptrace.c       |   12 +++---
 kernel/signal.c       |   91 +++++++++++++++++++++++++------------------------
 4 files changed, 61 insertions(+), 60 deletions(-)

index 8328beb..fd6a4fb 100644
@@ -1659,7 +1659,7 @@ static int zap_process(struct task_struct *start, int exit_code)
 
 	t = start;
 	do {
-		task_clear_group_stop_pending(t);
+		task_clear_jobctl_stop_pending(t);
 		if (t != current && t->mm) {
 			sigaddset(&t->pending.signal, SIGKILL);
 			signal_wake_up(t, 1);

index 3f53c25..1197573 100644
@@ -1260,7 +1260,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 jobctl;	/* JOBCTL_*, siglock protected */
 	/* ??? */
 	unsigned int personality;
 	unsigned did_exec:1;

@@ -1778,15 +1778,15 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *
 #define used_math() tsk_used_math(current)
 
 /*
- * task->group_stop flags
+ * task->jobctl 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 */
-#define GROUP_STOP_DEQUEUED	(1 << 19) /* stop signal dequeued */
+#define JOBCTL_STOP_SIGMASK	0xffff    /* signr of the last group stop */
+#define JOBCTL_STOP_DEQUEUED	(1 << 16) /* stop signal dequeued */
+#define JOBCTL_STOP_PENDING	(1 << 17) /* task should stop for group stop */
+#define JOBCTL_STOP_CONSUME	(1 << 18) /* consume group stop count */
+#define JOBCTL_TRAPPING		(1 << 22) /* switching to TRACED */
 
-extern void task_clear_group_stop_pending(struct task_struct *task);
+extern void task_clear_jobctl_stop_pending(struct task_struct *task);
 
 #ifdef CONFIG_PREEMPT_RCU
 

index 4b9248d..3c56f54 100644
@@ -76,7 +76,7 @@ void __ptrace_unlink(struct task_struct *child)
 	spin_lock(&child->sighand->siglock);
 
 	/*
-	 * Reinstate GROUP_STOP_PENDING if group stop is in effect and
+	 * Reinstate JOBCTL_STOP_PENDING if group stop is in effect and
 	 * @child isn't dead.  This will trigger TRACED -> RUNNING ->
 	 * STOPPED transition.  As this transition can affect the next
 	 * ptracer if it attaches before the transition completes, set

@@ -85,7 +85,7 @@ void __ptrace_unlink(struct task_struct *child)
 	if (!(child->flags & PF_EXITING) &&
 	    (child->signal->flags & SIGNAL_STOP_STOPPED ||
 	     child->signal->group_stop_count))
-		child->group_stop |= GROUP_STOP_PENDING | GROUP_STOP_TRAPPING;
+		child->jobctl |= JOBCTL_STOP_PENDING | JOBCTL_TRAPPING;
 
 	/*
 	 * If transition to TASK_STOPPED is pending or in TASK_TRACED, kick

@@ -93,7 +93,7 @@ void __ptrace_unlink(struct task_struct *child)
 	 * is in TASK_TRACED; otherwise, we might unduly disrupt
 	 * TASK_KILLABLE sleeps.
 	 */
-	if (child->group_stop & GROUP_STOP_PENDING || task_is_traced(child))
+	if (child->jobctl & JOBCTL_STOP_PENDING || task_is_traced(child))
 		signal_wake_up(child, task_is_traced(child));
 
 	spin_unlock(&child->sighand->siglock);

@@ -228,7 +228,7 @@ static int ptrace_attach(struct task_struct *task)
 	spin_lock(&task->sighand->siglock);
 
 	/*
-	 * If the task is already STOPPED, set GROUP_STOP_PENDING and
+	 * If the task is already STOPPED, set JOBCTL_STOP_PENDING and
 	 * TRAPPING, and kick it so that it transits to TRACED.  TRAPPING
 	 * will be cleared if the child completes the transition or any
 	 * event which clears the group stop states happens.  We'll wait

@@ -245,7 +245,7 @@ static int ptrace_attach(struct task_struct *task)
 	 * in and out of STOPPED are protected by siglock.
 	 */
 	if (task_is_stopped(task)) {
-		task->group_stop |= GROUP_STOP_PENDING | GROUP_STOP_TRAPPING;
+		task->jobctl |= JOBCTL_STOP_PENDING | JOBCTL_TRAPPING;
 		signal_wake_up(task, 1);
 	}
 

@@ -258,7 +258,7 @@ unlock_creds:
 	mutex_unlock(&task->signal->cred_guard_mutex);
 out:
 	wait_event(current->signal->wait_chldexit,
-		   !(task->group_stop & GROUP_STOP_TRAPPING));
+			   !(task->jobctl & JOBCTL_TRAPPING));
 	return retval;
 }
 

1.7.1

PTRACE_ATTACH implicitly issues SIGSTOP on attach which has side effects on tracee signal and job control states. This patch implements a new ptrace request PTRACE_SEIZE which attaches and traps tracee without affecting its signal and job control states.

The usage is the same with PTRACE_ATTACH but it takes PTRACE_SEIZE_* flags in @data. Currently, the only defined flag is PTRACE_SEIZE_DEVEL which is a temporary flag to enable PTRACE_SEIZE. PTRACE_SEIZE will change ptrace behaviors outside of attach itself. The changes will be implemented gradually and the DEVEL flag is to prevent programs which expect full SEIZE behavior from using it before all the behavior modifications are complete while allowing unit testing. The flag will be removed once SEIZE behaviors are completely implemented.

After PTRACE_SEIZE, tracee will trap. Which trap will happen isn't fixed. If other trap conditions exist (signal delivery or group stop), they might be taken; otherwise, a trap with exit_code SIGTRAP | (PTRACE_EVENT_INTERRUPT << 8) is taken. The followings are guaranteed.

* A trap will happen in finite amount of userland time.

* The trap can be PTRACE_EVENT_INTERRUPT which doesn't have any side effect. If a different trap is taken, no INTERRUPT trap is pending.

IOW, no matter what, one trap will happen, which might be INTERRUPT.

INTERRUPT trapping is implemented by adding a new trap site in get_signal_to_deliver() before the actual signal dispatch which is activated by any flag in JOBCTL_TRAP_MASK. It currently includes only JOBCTL_TRAP_SEIZE which is cleared whenever ptrace_stop() commits to trapping.

Seizing sets PT_SEIZED in ->ptrace of the tracee. This flag will be used to determine whether new SEIZE behaviors should be enabled.

Test program follows.

  #define PTRACE_SEIZE		0x4206
  #define PTRACE_SEIZE_DEVEL	0x80000000

  static const struct timespec ts100ms = { .tv_nsec = 100000000 };
  static const struct timespec ts1s = { .tv_sec = 1 };
  static const struct timespec ts3s = { .tv_sec = 3 };

int main(int argc, char **argv)

  {
	  pid_t tracee;

	  tracee = fork();
	  if (tracee == 0) {
		  nanosleep(&ts100ms, NULL);
		  while (1) {
			  printf("tracee: alive\n");
			  nanosleep(&ts1s, NULL);
		  }
	  }

	  if (argc > 1)
		  kill(tracee, SIGSTOP);

	  nanosleep(&ts100ms, NULL);

	  ptrace(PTRACE_SEIZE, tracee, NULL,
		 (void *)(unsigned long)PTRACE_SEIZE_DEVEL);
	  waitid(P_PID, tracee, NULL, WSTOPPED);
	  ptrace(PTRACE_CONT, tracee, NULL, NULL);
	  nanosleep(&ts3s, NULL);
	  printf("tracer: exiting\n");
	  return 0;

}

When the above program is called w/o argument, tracee is seized from running state and continued. When tracer exits, tracee is returned to running state and keeps printing out.

  # ./test-seize
  tracee: alive
  tracee: alive
  tracee: alive
  tracer: exiting
  # tracee: alive
  tracee: alive
  tracee: alive

When called with an argument, tracee is seized from stopped state and continued, and returns to stopped state when tracer exits.

  # ./test-seize
  tracee: alive
  tracee: alive
  tracee: alive
  tracer: exiting
  # ps -el|grep test-seize
  1 T     0  4720     1  0  80   0 -   941 signal ttyS0    00:00:00 test-seize

---
 include/linux/ptrace.h |    7 +++++++
 include/linux/sched.h  |    4 ++++
 kernel/ptrace.c        |   45 +++++++++++++++++++++++++++++++++++++++------
 kernel/signal.c        |   32 +++++++++++++++++++++++++-------
 4 files changed, 75 insertions(+), 13 deletions(-)

index a1147e5..705a47b 100644
@@ -47,6 +47,11 @@
 #define PTRACE_GETREGSET	0x4204
 #define PTRACE_SETREGSET	0x4205
 
+#define PTRACE_SEIZE		0x4206
+
+/* flags in @data for PTRACE_SEIZE */
+#define PTRACE_SEIZE_DEVEL	0x80000000 /* temp flag for development */
+
 /* options set using PTRACE_SETOPTIONS */
 #define PTRACE_O_TRACESYSGOOD	0x00000001
 #define PTRACE_O_TRACEFORK	0x00000002

@@ -65,6 +70,7 @@
 #define PTRACE_EVENT_EXEC	4
 #define PTRACE_EVENT_VFORK_DONE	5
 #define PTRACE_EVENT_EXIT	6
+#define PTRACE_EVENT_INTERRUPT	7
 
 #include <asm/ptrace.h>
 

@@ -77,6 +83,7 @@
  * flags.  When the a task is stopped the ptracer owns task->ptrace.
  */
 
+#define PT_SEIZED	0x00010000	/* SEIZE used, enable new behavior */
 #define PT_PTRACED	0x00000001
 #define PT_DTRACE	0x00000002	/* delayed trace (used on m68k, i386) */
 #define PT_TRACESYSGOOD	0x00000004

index 1197573..2f383eb 100644
@@ -1784,8 +1784,12 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *
 #define JOBCTL_STOP_DEQUEUED	(1 << 16) /* stop signal dequeued */
 #define JOBCTL_STOP_PENDING	(1 << 17) /* task should stop for group stop */
 #define JOBCTL_STOP_CONSUME	(1 << 18) /* consume group stop count */
+#define JOBCTL_TRAP_SEIZE	(1 << 19) /* trap for seize */
 #define JOBCTL_TRAPPING		(1 << 22) /* switching to TRACED */
 
+#define JOBCTL_TRAP_MASK	JOBCTL_TRAP_SEIZE
+#define JOBCTL_PENDING_MASK	(JOBCTL_STOP_PENDING | JOBCTL_TRAP_MASK)
+
 extern void task_clear_jobctl_stop_pending(struct task_struct *task);
 
 #ifdef CONFIG_PREEMPT_RCU

index 2b0d719..9249230 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->jobctl & JOBCTL_STOP_PENDING) ||
+	if ((t->jobctl & JOBCTL_PENDING_MASK) ||
 	    PENDING(&t->pending, &t->blocked) ||
 	    PENDING(&t->signal->shared_pending, &t->blocked)) {
 		set_tsk_thread_flag(t, TIF_SIGPENDING);

@@ -1752,12 +1752,13 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
 	set_current_state(TASK_TRACED);
 
 	/*
-	 * We're committing to trapping.  Clearing JOBCTL_TRAPPING and
-	 * transition to TASK_TRACED should be atomic with respect to
-	 * siglock.  This should be done after the arch hook as siglock is
-	 * released and regrabbed across it.
+	 * We're committing to trapping.  Adjust ->jobctl.  Updates to
+	 * these flags and transition to TASK_TRACED should be atomic with
+	 * respect to siglock.  This should be done after the arch hook as
+	 * siglock may be released and regrabbed across it.
 	 */
 	task_clear_jobctl_trapping(current);
+	current->jobctl &= ~JOBCTL_TRAP_SEIZE;
 
 	spin_unlock_irq(&current->sighand->siglock);
 	read_lock(&tasklist_lock);

@@ -1829,7 +1830,7 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
 	recalc_sigpending_tsk(current);
 }
 
-void ptrace_notify(int exit_code)
+static void ptrace_notify_locked(int exit_code)
 {
 	siginfo_t info;
 

@@ -1842,8 +1843,13 @@ void ptrace_notify(int exit_code)
 	info.si_uid = current_uid();
 
 	/* Let the debugger run.  */
-	spin_lock_irq(&current->sighand->siglock);
 	ptrace_stop(exit_code, CLD_TRAPPED, 1, &info);
+}
+
+void ptrace_notify(int exit_code)
+{
+	spin_lock_irq(&current->sighand->siglock);
+	ptrace_notify_locked(exit_code);
 	spin_unlock_irq(&current->sighand->siglock);
 }
 

@@ -2068,6 +2074,18 @@ relock:
 
 	for (;;) {
 		struct k_sigaction *ka;
+
+		/*
+		 * Check for ptrace trap conditions.  Jobctl traps are used
+		 * to trap ptracee while staying transparent regarding
+		 * signal and job control.
+		 */
+		if (unlikely(current->jobctl & JOBCTL_TRAP_MASK)) {
+			ptrace_notify_locked(SIGTRAP |
+					     (PTRACE_EVENT_INTERRUPT << 8));
+			continue;
+		}
+
 		/*
 		 * Tracing can induce an artifical signal and choose sigaction.
 		 * The return value in @signr determines the default action,

1.7.1

Personally, I think the new behaviour is fine. But, as usual, I'd like to know what Jan/Denys think.

As for the implementation,

	bool seize = (request == PTRACE_SEIZE);

Hey, Oleg.

Heh yeah, I usually try to avoid unnecessary parantheses mostly because that seems the easiest way to reach at least some level of style uniformity. The minimum is well defined idndependently of anyone's taste and can serve as a meaningful convergence point.

That said, it's true that there are times when things just look better with extra parantheses. If the above bothers you, I'll update it. :-)

Hmmm... if killed, the tracee would go through __ptrace_unlink() which always clears JOBCTL_TRAP_MASK which includes JOBCTL_TRAP_SEIZE. What am I missing?

Argh... right it has an early exit path there.

Probably the best way to do it is adding fatal_signal_pending() into may_ptrace_stop() so that the failure path shares most of the code path instead of doing quick dirty exit? It's just nasty to have early exit above there and walking through the normal code path wouldn't hurt SIGKILL either.

Yeah, I thought about that one too. I don't think it really matters one way or the other. I mostly just wanted to avoid an extra spin_unlock_irq() after ptrace_notify_locked().

Yeah, sure. I basically just wanted to say "continue" there. :-) I'll take it out of the loop and make it unlock and jump to relock.

Thank you.

tejun

No. If killed, the tracee becomes a zombie (but see below). __ptrace_unlink() won't be called until the tracee does wait().

Yes, and thus the tracee will loop forever until the tracer detaches, so it can't even become a zombie. And the tracer can't detach via ptrace().

Yes, probably. But, apart from the user-visible change which needs another discussion, there are more problems. fatal_signal_pending() can "wrongly" return false if the caller is tracehook_report_exit(). OTOH, it can correctly return true (exec), but we probably want to stop. Let's discuss this later, I think this has nothing to do with your patches. The main problem is not fatal_signal_pending(), we need the better definition of explicit SIGKILL behaviour. Not for ptrace only, there are other issues.

Oleg.

Hey, Oleg.

Ah, right.

I think this basically is the same problem as the using TRAPPING on PTRACE_DETACH one. Do you recall the original version where TRAPPING was cleared on kill via clear_pending instead of at the end of do_signal_stop()? I think that's what we should do. We should be clearing these conditions when the actions which should override these conditions happen instead of trying to put them in the tracee expecting control to be at certain places and to me task_clear_jobctl_stop_pending() seems to be the best place to do that. We might need to rename it and maybe add a parameter tho.

How does it sound to you? It would solve the TRAPPING issue too.

Alright, let's put it aside for now.

Thank you.

tejun

Yes, and it was me who suggested to not do this ;)

Yes, I thought about this too. But I am still not sure, _perhaps_ this needs more thinking. Just a fuzzy feeling, I do not have any particular idea right now.

Oleg.

PTRACE_SEIZE does not need to stop, there is that new PTRACE_INTERRUPT for it. This is not an improvement.

It was already addressed by me before so I will give more reasons:

	https://lkml.org/lkml/2011/3/1/309

GDB already has mode set observer on' (in this case we are interested in its part set may-interrupt off') - see: $ info '(gdb)Observer Mode'
# If you want to [...] observe program behavior without any chance of # disruption by GDB
This is an increasingly requested feature as one of the ways of monitoring.

There are also requests to handle applications using 10000+ threads, which currently have problems with GDB. One can imagine a needless waitpid+PTRACE_CONT is not a help.

There could be a new PTRACE_SEIZE_INTERRUPT option in @data so that applications does not have to use two syscalls (PTRACE_SEIZE
+ PTRACE_INTERRUPT) if the applications really want to perform some operations on the tracee requiring having it stopped after the attachment. (Personally I do not think this single vs. double syscall difference is worth the new flag.)

What if PTRACE_INTERRUPT is called by tracer only after the tracee has stopped on a signal delivery? It should be ignored in such case - as the first signal will not be PTRACE_EVENT_INTERRUPT. (Sorry if you have stated it somewhere.)

Thanks,
Jan

Hello, Jan.

Hmmm... I don't think the argument given in the message is valid. The new trap doesn't have any side effect (other than possible syscall restart (or -EINTR failure) and performance overhead) so there wouldn't be any problem from correctness POV and the code to SEIZE and establish initial state would be simple.

Hmmm... transparency. Other than signal_wakeup (syscall restart / -EINTR), there is no side effect of SEIZE - from transparency POV, this should be considered fully transparent. signal_wakeup can happen anytime even without signals. It's an integral part of the programming interface. I don't know. Maybe.

Yeap, performance could be a legitimate concern but then again I'm not sure here either - the whole thing is quite slow and not trapping on SEIZE might not amount to all that much. Again, maybe.

How long does it take to attach to / detach from 10000+ threads? If you don't do it serially, it shouldn't take that long.

I think you're somewhat confused here. Well, ptrace manpage is pretty confused too. Those are different traps. You can tell them apart from userland and it doesn't matter which order or how many times INTERRUPT occurs.

Thanks.

tejun

In normal case yes; but one needs to handle all the corner cases when the first signal is not INTERRUPT; which one usually does not handle as during development (=in normal cases) it is always INTERRUPT.

Such thing is even difficult to test in QA testcases as in some cases one just cannot reproduce the (in current case) non-SIGSTOP signal arriving as first one.

It is not (such) a problem it takes time. It is a problem it stops the tracee for a moment which completely changes the tracee's racy behavior one tries to debug.

I must know in which order they come to know when the tracee is still stopped and I collect the signals to be displayed to the user and at which moment there are no more signals in the queue and I start waiting on the debuggee which started running.

Otherwise I can workaround it by various waitpid(NOHANG)s but it is better if the ordering and when INTERRUPT is / is not reported is well defined.

Thanks,
Jan

Hello,

Understood. AFAICS, there will be two different initial traps (they're not signals!) which can happen - signal delivery trap and INTERRUPT (renamed to STOP) trap. We should be able to exclude signal delivery trap from happening as the first trap. Oleg, can you think of other traps which could happen right after SEIZE?

Maybe this is best solved with a test case which can reliably trigger different initial traps sites?

Okay, timing artifacts. Yes, it is a valid point but I'm still not sure. Let's think about it.

Hmmm... you should be able to tell that without resorting to WNOHANG or depending on order of traps. That's the goal anyway. I'm a bit confused tho. What do you mean by "the tracee is still stopped"? Tracee is always stopped (or rather trapped) after reporting a trap.

Thanks.

tejun

Hi Tejun,

[...]

How to trigger it reliably? One can just try it in a loop but it takes minutes and depends on hardware specifics making it I guess even unreproducible in various configurations. We were allocating various machines for hours in the farm but it may be unreproducible anyway.

http://sources.redhat.com/cgi-bin/cvsweb.cgi/~checkout~/tests/ptrace-tests/tests/attach-into-signal.c?cvsroot=systemtap
// Even DEFAULT_LOOPS of 400 is not enough to catch it reliably. // With TESTTIME=60 or more it should be close to 100%, // but takes long time (~10 minutes).
http://sources.redhat.com/cgi-bin/cvsweb.cgi/~checkout~/tests/ptrace-tests/tests/attach-sigcont-wait.c?cvsroot=systemtap
/* Failure occurs either immediately or in about 20 runs.
But sometimes not. */
etc.

When debugging races in multithreaded applications a thread may get multiple signals at once. GDB in the default all-stop mode (the other is non-stop mode) stops all the other threads when it sees the first event on some thread.

#include <pthread.h>
#include <assert.h>
#include <asm/unistd.h>
#include <unistd.h>
#include <signal.h>
#define tkill(tid, sig) syscall (__NR_tkill, (tid), (sig)) #define gettid() syscall (__NR_gettid)
static volatile pid_t tid;
static void *
start (void *arg)
{
int i = (intptr_t) arg;
while (!tid);
sleep (1);
tkill (tid, i & 1 ? SIGUSR1 : SIGUSR2);
pause ();
return arg;
}
int main (void)
{
pthread_t thread;
int i;
for (i = 0; i < 10; i++)

    pthread_create (&thread, NULL, start, (void *) (intptr_t) i);

tid = gettid (); /* line 25 */
sleep (1);
return pause ();
}

gdb -nx ./threadsigs -ex 'tb 25' -ex r -ex 'set debug lin-lwp 1' -ex c GNU gdb (GDB) 7.3.50.20110514-cvs
This GDB was configured as "x86_64-unknown-linux-gnu". Program received signal SIGUSR1, User defined signal 1. (gdb) info threads
Id Target Id Frame
11 Thread 0x7ffff3019700 (LWP 31784) "threadsigs" 0x00007ffff7bcecfd in pause () at ../sysdeps/unix/syscall-template.S:82
... [ everything in pause () ]
2 Thread 0x7ffff7822700 (LWP 31775) "threadsigs" 0x00007ffff7bcecfd in pause () at ../sysdeps/unix/syscall-template.S:82
* 1 Thread 0x7ffff7fe6720 (LWP 31772) "threadsigs" 0x00007ffff78d0ced in nanosleep () at ../sysdeps/unix/syscall-template.S:82
(gdb) _

$ grep SigCgt /proc/31772/task/31772/status
SigCgt:	0000000180000000

OK, so the threads managed to deliver both SIGUSR1 and SIGUSR2 but GDB has reported only SIGUSR1 to the user.

# The debugee does not handle SIGUSR1 so it would crash on its delivery:
(gdb) handle SIGUSR1 nopass
Signal Stop Print Pass to program Description
SIGUSR1 Yes Yes No User defined signal 1
(gdb) continue
Program received signal SIGUSR1, User defined signal 1.

OK, GDB has waitpid()ed SIGUSR1 already and still some thread has delivered afterwards before GDB has managed to stop that thread.

(gdb) continue
Program received signal SIGUSR2, User defined signal 2.

Only now the user has found SIGUSR2 has also been delivered. The main thread (receiving the signals) has not run yet been resumed at all. It would be nice if GDB could display all the signals the inferior has received as the other threads are stopped already after the signals were sent (in pause ()) - this gives user a skewed picture of different state in time for each thread.

I would prefer if GDB would print all the signals at once on a single stop:

Program received signal SIGUSR1, User defined signal 1. Program received signal SIGUSR2, User defined signal 2. (gdb) _

(This is not a simple change for GDB as it has many operations bound to receiving single signal.)

Currently when GDB receives SIGUSR1 it has to do PTRACE_CONT before waitpid() and receiving SIGUSR2. The time it does PTRACE_CONT it does not know if then waitpid() returns immediately or if the application will run for another hour.

There are similar problems GDB wanting to do something-like-INTERRUPT sends now SIGSTOP and then it wants to remove that SIGSTOP from the inferior's queue as it would confuse both user and the debuggee if left there. Fortunately this paragraph's pain will no longer be needed with PTRACE_INTERRUPT.

For example if you guarantee that after PTRACE_INTERRUPT the INTERRUPT even will always get delivered as the last one after all the other signals GDB could safely operate on all the delivered signals without a risk of accidentally resuming the debuggee before explicitly instructed to do so by the user.

This is not a real plan how it should be done - but I hope it gives a picture debuggers are interested the processing all the already delivered signals. GDB should probably check the SigCgt /proc field (it already does in some cases) for the informational display of delivered threads.

Thanks,
Jan

Hello,

First of all, I don't think hitting different trap spots would be that difficult. Handling different traps after SEIZE might not be easy but could just be something which needs to be done anyway. I'm reluctant to treat the initial trap differently because it's essentially identical to what happens when a program does PTRACE_INTERRUPT. It's just something the program should be able to deal with, somewhat like read(2) may return shorter amount of data than provided buffer.

I can't understand the above sentence. A thread can't deliver signal without going through tracer while ptraced. Can you elaborate a bit more?

There's no distinction between main or sub threads in terms of signal delivery unless signal itself is specifically directed to a thread.

Isn't that the signal pending mask?

Ditto.

Signal delivery is sequential in nature and delivering a signal which has user specified signal handler involves roundtrip to userland. I'm not following what you're suggesting.

Okay, I'm a bit confused, so let's clear things up a bit.

* Signal is sent to a group of threads of a specific thread. Note that SIGCONT wakes up stopped process at this point.

* On the receipient, the signal becomes pending. The mask of pending signals is visible through /proc.

* Signal is delievered when the receipient processes those pending signals. This, of course, happens one signal after another. Depending on signal and configuration, signal may be ignored, kill, stop the process or trigger signal handler which involves roundtrip to userland.

* ptrace is notified of and can alter signal delivery.

Given the different modes of signal deliveries, I don't think prioritizing signal delivery to other traps makes sense.

Hmmm... but I think what you want can be achieved with simply calling PTRACE_INTERRUPT on each signal delivery trap. The tracee will deliver the signal and then immediately take INTERRUPT trap. ie.

* Check if there are pending signals which can be delivered by this thread. Note that different threads may have different pending and blocked masks so there isn't a single thread which can do everything.

* If there are signals to deliver, CONT it and it will take the signal trap (eventually). During signal trap, do PTRACE_INTERRUPT and then let the tracee deliver the signal. Tracee will deliver the signal and take STOP trap.

Is the above enough for your use case?

Thanks.

tejun

Hi Tejun,

I tried to explain why GDB will see SIGUSR1 twice. Despite it is not a realtime signal and therefore the signal is "flag", it does not queue/count. You know better than me why GDB sees SIGUSR1 twice.

This sample code uses only tkill to avoid any mess with which TID will get which signal.

Yes but how do you query siginfo_t (GDB $_siginfo) of a pending signal to make it accessible to the user? You also need to mask out blocked signals and properly order them like kernel does - which is not guaranteed by POSIX. You need to reimplement part of the kernel functionality and if you implement it a bit differently it will break transparency of the debugging.

Normally yes but this sample code uses tkill to avoid it.

But not their siginfo_t, not which are blocked, their ordering etc.

This is the question if the debugger can reliably detect. Maybe it can.

If there is enough documentation - or one reads the soures - one can reimplement the signal delivery login in userland to expect what will kernel do. TBH I do not think it is the right API but you are right it is workaroundable in userland.

Thanks,
Jan

Hello,

Ah, okay. Well, there are five threads sending USR1 and five USR2, right? The main thread would enter signal delivery path when the first signal (be it USR1 or USR2) wakes it up and on dequeueing the first signal (USR1 here), it would trap for signal delivery. Depending on timing, this may happen after all signals are generated but more likely to happen before some of them haven't finished sending yet.

So, while the first USR1 is being delivered through ptrace and whatnot, the remaining signals are sent, some of them are USR1, thus making USR1 pending again.

Once you finish delivering USR1, signal delivery path is restarted and the new pending USR1 is delivered and then USR2.

Yeap. Missed that.

You can't, at least not yet, but wouldn't presenting list of pending signals be helpful enough?

I thought we export this through /proc. Maybe not. I'll check.

I don't get why the delivery ordering matters.

This shouldn't be too hard. You just need to know all the masks.

Oh, I would strongly recomment something like that. Don't depend on implementation details. I still don't understand why you need to know the order beforehand. Wouldn't pending list be enough? What are you trying to achieve?

Thanks.

tejun

Hi Tejun,

If you check the GDB debugging session transcript I gave GDB stopped in a moment when all the threads already returned from tkill()s sending SIGUSR1s and SIGUSR2.

All threads are stopped, user is investigating the situation. And GDB tells the user (only) SIGUSR1 was delivered. The user has no chance to find out SIGUSR2 is already pending / to be delivered. This is one of the many reasons why debugging various racy cases is a nightmare.

I was trying to suggest some ways how to give user the complete overview of the debuggee situation - where both SIGUSR1 and SIGUSR2 would be reported on the first stop.

You are right GDB could examine SigCgt, SigBlk (not sure if others) and report those signals. Maybe it is right that way and we can forget about it.

There is (was) a larger problem of signals reordering which I fixed in

	[patch 3/4]#3 linux-nat: Do not respawn signals
	http://sourceware.org/ml/gdb-patches/2010-09/msg00360.html

but that mess you should have fixed by PTRACE_INTERRUPT which no longer interacts with signals. But it gave me the idea of another situation above where the debugger may want to know all the currently pending signals at once.

Thanks,
Jan

Hello, Jan.

Yeap.

Yes, I think this is the correct way to deal with it. Multiple signals can be pending and/or blocked but a single thread can only deliver a single signal at any given time, which may involve userland execution. Parallel delivery simply isn't defined, so I think what you want here is the list of pending signals, not deliveries, and then consulting the pending mask is the obvious thing to do.

Yeap, I agree that it would be nice if gdb informs the user of the pending signals when it stops for signal delivery.

Thank you.

tejun

Hi Tejun,

If not clear I would like also PTRACE_INTERRUPT to deliver the INTERRUPT event always as the first one, if it is not clear.

tracer            tracee                                foreign process
---------------   ------                                ---------------

attaches tracee
normal run
tkill(tracee, SIGUSR1)
stopped, to report SIGUSR1 to tracer
PTRACE_INTERRUPT (but tracee is already stopped)
waitpid(tracee)

Currently that waitpid will report SIGUSR1 first and then the INTERRUPT trap.

I suggest - if possible - that tracer will get that INTERRUPT trap first.

But you made the optimization that the INTERRUPT gets suppressed in such case. Which improves performance for the cost of more complicated debuggers.

I guess after PTRACE_SEIZE if the first signal/event is not INTERRUPT there will not be any INTERRUPT later, as you said.

Thanks,
Jan

Hello, Jan.

Yes, it would.

INTERRUPT is suppressed if group stop is pending not signal. That was more about duplicity between INTERRUPT trap and group stop, which made the whole thing uglier. I merged the two together so there's no such distinction anymore.

I replied in another message too but now SEIZE and INTERRUPT behave identically. Any trap clears the condition.

I don't think INTERRUPT can be prioritized like that above existing trap conditions. Traps are taken sometimes deep in the kernel (e.g. fork/exec) and often after modifying states irrevocably (e.g. signal is already dequeued on signal trap). I don't think how it would be possible to rewind the state changes and replay it later.

FWIW, if INTERRUPT exists while signal is pending, INTERRUPT will be taken first but that doesn't mean all that much as we can't roll back from traps which already have happened and tracee might be in any trap by the time PTRACE_INTERRUPT is issued.

Thank you.

tejun

Hi Tejun,

OK, so that closes one of the major issues I was trying to get "fixed".

Thanks for info,
Jan

Hello,

Sorry, this one was too difficult and, even if somehow I pulled it, unlikely to make upstream. It's gonna be extremely fragile. Unfortunately, userland would still have to deal with arbitrary order of traps.

Thank you.

tejun

PTRACE_INTERRUPT is going to be added which should also skip task_is_traced() check in ptrace_check_attach(). Rename @kill to @ignore_state and make it bool. Add function comment while at it.

This patch doesn't introduce any behavior difference.

---
 include/linux/ptrace.h |    2 +-
 kernel/ptrace.c        |   24 +++++++++++++++++++-----
 2 files changed, 20 insertions(+), 6 deletions(-)

index 705a47b..8de301a 100644
@@ -112,7 +112,7 @@ extern long arch_ptrace(struct task_struct *child, long request,
 extern int ptrace_readdata(struct task_struct *tsk, unsigned long src, char __user *dst, int len);
 extern int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned long dst, int len);
 extern void ptrace_disable(struct task_struct *);
-extern int ptrace_check_attach(struct task_struct *task, int kill);
+extern int ptrace_check_attach(struct task_struct *task, bool ignore_state);
 extern int ptrace_request(struct task_struct *child, long request,
 			  unsigned long addr, unsigned long data);
 extern void ptrace_notify(int exit_code);

index d1e3740..0f0121a 100644
@@ -102,10 +102,24 @@ void __ptrace_unlink(struct task_struct *child)
 	spin_unlock(&child->sighand->siglock);
 }
 
-/*
- * Check that we have indeed attached to the thing..
+/**
+ * ptrace_check_attach - check whether ptracee is ready for ptrace operation
+ * @child: ptracee to check for
+ * @ignore_state: don't check whether @child is currently %TASK_TRACED
+ *
+ * Check whether @child is being ptraced by %current and ready for further
+ * ptrace operations.  If @ignore_state is %false, @child also should be in
+ * %TASK_TRACED state and on return the child is guaranteed to be traced
+ * and not executing.  If @ignore_state is %true, @child can be in any
+ * state.
+ *
+ * CONTEXT:
+ * Grabs and releases tasklist_lock and @child->sighand->siglock.
+ *
+ * RETURNS:
+ * 0 on success, -ESRCH if %child is not ready.
  */
-int ptrace_check_attach(struct task_struct *child, int kill)
+int ptrace_check_attach(struct task_struct *child, bool ignore_state)
 {
 	int ret = -ESRCH;
 

@@ -124,13 +138,13 @@ int ptrace_check_attach(struct task_struct *child, int kill)
 		 */
 		spin_lock_irq(&child->sighand->siglock);
 		WARN_ON_ONCE(task_is_stopped(child));
-		if (task_is_traced(child) || kill)
+		if (task_is_traced(child) || ignore_state)
 			ret = 0;
 		spin_unlock_irq(&child->sighand->siglock);
 	}
 	read_unlock(&tasklist_lock);
 
-	if (!ret && !kill)
+	if (!ret && !ignore_state)
 		ret = wait_task_inactive(child, TASK_TRACED) ? 0 : -ESRCH;
 
 	/* All systems go.. */

1.7.1

Currently, there's no way to trap a running ptracee short of sending a signal which has various side effects. This patch implements PTRACE_INTERRUPT which traps ptracee without any signal or job control related side effect.

The implementation is almost trivial. It uses the same trap site and event as PTRACE_SEIZE. A new trap flag JOBCTL_TRAP_INTERRUPT is added, which is set on PTRACE_INTERRUPT and cleared when tracee commits to INTERRUPT trap. As INTERRUPT should be useable regardless of the current state of tracee, task_is_traced() test in ptrace_check_attach() is skipped for INTERRUPT.

PTRACE_INTERRUPT is available iff tracee is attached with PTRACE_SEIZE.

Test program follows.

  #define PTRACE_SEIZE		0x4206
  #define PTRACE_INTERRUPT	0x4207

  #define PTRACE_SEIZE_DEVEL	0x80000000

  static const struct timespec ts100ms = { .tv_nsec = 100000000 };
  static const struct timespec ts1s = { .tv_sec = 1 };
  static const struct timespec ts3s = { .tv_sec = 3 };

int main(int argc, char **argv)

  {
	  pid_t tracee;

	  tracee = fork();
	  if (tracee == 0) {
		  nanosleep(&ts100ms, NULL);
		  while (1) {
			  printf("tracee: alive pid=%d\n", getpid());
			  nanosleep(&ts1s, NULL);
		  }
	  }

	  if (argc > 1)
		  kill(tracee, SIGSTOP);

	  nanosleep(&ts100ms, NULL);

	  ptrace(PTRACE_SEIZE, tracee, NULL,
		 (void *)(unsigned long)PTRACE_SEIZE_DEVEL);
	  waitid(P_PID, tracee, NULL, WSTOPPED);
	  ptrace(PTRACE_CONT, tracee, NULL, NULL);
	  nanosleep(&ts3s, NULL);

	  printf("tracer: INTERRUPT and DETACH\n");
	  ptrace(PTRACE_INTERRUPT, tracee, NULL, NULL);
	  waitid(P_PID, tracee, NULL, WSTOPPED);
	  ptrace(PTRACE_DETACH, tracee, NULL, NULL);
	  nanosleep(&ts3s, NULL);

	  printf("tracer: exiting\n");
	  kill(tracee, SIGKILL);
	  return 0;

}

When called without argument, tracee is seized from running state, continued, interrupted and then detached back to running state.

  # ./test-interrupt
  tracee: alive pid=4546
  tracee: alive pid=4546
  tracee: alive pid=4546
  tracer: INTERRUPT and DETACH
  tracee: alive pid=4546
  tracee: alive pid=4546
  tracee: alive pid=4546
  tracer: exiting

When called with argument, it's the same but tracee is detached back to stopped state.

  # ./test-interrupt  1
  tracee: alive pid=4548
  tracee: alive pid=4548
  tracee: alive pid=4548
  tracer: INTERRUPT and DETACH
  tracer: exiting

Before PTRACE_INTERRUPT, once the tracee was continued, there was no easy way to do PTRACE_DETACH without causing side effect as tracee couldn't be trapped without side effect.

---
 include/linux/ptrace.h |    1 +
 include/linux/sched.h  |    3 ++-
 kernel/ptrace.c        |   23 +++++++++++++++++++++--
 kernel/signal.c        |    4 ++++
 4 files changed, 28 insertions(+), 3 deletions(-)

index 8de301a..5b6128b 100644
@@ -48,6 +48,7 @@
 #define PTRACE_SETREGSET	0x4205
 
 #define PTRACE_SEIZE		0x4206
+#define PTRACE_INTERRUPT	0x4207
 
 /* flags in @data for PTRACE_SEIZE */
 #define PTRACE_SEIZE_DEVEL	0x80000000 /* temp flag for development */

index 2f383eb..221ab51 100644
@@ -1785,9 +1785,10 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *
 #define JOBCTL_STOP_PENDING	(1 << 17) /* task should stop for group stop */
 #define JOBCTL_STOP_CONSUME	(1 << 18) /* consume group stop count */
 #define JOBCTL_TRAP_SEIZE	(1 << 19) /* trap for seize */
+#define JOBCTL_TRAP_INTERRUPT	(1 << 20) /* trap for interrupt */
 #define JOBCTL_TRAPPING		(1 << 22) /* switching to TRACED */
 
-#define JOBCTL_TRAP_MASK	JOBCTL_TRAP_SEIZE
+#define JOBCTL_TRAP_MASK	(JOBCTL_TRAP_SEIZE | JOBCTL_TRAP_INTERRUPT)
 #define JOBCTL_PENDING_MASK	(JOBCTL_STOP_PENDING | JOBCTL_TRAP_MASK)
 
 extern void task_clear_jobctl_stop_pending(struct task_struct *task);

index 0f0121a..1262a36 100644
@@ -693,6 +693,23 @@ int ptrace_request(struct task_struct *child, long request,
 			ret = ptrace_setsiginfo(child, &siginfo);
 		break;
 
+	case PTRACE_INTERRUPT:
+		if (!likely(child->ptrace & PT_SEIZED))
+			break;
+		/*
+		 * Stop tracee without any side-effect on signal or job
+		 * control.  If @child is already trapped, the current trap
+		 * is not disturbed and INTERRUPT trap will happen after
+		 * the current trap is ended with PTRACE_CONT.  Note that
+		 * other traps may happen before the scheduled INTERRUPT.
+		 */
+		spin_lock(&child->sighand->siglock);
+		child->jobctl |= JOBCTL_TRAP_INTERRUPT;
+		signal_wake_up(child, 0);
+		spin_unlock(&child->sighand->siglock);
+		ret = 0;
+		break;
+
 	case PTRACE_DETACH:	 /* detach a process that was attached. */
 		ret = ptrace_detach(child, data);
 		break;

@@ -818,7 +835,8 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
 		goto out_put_task_struct;
 	}
 
-	ret = ptrace_check_attach(child, request == PTRACE_KILL);
+	ret = ptrace_check_attach(child, request == PTRACE_KILL ||
+				  request == PTRACE_INTERRUPT);
 	if (ret < 0)
 		goto out_put_task_struct;
 

@@ -960,7 +978,8 @@ asmlinkage long compat_sys_ptrace(compat_long_t request, compat_long_t pid,
 		goto out_put_task_struct;
 	}
 
-	ret = ptrace_check_attach(child, request == PTRACE_KILL);
+	ret = ptrace_check_attach(child, request == PTRACE_KILL ||
+				  request == PTRACE_INTERRUPT);
 	if (!ret)
 		ret = compat_arch_ptrace(child, request, addr, data);
 

index 9249230..7add912 100644
@@ -1711,6 +1711,7 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
 	__releases(&current->sighand->siglock)
 	__acquires(&current->sighand->siglock)
 {
+	bool is_intr = exit_code == (SIGTRAP | (PTRACE_EVENT_INTERRUPT << 8));
 	bool gstop_done = false;
 
 	if (arch_ptrace_stop_needed(exit_code, info)) {

@@ -1760,6 +1761,9 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
 	task_clear_jobctl_trapping(current);
 	current->jobctl &= ~JOBCTL_TRAP_SEIZE;
 
+	if (is_intr)
+		current->jobctl &= ~JOBCTL_TRAP_INTERRUPT;
+
 	spin_unlock_irq(&current->sighand->siglock);
 	read_lock(&tasklist_lock);
 	if (may_ptrace_stop()) {

1.7.1

What are the rules for the userspace? You said about PTRACE_SEIZE:

Are rules for PTRACE_INTERRUPT the same? That is, what happens if a different trap is taken?

Can you add API notes in the header, above corresponding defines? -

@@ -48,6 +48,7 @@
 #define PTRACE_SETREGSET       0x4205
 

<============================================ ADD COMMENT HERE
#define PTRACE_SEIZE 0x4206
<============================================ ADD COMMENT HERE
+#define PTRACE_INTERRUPT 0x4207

They will be much more visible and up-to-date there than in Documentation, git logs etc...

And finally, thanks for working on the long-needed evolutionary fixes to ptrace!

vda

Hello,

Oh, PTRACE_INTERRUPT is different. If it's scheduled, it will eventually happen. Other traps may happen before but INTERRUPT trap will take place before control is returned to userland.

Hmmm... I was thinking about writing a proper ptrace API doc with example programs under Documentation/. It's userland visible API so it shouldn't change all that much and the amount of necessary documentation would be too much for comments.

Thank you.

tejun

My motivation is mostly to prevent or make it less likely for someone who will hack on this code in the future to misunderstand semantics of these ops. A hacker changing PTRACE_INTERRUPT-related code is much more likely to notice a comment above PTRACE_INTERRUPT definition than a file buried in Documentation/.

Yes, if you'd try to put there a comprehensive doc. The comment I envision is short, yet captures the most important traits of the operation. For example:

/* After PTRACE_SEIZE, tracee will trap - once. It may be a signal delivery
* or group stop, if one is already pending; otherwise,
* a SIGTRAP | (PTRACE_EVENT_INTERRUPT << 8) trap is taken.
* A trap will happen in finite amount of userland time.
*/
#define PTRACE_SEIZE 0x4206

vda

Heh. As usual, I can never review the patches in time. Will continue tomorrow.

Right now I am a bit puzzled why do we have 2 bits, JOBCTL_TRAP_INTERRUPT and JOBCTL_TRAP_SEIZE... But I didn't read this + other patches yet.

At first glance, JOBCTL_TRAP_INTERRUPT has the same problem with the killed tracee. I think this is easy to fix.

Oleg.

Hello,

It eventually ends up with three trap flags - SEIZE, INTERRUPT and NOTIFY. They all use PTRACE_EVENT_INTERRUPT trap but are different as for when they're cleared. SEIZE is cleared after any trap. INTERRUPT is cleared after an INTERRUPT trap and NOTIFY is cleared after GETSIGINFO. We can add different pending flags and adjust INTERRUPT flag according to different pending conditions but I think it's cleaner to have multiple trap flags than multiplexing things over single trap flag.

Again, isn't this cleared during __ptrace_unlink()?

Thanks.

tejun

Hi,

Yes, I see, but still can't understand. OK, please ignore, let me read other patches first.

Well, in fact I seem to understand why do we have 2 bits. Unless I misread the code, PTRACE_INTERRUPT is sticky. It can't be lost even if the tracee can report another event before. Contrary, SEIZE is cleared if the tracee reports something else right after attach, but they both report the same PTRACE_EVENT_INTERRUPT. So, if the user does

	ptrace(PTRACE_SEIZE);
	ptrace(PTRACE_INTERRUPT);

we need 2 bits to ensure PTRACE_EVENT_INTERRUPT will be reported in any case.

And, you know, I am not sure this is very clear. What if we change the rules so that PTRACE_SEIZE always reports PTRACE_EVENT_INTERRUPT like PTRACE_INTERRUPT? This looks more symmetrical and simple to me. IOW, ptrace(PTRACE_SEIZE) simply implies the implicit PTRACE_INTERRUPT.

Oh. And this reminds me the previous discussion. Should PTRACE_SEIZE stop the tracee? Perhaps it should only attach or do PTRACE_INTERRUPT depending on flags? Personally I think it should stop...

To clarify, personally I do not know. Jan, Denys, all, please comment. If PTRACE_SEIZE doesn't stop the tracee, then we should probably pass more options.

Either way, these changes do not handle the auto-attach case correctly. tracehook_report_clone() shouldn't send SIGSTOP unconditionally, we should check PT_SEIZED. And perhaps we can do auto-attach better, but right now this is off-topic.

Please see the previous email.

Also,

Hey,

Heh, my communication skill sucks.

But you've figured it out anyway. :-)

I used the term 'sticky' for PTRACE_NOTIFY which may cause more than on trap, so using sticky for INTERRUPT could be a bit confusing. To sum up,

SEIZE : schedules INTERRUPT trap, cleared on any trap

INTERRUPT : schedules INTERRUPT trap, cleared on INTERRUPT trap

NOTIFY : schedules INTERRUPT trap, cleared on GETSIGINFO,
may cause multiple INTERRUPT traps

Actually, that was my initial implementation but as we don't guarantee that the first trap would be an INTERRUPT one, so it seemed nicer to avoid double trapping after SEIZE. Maybe we can make sure that INTERRUPT is always the trap to be taken but it would be a tad bit more complex than the current implementation. I think it's doable although there are some corner cases, I think. Would that be better?

I'm rather strongly against not trapping and somebody would have to give me _very_ convincing rationales to change my mind on the subject.

Oh yeah, that's something I meant to ask you but forgot. The SEIZE behaviors are not complete after this patchset and thus there's no patch to remove the DEVEL flag yet. ISTR you telling me there are two or three conditions where the reporting/stopping isn't transparent in a previous mail, which I can't find anymore. One apparently is clone. What were the others?

Definitely. Thanks for spotting that.

At first I made PTRACE_INTERRUPT noop if it was already in INTERRUPT trap but then changed my mind because guaranteeing that there will be at least one PTRACE_EVENT_INTERRUPT trap after PTRACE_INTERRUPT seems better than having subtle behavior difference which might be nicer in limited use cases.

Thank you.

tejun

Yes, and ptrace(PTRACE_INTERRUPT) doesn't guarantee this too,

Why? We can simply say that ptrace(PTRACE_SEIZE) == attach + INTERRUPT, the rules are the same.

Hmm. Suddenly I got lost. Perhaps instead JOBCTL_TRAP_INTERRUPT should be cleared on any trap too, like SEIZE.

Again, agaian, I don't have the strong opinion, I am asking.

I don't know. Personally, I do not think this will make the API much better.

Yes.

Yes, I agree.

Hmm. No, I can't recall anything related...

clone() is special because (I think) auto-attach should obviously inherit PT_SEIZED. ptrace_init_task() copies PT_SEIZED correctly, so we should only change tracehook_report_clone().

Another special (and nasty!) case is PTRACE_TRACEME. I do not know how often it is used, but probabaly it is important enough. At least, iirc, it is used by strace. Probably we need PTRACE_SEIZEME as well.

No, I meant -EALREADY if JOBCTL_TRAP_INTERRUPT is pending set but not yet reported. But anyway, this is really minor.

Oleg.

Hello,

Yeah, sure.

I guess the thing I wanted to avoid was programs looping over PTRACE_CONT waiting for the INTERRUPT trap to happen.

But, yeah, it might be easier to explain that way. Maybe I'll change it. I don't know.

I don't think that's a good idea especially because there are functionality differences among different traps. ie. group stop and interrupt traps support re-trapping on job control events while other traps don't, so there will be cases where the debugger wants to put tracee specifically into INTERRUPT trap. It's just cleaner to use and say that if you ask for INTERRUPT, you get an INTERRUPT.

Alright.

I don't agree. PTRACE_TRACEME predates PTRACE_ATTACH and is completely redundant. If you can make the child do PTRACE_TRACEME, you might as well just make it do pause() and PTRACE_SEIZE yourself, so unless there's something PTRACE_SEIZE can't do, I don't think I'll be adding SEIZEME.

Sure, noop or -EALREADY or whatever. As I wrote above, I think it's better not to add special cases, so the rule becomes "if you schedule an INTERRUPT, you'll get at least one INTERRUPT in the future, no matter what."

Thanks.

tejun

Hmm. This is not clear to me... OK, I'll read other emails first.

Heh. I think that you are very right technically and I thought the same. That is why I never mentioned PTRACE_TRACEME before. In fact I never understood why PTRACE_TRACEME exists.

However. Perhaps this is wrong from the practical pov. SEIZEME can simplify the conversion of the existing code. People are lazy, but we need the users of PTRACE_SEIZE.

Anyway. SEIZEME is absolutely trivial. We can add it at any moment, right now this is almost offtopic.

Oleg.

Hello, again.

Hmmm... given that the attaching part is somewhat isolated from the rest of code. I'm hoping to get away with providing a sample code in documentation. :-)

Thanks.

tejun

Off the top of my head, I don't see the use case for non-stopping SEIZE right not, but arguably some people might want that.

Regarding three SEIZE/INTERRUPT/NOTIFY bits. Let's see whether we really need them.

Tejun, why exactly do you want userspace to always see INTERRUPT stop?

If tracer did ptrace(PTRACE_INTERRUPT), it wants tracee to stop. It then goes to waitpid, and whatever stop it sees, it handles. I don't see any problem if it instead happens to see, say, a signal delivery stop, and no INTERRUPT stop after that, ever. No information is lost.

Therefore, we can merge SEIZE and INTERRUPT bits into one (or drop SEIZE bit altogether, if we decide that SEIZE doesn't stop).

Then, NOTIFY bit.
Tejun, let us know why did you design group stop notification in a "sticky" way. Is it because of some races with SIGSTOP/SIGCONT?

*one* INTERRUPT stop (that is, non-sticky one) every time there is a group stop state change. Tracer can retrieve status via GETSIGINFO just like as provided by your patch, but it doesn't absolutely has to: it can simply CONT the tracee.

(
No, a bit different. Not
"every time there is a group stop state change"
but
"every time there is a SIGCONT which releases tracee from group stop"
- because group stop notification is _already_ delivered to the tracer, even by the current kernel's code,
and it is already detectable (by observing that GETSIGINFO fails on it) and we can avoid changing this.
)

Therefore, NOTIFY bit is also not needed. Only INTERRUPT bit is. Unless I miss something...

Another note: even though PTRACE_INTERRUPT solves the problem that PTRACE_DETACH of a running tracee was butt-ugly thing to do correctly, the "new" way is still a bit ugly: tracer needs PTRACE_INTERRUPT, waitpid, and only then PTRACE_DETACH. Why not go all the way and make PTRACE_DETACH work on running tracee too?

vda

Hey,

First of all, I think it's cleaner that way - if you ask for INTERRUPT, you get an INTERRUPT. Secondly, because INTERRUPT trap is special regarding retrapping notifications and there will be cases where debugger wants to put a tracee into INTERRUPT trap and it will be pretty annoying to do that safely if INTERRUPT disappears on each trap and/or INTERRUPT doesn't work if tracee is already trapped.

Design like that is fragile like hell. Going back to the debugging stopped tracee case I talked about in another reply, let's say debugger issues PTRACE_CONT at the same time SIGCONT is generated. Tracee will continue execution and debugger would believe that the trap was continued by it and be oblivious about the SIGCONT which raced with PTRACE_CONT.

As I replied in another message, group stop may also be initiated while a tracee is INTERRUPT trapped.

Those different TRAP bits are invisible to userland. The only changes visible from userland are new ptrace requests - PTRACE_SEIZE and PTRACE_INTERRUPT, and a new trap condition - PTRACE_EVENT_INTERRUPT.

PTRACE_EVENT_INTERRUPT traps are mostly side-effect-less and I'm planning on documenting specifically that it may seem spurious and the debugger should only rely on the information obtained from GETSIGINFO.

I don't think I'll change that. It's only three syscall sequence - INTERRUPT, wait(STOPPED) and DETACH which will always work reliably (unless tracee gets killed or something). It might be nice but ultimately unnecessary and pre-SEIZE ptrace would have to be supported for a very long time anyway so I don't see much point in deviating DETACH behavior when there is no new capability to gain by doing so.

Thank you.

tejun

Hi Tejun,

If tracer issues PTRACE_CONT, it means that tracee was in some sort of ptrace stop anyway at that point.
Therefore "cont" notification, just like any other ptrace notification (say, signal notification) which was generated while tracee was ptrace-stopped, should be reported *at the next waitpid*.

If, as you say in this example, SIGCONT was generated while tracee was ptrace-stopped, then next waitpid will return "cont" notification to tracer. Tracer will know that it is a "cont" notification by looking at GETSIGINFO result.

Why do you need stickiness of "cont" notification? I don't see this need from userspace POV.

Yes...

...yes, until tracer gets the next waitpid result, which will inform tracer that "cont" has happened.

(After which, tracer will likely PTRACE_CONT, get SIGCONT signal delivery notification, inject it via PTRACE_CONT(SIGCONT) so that tracee can run the handler, and so forth...)

Sure, I'm not arguing about details of kernel-side machinery. If you need three bits (or 33) in task struct to make it work, so be it. Oleg is your guy to discuss that.

I understand this. I am trying to understand what feature are you trying to provide to userland, or what problematic race scenario you are trying to make resolve-able *in userland* by making "stop" and "cont" notifications sticky wrt GETSIGINFO. I just don't see this scenario.

vda

Hey,

If you still don't see how events can get lost, I'm afraid I can't explain any better. You're repeating that exit_code can record the event until it's fetched but it doesn't queue and any trap will overwrite it and debuggers don't have to call GETSIGINFO after each trap - it only has to do so for signal delivery, group stop and INTERRUPT trap. So, it can just look at the exit_code (which doesn't indicate continuation) and let tracee return to userland.

Maybe I'm horribly confused. Oleg, am I?

Thanks.

tejun

If a process issues PTRACE_INTERRUPT, it just wants the target process to stop. Presumably it doesn't really care _why_, as long as it doesn't introduce spurious effects visible to the target. As such, if the process stops on its own due to some other trap, that ought to be good enough, right? Is there really a need for a trap that will be reliably echoed back to the trapping process?

Hello,

It can go either way. I just chose one. One use case which made me favor not clearing INTERRUPT on other traps was that INTERRUPT, along with group stop trap, is special in that it allows notification re-traps. It serves as per-tracee job control trap site which debugger can use as it pleases.

As such, there are times where debugger wants to put tracee into that specific trap. If INTERRUPT gets cleared on every trap, it would have to set INTERRUPT on each trap until it reaches INTERRUPT trap which seemed kinda silly and given that INTERRUPT may happen for spurious notification reasons, there didn't seem much point in avoiding the trap because there was another trap.

That said, as it makes INTERRUPT behavior different from SEIZE and I think occassional multiple traps on SEIZE could be uglier. I think I'll change it such that it clears on any trap. If debugger wants to put tracee specifically into INTERRUPT trap, it will have to raise it after each unsuccessful trap. It would be annoying but I suppose it's lesser of the two evils.

Thanks.

tejun

I do not think this change is much related to this patchset.

But having to PTRACE_INTERRUPT the tracee before PTRACE_DETACH has no advantage, it is just a performance (see transparent tracking of 10000+ thread

	https://lkml.org/lkml/2011/5/15/115

) problem and also getting it correct. As when one wait()s and gets WIFSTOPPED one needs to respawn to signal otherwise the signal gets lost on PTRACE_ATTACH. How to respawn it? By PTRACE_INTERRUPT with DATA==signal? Or PTRACE_CONT with DATA==signal? With rapid signalling of the tracee the debugger may never have a chance to correctly quit. Handling other cases transparently for the original parent also may not be fully clear.

It would be nice to write documentation already while discussing this patch, I do not know if PTRACE_INTERRUPT respects DATA etc., it may show ptrace is still tricky.

Thanks,
Jan

Hey, again.

Hmmm... you seem to be a bit confused about different ptrace traps (again, a lot of this probably stems from incorrect manpage). They don't interact with each other that way and there is no correctness issue (if there is, it's a but and should be fixed) of INTERRUPT+DETACH compared to DETACH without interrupt. The only difference is performance, so let's concentrate on that.

The reason why I'm reluctant to drop trapped requirement from both SEIZE and DETACH is that removing those synchronization points opens up a lot of corner cases. I'm they can all be dealt with but it's gonna be measurably more complex and possibly fragile. Given that the current implementation mandates traps on both ATTACH and DETACH, I'd like to leave that part alone unless there are pretty strong arguments, and, sure, performance might as well be strong enough.

But, I'd really like to know how much it actually costs to attach and detach a lot of tasks before deciding anything.

Thank you.

tejun

Hi Tejun,

[...]

Here is a naive approach to PTRACE_DETACH with current kernels requiring inferior to be stopped. You can see it FAILs. Still I think it is offtopic problem for this specific patchset but it still should be fixed in ptrace:

#include <unistd.h>
#include <sys/wait.h>
#include <sys/ptrace.h>
#include <stdio.h>
#include <stdlib.h>
static int status;
static pid_t child;
void
handler (int signo)
{
puts ("PASS");
exit (0);
}
/* Ensure CHILD is stopped even if it is running now - for PTRACE_DETACH. */ void
stop_child_for_detach (void)
{
kill (child, SIGCONT); /* To be PTRACE_INTERRUPT in the future. */
/* The problem - here a signal may get lost. */
wait (&status);
}
int
main (void)
{
child = fork ();
switch (child)

    {
    case 0:
      signal (SIGUSR2, handler);
      ptrace (PTRACE_TRACEME, 0, NULL, NULL);
      raise (SIGUSR1);
      puts ("FAIL");
      exit (1);
    default:
      wait (&status);
      /* Sent by arbitrary external program.  */
      kill (child, SIGUSR2);
      /* Comment out for PTRACE_DETACH not requiring stopped tracee.  */
#if 1
      ptrace (PTRACE_CONT, child, NULL, NULL);
      stop_child_for_detach ();
#endif
      ptrace (PTRACE_DETACH, child, NULL, NULL);
    }

return 0;
}

They are either (currently) in the userland or they would be in the kernel.

Thanks,
Jan

Hey,

        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

You can lose signal here. You must check which trap happened why and then take action accordingly. Argh... we really need better documentation of ptrace behaviors and traps. Anyways, sans the confusion between debugger sent signals and signals from other sources and the side effects caused by debugger sent ones, the current ptrace interface isn't broken to the point where you just lose signal.

Nope. What userland is currently dealing with isn't that type of conditions. It's dealing with nasty side effects of implied and required signals, which will be removed with the new interface. Those attach/detach sync points are currently in the kernel and wouldn't change with the proposed updates. You're suggesting to remove them.

So, it's not about moving them, at all. It's about removing them.

That said, it might not be such a bad idea. Let's see how difficult or easy it actually is.

Thanks.

tejun

Hi Tejun,

Not here but there should be PTRACE_CONT in stop_child_for_detach.

That SIGUSR1 was intended to be dropped - it was there just to get stopped by PTRACE_TRACEME.

We could misunderstand each other here. Sure I know where is the problem. Just demonstration that writing correct `stop_child_for_detach' is not easy and naive programmer may write it looking right and working right during development but in fact it will break debuggees in corner cases.

They won't as there will be new INTERRUPT event and when one wants to trap it one has to deal with various signals coming before or after it.

I am suggesting not to introduce the existing pain into new API.

Thanks,
Jan

I'd rather lean toward handling it properly from userland. Strictly defining trap order is too fragile. I think the right thing to do here is properly documenting how to recognize and handle different types of traps.

Thanks.

tejun

Flatten ptrace_getsiginfo() to prepare for more logic in the success path. While at it, remove [un]likely() on child->last_siginfo check - signal delivery and group stop traps can only be distinguished by NULL siginfo and group stop isn't that unlikely.

This patch doesn't introduce any functional change.

---
 kernel/ptrace.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

index 1262a36..2a8e930 100644
@@ -529,16 +529,19 @@ static int ptrace_setoptions(struct task_struct *child, unsigned long data)
 static int ptrace_getsiginfo(struct task_struct *child, siginfo_t *info)
 {
 	unsigned long flags;
-	int error = -ESRCH;
+	int error;
 
-	if (lock_task_sighand(child, &flags)) {
-		error = -EINVAL;
-		if (likely(child->last_siginfo != NULL)) {
-			*info = *child->last_siginfo;
-			error = 0;
-		}
-		unlock_task_sighand(child, &flags);
-	}
+	if (!lock_task_sighand(child, &flags))
+		return -ESRCH;
+
+	error = -EINVAL;
+	if (!child->last_siginfo)
+		goto out_unlock;
+
+	error = 0;
+	*info = *child->last_siginfo;
+out_unlock:
+	unlock_task_sighand(child, &flags);
 	return error;
 }
 

1.7.1

Ptracer can detect tracee entering group stop by watching for the group stop trap; however, there is no reliable way to find out when group stop ends - SIGCONT may be processed by another thread and signal delivery is blocked while tracee is trapped.

This patch adds siginfo.si_pt_flags and uses PTRACE_SI_STOPPED flag to indicate whether group stop is in effect or not. While tracee is trapped for anything other than signal delivery and group stop itself, tracer can use PTRACE_GETSIGINFO to access this information. Note that it's only available if tracee was seized.

This doesn't address notification and tracer has to put tracee in an appropriate trap and poll the flag. Later patches will deal with notification and trap transition.

This essentially is a simple addition of a flag but seems complicated thanks to siginfo_t convolution. _sigtrap struct, which contains all the fields used by ptrace_notify[_locked]() and the new _pt_flags, is added to siginfo._sifields union along with the field abbreviation macro si_pt_flags; then, __SI_TRAP is defined to implement copying of the new field to userland.

Two architectures - ia64 and mips - define their own versions of siginfo_t and ia64 implements its own copy_siginfo_to_user(). Also, x86, mips, parisc, powerpc, s390, sparc and tile have compat_siginfo_t and copy_siginfo_to_user32() for 32bit compatibility. All are updated such that [compat_]siginfo_t have _sigtrap and all the fields are copied out.

x86 is tested. Affected code in mips, powerpc, s390 and sparc are compile tested. mips and tile are untested.

Test program follows.

  #define PTRACE_SEIZE		0x4206
  #define PTRACE_INTERRUPT	0x4207

  #define PTRACE_SEIZE_DEVEL	0x80000000

  static const struct timespec ts1s = { .tv_sec = 1 };

int main(int argc, char **argv)

  {
	  pid_t tracee, tracer;
	  int i;

	  tracee = fork();
	  if (!tracee)
		  while (1)
			  nanosleep(&ts1s, NULL);

	  tracer = fork();
	  if (!tracer) {
		  int last_stopped = 0, stopped;
		  siginfo_t si;

		  ptrace(PTRACE_SEIZE, tracee, NULL,
			 (void *)(unsigned long)PTRACE_SEIZE_DEVEL);
	  repeat:
		  waitid(P_PID, tracee, NULL, WSTOPPED);

		  if (!ptrace(PTRACE_GETSIGINFO, tracee, NULL, &si)) {
			  if (si.si_code) {
				  stopped = !!si.si_status;
				  if (stopped != last_stopped)
					  printf("tracer: stopped=%d\n", stopped);
				  last_stopped = stopped;
				  ptrace(PTRACE_CONT, tracee, NULL, NULL);
			  } else {
				  printf("tracer: SIG %d\n", si.si_signo);
				  ptrace(PTRACE_CONT, tracee, NULL,
					 (void *)(unsigned long)si.si_signo);
			  }
		  } else
			  ptrace(PTRACE_CONT, tracee, NULL, NULL);
		  ptrace(PTRACE_INTERRUPT, tracee, NULL, NULL);
		  goto repeat;
	  }

	  for (i = 0; i < 3; i++) {
		  nanosleep(&ts1s, NULL);
		  printf("mother: SIGSTOP\n");
		  kill(tracee, SIGSTOP);
		  nanosleep(&ts1s, NULL);
		  printf("mother: SIGCONT\n");
		  kill(tracee, SIGCONT);
	  }
	  nanosleep(&ts1s, NULL);

	  kill(tracer, SIGKILL);
	  kill(tracee, SIGKILL);
	  return 0;

}

Tracer delivers signal, resumes group stop, induces INTERRUPT traps and reports group stop state change in busy loop. Mother sends SIGSTOP or CONT to tracee on each second. Note that si_pt_flags and flag testing are replaced with si_status testing. si_status occupies the same offset as si_pt_flags and PTRACE_SI_STOPPED is the only flag defined, so I took a dirty short cut.

  # ./test-stopped
  mother: SIGSTOP
  tracer: SIG 19
  tracer: stopped=1
  mother: SIGCONT
  tracer: SIG 18
  tracer: stopped=0
  mother: SIGSTOP
  tracer: SIG 19
  tracer: stopped=1
  mother: SIGCONT
  tracer: stopped=0
  tracer: SIG 18
  mother: SIGSTOP
  tracer: SIG 19
  tracer: stopped=1
  mother: SIGCONT
  tracer: SIG 18
  tracer: stopped=0

---
 arch/ia64/include/asm/siginfo.h       |    7 +++++++
 arch/ia64/kernel/signal.c             |    5 +++++
 arch/mips/include/asm/compat-signal.h |    7 +++++++
 arch/mips/include/asm/siginfo.h       |    7 +++++++
 arch/mips/kernel/signal32.c           |    5 +++++
 arch/parisc/kernel/signal32.c         |    5 +++++
 arch/parisc/kernel/signal32.h         |    7 +++++++
 arch/powerpc/kernel/ppc32.h           |    7 +++++++
 arch/powerpc/kernel/signal_32.c       |    5 +++++
 arch/s390/kernel/compat_linux.h       |    7 +++++++
 arch/s390/kernel/compat_signal.c      |    5 +++++
 arch/sparc/kernel/signal32.c          |   12 ++++++++++++
 arch/tile/kernel/compat_signal.c      |   11 +++++++++++
 arch/x86/ia32/ia32_signal.c           |    4 ++++
 arch/x86/include/asm/ia32.h           |    7 +++++++
 include/asm-generic/siginfo.h         |   10 ++++++++++
 include/linux/ptrace.h                |    3 +++
 kernel/ptrace.c                       |   13 +++++++++++++
 kernel/signal.c                       |    7 ++++++-
 19 files changed, 133 insertions(+), 1 deletions(-)

index c8fcaa2..2cff1ce 100644
@@ -70,6 +70,13 @@ typedef struct siginfo {
 			long _band;	/* POLL_IN, POLL_OUT, POLL_MSG (XPG requires a "long") */
 			int _fd;
 		} _sigpoll;
+
+		/* SIGTRAP */
+		struct {
+			pid_t _pid;		/* sender's pid */
+			uid_t _uid;		/* sender's uid */
+			unsigned int _pt_flags;
+		} _sigtrap;
 	} _sifields;
 } siginfo_t;
 

index 7bdafc8..ee18366 100644
@@ -142,6 +142,11 @@ copy_siginfo_to_user (siginfo_t __user *to, siginfo_t *from)
 			err |= __put_user(from->si_addr, &to->si_addr);
 			err |= __put_user(from->si_imm, &to->si_imm);
 			break;
+		      case __SI_TRAP >> 16:
+			err |= __put_user(from->si_uid, &to->si_uid);
+			err |= __put_user(from->si_pid, &to->si_pid);
+			err |= __put_user(from->si_pt_flags, &to->si_pt_flags);
+			break;
 		      case __SI_TIMER >> 16:
 			err |= __put_user(from->si_tid, &to->si_tid);
 			err |= __put_user(from->si_overrun, &to->si_overrun);

index 368a99e..47b2e4f 100644
@@ -54,6 +54,13 @@ typedef struct compat_siginfo {
 			int _fd;
 		} _sigpoll;
 
+		/* SIGTRAP */
+		struct {
+			compat_pid_t _pid;	/* sender's pid */
+			compat_uid_t _uid;	/* sender's uid */
+			unsigned int _pt_flags;
+		} _sigtrap;
+
 		/* POSIX.1b timers */
 		struct {
 			timer_t _tid;		/* timer id */

index 1ca64b4..232920f 100644
@@ -96,6 +96,13 @@ typedef struct siginfo {
 			__ARCH_SI_BAND_T _band;	/* POLL_IN, POLL_OUT, POLL_MSG */
 			int _fd;
 		} _sigpoll;
+
+		/* SIGTRAP */
+		struct {
+			pid_t _pid;		/* sender's pid */
+			__ARCH_SI_UID_T _uid;	/* sender's uid */
+			unsigned int _pt_flags;
+		} _sigtrap;
 	} _sifields;
 } siginfo_t;
 

index aae9866..7e392e1 100644
@@ -452,6 +452,11 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, siginfo_t *from)
 			err |= __put_user(from->si_band, &to->si_band);
 			err |= __put_user(from->si_fd, &to->si_fd);
 			break;
+		case __SI_TRAP >> 16:
+			err |= __put_user(from->si_pid, &to->si_pid);
+			err |= __put_user(from->si_uid, &to->si_uid);
+			err |= __put_user(from->si_pt_flags, &to->si_pt_flags);
+			break;
 		case __SI_RT >> 16: /* This is not generated by the kernel as of now.  */
 		case __SI_MESGQ >> 16:
 			err |= __put_user(from->si_pid, &to->si_pid);

index e141324..ead8ca4 100644
@@ -482,6 +482,11 @@ copy_siginfo_to_user32 (compat_siginfo_t __user *to, siginfo_t *from)
 			err |= __put_user(from->si_band, &to->si_band);
 			err |= __put_user(from->si_fd, &to->si_fd);
 			break;
+		case __SI_TRAP >> 16:
+			err |= __put_user(from->si_pid, &to->si_pid);
+			err |= __put_user(from->si_uid, &to->si_uid);
+			err |= __put_user(from->si_pt_flags, &to->si_pt_flags);
+			break;
 		case __SI_TIMER >> 16:
 			err |= __put_user(from->si_tid, &to->si_tid);
 			err |= __put_user(from->si_overrun, &to->si_overrun);

index c780084..8016f51 100644
@@ -104,6 +104,13 @@ typedef struct compat_siginfo {
                         int _band;      /* POLL_IN, POLL_OUT, POLL_MSG */
                         int _fd;
                 } _sigpoll;
+
+		/* SIGTRAP */
+		struct {
+			unsigned int _pid;      /* sender's pid */
+			unsigned int _uid;      /* sender's uid */
+			unsigned int _pt_flags;
+		} _sigtrap;
         } _sifields;
 } compat_siginfo_t;
 

index dc16aef..4293542 100644
@@ -64,6 +64,13 @@ typedef struct compat_siginfo {
 			int _band;	/* POLL_IN, POLL_OUT, POLL_MSG */
 			int _fd;
 		} _sigpoll;
+
+		/* SIGTRAP */
+		struct {
+			compat_pid_t _pid;		/* sender's pid */
+			compat_uid_t _uid;		/* sender's uid */
+			unsigned int _pt_flags;
+		} _sigtrap;
 	} _sifields;
 } compat_siginfo_t;
 

index b96a3a0..d072458 100644
@@ -716,6 +716,11 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *d, siginfo_t *s)
 		err |= __put_user(s->si_band, &d->si_band);
 		err |= __put_user(s->si_fd, &d->si_fd);
 		break;
+	case __SI_TRAP:
+		err |= __put_user(s->si_pid, &d->si_pid);
+		err |= __put_user(s->si_uid, &d->si_uid);
+		err |= __put_user(s->si_pt_flags, &d->si_pt_flags);
+		break;
 	case __SI_TIMER >> 16:
 		err |= __put_user(s->si_tid, &d->si_tid);
 		err |= __put_user(s->si_overrun, &d->si_overrun);

index 9635d75..f8c973f 100644
@@ -72,6 +72,13 @@ typedef struct compat_siginfo {
 			int	_band;	/* POLL_IN, POLL_OUT, POLL_MSG */
 			int	_fd;
 		} _sigpoll;
+
+		/* SIGTRAP */
+		struct {
+			pid_t		_pid;	/* sender's pid */
+			uid_t		_uid;	/* sender's uid */
+			unsigned int _pt_flags;
+		} _sigtrap;
 	} _sifields;
 } compat_siginfo_t;
 

index eee9998..b3c9f6b 100644
@@ -96,6 +96,11 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, siginfo_t *from)
 			err |= __put_user(from->si_band, &to->si_band);
 			err |= __put_user(from->si_fd, &to->si_fd);
 			break;
+		case __SI_TRAP >> 16:
+			err |= __put_user(from->si_pid, &to->si_pid);
+			err |= __put_user(from->si_uid, &to->si_uid);
+			err |= __put_user(from->si_pt_flags, &to->si_pt_flags);
+			break;
 		case __SI_TIMER >> 16:
 			err |= __put_user(from->si_tid, &to->si_tid);
 			err |= __put_user(from->si_overrun, &to->si_overrun);

index 75fad42..c545212 100644
@@ -102,6 +102,13 @@ typedef struct compat_siginfo{
 			int _band;	/* POLL_IN, POLL_OUT, POLL_MSG */
 			int _fd;
 		} _sigpoll;
+
+		/* SIGTRAP */
+		struct {
+			compat_pid_t _pid;		/* sender's pid */
+			unsigned int _uid;		/* sender's uid */
+			unsigned int _pt_flags;
+		} _sigtrap;
 	} _sifields;
 }compat_siginfo_t;
 

@@ -165,6 +172,11 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, siginfo_t *from)
 			err |= __put_user(from->si_band, &to->si_band);
 			err |= __put_user(from->si_fd, &to->si_fd);
 			break;
+		case __SI_TRAP >> 16:
+			err |= __put_user(from->si_pid, &to->si_pid);
+			err |= __put_user(from->si_uid, &to->si_uid);
+			err |= __put_user(from->si_pt_flags, &to->si_pt_flags);
+			break;
 		case __SI_RT >> 16: /* This is not generated by the kernel as of now.  */
 		case __SI_MESGQ >> 16:
 			err |= __put_user(from->si_pid, &to->si_pid);

index dbb0dfc..0a5d694 100644
@@ -109,6 +109,13 @@ struct compat_siginfo {
 			int _band;	/* POLL_IN, POLL_OUT, POLL_MSG */
 			int _fd;
 		} _sigpoll;
+
+		/* SIGTRAP */
+		struct {
+			unsigned int _pid;	/* sender's pid */
+			unsigned int _uid;	/* sender's uid */
+			unsigned int _pt_flags;
+		} _sigtrap;
 	} _sifields;
 };
 

@@ -219,6 +226,10 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, siginfo_t *from)
 		case __SI_POLL >> 16:
 			err |= __put_user(from->si_fd, &to->si_fd);
 			break;
+		case __SI_TRAP >> 16:
+			err |= __put_user(from->si_uid, &to->si_uid);
+			err |= __put_user(from->si_pt_flags, &to->si_pt_flags);
+			break;
 		case __SI_TIMER >> 16:
 			err |= __put_user(from->si_overrun, &to->si_overrun);
 			err |= __put_user(ptr_to_compat(from->si_ptr),

index 588a7aa..1df88cc 100644
@@ -85,6 +85,10 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, siginfo_t *from)
 			case __SI_POLL >> 16:
 				put_user_ex(from->si_fd, &to->si_fd);
 				break;
+			case __SI_TRAP:
+				put_user_ex(from->si_uid, &to->si_uid);
+				put_user_ex(from->si_pt_flags, &to->si_pt_flags);
+				break;
 			case __SI_TIMER >> 16:
 				put_user_ex(from->si_overrun, &to->si_overrun);
 				put_user_ex(ptr_to_compat(from->si_ptr),

index 1f7e625..7eab27a 100644
@@ -126,6 +126,13 @@ typedef struct compat_siginfo {
 			int _band;	/* POLL_IN, POLL_OUT, POLL_MSG */
 			int _fd;
 		} _sigpoll;
+
+		/* SIGTRAP */
+		struct {
+			unsigned int _pid;	/* sender's pid */
+			unsigned int _uid;	/* sender's uid */
+			unsigned int _pt_flags;
+		} _sigtrap;
 	} _sifields;
 } compat_siginfo_t;
 

index 942d30b..e243927 100644
@@ -90,6 +90,13 @@ typedef struct siginfo {
 			__ARCH_SI_BAND_T _band;	/* POLL_IN, POLL_OUT, POLL_MSG */
 			int _fd;
 		} _sigpoll;
+
+		/* SIGTRAP */
+		struct {
+			__kernel_pid_t _pid;	/* sender's pid */
+			__ARCH_SI_UID_T _uid;	/* sender's uid */
+			unsigned int _pt_flags;
+		} _sigtrap;
 	} _sifields;
 } siginfo_t;
 

@@ -116,6 +123,7 @@ typedef struct siginfo {
 #define si_addr_lsb	_sifields._sigfault._addr_lsb
 #define si_band		_sifields._sigpoll._band
 #define si_fd		_sifields._sigpoll._fd
+#define si_pt_flags	_sifields._sigtrap._pt_flags
 
 #ifdef __KERNEL__
 #define __SI_MASK	0xffff0000u

@@ -126,6 +134,7 @@ typedef struct siginfo {
 #define __SI_CHLD	(4 << 16)
 #define __SI_RT		(5 << 16)
 #define __SI_MESGQ	(6 << 16)
+#define __SI_TRAP	(7 << 16)
 #define __SI_CODE(T,N)	((T) | ((N) & 0xffff))
 #else
 #define __SI_KILL	0

@@ -135,6 +144,7 @@ typedef struct siginfo {
 #define __SI_CHLD	0
 #define __SI_RT		0
 #define __SI_MESGQ	0
+#define __SI_TRAP	0
 #define __SI_CODE(T,N)	(N)
 #endif
 

index 5b6128b..567e189 100644
@@ -73,6 +73,9 @@
 #define PTRACE_EVENT_EXIT	6
 #define PTRACE_EVENT_INTERRUPT	7
 
+/* flags in siginfo.si_pt_flags from PTRACE_GETSIGINFO */
+#define PTRACE_SI_STOPPED	0x00000001 /* tracee is job control stopped */
+
 #include <asm/ptrace.h>
 
 #ifdef __KERNEL__

index 2a8e930..b18a9b3 100644
@@ -528,11 +528,13 @@ static int ptrace_setoptions(struct task_struct *child, unsigned long data)
 
 static int ptrace_getsiginfo(struct task_struct *child, siginfo_t *info)
 {
+	struct signal_struct *sig;
 	unsigned long flags;
 	int error;
 
 	if (!lock_task_sighand(child, &flags))
 		return -ESRCH;
+	sig = child->signal;
 
 	error = -EINVAL;
 	if (!child->last_siginfo)

@@ -540,6 +542,17 @@ static int ptrace_getsiginfo(struct task_struct *child, siginfo_t *info)
 
 	error = 0;
 	*info = *child->last_siginfo;
+
+	/*
+	 * If reporting ptrace trap for a seized tracee, enable reporting
+	 * of info->si_pt_flags.
+	 */
+	if ((child->ptrace & PT_SEIZED) &&
+	    (info->si_code & (0x7f | ~0xffff)) == (__SI_TRAP | SIGTRAP)) {
+		/* report whether group stop is in effect w/ SI_STOPPED */
+		if (sig->group_stop_count || (sig->flags & SIGNAL_STOP_STOPPED))
+			info->si_pt_flags |= PTRACE_SI_STOPPED;
+	}
 out_unlock:
 	unlock_task_sighand(child, &flags);
 	return error;

index 7add912..9705f5d 100644
@@ -1842,7 +1842,7 @@ static void ptrace_notify_locked(int exit_code)
 
 	memset(&info, 0, sizeof info);
 	info.si_signo = SIGTRAP;
-	info.si_code = exit_code;
+	info.si_code = __SI_TRAP | exit_code;
 	info.si_pid = task_pid_vnr(current);
 	info.si_uid = current_uid();
 

@@ -2494,6 +2494,11 @@ int copy_siginfo_to_user(siginfo_t __user *to, siginfo_t *from)
 		err |= __put_user(from->si_band, &to->si_band);
 		err |= __put_user(from->si_fd, &to->si_fd);
 		break;
+	case __SI_TRAP:
+		err |= __put_user(from->si_pid, &to->si_pid);
+		err |= __put_user(from->si_uid, &to->si_uid);
+		err |= __put_user(from->si_pt_flags, &to->si_pt_flags);
+		break;
 	case __SI_FAULT:
 		err |= __put_user(from->si_addr, &to->si_addr);
 #ifdef __ARCH_SI_TRAPNO

1.7.1

Confused.

IOW, if the tracee reports via ptrace_notify*, the tracee can look at si_pt_flags == stop-in-effect. If the tracer reports a signal, the tracer obviously lacks this info, hmm.

Probably I need more time to get used to this... But at first glance this looks a bit unnatural. Say, can't we simply implement PTRACE_GET_GROUP_STOP_STATUS request which returns this (and probably more) info?

OK, probably I'll understand the intent later.

Heh. I am shy to admit, I didn't know copy_siginfo_to_user() trims si_code, that is why your change is correct but I spent a lot of time before I was able to understand this.

In this case this "si_code != 0" check is correct, but how can the tracer detect this case in general?

                     ^^^^^^^^^^^^^^^^^
                     PTRACE_GETSIGINFO

Forgot to mention... Probably we can ignore this but the tracer can set/clear __SI_TRAP via PTRACE_SETSIGINFO, after that PTRACE_GETSIGINFO changes its behaviour.

Oleg.

Hello,

Which indicates tracee is in group stop trap.

I don't know. PTRACE_GETSIGINFO seemed to already fit the bill and I want to avoid introducing a new request if at all possible. It sure is a bit quirky but doesn't compromisea functionality.

Oh, don't be shy. I scratched my head for quite a while trying to figure out why the hell the new flag field isn't getting out to userland. It's an ugly piece of sh*t. :-)

This was quick hack. Proper test would look like,

	si.si_code && (si.si_pt_flags & PTRACE_SI_STOPPED)

Right, I originally lifted the test from ptrace_notify() before adding __SI_TRAP and forgot to update it later. Will change.

Yeah, how about group_stop_in_effect()?

(answering to both get/setsiginfo concerns)

* I think we better block PTRACE_SETSIGINFO for non signal delivery traps. It doesn't make any sense. Let's just fail that with -EINVAL if PT_SEIZED.

* I don't think PTRACE_GETSIGINFO returning volatile information to be problematic. The information is generated on the fly on trap anyway. For non signal delivery traps, PTRACE_GETSIGINFO is basically (ab)using siginfo as a container for debugging information. It might have been better if something else was used from the beginning but the damage is already done and I don't see too much benefit in making things pretty at this point.

Thank you.

tejun

What do you mean?

si_pt_flags doesn't "exist" when the tracee reports the signal or CLD_STOPPED. This doesn't look clean.

I am not sure too, but the new request is much simpler to use, and it is more extensible. We can report more info. Say, the state of JOBCTL_STOP_CONSUME or something else.

This doesn't look right too? How can we know we can trust si_pt_flags? This needs some YES_YOU_CAN_CHECK_si_pt_flags(si_code), but I can't understand what it should do right now...

Or may me signal_stop_stopped(struct signal_struct *sig), like signal_group_exit/SIGNAL_GROUP_EXIT. But I am fine with group_stop_in_effect, probably it is more explanatorily.

Oh I agree, it does not make any sense. Should we change the current behaviour for PT_SEIZED? I don't really care, this looks minor.

Yes. And I'd understand if si_pt_flags was filled by the tracee during the trap (although I do not think this makes sense) to record the state at the time of this trap.

But PTRACE_GETSIGINFO returns the dynamic info which reflects the process-wide state at the time of syscall.

Oleg.

Hello,

I meant that whether group stop is in effect or not can be determined reliably in most cases. The only exception is signal delivery trap as siginfo there isn't from trap but I think we can survive that.

Sure, it ain't clean. Fully agreed.

Functionality-wise, I don't think GETSIGINFO is worse. We can add more flags and fields there too. It sure is cumbersome to use because the information isn't there for signal delivery and group stop traps; however, both aren't critical. Debugger can simply force INTERRUPT trap and get it there.

Ah, okay, you were asking how to tell si has the SIGTRAP info. IIUC, si_code is non-zero iff the information is generated from kernel so testing lower bits against SIGTRAP should be good enough. It can't happen by user generated signals. The only way to side step that would be using PTRACE_SETSIGINFO - but that's the debugger shooting its own foot. I don't feel particularly sympathetic.

Given that stop might be in progress (not complete yet), I think a name which clearly notes that is preferable. signal_stpo_stopped() sounds like it's already stopped.

Hmmm... I don't wanna change PTRACE_ATTACH behavior unless necessary. The pt_flags part isn't even reported if ATTACHed.

Hmmm... Well, if we're gonna introduce a new request, it's gonna have to be able to replace PTRACE_GETSIGINFO. I don't wanna ask userland to make two ptrace requests on each trap to tell what's going on.

Another point is that most programs would have to support pre-SEIZE behavior as well so I'd like to avoid deviating the interface too much if possible.

If you combine the two, we basically end up something which carries both siginfo and something else at one go. Its semantics can definitely be made prettier, I agree, but I'm not sure whether the benefits would be enough to justify a new request. Maybe it would be.

If you can propose something sane, it would be awesome.

Thanks.

tejun

For to-be-added end of group stop notification, ptracer needs to tell whether tracee is in group stop or INTERRUPT trap. Add JOBCTL_TRAPPED flag which is set when tracee commits to group stop or INTERRUPT trap in ptrace_stop() and cleared when leaving ptrace_stop().

The flag isn't yet used and doesn't cause any behavior difference.

---
 include/linux/sched.h |    1 +
 kernel/signal.c       |    9 +++++++++
 2 files changed, 10 insertions(+), 0 deletions(-)

index 221ab51..9d92444 100644
@@ -1787,6 +1787,7 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *
 #define JOBCTL_TRAP_SEIZE	(1 << 19) /* trap for seize */
 #define JOBCTL_TRAP_INTERRUPT	(1 << 20) /* trap for interrupt */
 #define JOBCTL_TRAPPING		(1 << 22) /* switching to TRACED */
+#define JOBCTL_TRAPPED		(1 << 23) /* trapped for group stop */
 
 #define JOBCTL_TRAP_MASK	(JOBCTL_TRAP_SEIZE | JOBCTL_TRAP_INTERRUPT)
 #define JOBCTL_PENDING_MASK	(JOBCTL_STOP_PENDING | JOBCTL_TRAP_MASK)

index 9705f5d..208f061 100644
@@ -1764,6 +1764,14 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
 	if (is_intr)
 		current->jobctl &= ~JOBCTL_TRAP_INTERRUPT;
 
+	/*
+	 * Traps for both actual group stop and INTERRUPT are considered
+	 * trapped for job control and have TRAPPED set.  This enables
+	 * end-of-group-stop retrapping of INTERRUPTed tracees.
+	 */
+	if (why == CLD_STOPPED || is_intr)
+		current->jobctl |= JOBCTL_TRAPPED;
+
 	spin_unlock_irq(&current->sighand->siglock);
 	read_lock(&tasklist_lock);
 	if (may_ptrace_stop()) {

@@ -1824,6 +1832,7 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
 	 * any signal-sending on another CPU that wants to examine it.
 	 */
 	spin_lock_irq(&current->sighand->siglock);
+	current->jobctl &= ~JOBCTL_TRAPPED;
 	current->last_siginfo = NULL;
 
 	/*

1.7.1

JOBCTL_TRAPPING is currently used to hide TASK_STOPPED -> TASK_TRACED transition on ptrace attach/seize. As such, it is set only while tracee is inside do_signal_stop() and gets cleread by entering TASK_TRACED in ptrace_stop(); however, if attach races with kill, ptrace_stop() can be skipped. To make sure the tracer is woken up in this case, task_clear_jobctl_trapping() is always called before leaving do_signal_stop().

To-be-added end of group stop notification will extend the use of JOBCTL_TRAPPING to move tracee from group stop trap to INTERRUPT trap or repeat INTERRUPT trap. This requires TASK_TRAPPING to be maintained inside signal delivery path.

Move the fallback clearing to the end of get_signal_to_deliver() so that TRAPPING is maintained while tracee is inside signal delivery path. When killed, tracee is guaranteed to leave signal delivery path in finite amount of time and thus TRAPPING is still guaranteed to be cleared on kill.

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

index 208f061..a7f65a6 100644
@@ -227,7 +227,10 @@ static inline void print_dropped_signal(int sig)
  * task_clear_jobctl_trapping - clear jobctl trapping bit
  * @task: target task
  *
- * If JOBCTL_TRAPPING is set, a ptracer is waiting for us to enter TRACED.
+ * If %JOBCTL_TRAPPING is set, ptracer is waiting for us to enter
+ * %TASK_TRACED.  It can be set only while we're inside do_signal_stop()
+ * and must be cleared before leaving signal delivery path.
+ *
  * Clear it and wake up the ptracer.  Note that we don't need any further
  * locking.  @task->siglock guarantees that @task->parent points to the
  * ptracer.

@@ -1978,9 +1981,6 @@ retry:
 		goto retry;
 	}
 
-	/* PTRACE_ATTACH might have raced with task killing, clear trapping */
-	task_clear_jobctl_trapping(current);
-
 	spin_unlock_irq(&current->sighand->siglock);
 
 	tracehook_finish_jctl();

@@ -2226,6 +2226,13 @@ relock:
 		do_group_exit(info->si_signo);
 		/* NOTREACHED */
 	}
+
+	/*
+	 * PTRACE_ATTACH might have raced with task killing.  Make sure
+	 * trapping is clear before leaving signal delivery path.
+	 */
+	task_clear_jobctl_trapping(current);
+
 	spin_unlock_irq(&sighand->siglock);
 	return signr;
 }

1.7.1

Mostly yes, but we can race with freeze_processes() and deadlock.

Hello,

Ah... the try_to_freeze(). Nice spotting. It isn't necessarily a deadlock tho. freeze_processes() will back out after a while. I think the right thing to do here is making the TRAPPING sleep an interruptible one and let the syscall restart logic deal with it. This will also remove the dreaded unkillable tracer even if we get something wrong somewhere. How does that sound?

Yeah, as I wrote before, I wanna do two things.

* Make TRAPPING wait INTERRUPTIBLE.

* Move clearing of pending group stop and traps to the actions which require such clearing.

Thanks.

tejun

Yes, it is not "fatal" for the system.

Oh, yes, we can. But this adds more complications again. We can fix this particular problem simpler. Say, freezer_do_not_count(). There are other options afaics.

I am starting to strongly dislike TRAPPING outside of do_signal_stop().

Not sure this is enough. OTOH, perhaps this is unnneeded.

I'll write another email to explain what I mean.

Oleg.

wait_task_stopped() tested task_stopped_code() without acquiring siglock and, if stop condition existed, called wait_task_stopped() and directly returned the result. This patch moves the initial task_stopped_code() testing into wait_task_stopped() and make wait_consider_task() fall through to wait_task_continue() on 0 return.

This is for the following two reasons.

* Because the initial task_stopped_code() test is done without acquiring siglock, it may race against SIGCONT generation. The stopped condition might have been replaced by continued state by the time wait_task_stopped() acquired siglock. This may lead to unexpected failure of WNOHANG waits.

This reorganization addresses this single race case but there are other cases - TASK_RUNNING -> TASK_STOPPED transition and EXIT_* transitions.

It seems that WNOHANG wait correctness has never been guaranteed and everybody has been happy with it for very long time. As such, although this reorganization improves the situation a bit, I don't consider this to be a bug fix.

* Scheduled ptrace updates require changes to the initial test which would fit better inside wait_task_stopped().

---
 kernel/exit.c |   30 +++++++++++++++++++++++-------
 1 files changed, 23 insertions(+), 7 deletions(-)

index 5cbc83e..3383793 100644
@@ -1377,11 +1377,23 @@ static int *task_stopped_code(struct task_struct *p, bool ptrace)
 	return NULL;
 }
 
-/*
- * Handle sys_wait4 work for one task in state TASK_STOPPED.  We hold
- * read_lock(&tasklist_lock) on entry.  If we return zero, we still hold
- * the lock and this task is uninteresting.  If we return nonzero, we have
- * released the lock and the system call should return.
+/**
+ * wait_task_stopped - Wait for %TASK_STOPPED or %TASK_TRACED
+ * @wo: wait options
+ * @ptrace: is the wait for ptrace
+ * @p: task to wait for
+ *
+ * Handle sys_wait4() work for %p in state %TASK_STOPPED or %TASK_TRACED.
+ *
+ * CONTEXT:
+ * read_lock(&tasklist_lock), which is released if return value is
+ * non-zero.  Also, grabs and releases @p->sighand->siglock.
+ *
+ * RETURNS:
+ * 0 if wait condition didn't exist and search for other wait conditions
+ * should continue.  Non-zero return, -errno on failure and @p's pid on
+ * success, implies that tasklist_lock is released and wait condition
+ * search should terminate.
  */
 static int wait_task_stopped(struct wait_opts *wo,
 				int ptrace, struct task_struct *p)

@@ -1397,6 +1409,9 @@ static int wait_task_stopped(struct wait_opts *wo,
 	if (!ptrace && !(wo->wo_flags & WUNTRACED))
 		return 0;
 
+	if (!task_stopped_code(p, ptrace))
+		return 0;
+
 	exit_code = 0;
 	spin_lock_irq(&p->sighand->siglock);
 

@@ -1607,8 +1622,9 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,
 	 * Wait for stopped.  Depending on @ptrace, different stopped state
 	 * is used and the two don't interact with each other.
 	 */
-	if (task_stopped_code(p, ptrace))
-		return wait_task_stopped(wo, ptrace, p);
+	ret = wait_task_stopped(wo, ptrace, p);
+	if (ret)
+		return ret;
 
 	/*
 	 * Wait for continued.  There's only one continued state and the

1.7.1

Hmm. This is the plain bug, even if unlikely and minor.

Yes, the window is tiny. May be it was never noticed or never reported because this is hard to diagnose/reproduced.

But it is?

Can't we push this patch ahead of these changes? I can merge it into ptrace branch.

Hello,

Yeah, most likely.

It doesn't really fix the problem tho. The whole thing is full of holes and I think it would be better to just declare "WNOHANG might fail even when it's not supposed to, retry later" than making the locking heavier there, which could easily be much more relevant regression. Of course, if we can fix it without adding extra locking or too much complexity, it would be nice.

Anyways, yeah, sure. I'll resend it as a separate patch.

Thanks.

tejun

Hmm. Could you explain? (unless you mean ptrace holes)

Thanks, I'll aply the patch you sent today.

Oleg.

Hello,

I meant other cases, RUNNING -> STOPPED and EXIT_* transitions. Sleeping wait(2) is reliable without grabbing siglock thanks to setting TASK_INTERRUPTIBLE on start and events waking up the waiter after updating the state, so wait(2) is guaranteed to check the states at least once after change actually has happened.

WNOHANG disables that mechanism. We probably can add another set of memory barrier hackery to fix that, but that's gonna be ugly.

Thanks.

tejun

Yes, this is clear. WNOHANG can "race" with the transitions above. But we do not care, this is like reading the word which can be changed by another thread, no?

But this bug is different. Say, the parent does wait(WNOWAIT) and gets CLD_STOPPED. After that it has all rights to assume that wait(WNOHANG) must report either STOPPED or CONTINUED.

Oleg.

Hello,

They aren't that different. Please consider the following program.

  #define PTRACE_SEIZE		0x4206
  #define PTRACE_INTERRUPT	0x4207

  #define PTRACE_SEIZE_DEVEL	0x80000000

  static const struct timespec ts1ms = { .tv_nsec = 1000000 };

int main(int argc, char **argv)

  {
	  pid_t child, control;

	  child = fork();
	  if (!child)
		  while (1)
			  pause();

	  kill(child, SIGSTOP);
	  waitid(P_PID, child, NULL, WSTOPPED | WNOWAIT);

	  control = fork();
	  if (!control) {
		  while (1) {
			  kill(child, SIGCONT);
			  nanosleep(&ts1ms, NULL);
			  kill(child, SIGSTOP);
			  nanosleep(&ts1ms, NULL);
		  }
	  }

	  while (1) {
		  siginfo_t si = {};

		  waitid(P_PID, child, &si,
			 WSTOPPED | WCONTINUED | WNOWAIT | WNOHANG);
		  if (!si.si_pid)
			  break;
	  }

	  kill(control, SIGKILL);
	  kill(child, SIGKILL);
	  return 0;

}

waitid(2) should always succeed as it's never consuming wait state, but it does, with or without the patch. All transitions need to be made water tight to remove the bug.

Thanks.

tejun

I meant "but it does fail (0 in si.si_pid)".

tejun

Damn, you are right, I think.

At first glance, do_wait() does

	wait_task_stopped();
	wait_task_continued();

and the state can be changed CONTINIUED -> STOPPED in between, right? Or something else?

Thanks, for the explanation.

Oleg.

Hello,

Yay, for once!

Yeah and exit transitions too. There simply is no synchronization there. We can probably solve it without acquiring siglock by adding "clear this before making state transitions" flag followed by a mb() and then making children clear it before transitions. Then, we can retry WNOHANG wait if the flag is cleared after checking.

Thanks.

tejun

Hi,

I am not sure... but probably this depends on definition.

We already checked ->exit_state != ZOMBIE, and we are holding tasklist. The child can't exit. I mean, it can't change its ->exit_state.

However, SIGKILL can clear SIGNAL_STOP_STOPPED, and we can "miss" it. But this looks correct, the child is no longer stopped but it is still not dead. So I think in this case wait(WNOHANG | WEXITED | WSTOPPED) can fail, notabug.

OTOH, perhaps SIGKILL should set SIGNAL_STOP_CONTINUED in this case? And keep it if it was already set.

perhaps even simpler if ->EXIT transition is fine.

Oleg.

Hello, Oleg.

Hmmm... I don't know.

I'd rather avoid that. First of all, it's an extreme corner case and I think introducing an extra state transition there is more likely to cause trouble than helping anything. It might be theoretically correct but there's no reason to introduce that at this point. If at all possible, I think it would be better to make it either see STOPPED or EXIT.

Thanks.

tejun

On Thu, 12 May 2011 19:32:28 +0200, Tejun Heo wrote:
[...]

It may be not related, I do not inderstand the kernel internals being discussed.

I was told by Roland McGrath that after SIGCHLD of a ptrace even from tracee invokes sighandler for that SIGCHLD in the tracer then waitpid(WNOHANG) still may return 0 as the signal is not yet ready. I was not able to reproduce it with a testcase myself.

But if it is a case it should be fixed as there is no later notification when to call waitpid(WNOHANG) again. And sure the debugger cannot busy-loop poll it.

Thanks,
Jan

Hello, Jan.

Hmmm... I could easily be wrong but AFAICS that shouldn't happen. If you can reproduce the problem, please let us know.

But, the current WNOHANG wait is racy. It's unlikely but definitely possible for WNOHANG to fail when it's expected to succeed (not the above case but more convoluted ones).

But, just out of curiosity, is there any reason the ptracer itself should be doing something other than waitpid() while tracee is running? It's not like ptrace requests can be issued during that time and sleeping waitpid() is way saner mechanism to wait for tracee events than signal.

Thanks.

tejun

Hello,

BTW, I'm pretty close to send the second take and the INTERRUPT trap has changed quite a bit. It got renamed to PTRACE_EVENT_STOP and now acts as both group stop trap and INTERRUPT trap. IOW, PTRACE_INTERRUPT makes tracee go park at group stop trap site. The whole thing is much prettier this way. Well, being ptrace, it's not really pretty but at least it is much less disgusting.

Thanks.

tejun

Hi Tejun,

I was unable to reproduce it before so OK, I will follow now your statement.

OK, so FYI it breaks current GDB.

If the debugger wants to be single-threaded ("poll() model", not "threads model") and it wants to communicate with user and examine debuggee symbols and memory data it cannot use sleeping wait. GDB is single-threaded and it supports `set target-async 1': info '(gdb)Background Execution'

Thanks,
Jan

Hey, again.

That might be true but the race cases are very obscure. Not sure whether the race conditions could actually affect ptracer. With SEIZE, it won't, I think.

I don't think target-async is necessarily related. It doesn't really matter whether the execution per-se is async or not. The ptracer can be a separate thread regardless and the interlocking can be added on top (or not).

Anyways, I would recommend using sleeping wait(2)'s for ptrace event tracking. Well, signal notification would work but I think that would be much more error-prone and has much higher chance of being fragile.

Thanks.

tejun

Hi Tejun,

It cannot as various GDB-supported platforms do not support threads properly. And you want to have a common codebase to get it will supported.

Besides that it is a matter of coding style, I perfer "poll() model" even on threads-supporting GNU/Linux.

You cannot as I described above.

Thanks,
Jan

Hey,

The above wouldn't be linux kernel's top design concerns, right?

If you prefer that, go ahead. It's not like new interface is gonna break that, but I think it's gonna be more fragile. Well, it shouldn't be worse than now.

Thanks.

tejun

Hi Tejun,

They are - as without threads one needs reliable WNOHANG - while which you say it is not reliable.

BTW I really do not think this WNOHANG issue is anyhow related to this whole discussion.

Thanks,
Jan

Hello,

As I wrote before, I think it's reliable for ptrace. The problematic ones are transitions between continued and stopped and running and I don't think it would affect ptrace.

Probably not but I've seen gdb going out of sync with the expected state of the tracee. Dunno whether it was kernel bug or gdb bug but it seemed pretty fragile and adding group stop notifications might make it a bit more difficult. I don't know. It was just my five cents. Feel free to ignore.

Thanks.

tejun

Currently, JOBCTL_TRAPPING is used by PTRACE_ATTACH and SEIZE to hide TASK_STOPPED -> TRACED transition from ptracer. If tracee is in group stop, TRAPPING is set, tracee is kicked and tracer waits for the transition to complete before completing attach. This prevents tracer from seeing tracee during transition.

The transition is visible only through wait(2) and following ptrace(2) requests. Without TRAPPING, WNOHANG which should succeed right after attach (when tracer knows tracee was stopped) might fail and likewise for the following ptrace requests.

TRAPPING will also be used to implement end of group stop retrapping, which can be initiated by tasks other than tracer. To allow this, this patch moves TRAPPING wait from attach completion path to operations which are actually affected by the transition - wait(2) and following ptrace(2) requests.

As reliably checking and modifying TASK_STOPPED/TRACED transition together with JOBCTL_TRAPPING require siglock and both ptrace and wait paths are holding tasklist_lock and siglock where TRAPPING check is needed, ptrace_wait_trapping() assumes both locks to be held on entry and releases them if it actually had to wait for TRAPPING.

Both wait and ptrace paths are updated to retry the operation after TRAPPING wait. Note that wait_task_stopped() now always grabs siglock for ptrace waits. This can be avoided with "task_stopped_code() -> rmb() -> TRAPPING -> rmb() -> task_stopped_code()" sequence but given that ptrace isn't particularly sensitive to performance or scalability, choosing simpler implementation seems better.

---
 include/linux/ptrace.h |    1 +
 include/linux/sched.h  |    2 +-
 kernel/exit.c          |   21 +++++++++++++++++++--
 kernel/ptrace.c        |   48 +++++++++++++++++++++++++++++++++++++++++++++---
 kernel/signal.c        |    5 +++--
 5 files changed, 69 insertions(+), 8 deletions(-)

index 567e189..4b42a15 100644
@@ -116,6 +116,7 @@ extern long arch_ptrace(struct task_struct *child, long request,
 extern int ptrace_readdata(struct task_struct *tsk, unsigned long src, char __user *dst, int len);
 extern int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned long dst, int len);
 extern void ptrace_disable(struct task_struct *);
+extern bool ptrace_wait_trapping(struct task_struct *child);
 extern int ptrace_check_attach(struct task_struct *task, bool ignore_state);
 extern int ptrace_request(struct task_struct *child, long request,
 			  unsigned long addr, unsigned long data);

index 9d92444..972f1db 100644
@@ -1786,7 +1786,7 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *
 #define JOBCTL_STOP_CONSUME	(1 << 18) /* consume group stop count */
 #define JOBCTL_TRAP_SEIZE	(1 << 19) /* trap for seize */
 #define JOBCTL_TRAP_INTERRUPT	(1 << 20) /* trap for interrupt */
-#define JOBCTL_TRAPPING		(1 << 22) /* switching to TRACED */
+#define JOBCTL_TRAPPING		(1 << 22) /* switching to TRACED/STOPPED */
 #define JOBCTL_TRAPPED		(1 << 23) /* trapped for group stop */
 
 #define JOBCTL_TRAP_MASK	(JOBCTL_TRAP_SEIZE | JOBCTL_TRAP_INTERRUPT)

index 3383793..a43af3a 100644
@@ -1409,15 +1409,29 @@ static int wait_task_stopped(struct wait_opts *wo,
 	if (!ptrace && !(wo->wo_flags & WUNTRACED))
 		return 0;
 
-	if (!task_stopped_code(p, ptrace))
+	/*
+	 * For ptrace waits, we can't reliably check whether wait condition
+	 * exists without grabbing siglock due to JOBCTL_TRAPPING
+	 * transitions.  A task might be temporarily in TASK_RUNNING while
+	 * trapping which should be transparent to the ptracer.
+	 *
+	 * Note that we can avoid unconditionally grabbing siglock with by
+	 * wrapping TRAPPING test with two rmb's; however, let's stick with
+	 * simpler implementation for now.
+	 */
+	if (!ptrace && !(p->signal->flags & SIGNAL_STOP_STOPPED))
 		return 0;
 
 	exit_code = 0;
 	spin_lock_irq(&p->sighand->siglock);
 
 	p_code = task_stopped_code(p, ptrace);
-	if (unlikely(!p_code))
+	if (unlikely(!p_code)) {
+		/* if trapping, wait for it and restart the whole process */
+		if (ptrace && ptrace_wait_trapping(p))
+			return -EAGAIN;
 		goto unlock_sig;
+	}
 
 	exit_code = *p_code;
 	if (!exit_code)

@@ -1740,6 +1754,9 @@ notask:
 		}
 	}
 end:
+	/* -EAGAIN if we hit trapping ptracee and slept - retry */
+	if (unlikely(retval == -EAGAIN))
+		goto repeat;
 	__set_current_state(TASK_RUNNING);
 	remove_wait_queue(&current->signal->wait_chldexit, &wo->child_wait);
 	return retval;

index a7f65a6..dce2961 100644
@@ -228,8 +228,9 @@ static inline void print_dropped_signal(int sig)
  * @task: target task
  *
  * If %JOBCTL_TRAPPING is set, ptracer is waiting for us to enter
- * %TASK_TRACED.  It can be set only while we're inside do_signal_stop()
- * and must be cleared before leaving signal delivery path.
+ * %TASK_TRACED or %TASK_STOPPED.  It can be set only while we're inside
+ * do_signal_stop() and must be cleared before leaving signal delivery
+ * path.
  *
  * Clear it and wake up the ptracer.  Note that we don't need any further
  * locking.  @task->siglock guarantees that @task->parent points to the

1.7.1

I didn't read the next patch yet, so I can't undestand/comment the motivation.

But,

You know, I'd wish I could find the serious bugs in this patch. The code becomes really hairy. -EAGAIN in do_wait() doesn't make it more simple ;)

And so far I think this would be better, because it seems we can avoid the retry logic.

First of all, this patch returns one of the user-visible and undesirable changes. The tracer know that the task is stopped, attaches, and then it can see the TASK_RUNNING tracee after ptrace(PTRACE_ATTACH) returns.

I agree, this looks minor. But if we can tolerate this, probably we can tolerate another oddity: wait_task_stopped() can succeed and eat the stop code before the tracee actually stopps, no?

IOW, ignoring mb's and read-ordering, suppose that we simply change task_stopped_code:

		if (ptrace) {
	-		if (task_is_stopped_or_traced(p))
	+		if (task_is_traced(p) || JOBCTL_TRAPPING)
				return &p->exit_code;
		} else {

As for ptrace_check_attach(), it can simply do wait_event(), we only need to verify the caller is the tracer. No need to play with lock/unlock/retry.

What do you think?

Oleg.

Mwahaha! I seem to see the bug ;)

	bool ptrace_wait_trapping(struct task_struct *child)
		__releases(&child->sighand->siglock)
		__releases(&tasklist_lock)
	{
		if (likely(!(child->jobctl & JOBCTL_TRAPPING)))
			return false;

		spin_unlock_irq(&child->sighand->siglock);
		get_task_struct(child);
		read_unlock(&tasklist_lock);

--> WINDOW

		wait_event(current->signal->wait_chldexit,
			   !(child->jobctl & JOBCTL_TRAPPING));
		put_task_struct(child);
		return true;
	}

When the caller is do_wait(), we can't assume we are the tracer when we drop tasklist. Original tracer can detach, then another unrelated process can attach again and provoke JOBCTL_TRAPPING. wait_event() can hang forever.

Unfortunately, this is easy to fix :/

Oleg.

Hello,

:-)

Yeah, it's basically the same problem raised in other two patches. Making wait interruptible and clearing pending stop/traps on detach/kill/cont/whatever should do the trick. I'll reply the previous message for the suggested change.

Thanks.

tejun

Hello,

I don't know. Why is retrying hairy? The whole waiting logic is built for clean retries. The suggested change just does it without intervening sleeping and waking up. I don't see anything particularly hairy there.

Well, the above memory barrier dance wouldn't really change whether retry logic is required or not and I'd _really_ like to avoid complex barrier dances. Even the typical write-B wmb() write-A / read-A rmb() read-B barriers often confuse people. I don't wanna throw in stacked wmb()/rmb() pairs there even if that means an extra locking for ptrace waits.

Yes, it does. Sorry about forgetting to mention it in the patch description. I believe this is something we can swallow.

Hmmm... interesting. Yeah, the state is visible only through wait(2) and ptrace(2) and for wait(2) TRAPPING is as good as STOPPED/TRACED and we can wait all we want in ptrace_check_attach(). I'll think more about it but seems like a nice idea.

Thank you.

tejun

Hello, Oleg.

Eh, problem. Please consider the following scenario.

* A task is in TASK_STOPPED. current->exit_code contains zero.

* Tracer seizes the task which triggers TRAPPING.

* Tracer wait(2)s which sees TRAPPING but tracee->exit_code is still zero.

I think retrying and ensuring that tracee is in the expected state after going through the regular trapping procedure is better way to handle this. Bypassing wait(2) seems smart and simple but it entails much more subtleties than simple stupid waiting/retrying.

Thanks.

tejun

At first glance, this is easy to fix. do_signal_stop() can set ->exit_code unconditionally as it did before.

Agreed. But this wait_event(!TRAPPING) logic has a lot of subtleties too.

Oleg.

As always, this is subjective. But I didn't mean -EAGAIN itself. In fact I was going to add this (simple) logic some time ago and kill the EXIT_DEAD state. Hmm, and I'd still like to do this...

I meant the whole ptrace_wait_trapping() + lock dance + retry thing. But of course I do not pretend my feeling is right.

Also. _Perhaps_ we can rethink the SIGCONT trapping, and perhaps in this case do_wait() won't need any changes. May be.