exit_ptrace() && ptrace_report_signal() problems

[Oleg Nesterov]

On 09/14, Oleg Nesterov wrote:
>
> On 09/13, Roland McGrath wrote:
> >
> > Locks just between the tracer and ptrace_report_signal are not bad.
>
> OK, but let me think a bit more. I'd really like to avoid adding the
> new lock to avoid the very unlikely race with the exiting tracer.

Oh, please take a look at this (untested) patch.

But I think I should make another attempt, probably spinlock (->siglock ?)
would be cleaner.

Oleg.


--- kstub/kernel/ptrace-utrace.c~9_EXIT_PTRACE_CAN_LOSE_SIG	2010-08-24 14:31:17.000000000 +0200
+++ kstub/kernel/ptrace-utrace.c	2010-09-14 23:37:59.000000000 +0200
@@ -67,6 +67,7 @@ struct ptrace_context {
 #define PT_UTRACED			0x00001000
 
 #define PTRACE_O_SYSEMU			0x100
+#define PTRACE_O_REUSABLE		0x200
 
 #define PTRACE_EVENT_SYSCALL		(1 << 16)
 #define PTRACE_EVENT_SIGTRAP		(2 << 16)
@@ -115,7 +116,7 @@ ptrace_reuse_engine(struct task_struct *
 		return engine;
 
 	ctx = ptrace_context(engine);
-	if (unlikely(ctx->resume == UTRACE_DETACH)) {
+	if (unlikely(ctx->options == PTRACE_O_REUSABLE)) {
 		/*
 		 * Try to reuse this self-detaching engine.
 		 * The only caller which can hit this case is ptrace_attach(),
@@ -246,24 +247,48 @@ static void ptrace_detach_task(struct ta
 	 */
 	bool voluntary = (sig >= 0);
 	struct utrace_engine *engine = ptrace_lookup_engine(tracee);
+	struct ptrace_context *ctx = ptrace_context(engine);
 	enum utrace_resume_action action = UTRACE_DETACH;
 
 	if (unlikely(IS_ERR(engine)))
 		return;
 
-	if (sig) {
-		struct ptrace_context *ctx = ptrace_context(engine);
+	if (!voluntary) {
+		int err;
+
+		ctx->resume = UTRACE_DETACH;
+		/* synchronize with ptrace_report_signal() */
+		do {
+			err = utrace_barrier(tracee, engine);
+		} while (err == -ERESTARTSYS);
+
+		if (!err) {
+			/*
+			 * The tracee has already reported a signal. If we
+			 * see ->siginfo != NULL it is safe to mark this ctx
+			 * as reusable and resume the tracee. We can race
+			 * with ptrace_report_signal(UTRACE_SIGNAL_REPORT)
+			 * already in progress but this doesn't matter, it
+			 * must see ->resume = UTRACE_DETACH.
+			 */
+			if (ctx->siginfo) {
+				smp_wmb();
+				ctx->options = PTRACE_O_REUSABLE;
+				action = UTRACE_RESUME;
+			}
+		}
 
+	} else if (sig) {
 		switch (get_stop_event(ctx)) {
 		case PTRACE_EVENT_SYSCALL:
-			if (voluntary)
-				send_sig_info(sig, SEND_SIG_PRIV, tracee);
+			send_sig_info(sig, SEND_SIG_PRIV, tracee);
 			break;
 
 		case PTRACE_EVENT_SIGNAL:
-			if (voluntary)
-				ctx->signr = sig;
+			ctx->signr = sig;
 			ctx->resume = UTRACE_DETACH;
+			smp_wmb();
+			ctx->options = PTRACE_O_REUSABLE;
 			action = UTRACE_RESUME;
 			break;
 		}
@@ -410,6 +435,9 @@ static u32 ptrace_report_syscall_exit(u3
 		return UTRACE_STOP;
 
 	if (ctx->resume != UTRACE_RESUME) {
+		if (ctx->resume == UTRACE_DETACH)
+			return UTRACE_RESUME;
+
 		WARN_ON(ctx->resume != UTRACE_BLOCKSTEP &&
 			ctx->resume != UTRACE_SINGLESTEP);
 		ctx->resume = UTRACE_RESUME;
@@ -502,6 +530,9 @@ static u32 ptrace_report_signal(u32 acti
 			ctx->siginfo = NULL;
 
 		if (resume != UTRACE_RESUME) {
+			if (resume == UTRACE_DETACH)
+				return action;
+
 			WARN_ON(resume != UTRACE_BLOCKSTEP &&
 				resume != UTRACE_SINGLESTEP);
 
@@ -531,6 +562,11 @@ static u32 ptrace_report_signal(u32 acti
 	}
 
 	WARN_ON(ctx->siginfo);
+
+	/* Raced with the exiting tracer ? */
+	if (resume == UTRACE_DETACH)
+		return action;
+
 	ctx->siginfo = info;
 	/*
 	 * ctx->siginfo points to the caller's stack.
@@ -759,7 +795,7 @@ void exit_ptrace(struct task_struct *tra
 static int ptrace_set_options(struct task_struct *tracee,
 				struct utrace_engine *engine, long data)
 {
-	BUILD_BUG_ON(PTRACE_O_MASK & PTRACE_O_SYSEMU);
+	BUILD_BUG_ON(PTRACE_O_MASK & (PTRACE_O_SYSEMU | PTRACE_O_REUSABLE));
 
 	ptrace_set_events(tracee, engine, data & PTRACE_O_MASK);
 	return (data & ~PTRACE_O_MASK) ? -EINVAL : 0;