ptrace: implement PTRACE_SEIZE/INTERRUPT and group stop notification, take#2

Hello,

This is the second try at implementing PTRACE_SEIZE/INTERRUPT and group stop notification. Notable changes from the first take[1] are,

* Prep patches moved to a separate patchset[2].

* JOBCTL_EVENT_INTERRUPT renamed to JOBCTL_EVENT_STOP and now also used for group stop. This means that PTRACE_GETSIGINFO will always return a valid siginfo. For signal delivery traps, it will contain the siginfo for the signal being delivered. For all other traps, it will contain the trap information which includes group stop info.

* JOBCTL_TRAP_STOP trap doesn't carry signo in si_code and the signal which initiated the group stop currently in effect can be obtained from siginfo.si_signo.

* JOBCTL_INTERRUPT triggers JOBCTL_EVENT_STOP and gets cleared on any trap, so the initial trap by JOBCTL_SEIZE is identical to JOBCTL_INTERRUPT.

* PTRACE_SETSIGINFO disallows overriding __SI_TRAP siginfo.

* Re-trap delay mechanism added to avoid re-trapping while ptrace request is in progress.

So, PTRACE_INTERRUPT simply puts the tracee into the same state as group stop and this simplifies things a lot. Trap siginfo is also accessible during group stop and group stop notification happening during PTRACE_EVENT_STOP makes sense and there's no awkward careful sequencing of traps to avoid unnecessary traps. I think it makes whole lot more sense this way.

This patchset contains the following ten patches.

0001-signal-remove-three-noop-tracehooks.patch 0002-job-control-introduce-JOBCTL_TRAP_STOP-and-use-it-fo.patch 0003-ptrace-implement-PTRACE_SEIZE.patch 0004-ptrace-implement-PTRACE_INTERRUPT.patch 0005-ptrace-restructure-ptrace_getsiginfo.patch 0006-ptrace-add-siginfo.si_pt_flags.patch 0007-ptrace-make-group-stop-state-visible-via-PTRACE_GETS.patch 0008-ptrace-don-t-let-PTRACE_SETSIGINFO-override-__SI_TRA.patch 0009-ptrace-add-JOBCTL_BLOCK_NOTIFY.patch 0010-ptrace-implement-group-stop-notification-for-ptracer.patch

0001 removes three signal tracehooks. Will be moved to the prep patchset on the next round.

0002 introduces TRAP_STOP and 0003-0004 implement SEIZE and INTERRUPT.

0005-0008 expose group stop info via GETSIGINFO.

0009-0010 implement group stop notification.

This patchset is on top of Oleg's ptrace branch[3] + prep patchset[2] and available in the following git branch.

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

The HEAD is 8026ce454dd46dc3c8cf6bbc57bcd30f5efa4e55. If you see older branch, please retry after a while (korg is still syncing).

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 +
 include/asm-generic/siginfo.h         |   10 +
 include/linux/ptrace.h                |   13 ++
 include/linux/sched.h                 |    6 
 include/linux/tracehook.h             |   52 --------
 kernel/ptrace.c                       |  202 +++++++++++++++++++++++++++----
 kernel/signal.c                       |  215 +++++++++++++++++++++++-----------
 21 files changed, 452 insertions(+), 147 deletions(-)

--
tejun

[1] http://thread.gmane.org/gmane.linux.kernel/1136930
[2] http://thread.gmane.org/gmane.linux.kernel/1139751
[3] git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc.git

Remove the following three noop tracehooks in signals.c.

* tracehook_force_sigpending()
* tracehook_get_signal()
* tracehook_finish_jctl()

The code area is about to be updated and these hooks don't do anything other than obfuscating the logic.

---
 include/linux/tracehook.h |   52 ---------------------------------------------
 kernel/signal.c           |   44 ++++++++++++--------------------------
 2 files changed, 14 insertions(+), 82 deletions(-)

index b073f3c..85dae19 100644
@@ -425,58 +425,6 @@ static inline int tracehook_consider_fatal_signal(struct task_struct *task,
 	return (task_ptrace(task) & PT_PTRACED) != 0;
 }
 
-/**
- * tracehook_force_sigpending - let tracing force signal_pending(current) on
- *
- * Called when recomputing our signal_pending() flag.  Return nonzero
- * to force the signal_pending() flag on, so that tracehook_get_signal()
- * will be called before the next return to user mode.
- *
- * Called with @current->sighand->siglock held.
- */
-static inline int tracehook_force_sigpending(void)
-{
-	return 0;
-}
-
-/**
- * tracehook_get_signal - deliver synthetic signal to traced task
- * @task:		@current
- * @regs:		task_pt_regs(@current)
- * @info:		details of synthetic signal
- * @return_ka:		sigaction for synthetic signal
- *
- * Return zero to check for a real pending signal normally.
- * Return -1 after releasing the siglock to repeat the check.
- * Return a signal number to induce an artifical signal delivery,
- * setting *@info and *@return_ka to specify its details and behavior.
- *
- * The @return_ka->sa_handler value controls the disposition of the
- * signal, no matter the signal number.  For %SIG_DFL, the return value
- * is a representative signal to indicate the behavior (e.g. %SIGTERM
- * for death, %SIGQUIT for core dump, %SIGSTOP for job control stop,
- * %SIGTSTP for stop unless in an orphaned pgrp), but the signal number
- * reported will be @info->si_signo instead.
- *
- * Called with @task->sighand->siglock held, before dequeuing pending signals.
- */
-static inline int tracehook_get_signal(struct task_struct *task,
-				       struct pt_regs *regs,
-				       siginfo_t *info,
-				       struct k_sigaction *return_ka)
-{
-	return 0;
-}
-
-/**
- * tracehook_finish_jctl - report about return from job control stop
- *
- * This is called by do_signal_stop() after wakeup.
- */
-static inline void tracehook_finish_jctl(void)
-{
-}
-
 #define DEATH_REAP			-1
 #define DEATH_DELAYED_GROUP_LEADER	-2
 

1.7.1

Btw, I think we should kill that stupid <linux/tracehook.h> layer entirely. It consists of a few relatively useful function which should just be renamed to ptrace_* and moved to ptrace.c, and a lot of entirely pointless wrappers which generally have only a single user, and often tons of arguments that aren't actually used.

Hello,

Yeap, agreed. It's on the todo list.

Thanks.

tejun

Heh. What can I say? Yes, this is true. Of course this breaks the out-of-tree project, completely, and you even know its name ;) Although tracehook_get_signal() logic was already broken by other changes...

Hello,

Yeap, but with ptrace updates, tracehook behaviors, which can't even be fully determined without consulting out-of-tree code, are bound to change. Now that ptrace is receiving more capabilities, I don't see much point in maintaining them.

Thanks.

tejun

Hi,

I didn't try to argue against this change. Although it will certainly complicate my life ;)

Btw, this patch conflicts with 25985edcedea6396277003854657b5f3cb31a628 "Fix common misspellings", trivial.

Oleg.

do_signal_stop() implemented both normal group stop and trap for group stop while ptraced. This approach has been enough but scheduled changes require trap mechanism which can be used in more generic manner and using group stop trap for generic trap site simplifies both userland visible interface and implementation.

This patch adds a new jobctl flag - JOBCTL_TRAP_STOP. When set, it triggers a trap site, which behaves like group stop trap, in get_signal_to_deliver() before checking for pending signals. While ptraced, do_signal_stop() doesn't stop itself. It initiates group stop if requested and schedules JOBCTL_TRAP_STOP and returns, which makes its caller - get_signal_to_deliver() - to relock, check and enter the trap.

Although this adds an unlock-relocking between checking of JOBCTL_STOP_PENDING and actually trapping for STOP, this doesn't affect correctness. ptrace_stop() already had conditional unlock-relocking depending on arch and, if SIGCONT is generated inbetween, it's ignored as if it were received after the task entered TASK_TRACED. The extra unlock-relocking follows the same rule and the race window will be properly handled by notification mechanism which will be added later.

ptrace_attach() is updated to use JOBCTL_TRAP_STOP instead of JOBCTL_STOP_PENDING and __ptrace_unlink() to clear all pending bits so that TRAP_STOP and future trap bits don't linger after detach.

While at it, add proper function comment to do_signal_stop() and make it return bool.

---
 include/linux/sched.h |    4 ++-
 kernel/ptrace.c       |   10 +++++-
 kernel/signal.c       |   80 +++++++++++++++++++++++++++++++------------------
 3 files changed, 62 insertions(+), 32 deletions(-)

index 86f21e7..d8a11cb 100644
@@ -1784,9 +1784,11 @@ 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_STOP	(1 << 19) /* trap for STOP */
 #define JOBCTL_TRAPPING		(1 << 21) /* switching to TRACED */
 
-#define JOBCTL_PENDING_MASK	JOBCTL_STOP_PENDING
+#define JOBCTL_TRAP_MASK	JOBCTL_TRAP_STOP
+#define JOBCTL_PENDING_MASK	(JOBCTL_STOP_PENDING | JOBCTL_TRAP_MASK)
 
 extern void task_clear_jobctl_pending(struct task_struct *task,
 				      unsigned int mask);

index 9e0423c..7f02129 100644
@@ -127,6 +127,12 @@ void __ptrace_unlink(struct task_struct *child)
 	spin_lock(&child->sighand->siglock);
 
 	/*
+	 * Clear jobctl bits owned by this tracer along with STOP_PENDING,
+	 * which is reinstated below if necessary.
+	 */
+	task_clear_jobctl_pending(child, JOBCTL_PENDING_MASK);
+
+	/*
 	 * Reinstate JOBCTL_STOP_PENDING if group stop is in effect and
 	 * @child isn't dead.
 	 */

@@ -292,7 +298,7 @@ static int ptrace_attach(struct task_struct *task)
 	spin_lock(&task->sighand->siglock);
 
 	/*
-	 * If the task is already STOPPED, set JOBCTL_STOP_PENDING and
+	 * If the task is already STOPPED, set JOBCTL_TRAP_STOP 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.

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

1.7.1

Heh. Starting from this patch, I think I will never understand this code in details ;)

Well, I think it does affect. Although the problem is minor.

Yes, but ptrace_stop() can't send the CLD_STOPPED notfication before the tracee is ready for do_wait(WNOHANG). Contrary, get_signal_to_deliver() can if we race with SIGCONT.

Well, the traced task didn't participate yet... Nevermind.

So far I can't really understand why do we have both JOBCTL_TRAP_STOP and and JOBCTL_TRAPPING... Please ignore, I didn't read other patches yet.

Hmm. And afaics there is a bug in do_signal_stop(), after
"[PATCH 6/9] job control: make task_clear_jobctl_pending() clear TRAPPING automatically" ->jobctl &= ~JOBCTL_STOP_SIGMASK is no longer safe. We can clear _TRAPPING without wakeup.

Oleg.

OK, SEIZE sets JOBCTL_TRAP_STOP.

And this also affects ptrace_attach(PTRACE_SEIZE), TRAP_STOP is cleared as well.

Oleg.

Hello, Oleg.

That CLD_STOPPED notification is actually for continuation (although it's indistinguishible from actual stopped notification) and as such the ptracer has to query the tracee state after the notification signal and can't expect it to be in TRACED. Please consider the following scenario.

1. thread t1 and t2. t1 is ptraced. Both running.

2. SIGSTOP delivered by t2. Group stop starts.

3. SIGCONT is generated before t1 participates.

4. t1 will enter get_signal_to_deliver() and deliver CLD_STOPPED but won't trap.

So, I don't think this affects correctness. The above can happen if both t1 and t2 are attached by the same ptracer. What changes is that it now may happen with single thread too. We can put STOP trap before CONTINUED notification but I don't think that's necessary.

Eh, should have read this earlier. :-)

JOBCTL_TRAP_STOP is the renamed JOBCTL_TRAP_INTERRUPT. It's a trap condition while TRAPPING is synchronization flag to protect -> TRACED transitions. JOBCTL_TRAP_STOP might not be the best name, but it isn't INTERRUPT trap anymore JOBCTL_TRAP_GROUP_STOP or JOBCTL_TRAP_GSTOP seemed a bit inconsistent with other flags.

Hmmm? ->jobctl &= ~JOBCTL_STOP_SIGMASK clears lower sixteen bits. All the flags including TRAPPING live in the upper sixteen bits.

Thank you.

tejun

OK, agreed.

Indeed, I was confused. Thanks for correcting me.

Oleg.

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 (e.g. signal delivery), they might be taken; otherwise, a trap with exit_code == (SIGTRAP | PTRACE_EVENT_STOP << 8) is taken. If seized, this trap is also used for group stop traps instead of exit_code == 0 with NULL GETSIGINFO.

* PTRACE_SEIZE doesn't affect signal or group stop state.

* After PTRACE_SEIZE, one trap will happen which might be a PTRACE_EVENT_STOP trap.

* If PTRACE_SEIZE'd, group stop also uses PTRACE_EVENT_STOP trap which uses exit_code of (SIGTRAP | PTRACE_EVENT_STOP << 8) instead of the stopping signal number and returns usual trap siginfo on PTRACE_GETSIGINFO instead of NULL.

Note that there currently is no way to find out the stopping signal number while seized. This will be improved by future patches.

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 +++++++
 kernel/ptrace.c        |   38 ++++++++++++++++++++++++++++++++------
 kernel/signal.c        |   32 ++++++++++++++++++++++++--------
 3 files changed, 63 insertions(+), 14 deletions(-)

index 6b359cd..3fd389d 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_STOP	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 7f02129..7aefd43 100644
@@ -254,10 +254,28 @@ bool ptrace_may_access(struct task_struct *task, unsigned int mode)
 	return !err;
 }
 
-static int ptrace_attach(struct task_struct *task)
+static int ptrace_attach(struct task_struct *task, long request,
+			 unsigned long flags)
 {
+	bool seize = (request == PTRACE_SEIZE);
 	int retval;
 
+	/*
+	 * SEIZE will enable new ptrace behaviors which will be implemented
+	 * gradually.  SEIZE_DEVEL is used to prevent applications
+	 * expecting full SEIZE behaviors trapping on kernel commits which
+	 * are still in the process of implementing them.
+	 *
+	 * Only test programs for new ptrace behaviors being implemented
+	 * should set SEIZE_DEVEL.  If unset, SEIZE will fail with -EIO.
+	 *
+	 * Once SEIZE behaviors are completely implemented, this flag and
+	 * the following test will be removed.
+	 */
+	retval = -EIO;
+	if (seize && !(flags & PTRACE_SEIZE_DEVEL))
+		goto out;
+
 	audit_ptrace(task);
 
 	retval = -EPERM;

@@ -289,11 +307,16 @@ static int ptrace_attach(struct task_struct *task)
 		goto unlock_tasklist;
 
 	task->ptrace = PT_PTRACED;
+	if (seize)
+		task->ptrace |= PT_SEIZED;
 	if (task_ns_capable(task, CAP_SYS_PTRACE))
 		task->ptrace |= PT_PTRACE_CAP;
 
 	__ptrace_link(task, current);
-	send_sig_info(SIGSTOP, SEND_SIG_FORCED, task);
+
+	/* SEIZE uses TRAP_STOP instead of SIGSTOP for initial trap */
+	if (!seize)
+		send_sig_info(SIGSTOP, SEND_SIG_FORCED, task);
 
 	spin_lock(&task->sighand->siglock);
 

@@ -315,6 +338,9 @@ static int ptrace_attach(struct task_struct *task)
 	if (task_is_stopped(task)) {
 		task->jobctl |= JOBCTL_TRAP_STOP | JOBCTL_TRAPPING;
 		signal_wake_up(task, 1);
+	} else if (seize) {
+		task->jobctl |= JOBCTL_TRAP_STOP;
+		signal_wake_up(task, 0);
 	}
 
 	spin_unlock(&task->sighand->siglock);

@@ -826,8 +852,8 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
 		goto out;
 	}
 
-	if (request == PTRACE_ATTACH) {
-		ret = ptrace_attach(child);
+	if (request == PTRACE_ATTACH || request == PTRACE_SEIZE) {
+		ret = ptrace_attach(child, request, data);
 		/*
 		 * Some architectures need to do book-keeping after
 		 * a ptrace attach.

@@ -968,8 +994,8 @@ asmlinkage long compat_sys_ptrace(compat_long_t request, compat_long_t pid,
 		goto out;
 	}
 
-	if (request == PTRACE_ATTACH) {
-		ret = ptrace_attach(child);
+	if (request == PTRACE_ATTACH || request == PTRACE_SEIZE) {
+		ret = ptrace_attach(child, request, data);
 		/*
 		 * Some architectures need to do book-keeping after
 		 * a ptrace attach.

index 50a4e8a..84e75db 100644
@@ -1840,7 +1840,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_do_notify(int exit_code, int why)
 {
 	siginfo_t info;
 

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

@@ -2079,14 +2084,25 @@ relock:
 	}
 
 	/*
-	 * Take care of ptrace jobctl traps.  It currently is only used to
-	 * trap for group stop while ptraced.
+	 * Take care of ptrace jobctl traps.
+	 *
+	 * When PT_SEIZED, it's used for both group stop and explicit
+	 * SEIZE/INTERRUPT traps.  Both generate PTRACE_EVENT_STOP trap
+	 * with accompanying siginfo.
+	 *
+	 * When !PT_SEIZED, it's used only for group stop trap with
+	 * CLD_STOPPED as exit_code and no siginfo.
 	 */
 	if (unlikely(current->jobctl & JOBCTL_TRAP_MASK)) {
-		signr = current->jobctl & JOBCTL_STOP_SIGMASK;
-		WARN_ON_ONCE(!signr);
-		ptrace_stop(signr, CLD_STOPPED, 0, NULL);
-		current->exit_code = 0;
+		if (current->ptrace & PT_SEIZED) {
+			ptrace_do_notify(SIGTRAP | PTRACE_EVENT_STOP << 8,
+					 CLD_STOPPED);
+		} else {
+			signr = current->jobctl & JOBCTL_STOP_SIGMASK;
+			WARN_ON_ONCE(!signr);
+			ptrace_stop(signr, CLD_STOPPED, 0, NULL);
+			current->exit_code = 0;
+		}
 		spin_unlock_irq(&sighand->siglock);
 		goto relock;
 	}

1.7.1

This makes PTRACE_EVENT_STOP similar to other PTRACE_EVENTs. The only difference is that it can't be set by PTRACE_SETOPTIONS as other events do, but activated implicitly by PTRACE_SEIZE.

This made me thinking.

How about making API even more similar to existing one?

Create PTRACE_O_TRACESTOP, make it settable by PTRACE_SETOPTIONS too.

Make PTRACE_SEIZE take the mask of PTRACE_O_xyz flags
as data argument.
If PTRACE_O_TRACESTOP is set, it works as you described above. If PTRACE_O_TRACESTOP is not set, then it works as good old PTRACE_ATTACH. In both cases, immediately at attach it sets opts a-la PTRACE_SETOPTIONS.

We can even avoid introducing PTRACE_SEIZE at all, because currently PTRACE_ATTACH ignores its data argument.

I know, I know, "this changes API", but did we ever promise that PTRACE_ATTACH with nonzero data arg is a valid usage? Also, I perused first 10 pages of google code search results and I see that everybody passes 0 or NULL.

vda

Hello, Denys.

Also by PTRACE_INTERRUPT and group stop.

But as SEIZE introduces behavior differences throughout ptrace operation, I think it's actually beneficial to use a distinctively new request. It's not like it costs anything or we're short on request number space.

Similar issue with PTRACE_O_TRACESTOP. It won't only enable TRACESTOP it will change other behaviors too and I think allowing clearing the option while attached would open up a lot of new issues without much benefit. Making it a PTRACE_O_ flag and not allowing clearing is weird too.

I've been thinking about Jan's suggestion to make ATTACH and DETACH not require tracee to trap. We already have this for DETACH for cases where the tracer is killed and it seems it wouldn't be too difficult to make that happen for ATTACH either and for that to be truly useful I suppose PTRACE_SETOPTIONS shouldn't require trapped state either. Jan, would that be enough for the use cases you have on mind?

So, if we do that, there would be a clear separation between SEIZE and INTERRUPT and there wouldn't be any reason to make SEIZE to set PTRACE_O_ options or whatever. It just attaches and the rest can be done with appropriate requests. SEIZE might not be the best name with this functionality but I think ATTACH_NOINTERRUPT would be too confusing in that it implies that the only difference is not interrupting. Can somebody think of a better verb?

Oleg, what do you think?

Thanks.

tejun

"Activating" meaning "enabling", not "causing". Only PTRACE_SEIZE enables PTRACE_EVENT_STOP.

All these differences revolve around making handling of stops and SIGCONT better. It seems fitting to the option name.

PTRACE_SEIZE sets a flag somewhere "please convert group-stops into PTRACE_EVENT_STOPs".

But, we *already have* ptrace op which performs this action of modifying (or adding) stops - it's PTRACE_SETOPTIONS. With PTRACE_O_TRACESYSGOOD it modifies syscall-stops. With PTRACE_O_TRACEEXEC - post-execve stop.
With PTRACE_O_TRACEFORK it adds a stop after fork.

Since we have this API, why not use it for the very similar concept of modifying group-stops too?

"Entities must not be multiplied beyond necessity"?

vda

Hello, Denys.

I don't know. It seems a bit too subtle. The new behaviors are a lot more pervasive than any controlled by PTRACE_O_ flags and there's the problem of allowing its clearing.

Different scope. Conflicting semantics (clearing). API compatibility as you pointed out.

Sure, but whether to have a flag or a request is trivial. When one of them has issues like above, I see a new request number go above the necessity threshold.

Thanks.

tejun

Hello,

I've been trying this and clean DETACH requires the tracee to be trapped (or not running). The arch detach hook, which BTW is not executed when the tracer is killed, modifies tracee state expecting it to be off-cpu.

But making SEIZE not trigger INTERRUPT and SETOPTIONS without requiring TRACED don't seem too difficult. Jan, would that be enough? Oleg, what do you think?

Thanks.

tejun

Hey, again.

Even the implementation is rather simple. If SEIZE and INTERRUPT are okay as implemented, the following should be fine too.

Thanks.

---
 kernel/ptrace.c |   22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

Index: work/kernel/ptrace.c
===================================================================
@@ -329,7 +329,7 @@ static int ptrace_attach(struct task_str
 
 	__ptrace_link(task, current);
 
-	/* SEIZE uses TRAP_STOP instead of SIGSTOP for initial trap */
+	/* SEIZE doesn't trap tracee on attach */
 	if (!seize)
 		send_sig_info(SIGSTOP, SEND_SIG_FORCED, task);
 

@@ -353,9 +353,6 @@ static int ptrace_attach(struct task_str
 	if (task_is_stopped(task)) {
 		task->jobctl |= JOBCTL_TRAP_STOP | JOBCTL_TRAPPING;
 		signal_wake_up(task, 1);
-	} else if (seize) {
-		task->jobctl |= JOBCTL_TRAP_STOP;
-		signal_wake_up(task, 0);
 	}
 
 	spin_unlock(&task->sighand->siglock);

@@ -907,6 +904,17 @@ static struct task_struct *ptrace_get_ta
 #define arch_ptrace_attach(child)	do { } while (0)
 #endif
 
+static bool ptrace_is_async_req(struct task_struct *child, int req)
+{
+	if (req == PTRACE_KILL)
+		return true;
+
+	if (!(child->ptrace & PT_SEIZED))
+		return false;
+
+	return req == PTRACE_SETOPTIONS || req == PTRACE_INTERRUPT;
+}
+
 /**
  * ptrace_put_task_struct - ptrace request processing done, put child
  * @child: child task struct to put

@@ -971,8 +979,7 @@ SYSCALL_DEFINE4(ptrace, long, request, l
 		goto out_put_task_struct;
 	}
 
-	ret = ptrace_check_attach(child, request == PTRACE_KILL ||
-				  request == PTRACE_INTERRUPT);
+	ret = ptrace_check_attach(child, ptrace_is_async_req(child, request));
 	if (ret < 0)
 		goto out_put_task_struct;
 

@@ -1114,8 +1121,7 @@ asmlinkage long compat_sys_ptrace(compat
 		goto out_put_task_struct;
 	}
 
-	ret = ptrace_check_attach(child, request == PTRACE_KILL ||
-				  request == PTRACE_INTERRUPT);
+	ret = ptrace_check_attach(child, ptrace_is_async_req(child, request));
 	if (!ret)
 		ret = compat_arch_ptrace(child, request, addr, data);
 

UUIC, that opens a race where between SEIZEing and SETOPTIONS(O_TRACE FORK|VFORK|EXEC...), the tracee can fork/vfork/clone/exec, without the tracer getting the nice corresponding PTRACE_EVENT_ events.

In GDBs case, GDB will want to poke at memory right after attaching, to at least read the loaded DSO list, so if you can't read tracee memory without interrupting it, you won't get much benefit from SEIZE not interrupting.
Not to say other users wouldn't benefit.

Pedro Alves

SEIZE,fork-in-tracee,INTERRUPT sequence is indistinguishable from SEIZE happening two microseconds later:

fork-in-tracee,SEIZE,INTERRUPT

...where "right after attaching" is defined as "when the first ptrace-stop is reported". Which will happen very soon.

vda

SEIZE,execvd,INTERRUPT (SETOPTS on interrupt)

will make the tracer see a SIGTRAP that

execvd,SEIZE,INTERRUPT

nor

SEIZE,SETOPTS,execvd (SETOPTS on interrupt)

would cause, isn't it?

Now, if it were possible for the tracer to set the default OPTS _before_ PTRACE_ATTACH/PTRACE_SEIZE...

Hmm? Why would it happen very soon? Isn't the point of SEIZE not interrupting that you'd not get any INTERRUPT or stop at all? Where is the ptrace-stop coming from?

Pedro Alves

Yes, you are right about this particular case.

Execve's extra SIGTRAP is a particularly painful misfeature.

I propose to do it *during* SEIZE then. Say, by passing SETOPTION style option flags in data argument. To fight above example, we'd want to pass PTRACE_O_TRACEEXEC.

Tejun, what do you think?

allowed poking of running tracees. You need to stop it first.

vda

Which is a bit lame.

That was my point... I was just pointing out that GDB will end up PTRACE_INTERRUPTing the target anyway. Maybe we could extend GDB's "observer" mode to be even more observer-only, and delay reading the DSO list and whatever else GDB does on attach until the first stop, or to user request. Some archs need reading registers as soon as possible in order to actually know which arch variant we've attached to. Anyway, this is GDBs business. SEIZE not interrupting won't hurt GDB, and is obviously useful for some use cases and tracers, _provided the race with SETOPTS is fixed_.

Pedro Alves

Hello,

Lame as it may be, it would be quite challenging to change that.

Other than the to-be-removed SIGTRAP, I don't think the trace is observable from userland. Do you have anything specific on mind?

Thanks.

tejun

Hello, Pedro.

Does it matter? The order of execution isn't even well defined without synchronization border. If you want full synchronization, you can INTERRUPT tracee.

Yes, SIGTRAP on exec(2) is nasty but also is scheduled to be removed if SEIZED.

I don't see why that would be necessary.

Thanks.

tejun

The point I was trying to raise was not about the order of execution, but about letting the old pre-nice PTRACE_EVENT_ events quirks stick through.

Okay, good to hear that. Looks like the tracer can do:

SEIZE,execve,SETOPTS,'readlink /proc/pid/exe'

and pretend it SEIZED after the execve.

I'm happy for now.

Thanks.

Pedro Alves

Hello, Pedro.

I see.

Yeap, and I was trying to say that if tracer and tracee are running on different CPUs, the order between SEIZE and execve isn't even well defined (sans the nasty automatic SIGTRAP).

Awesome, thanks.

tejun

I see, indeed, thanks.

A couple interface questions that just crossed my mind:

- on a fork/vfork/clone, if PTRACE_EVENT_FORK|VFORK|CLONE have been enabled, will the tracer still see the new child stop with a SIGSTOP, or will it see a PTRACE_EVENT_INTERRUPT?

- is PTRACE_INTERRUPT on PTRACE_TRACEME-traced-child planed to be allowed (for convenience)?
A PTRACE_O_TRACEINTERRUPT, or some such PTRACE_SETOPTIONS option might be necessary to get PTRACE_EVENT_INTERRUPT instead of SIGSTOP in the point above.

Pedro Alves

Hello,

This won't change, so SIGSTOP although we probably want to improve it such that this can be distinguished from SIGTRAP from userland.

I'm currently leaning toward deprecating PTRACE_TRACEME. If a task can PTRACE_TRACEME, it may as well just do pause(2) and let the parent SEIZE it.

Thanks.

tejun

(I assume you meant SIGSTOP from userland.) So that if a SIGSTOPs from userland is sent before the tracer waits for the child, the tracer sees a siginfo corresponding to the userland SIGSTOP? Sounds like it might work.

Debuggers will want to nurse the child through a couple of execs (shell, then real debuggee), so that scheme requires a bit more synchronization, because SEIZE hides the magic exec SIGTRAP, and so the tracer needs to set the O_TRACEXEC option before the first exec, and make sure external signals don't break the synchronization. Reading/writing to/from blocking pipes for that initial synchronization is what GDB uses instead for e.g., hpux/ttrace support, which looks similar to using PTRACE_SEIZE for PTRACE_TRACEME. A bit more cumbersome, though doable, I suppose.

Thanks.

Pedro Alves

Hello,

Now that thinking more about it, TRAP_STOP (INTERRUPT trap) would probably be better. I'll think more about it. For fork, it doesn't really matter but deliverying SIGSTOP on CLONE isn't too good. From user's POV, TRAP_STOP should work too, right?

Yes, it would require some sort of synchronization. I was thinking more along the line of ptracer modifying tracee so that it exits pause(2) loop after ptracer issues PTRACE_CONT, but I agree using pipes would be more straight-forward.

Thank you.

tejun

AFAICS, it should be no problem.

Thanks,

Pedro Alves

Sorry for delay again, I was a bit offline.

I am a bit confused, sorry if I missed/misunderstood the context...

I thought, we already discussed this. Obviously, PT_SEIZED should be inherited by the child during the auto-attach (and it is indeed copied by ptrace_init_task). Why should we send SIGSTOP to the child then? Even in the fork case, this leads to the same problems as the current PTRACE_ATTACH has with the bogus SIGSTOP it sends. I think TRAP_STOP is the only sane option, no?

Btw. Speaking of SEIZE->execvd->INTERRUPT which makes the tracee see a SIGTRAP. Stupid question. Perhaps PTRACE_SEIZE should set PT_TRACESYSGOOD | PT_TRACE_EXEC along with PT_SEIZED automatically? PT_SEIZED implies the new behaviour anyway.

Oleg.

Hey,

I forgot about that and was thinking about events reported by SIGTRAPs. :)

Yes, in this case, TRAP_STOP is the only sane solution.

Yeap, it makes sense to set them by default. I was thinking about defining a new PTRACE_EVENT_* rather than using 0x80 tho. That way we should be able to distinguish between enter and exit too.

This reminded me of another issue. Currently we have two different methods of reporting debug events. One is direct ptrace event trap, which ends up in ptrace_stop() from the event site and thus is synchronous. The other is sending SIGTRAP, literally. Besides the usual problems related with implicitly sending signals, this also makes it difficult to determine which event is happening exactly and makes the whole signal delivery/injection thing much worse.

I suspect that all ptrace traps probably started as actual SIGTRAPs and so all the events were reported via signal delivery path and so the signal "injection" worked; however, now we have synchronous traps and some SIGTRAPs, which are confusing and unreliable. I don't see any reason why the events which are currently reported via SIGTRAPs can't be reported via ptrace traps with unique PTRACE_EVENT_*. I don't think we can take synchronous traps directly but we can schedule them, record relevant information in preallocated area and just set TIF_SIGPENDING and let the signal delivery path report the trap. This will make all the events reliable and easily distinguishible and we can fix the SIGTRAP signal problem. What do you think?

Thanks.

tejun

I was told before that when SEIZE was in effect, there's no magic SIGTRAP on exec.

SYSGOOD makes sense, it just enables a means to distinguish syscall SIGTRAPs from regular SIGTRAPs -- it doesn't cause child stops itself. TRACE_EXEC, I'm not so sure. (and it appears to have been proposed on the premise that SEIZE would still report the SIGTRAP).
Why would that make sense, and not TRACE_FORK, for example? I can imagine a tracer only caring for syscall entry/exit, and not needing a special event on exec. IMO, any kind of event that forces a child stop that would't happen if the child wasn't traced should have to be enabled explicitly.

Heck, GDB passes a subset of signals straight down to
the child without informing the user (e.g., see "handle SIGALRM" command), and it would be an improvement in
the tracer-affects-tracee's-scheduling department to have a means to let ptrace know a tracer isn't interested in such-and-such signals. Conversely, going with the non-intrusive tracing theme, it would even make sense for the tracer to have to request "let me know about signals (all or a subset) sent to tracee too"

Pedro Alves

Hello, Pedro.

The problem with exec is that very weird things happen during exec. Tasks change their ids, tracees get silently pruned and so on, so there might not be a transparent way for a ptracer to deal with it. It needs to be notified and handle the situation whether it wants or not.

What I was saying was there won't be SIGTRAP. In general, we're trying to move away from kernel implicitly sending actual signals. If we enable it by default, it will be a proper ptrace trap.

Thanks.

tejun

Agreed, it would be nice to avoid SIGTRAP's. Although right now it is not clear to me how we can do this. The problematic case is the single-stepping, afaics. Say, int3. Even if the task is ptraced, we can't know why do we hit this insn. It is possible that this task auto-debugs itself and thus we should send SIGTRAP. Even the X86_EFLAGS_TF case is not that simple although we can probably take TIF_SINGLESTEP into account. But again, the task can set X86_EFLAGS_TF itself.

Oleg.

Argh. Yes, ptrace_disable() is the problem, I didn't think about it :/

Oleg.

Yes, I still think that the new DETACH_XXX request which doesn't need the stopped tracee makes sense. Yes, we have PTRACE_INTERRUPT. But please recall the previous discussion, it is possible that the tracee can't react to PTRACE_INTERRUPT and trap because it waits for other threads we are tracing.

And. Currently there is no way to detach a zombie leader. Perhaps we should change do_wait(), but it is not clear what should we do if the tracer is the real parent (we already discussed this a bit).

Yes, I think this is simple to do. Do we need this? I leave this up to you and Jan.

To me personally attach-implies-trap looks more natural, but probably gdb has another opinion.

Anyway. IIUC, gdb wants the auto-attach-on-clone without the trap, this is more important but this opens a lot of problems.

Hmm. Why? we could pass this options along with PTRACE_SEIZE?

Jan?

Oleg.

Hello, Oleg.

Yeah, untrapped DETACH sounds nice but as you've already acknowledged in another reply, we have those nasty disable traps. As the impact on debugging capability is much lower, having a synchronization barrier on detach might not be such a bad idea. As for debugger building dependency loop with debuggees, I think SIGKILL is the solution there. Debugger can do that with PTRACE_INTERRUPT regardless of DETACH after all.

Hmmm... maybe just allow detaching zombie leader? As it's guaranteed to be not running, we don't have problem with ptrace_disable.

Hmmm... I just don't see any reason to combine them. If the tracer wants sync points, it can INTERRUPT tracee and adjust options; otherwise, it can just do it. Isn't that simpler?

Thanks.

tejun

Yes. Which I never thought about, I alway assume PTRACE_DETACH_ASYNC is trivial, but it is not. Lets forget it for now.

Yes, I think we should do this.

If we change PTRACE_DETACH (or add the new request) to allow this, then I think it it should detach any zombie, leader or not.

Or we can change do_wait() to detach a zombie leader. In this case it is not clear what should we do if the debugger is the real parent. Perhaps do_wait() should do the same: detach a leader (but not reap). When the last thread does, the real parent will be notified again. IOW, wait(tgid) can succeed twice.

Agreed. In fact it can be running, but it can't return to the user-space, and I think this is enough.

ptrace_detach()->ptrace_disable() can race with SIGKILL anyway, this means it should safe to call it if the tracee is exiting/exited.

Oleg.

Hello,

I think we can just make PTRACE_DETACH to succeed for zombies. No reason to add a new request for this.

Just letting PTRACE_DETACH work for zombies sounds much simpler to me.

Yeap, unless userland gets to run again, I don't think there's any problem.

Thanks.

tejun

OK.

Probably, but please note we have to modify do_wait() anyway. Otherwise in general the tracer simply can not know the tracee has exited. IOW, waitpid(zombie_leader_pid, WEXITED) should succeed without reaping if delay_group_leader(), then the tracer can do PTRACE_DETACH. But this is not symmetrical with sub-thread zombies.

Oleg.

Hello,

Yes, complicated. The task/process duality conflicts. wait(2) already is different for group leader and succeeds twice for the ptracer and the real parent (when they're different).

If we relocate ptrace group leader zombie wait, as suggested, such that it waits for the task itself rather than the whole group, we would be taking away a feature - ptracer waiting for the whole process to exit and having access to group exit code, which might not mean much, I have no idea.

I think group leader wait becoming asymmetrical with sub-thread zombies is perfectly fine - it already is. But would it be okay to change ptrace wait(2) on group leader to wait for the task itself rather than the whole group?

Thanks.

tejun

Partly yes. The parent will be notified again and it can do another wait() later. The problem is, this is confusing.

Well, probably yes... But why? We have to change do_wait() anyway to report the death of the leader. If we do this, we can detach it as well.

Yes, this is iffy. Simply because this can confuse the userspace. May be we can add W_PTRACED_THREAD_EXITED? (should be used instead of WEXITED by ptracer). Or, perhaps WEXITED should succeed but put something special into status?

Oleg.

IOW, you are starting to agree that ptrace is not fixable ;)

On a seriouse note, of course you are right, but what we should do?

Speaking of this particular problem, it is really annoying. The leader exits, and it even notifies the tracer. For what? waitpid() returns 0 or hangs depending on WNOHANG, any ptrace() request fails with ESRCH.

Ironically, recently I got the bug-report which blaims this behaviour, although it was mentioned many times during the previous discussions.

I agree, we can't change the current behaviour of WEXITED in this case.

I also agree that if we, say, add WPTRACE_EXITED or change the semantics of WEXITED when the traced leader is PT_SEIZED, then probably this new feature won't be used by gdb/etc in the forseeable future.

So what should we do?

OK, this particular problem is minor, perhaps we can simply ignore it and do not try to help the userspace.

But the same objection applies to any new feature in this area. Say, PTRACE_INTERRUPT. To me, this request is obviously good/useful and nobody argued so far. But, will it ever be used by gdb? I am not sure, but at least we add the possibility.

Oleg.

Have we *ever* gotten a bug-report about these kinds of things from actual users?

IOW, it may be an annoying issue from a theoretical standpoint, but afaik it's not an annoying issue from a practical user standpoint.

Compare it to the _practically_ annoying issue we had with ^C and bash, where it was fundamentally impossible to know whether the child actually exited due to the ^C or not due to the interfaces. That one caused actual practical issues that people hit, and that were impossible to work around in theory, and hard to work around in practice..

In comparison, I don't think pid-vs-namespace issues for ptrace or some subtle interaction with execve is really all that big of a problem in practice.

Keep the eye on the ball: look around what people actually complain about for practical reason that they hit, rather than issues that people say "wouldn't it be nice if.." about.

Linus

Yes ;) https://bugzilla.redhat.com/show_bug.cgi?id=705583

Probably...

Oleg.

Hello, Oleg.

Hmm... can't we just allow PTRACE_DETACH after PTRACE_EVENT_EXIT is reported? It doesn't involve changes to any visible behavior (other than allowing DETACH of course) and we don't lose anything. Haven't really thought about the implementation but I don't think this would be too difficult.

Thanks.

tejun

Confused. PTRACE_DETACH surely works if the tracee reports EVENT_EXIT... Could you explain what you meant?

Oleg.

Hey,

Right, I briefly forgot it's already a ptrace stop and was thinking about using PTRACE_EVENT_EXIT as the event notification point. Anyways, back to the topic.

Maybe I misunderstood the problem but wasn't the problem about not being able to wait for the exit of a leader thread and detach it? We have reliable (sans exec but that's a different story) exit notification with EVENT_EXIT which even reports the exit_code, so I don't see what the problem is. What am I missing?

Thanks.

tejun

Hello,

Confused. How can do_signal_stop() clear TRAP_STOP?

Yeah, I like it much better. INTERRUPT trap and group stop trap being separate while sharing some attributes was disturbing.

Thanks.

tejun

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 group stop trap - SIGTRAP | PTRACE_EVENT_STOP << 8. A new trap flag JOBCTL_TRAP_INTERRUPT is added, which is set on PTRACE_INTERRUPT and cleared when any trap happens. 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 +
 kernel/ptrace.c        |   27 +++++++++++++++++++++++++--
 2 files changed, 26 insertions(+), 2 deletions(-)

index 3fd389d..b07b9e3 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 7aefd43..351db7c 100644
@@ -706,6 +706,7 @@ int ptrace_request(struct task_struct *child, long request,
 	siginfo_t siginfo;
 	void __user *datavp = (void __user *) data;
 	unsigned long __user *datalp = datavp;
+	unsigned long flags;
 
 	switch (request) {
 	case PTRACE_PEEKTEXT:

@@ -738,6 +739,26 @@ int ptrace_request(struct task_struct *child, long request,
 			ret = ptrace_setsiginfo(child, &siginfo);
 		break;
 
+	case PTRACE_INTERRUPT:
+		/*
+		 * Stop tracee without any side-effect on signal or job
+		 * control.  At least one trap is guaranteed to happen
+		 * after this request.  If @child is already trapped, the
+		 * current trap is not disturbed and another trap will
+		 * happen after the current trap is ended with PTRACE_CONT.
+		 *
+		 * The actual trap might not be PTRACE_EVENT_STOP trap but
+		 * the pending condition is cleared regardless.
+		 */
+		if (likely(child->ptrace & PT_SEIZED) &&
+		    lock_task_sighand(child, &flags)) {
+			child->jobctl |= JOBCTL_TRAP_STOP;
+			signal_wake_up(child, 0);
+			unlock_task_sighand(child, &flags);
+			ret = 0;
+		}
+		break;
+
 	case PTRACE_DETACH:	 /* detach a process that was attached. */
 		ret = ptrace_detach(child, data);
 		break;

@@ -863,7 +884,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;
 

@@ -1005,7 +1027,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);
 

1.7.1

Hello,

I didn't understand that one, so you'll need to elaborate.

Great. :)

Thanks.

tejun

Yeah, I was wrong, see the previous email.

Oleg.

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 351db7c..42037a4 100644
@@ -574,16 +574,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

This essentially is a simple addition of a flag field 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.

This patch doesn't actually make use of the new field.

---
 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 ++++++++++
 kernel/signal.c                       |    7 ++++++-
 17 files changed, 117 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 84e75db..c05f4d1 100644
@@ -1848,7 +1848,7 @@ static void ptrace_do_notify(int exit_code, int why)
 
 	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();
 

@@ -2501,6 +2501,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

If tracee is SEIZED, PTRACE_EVENT_STOP trap is used for group stop; however, there currently is no way to find out which signal initiated the group stop or if the group stop is still in effect.

This patch changes PTRACE_GET_SIGINFO on ptrace traps to report group stop information via si.si_signo and si.si_pt_flags. 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.

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);

		  ptrace(PTRACE_GETSIGINFO, tracee, NULL, &si);
		  if (si.si_code) {
			  stopped = !!si.si_status;
			  if (stopped != last_stopped)
				  printf("tracer: stopped=%d signo=%d\n",
					 stopped, si.si_signo);
			  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);
		  }

		  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 signo=19
  mother: SIGCONT
  tracer: stopped=0 signo=5
  tracer: SIG 18
  mother: SIGSTOP
  tracer: SIG 19
  tracer: stopped=1 signo=19
  mother: SIGCONT
  tracer: stopped=0 signo=5
  tracer: SIG 18
  mother: SIGSTOP
  tracer: SIG 19
  tracer: stopped=1 signo=19
  mother: SIGCONT
  tracer: SIG 18
  tracer: stopped=0 signo=5

---
 include/linux/ptrace.h |    3 +++
 kernel/ptrace.c        |   19 +++++++++++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

index b07b9e3..72b9150 100644
@@ -73,6 +73,9 @@
 #define PTRACE_EVENT_EXIT	6
 #define PTRACE_EVENT_STOP	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 42037a4..30d2331 100644
@@ -573,11 +573,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)

@@ -585,6 +587,23 @@ 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 & __SI_MASK) == __SI_TRAP) {
+		/*
+		 * Report whether group stop is in effect w/ SI_STOPPED and
+		 * if so which signal caused it.
+		 */
+		if (sig->group_stop_count || sig->flags & SIGNAL_STOP_STOPPED) {
+			info->si_pt_flags |= PTRACE_SI_STOPPED;
+			info->si_signo = child->jobctl & JOBCTL_STOP_SIGMASK;
+			WARN_ON_ONCE(!info->si_signo);
+		}
+	}
 out_unlock:
 	unlock_task_sighand(child, &flags);
 	return error;

1.7.1

Yeap, sure. Will update.

It's not the prettiest, I agree, but it is more or less in line with the rest of the interface, so...

Thanks.

tejun

__SI_TRAP siginfo is special in the operation of ptrace. It reports group stop related information and will also interact with notification retraps. Don't let userland mess with it.

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

index 30d2331..c12daec 100644
@@ -612,16 +612,29 @@ out_unlock:
 static int ptrace_setsiginfo(struct task_struct *child, const 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)) {
-			*child->last_siginfo = *info;
-			error = 0;
-		}
-		unlock_task_sighand(child, &flags);
-	}
+	if (!lock_task_sighand(child, &flags))
+		return -ESRCH;
+
+	error = -EINVAL;
+	if (unlikely(!child->last_siginfo))
+		goto out_unlock;
+
+	/*
+	 * If seized, __SI_TRAP siginfo is used to communicate information
+	 * regarding traps and contains dynamic information generated on
+	 * GETSIGINFO.  Don't let userland override or fake it.
+	 */
+	if ((child->ptrace & PT_SEIZED) &&
+	    unlikely((child->last_siginfo->si_code & __SI_MASK) == __SI_TRAP ||
+		     (info->si_code & __SI_MASK) == __SI_TRAP))
+		goto out_unlock;
+
+	*child->last_siginfo = *info;
+	error = 0;
+out_unlock:
+	unlock_task_sighand(child, &flags);
 	return error;
 }
 

1.7.1

For to-be-added notification retraps, other tasks need to be able to tell whether ptrace request is currently in progress while tracee is in STOP trap. This patch adds JOBCTL_BLOCK_NOTIFY which is set on ptrace_check_attach() if the request requires tracee to be trapped and it's trapped for STOP, and cleared when ptrace syscall finishes.

This flag isn't used yet.

---
 include/linux/ptrace.h |    2 +
 include/linux/sched.h  |    1 +
 kernel/ptrace.c        |   51 ++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 50 insertions(+), 4 deletions(-)

index 72b9150..b0c1347 100644
@@ -73,6 +73,8 @@
 #define PTRACE_EVENT_EXIT	6
 #define PTRACE_EVENT_STOP	7
 
+#define PTRACE_STOP_SI_CODE	(__SI_TRAP | SIGTRAP | PTRACE_EVENT_STOP << 8)
+
 /* flags in siginfo.si_pt_flags from PTRACE_GETSIGINFO */
 #define PTRACE_SI_STOPPED	0x00000001 /* tracee is job control stopped */
 

index d8a11cb..1f082d9 100644
@@ -1786,6 +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_STOP	(1 << 19) /* trap for STOP */
 #define JOBCTL_TRAPPING		(1 << 21) /* switching to TRACED */
+#define JOBCTL_BLOCK_NOTIFY	(1 << 22) /* block NOTIFY re-traps */
 
 #define JOBCTL_TRAP_MASK	JOBCTL_TRAP_STOP
 #define JOBCTL_PENDING_MASK	(JOBCTL_STOP_PENDING | JOBCTL_TRAP_MASK)

1.7.1

Hello,

Hmmm... I suppose we can make the error paths finer grained so that attach failure doesn't end up calling ptrace_put_task_struct() but I think it would be better to keep the two tests there so that the function can be called regardless of what happened.

Thanks.

tejun

Ah, you are right again, I forgot about PTRACE_ATTACH path.

Thanks,

Oleg.

We can move the PT_PTRACED and parenthood test outside the siglock tho and I agree that the 'still ptraced' comment is misleading. I'll update it.

Thanks.

tejun

Currently there's no way for ptracer to find out whether group stop that tracee was in finished other than polling with PTRACE_GETSIGINFO. Also, tracer can't detect new group stop started by an untraced thread if tracee is already trapped. This patch implements group stop notification for ptracer using STOP traps.

When group stop state of a seized tracee changes, JOBCTL_TRAP_NOTIFY is set, which triggers STOP trap but is sticky until the next PTRACE_GETSIGINFO. As GETSIGINFO exports the current group stop state, this guarantees that tracer checks the current group stop state at least once after group stop state change. Stickiness is necessary because notification trap may race with PTRACE_CONT for other traps and get lost.

Note that simply scheduling such trap isn't enough. If tracee is running (PTRACE_CONT'd from group stop trap), the usual trapping - setting NOTIFY followed by the usual signal_wake_up() - is enough; however, if tracee is trapped, the scheduled trap won't happen until the trap is continued.

This is solved by re-trapping if tracee is in STOP trap. Along with JOBCTL_TRAP_NOTIFY, JOBCTL_TRAPPING is set and tracee is woken up from TASK_TRACED. Tracee then (re-)enters INTERRUPT trap generating notification for tracer. TRAPPING hides the TRACED -> RUNNING -> TRACED transition from tracer.

Many ptrace requests expect tracee to remain trapped until they finish. Such conditions are marked with JOBCTL_BLOCK_NOTIFY and if notification happens while BLOCK_NOTIFY is set, JOBCTL_TRAPPING is set but the actual wake up and re-trapping takes place when the ptrace request finishes. This is safe as the only task which can wait for TRAPPING is the ptracer.

Re-trapping is used only for STOP trap. If tracer wants to get notified about group stop, it either leaves tracee in the initial STOP trap or puts it into STOP trap using PTRACE_INTERRUPT. If STOP trap is scheduled while tracee is already in a trap, it's guaranteed that tracee will enter a trap without returning to userland, so tracer doesn't lose any control over tracee execution for group stop notification.

An example 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)
			  pause();

	  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);

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

		  if (stopped != last_stopped)
			  printf("tracer: stopped=%d signo=%d\n",
				 stopped, si.si_signo);
		  last_stopped = stopped;

		  if (!stopped)
			  ptrace(PTRACE_CONT, 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;

}

In the above program, tracer gets notification of group stop state changes and can track stopped state without polling PTRACE_GETSIGINFO.

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

---
 include/linux/sched.h |    3 +-
 kernel/ptrace.c       |   11 +++++++-
 kernel/signal.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 76 insertions(+), 6 deletions(-)

index 1f082d9..5573930 100644
@@ -1785,10 +1785,11 @@ 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_STOP	(1 << 19) /* trap for STOP */
+#define JOBCTL_TRAP_NOTIFY	(1 << 20) /* sticky trap for notifications */
 #define JOBCTL_TRAPPING		(1 << 21) /* switching to TRACED */
 #define JOBCTL_BLOCK_NOTIFY	(1 << 22) /* block NOTIFY re-traps */
 
-#define JOBCTL_TRAP_MASK	JOBCTL_TRAP_STOP
+#define JOBCTL_TRAP_MASK	(JOBCTL_TRAP_STOP | JOBCTL_TRAP_NOTIFY)
 #define JOBCTL_PENDING_MASK	(JOBCTL_STOP_PENDING | JOBCTL_TRAP_MASK)
 
 extern void task_clear_jobctl_pending(struct task_struct *task,

index d382f81..2640761 100644
@@ -130,6 +130,7 @@ void __ptrace_unlink(struct task_struct *child)
 	 * Clear jobctl bits owned by this tracer along with STOP_PENDING,
 	 * which is reinstated below if necessary.
 	 */
+	child->jobctl &= ~JOBCTL_BLOCK_NOTIFY;
 	task_clear_jobctl_pending(child, JOBCTL_PENDING_MASK);
 
 	/*

@@ -617,6 +618,9 @@ static int ptrace_getsiginfo(struct task_struct *child, siginfo_t *info)
 			info->si_signo = child->jobctl & JOBCTL_STOP_SIGMASK;
 			WARN_ON_ONCE(!info->si_signo);
 		}
+
+		/* tracer got siginfo, clear the sticky trap */
+		child->jobctl &= ~JOBCTL_TRAP_NOTIFY;
 	}
 out_unlock:
 	unlock_task_sighand(child, &flags);

@@ -923,9 +927,14 @@ static void ptrace_put_task_struct(struct task_struct *child)
 
 	/*
 	 * Make sure @chlid is still ptraced by us and clear BLOCK_NOTIFY.
+	 * If TRAPPING is set, it means NOTIFY occurred in-between and
+	 * re-trap was blocked.  Trigger re-trap.
 	 */
-	if (likely((child->ptrace & PT_PTRACED) && child->parent == current))
+	if (likely((child->ptrace & PT_PTRACED) && child->parent == current)) {
 		child->jobctl &= ~JOBCTL_BLOCK_NOTIFY;
+		if (child->jobctl & JOBCTL_TRAPPING)
+			signal_wake_up(child, task_is_traced(child));
+	}
 
 	unlock_task_sighand(child, &flags);
 out_put:

1.7.1

I simply can't understand this patch. And the supposed API as it seen by the user-space. I'll try to read it again and think more.

A couple of questions,

Hey,

Yes, group stop state change raises a sticky trap condition which is cleared by GETSIGINFO.

It's an optimization. If we set the flag, we'll have to acquire siglock and clear it before returning to userland. Setting BLOCK_NOTIFY isn't necessary unless tracee is in TRAP_STOP, so...

Please consider the following scenario.

1. Tracee is in group stop and stops at TRAP_STOP notifying the tracer.

2. Tracer does WNOWAIT wait(2) and determines that the tracee is trapped in TRAP_STOP.

3. Something generates SIGCONT which finishes the group stop and triggers the notification re-trapping.

4. While tracee is re-trapping, tracer issues WNOHANG wait(2) expecting it to succeed but the tracee could be RUNNING for re-trap thus failing WNOHANG wait(2).

Yes, and the reported trap doesn't interfere with wait(2) or ptrace requests. Trapping is the only event notification mechanism available from tracee to tracer and we already have interface to wait and poll them, so I think it makes sense to use it.

Thank you.

tejun

Hmm. At least now I understand the meaining what "sticky" means in this discussion ;) I was confused.

OK, I see.

OK. I still hope we can avoid this somehow. May be play with exit_code so that do_wait() can succeed even if the JOBCTL_TRAPPING tracee is running. Perhaps.

If only we could notify the tracer from ptrace_trap_notify... IIUC, this is the only problem? I mean, apart from this there is no need to wake up the tracee.

Oleg.

I still don't see why is it needed. Userspace API doesn't require it.

Are you trying to solve/simplify something on the kernel side with it?

vda

Hello, Denys.

No, no. Please consider the following scenario.

1. A process has two threads t1 and t2. t1 is ptraced by p1. Both t1 and t2 are running.

2. p1 INTERRUPTs t1. t1 enters TRAP_STOP and p1 notices it.

3. Something else sends SIGSTOP to t2 which initiates group stop.

4. As t1 re-traps to notify p1 of group stop, p1 issues PTRACE_CONT.

5. PTRACE_CONT succeeds right after t1 re-traps for notification and t1 resumes execution.

6. p1 has no way to find out the group stop which is now in effect.

And vice-versa for SIGCONT. When an event needs to be delivered, the event should remain pending until the target the event is destined to has fetched the event; otherwise, the event can be lost. Here, the event to be delivered is change of group stop state and fetching mechanism is GETSIGINFO. The event should remain pending until GETSIGINFO is called.

The only requirement is for the ptracer to call PTRACE_GETSIGINFO on PTRACE_EVENT_STOP's which can be determined from the wait(2) exit code. If ptracer does that, there won't be any unnecessary trap.

Thanks.

tejun

IOW. The root of the problem is that TASK_TRACED no longer means the tracee is stopped, it can change its state and the "volatile" info in si_pt_flags reflects this fact.

This makes me nervous ;) Yes, we hide the TRACED->RUNNING->TRACED transitions, and I have to agree that the proposed API looks consistent to me. Still, can't we do this differently ? (see another email).

Oleg.

Hello, Oleg.

I considered that but re-trapping seemed cleaner to me. wait(2) already depends on retries so I don't think adding TRAPPING wait introduces too much extra complexity. That said, if you can come up with something simpler, by all means.

Yeap, correct.
ahead.

Thanks.

tejun

I'm afraid I wasn't clear. I didn't mean the patch is bad, I only meant that I got lost in details and I feel I'll ask more questions later.

Oleg.

... and I am greatly disappointed by the fact all technical problems I noticed were bogus, I should try more ;)

1. __ptrace_unlink:

	task_clear_jobctl_pending(child, JOBCTL_PENDING_MASK);

	/*
	 * Reinstate JOBCTL_STOP_PENDING if group stop is in effect and
	 * @child isn't dead.
	 */
	if (!(child->flags & PF_EXITING) &&
	    (child->signal->flags & SIGNAL_STOP_STOPPED ||
	     child->signal->group_stop_count))
		child->jobctl |= JOBCTL_STOP_PENDING;

Yes, we set JOBCTL_STOP_PENDING correctly. But what about JOBCTL_STOP_CONSUME? It was potentially flushed by task_clear_jobctl_pending() above.

2. ptrace_trap_notify() always sets JOBCTL_TRAP_NOTIFY. Even if the caller is prepare_signal(SIGCONT) and there were no SIGSTOP in the past. This looks a bit strange to me. I mean, perhaps it would be better to provoke the trap only if this SIGCONT is going to change the jobctl state.

In any case, we shouldn't set JOBCTL_TRAP_NOTIFY if fatal_signal_pending(). do_signal_stop() is fine, and prepare_signal(SIGCONT) can't race with SIGKILL. but it can race with the zap_other_threads-like code (exec, coredump). If we set JOBCTL_TRAP_NOTIFY when fatal_signal_pending() is true, the tracee can't stop, it will call get_signal_to_deliver()->ptrace_stop() in a loop forever and the tracer can't clear this bit.

Minor, but perhaps it would be more consistent to check PF_EXITING in the !task_is_traced() case.

3. The similar (but currently theoretical) problem with PTRACE_TRAP_STOP. We shouldn't assume anything about arch_ptrace_stop_needed(), the code should work correctly even if it always returns true. This means that, say, PTRACE_INTERRUPT should not set JOBCTL_TRAP_STOP if the tracee was killed, or ptrace_stop() should clear JOBCTL_TRAP_MASK if it aborts.

-------------------------------------------------------------------------------
Now about the API. Can't we avoid the "asynchronous" re-trapping and the subsequent complications like PTRACE_GETSIGINFO-must-be-called-to-cont?

What if we simply add the new request, say, PTRACE_JOBCTL_CONT which acts as "do not cont, but please report me the next change in jctl state". IOW, in short, instead of JOBCTL_BLOCK_NOTIFY we add JOBCTL_PLEASE_NOTIFY which is set by this request.

Roughly, PTRACE_JOBCTL_CONT works as follows:

- it is only valid if the tracee reported PTRACE_EVENT_STOP

- SIGCONT/do_signal_stop/prepare_signal(SIGCONT) set JOBCTL_TRAP_NOTIFY but do not wakeup unless JOBCTL_PLEASE_NOTIFY was set by PTRACE_JOBCTL_CONT

- of course, PTRACE_JOBCTL_CONT checks if JOBCTL_TRAP_NOTIFY is already set.

As for PTRACE_CONT, it should set JOBCTL_PLEASE_NOTIFY if we resume the stopped tracee. Or the tracer can do PTRACE_JOBCTL_CONT + PTRACE_CONT.

So. If the tracer wants to wait for SIGCONT after the tracee reports the jobctl stop, it can simply do PTRACE_JOBCTL_CONT and wait(). This looks much simpler to me. The only (afaics) complication is: PTRACE_INTERRUPT should guarantee the trap after PTRACE_JOBCTL_CONT (in this case we can simply set ->exit_code, no need to re-trap).

si_pt_flags (or whatever we use to report the jobctl state) can be recorded during the trap, not at the time of ptrace() syscall.

The implementation should be simpler too. Only ptrace_attach() needs the wait_trapping() logic, no need to complicate do_wait(). The tracee re-traps only if the tracer asks for this.

And _imho_ this is more understandable from the user-space pov. To me it is a bit strange a TASK_TRACED tracee can run and then take another trap without some sort of CONT from debugger, even if we hide the transition.

What do you think?

I must admit, I didnt think over this all thoroughly, just a sudden idea.

Oleg.

Hello, Oleg.

Yay, do I get to win for once? :)

Right, will fix. Probably best to just clear trap ones there.

Sure. It doesn't really matter tho and might even be better for weeding out invalid assumptions.

Yes, indeed. Will update. It would be nice if we have a helper which checks PF_EXITING before manipulating the flags. It seems a bit too fragile as it currently stands.

Hmmm... yes. I think I'll add a wrapper to raise trap conditions which always check PF_EXITING.

The main idea was that per-task execution state while ptraced is separate, and thus asynchronous, from group stop state. Group stop affects task execution but is ultimiately a separate attribute. This is reflected in the modifications made upto this point - which exit_code to use, and PTRACE_CONT working beneath group stop state.

While ptraced, group stop is a separate and asynchronous event and I think it would be best if the interface reflects that rather than trying to make it look like something different.

IIUC, the interface you suggested doesn't remove asynchronousity but, rather, confines it, but to me it seems to be a too thin a veil. e.g. What is the state of the tracee after PTRACE_JOBCTL_CONT? It is trapped but could be waken asynchronously and the ptracer isn't equipped to deal with that. What happens if the ptracer issues further PTRACE requests to the tracee? Are they rejected? What does it mean to PTRACE_INTERRUPT while tracee is in this state?

I'm somewhat uncomfortable with the fact that SIGCONT is directly resuming tracee. Re-trapping is different. The implementation might not be the easiest to swallow but conceptually it's just generating a notification telling the ptracer that something has changed (and we can try to do that from SIGCONT generator although I think re-trapping is cleaner). The execution state is still fully under the control of ptracer.

(oops, please read on before replying. I thought you were suggesting
resuming tracee without tracer intervention.)

Hmmm... so, if tracee is PTRACE_CONT'd from group stop and then SIGCONT happens, what happens? Does the tracee trap? IOW, what does it mean to have JOBCTL_PLEASE_NOTIFY set while tracee is not in TRAP_STOP?

The reason why si_pt_flags is generated on PTRACE_GETSIGINFO is because it reflects information which is asynchronous in nature. It doesn't make sense to sample the state when the task is going into trap. When a task enters a trap is fundamentally unrelated to how group stop state changes. I think sampling it on GETSIGINFO is the right thing to do as group stop state _is_ asynchronous to whatever the task itself is doing; otherwise, we end up with situation where no job control state is being changed but two tracees consistently report different group stop state depending on when they took trap. How should that be interpreted? What does that even mean? It gets weirder if there are untraced tasks in the group.

Ah, okay, so, it does re-trap on SIGCONT but it happens only after JOBCTLY_PLEASE_NOTIFY is set. Then the only thing which changes is removal of wait(2) retry logic at the cost of requiring the user to use a new request and stating that WNOHANG wait might fail unexpectedly after PTRACE_JOBCTL_CONT. Am I misunderstanding something?

I think it's best to represent a consistent interface / information which reflects the actual interworking of job control and tracees. Trying to mask it ultimately makes it more confusing, so I think it makes sense to actually report job control state changes as asynchronous notifications. Re-trapping is an implementation detail which is chosen to generate such notifications - we can try a different approach - which I think would be worse but anyways.

If we take a step back and forget about the implementation itself for a while, the re-trapping and on-the-fly GETSIGINFO are adding a rather generic async notification mechanism for ptrace, and I still think that's the better way to take.

Thanks.

tejun

Hello, Oleg.

Hmmm... tried to change this but I think it's better as-is. As an optimization, it isn't whole lot meaningful, and, if I do it, I would probably end up moving SIGNAL_STOP_STOPPED and group_stop_count test above actual wake up and then skipping wake up if none was true - it would make SIGCONT handling more fragile without much benefit.

Thanks.

tejun

Hello Tejun,

I agree. And it continues to be asynchronous, just the tracer can explicitly tell when it wants to know about the state change.

Yes,

It can be waken by SIGCONT, yes, but the tracer is equipped, it asked for this.

Yes. Except PTRACE_INTERRUPT, of course. (OK, may be PTRACE_CONT makes sense too but this is minor).

It should guarentee PTRACE_EVENT_STOP, as if the tracee was running.

IOW. The tracee is no longer quiescent after PTRACE_JOBCTL_CONT. It is still TASK_TRACED (although we could use another state), but it is waiting for another jobctl state change. The tracer should wait for notification (or call wait) as usual. No need to hide the TRACED-> RUNNING->TRACED transition during the re-trapping.

Yes. This is (almost) the same case as above, ignoring the small implementation details.

To me this looks really simpler and more understandable for the user space. If the simple tracer doesn't care about SIGCONT it can simply do PTRACE_CONT without PTRACE_GETSIGINFO.

I see. And I understand that this is necessary if the tracee re-traps asynchronously. Either the tracer gets the up-to-date info, or the tracee can't be continued without another trap. Honestly, I do not like this, but I agree this works.

Sure. But:

The group stop state _is_ asynchronous to GETSIGINFO as well. Once again, I understand (I hope) how this works, but dislike.

If we add PTRACE_JOBCTL_CONT, I think we can sample the state when the task is going into trap. If the tracer cares about the next state change, the tracee will trap again.

Yes. Except "unexpectedly" is a bit confusing. Once again, the tracee is no longer quiescent.

Hmm. Here we start to disagree ;)

I must admit, I am a bit biased because I think this makes the implementation more simple. But my point is, _in my opinion_ PTRACE_JOBCTL_CONT is better from the user-space pov.

For what? Why this is good?

It was always true that once the tracee reports the trap it is completely quiescent untill debugger resumes it (except SIGKILL). This is simple and understandable. Why should we change this?

Oleg.

Hey, Oleg.

Ah, okay, this makes it much clearer to me. Thanks for the explanation.

But, I'm not sure it achieves that. Please read on.

Oh, this is a separate issue. Your suggestoin only narrows the window in which re-trap is allowed to happen. GETSIGINFO being generated on request doesn't have much to do with that. It's determined by who is considered to be notification consumer.

I suppose you're suggesting STOP_TRAP to be the consumer such that there will be at least one STOP_TRAP after each group stop state change and the user is required to fetch what happened after each STOP_TRAP - ie. the chain of custody becomes event -> TRAP_STOP -> GETSIGINFO instead of event -> GETSIGINFO.

I did the direct thing primarily because I thought it was a primarily separate thing from tracee state itself but yeah I can see going through TRAP_STOP could be better, but I don't think it has anything to do with when to allow re-trap.

But we better make the behavior consistent. If it's non-quiescent, make wait(2) and non-INTERRUPT ptrace requests should fail consistently; otherwise, users are likely to get it wrong and such bugs can be very difficult to track down and reproduce.

I can't agree there. More on this below.

The thing I liked about it was that it's a straight forward mostly independent event mechanism. It may be used for other events and it forces the user to follow event delivery protocol (otherwise, trap won't clear). If you forget about GETSIGINFO and consider it to be a different thing - say PTRACE_GETEVENTS or something - it really is straight forward. The entanglement with GETSIGINFO is a bit messy but that's what we compromise when trying to live with legacy interface.

Anyways, I'm perfectly fine with sticky TRAP_STOP too. It makes sense in a different, more group stop specific way.

It's a bit besides the point but just for the record. It hasn't been true upto 2.6.40-rc1. Group stopped tracee could be woken up by SIGCONT before, so we actually had userland-visible non-quiescent state.

Anyways, the task is quiescent when viewed from userland. That's the whole point of TRAPPING. Making the re-trapping transparent. When viewed from userland, the only difference between TRAPPING and JOBCTL_CONT is when re-trapping is allowed.

With TRAPPING, re-trapping is allowed whenever tracee is in TRAP_STOP and the tracee is considered to be in quiescent state through the whole time - ie. wait(2) and ptrace(2) operatons aren't affected by generation of new event - I suppose this is the part that you don't like.

With JOBCTL_CONT, re-trapping is allowed while tracee is in TRAP_STOP and after JOBCL_CONT is issued and after JOBCTL_CONT is issued the tracee is considered non-quiescent until the next trap (which can be caused by group stop event or explicit INTERRUPT request). As tracee can be trated as non-quiescent, wait(2) and ptrace(2) don't need to be transparent against async events - we can just fail them. Note that we still need to change wait(2) so that it recognizes the state and fails.

Hmmm... yeah, I agree, the making-it-transparent part is tricky and implementation would be simpler with JOBCTL_CONT (BTW I hope we can find a better name. It's a bit confusing to me.); however, if we forget about implementation details and view it from userland POV, I think TRAPPING is the better behavior.

The notification itself isn't intrusive - for the most part it's just generation of another exit_code which can be fetched by the user if it wants to, so there really isn't much to be gained from userland with JOBCT_CONT. Its primary appeal is that it can simplify kernel implementation, which is not a bad thing at all. It's a bit messier to use from userland but not by too much and doesn't compromise any functionality AFAICS, so yeah, maybe.

Thanks.

tejun

IIUC 8/9 from prep patchset[2] needs the fix, do_wait()->restart_syscall() is not right. Do you agree or I misunderstood the result of our discussion?

I'll continue the reading tomorrow.

Oleg.

Yeah, I agree. I was just waiting for review of this series before proceeding with the next round.

Thanks.

tejun

So having followed the discussion so far, quite frankly I'm not convinced this series is 2.6.40 material.

I think that conceptually the split-up of PTRACE_ATTACH into SEIZE/INTERRUPT might be fine, but I don't think the interface is necessarily cooked, and perhaps more importantly I'm not at all sure that the (few) current users of ptrace() would even switch over.

So I think Oleg's branch with cleanups is probably ready, and maybe a few of the preparatory patches from this branch can be merged, but I would _strongly_ suggest that the plan for 2.6.40 should be to not actually mess with interfaces to the kernel, but just cleaning up the actual internal implementation. I would like to keep 2.6.40 small and simple.

Comments?

Linus

Hello, Linus.

Definitely not. The interface is not even fixed/complete yet. I think we should document the whole ptrace, job control and signal interaction properly before committing to the interface.

I think it shouldn't be too difficult for strace. gdb would be a challenge tho. At any rate, given that the existing mechanism is simply broken, I'm hoping they would switch over soonish.

Upto the patchset which cleaned up job control notifications, Oleg's recent sigprocmask plus some odd fix patches should be basically it for 2.6.40. The current ptrace branch in Oleg's tree seems to have just that.

Thanks.

tejun

I will push strace to at least use what we already have (TRACESYSGOOD etc).

strace wasn't handling ^Z correctly for many years. It definitely can wait for another kernel release cycle.

vda