linux-kernel - [pseudo patch] ptrace should respect the group stop

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110221161140.GA24336@redhat.com>
Date:	Mon, 21 Feb 2011 17:11:40 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	Denys Vlasenko <vda.linux@...glemail.com>,
	Roland McGrath <roland@...hat.com>, jan.kratochvil@...hat.com,
	linux-kernel@...r.kernel.org, torvalds@...ux-foundation.org,
	akpm@...ux-foundation.org
Subject: [pseudo patch] ptrace should respect the group stop

On 02/21, Oleg Nesterov wrote:
>
> On 02/21, Tejun Heo wrote:
> >
> > That preciesly is what is being discussed.  IIUC, Oleg and Roland are
> > saying that the tracee should enter group stop but not ptrace trap at
> > that point and then transition into ptrace trap on the first PTRACE
> > call.
>
> Actually I am not saying this (at least now, probably I did).
>
> Once again. We have the bug with arch_ptrace_stop_needed(), but lets
> ignore it to simplify the discussion.
>
> Suppose that the tracee calls do_signal_stop() and participates in the
> group stop. To me, it doesn't really matter (in the context of this
> discussion) if it stops in TASK_STOPPED or TASK_TRACED (and where it
> stops).
>
> However, I am starting to agree that TASK_TRACED looks more clean.
>
> What is important, I think ptrace should respect SIGNAL_STOP_STOPPED.
> IOW, when the tracee is group-stopped (TASK_STOPPED or TASK_TRACED,
> doesn't matter), ptrace_resume() should not wake it up, but merely
> do set_task_state(TASK_STATE) and make it resumeable by SIGCONT.

IOW. I mean something like the (uncompiled, incomplete) patch below as
as the initial approximation.

Oleg.

--- x/kernel/ptrace.c
+++ x/kernel/ptrace.c
@@ -38,15 +38,15 @@ void __ptrace_link(struct task_struct *c
 }
 
 /*
- * Turn a tracing stop into a normal stop now, since with no tracer there
- * would be no way to wake it up with SIGCONT or SIGKILL.  If there was a
- * signal sent that would resume the child, but didn't because it was in
- * TASK_TRACED, resume it now.
- * Requires that irqs be disabled.
+ * NEW COMMENT
  */
-static void ptrace_untrace(struct task_struct *child)
+static void ptrace_wake_up(struct task_struct *child)
 {
-	spin_lock(&child->sighand->siglock);
+	unsigned long flags;
+
+	if (!lock_task_sighand(child, &flags))
+		return;
+
 	if (task_is_traced(child)) {
 		/*
 		 * If the group stop is completed or in progress,
@@ -56,9 +56,9 @@ static void ptrace_untrace(struct task_s
 		    child->signal->group_stop_count)
 			__set_task_state(child, TASK_STOPPED);
 		else
-			signal_wake_up(child, 1);
+			wake_up_state(child, TASK_TRACED);
 	}
-	spin_unlock(&child->sighand->siglock);
+	unlock_task_sighand(tsk, &flags);
 }
 
 /*
@@ -76,7 +76,7 @@ void __ptrace_unlink(struct task_struct 
 	list_del_init(&child->ptrace_entry);
 
 	if (task_is_traced(child))
-		ptrace_untrace(child);
+		ptrace_wake_up(child);
 }
 
 /*
@@ -312,8 +312,6 @@ int ptrace_detach(struct task_struct *ch
 	if (child->ptrace) {
 		child->exit_code = data;
 		dead = __ptrace_detach(current, child);
-		if (!child->exit_state)
-			wake_up_state(child, TASK_TRACED | TASK_STOPPED);
 	}
 	write_unlock_irq(&tasklist_lock);
 
@@ -514,7 +512,7 @@ static int ptrace_resume(struct task_str
 	}
 
 	child->exit_code = data;
-	wake_up_process(child);
+	ptrace_wake_up(child);
 
 	return 0;
 }
--- x/kernel/signal.c
+++ x/kernel/signal.c
@@ -1644,8 +1644,11 @@ static void ptrace_stop(int exit_code, i
 	 * If there is a group stop in progress,
 	 * we must participate in the bookkeeping.
 	 */
-	if (current->signal->group_stop_count > 0)
-		--current->signal->group_stop_count;
+	if (current->signal->group_stop_count) {
+		// XXX: this is not enough, we can race with detach
+		if (!--current->signal->group_stop_count)
+			current->signal->flags = SIGNAL_STOP_STOPPED;
+	}
 
 	current->last_siginfo = info;
 	current->exit_code = exit_code;
@@ -1825,6 +1828,9 @@ static int ptrace_signal(int signr, sigi
 	if (sigismember(&current->blocked, signr)) {
 		specific_send_sig_info(signr, info, current);
 		signr = 0;
+	} else if (sig_kernel_stop(signr)) {
+		// XXX: not exactly right but anyway better
+		current->signal->flags |= SIGNAL_STOP_DEQUEUED;
 	}
 
 	return signr;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/