[PATCH ptrace] ptrace: use GROUP_STOP_TRAPPING for PTRACE_DETACH too

[PATCH ptrace] ptrace: use GROUP_STOP_TRAPPING for PTRACE_DETACH too

[Tejun Heo]

Currently GROUP_STOP_TRAPPING is used only for PTRACE_ATTACH to hide
STOPPED -> RUNNING -> TRACED transition; however, DETACH involves
similar transition in the reverse direction, which can be visible to
the next ptracer if it attaches before the transition is complete.

This patch makes DETACH also use TRAPPING and ptrace_attach() always
wait if TRAPPING is set to hide the transition.

Test program follows.

  int main(int argc, char **argv)
  {
	  pid_t tracee;
	  siginfo_t si = {};
	  int i, nr_wait_fails = 0, nr_ptrace_fails = 0;

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

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

	  for (i = 0; i < 100000; i++) {
		  ptrace(PTRACE_ATTACH, tracee, NULL, NULL);
		  waitid(P_PID, tracee, &si, WSTOPPED | WNOHANG);
		  if (!si.si_pid)
			  nr_wait_fails++;
		  if (ptrace(PTRACE_DETACH, tracee, NULL, NULL)) {
			  nr_ptrace_fails++;
			  waitid(P_PID, tracee, NULL, WSTOPPED);
			  ptrace(PTRACE_DETACH, tracee, NULL, NULL);
		  }
	  }
	  printf("wait/ptrace failed %d/%d times out of %d tries\n",
		 nr_wait_fails, nr_ptrace_fails, i);
	  return 0;
  }

Before the patch, transition from DETACH causes sporadic failures on
waitid(2) and ptrace(2) calls after re-attaching.

  # ./test-trapping-race
  wait/ptrace failed 453/449 times out of 100000 tries

After, PTRACE_ATTACH waits for the previous transitions to complete
too and they don't fail.

  # ./test-trapping-race-simple 
  wait/ptrace failed 0/0 times out of 100000 tries

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 kernel/ptrace.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Index: work/kernel/ptrace.c
===================================================================
--- work.orig/kernel/ptrace.c
+++ work/kernel/ptrace.c
@@ -77,12 +77,15 @@ void __ptrace_unlink(struct task_struct
 
 	/*
 	 * Reinstate GROUP_STOP_PENDING if group stop is in effect and
-	 * @child isn't dead.
+	 * @child isn't dead.  This will trigger TRACED -> RUNNING ->
+	 * STOPPED transition.  As this transition can affect the next
+	 * ptracer if it attaches before the transition completes, set
+	 * TRAPPING too.  Read comment in ptrace_attach() for more details.
 	 */
 	if (!(child->flags & PF_EXITING) &&
 	    (child->signal->flags & SIGNAL_STOP_STOPPED ||
 	     child->signal->group_stop_count))
-		child->group_stop |= GROUP_STOP_PENDING;
+		child->group_stop |= GROUP_STOP_PENDING | GROUP_STOP_TRAPPING;
 
 	/*
 	 * If transition to TASK_STOPPED is pending or in TASK_TRACED, kick
@@ -183,7 +186,6 @@ bool ptrace_may_access(struct task_struc
 
 static int ptrace_attach(struct task_struct *task)
 {
-	bool wait_trap = false;
 	int retval;
 
 	audit_ptrace(task);
@@ -245,7 +247,6 @@ static int ptrace_attach(struct task_str
 	if (task_is_stopped(task)) {
 		task->group_stop |= GROUP_STOP_PENDING | GROUP_STOP_TRAPPING;
 		signal_wake_up(task, 1);
-		wait_trap = true;
 	}
 
 	spin_unlock(&task->sighand->siglock);
@@ -256,9 +257,8 @@ unlock_tasklist:
 unlock_creds:
 	mutex_unlock(&task->signal->cred_guard_mutex);
 out:
-	if (wait_trap)
-		wait_event(current->signal->wait_chldexit,
-			   !(task->group_stop & GROUP_STOP_TRAPPING));
+	wait_event(current->signal->wait_chldexit,
+		   !(task->group_stop & GROUP_STOP_TRAPPING));
 	return retval;
 }
 

Re: [PATCH ptrace] ptrace: use GROUP_STOP_TRAPPING for PTRACE_DETACH too

[Oleg Nesterov]

On 05/08, Tejun Heo wrote:
>
> Currently GROUP_STOP_TRAPPING is used only for PTRACE_ATTACH to hide
> STOPPED -> RUNNING -> TRACED transition; however, DETACH involves
> similar transition in the reverse direction, which can be visible to
> the next ptracer if it attaches before the transition is complete.

Yes...

> This patch makes DETACH also use TRAPPING and ptrace_attach() always
> wait if TRAPPING is set to hide the transition.

I am not sure, please see below.

> Test program follows.
>
>   int main(int argc, char **argv)
>   {
> 	  pid_t tracee;
> 	  siginfo_t si = {};
> 	  int i, nr_wait_fails = 0, nr_ptrace_fails = 0;
>
> 	  tracee = fork();
> 	  if (!tracee)
> 		  while (1)
> 			  pause();
>
> 	  kill(tracee, SIGSTOP);
> 	  waitid(P_PID, tracee, NULL, WSTOPPED | WNOWAIT);
>
> 	  for (i = 0; i < 100000; i++) {
> 		  ptrace(PTRACE_ATTACH, tracee, NULL, NULL);
> 		  waitid(P_PID, tracee, &si, WSTOPPED | WNOHANG);
> 		  if (!si.si_pid)
> 			  nr_wait_fails++;

OK, this is clear, waitid(WSTOPPED | WNOHANG) can fail if it sees the
tracee inside the transition.

But,

> 		  if (ptrace(PTRACE_DETACH, tracee, NULL, NULL)) {
> 			  nr_ptrace_fails++;

I assume this can only fail for the same reason if waitid() fails?
Or there is something else?

> --- work.orig/kernel/ptrace.c
> +++ work/kernel/ptrace.c
> @@ -77,12 +77,15 @@ void __ptrace_unlink(struct task_struct
>
>  	/*
>  	 * Reinstate GROUP_STOP_PENDING if group stop is in effect and
> -	 * @child isn't dead.
> +	 * @child isn't dead.  This will trigger TRACED -> RUNNING ->
> +	 * STOPPED transition.  As this transition can affect the next
> +	 * ptracer if it attaches before the transition completes, set
> +	 * TRAPPING too.  Read comment in ptrace_attach() for more details.
>  	 */
>  	if (!(child->flags & PF_EXITING) &&
>  	    (child->signal->flags & SIGNAL_STOP_STOPPED ||
>  	     child->signal->group_stop_count))
> -		child->group_stop |= GROUP_STOP_PENDING;
> +		child->group_stop |= GROUP_STOP_PENDING | GROUP_STOP_TRAPPING;

This doesn't look safe, see below. We do not know what the tracee does,
it can be even running.

>  static int ptrace_attach(struct task_struct *task)
>  {
> -	bool wait_trap = false;
>  	int retval;
>
>  	audit_ptrace(task);
> @@ -245,7 +247,6 @@ static int ptrace_attach(struct task_str
>  	if (task_is_stopped(task)) {
>  		task->group_stop |= GROUP_STOP_PENDING | GROUP_STOP_TRAPPING;
>  		signal_wake_up(task, 1);
> -		wait_trap = true;
>  	}
>
>  	spin_unlock(&task->sighand->siglock);
> @@ -256,9 +257,8 @@ unlock_tasklist:
>  unlock_creds:
>  	mutex_unlock(&task->signal->cred_guard_mutex);
>  out:
> -	if (wait_trap)
> -		wait_event(current->signal->wait_chldexit,
> -			   !(task->group_stop & GROUP_STOP_TRAPPING));
> +	wait_event(current->signal->wait_chldexit,
> +		   !(task->group_stop & GROUP_STOP_TRAPPING));

Suppose that SIGCONT or, worse, SIGKILL comes in between.

Oleg.

Re: [PATCH ptrace] ptrace: use GROUP_STOP_TRAPPING for PTRACE_DETACH too

[Tejun Heo]

Hey, Oleg.

On Sun, May 08, 2011 at 06:07:20PM +0200, Oleg Nesterov wrote:
> > 	  for (i = 0; i < 100000; i++) {
> > 		  ptrace(PTRACE_ATTACH, tracee, NULL, NULL);
> > 		  waitid(P_PID, tracee, &si, WSTOPPED | WNOHANG);
> > 		  if (!si.si_pid)
> > 			  nr_wait_fails++;
> 
> OK, this is clear, waitid(WSTOPPED | WNOHANG) can fail if it sees the
> tracee inside the transition.
> 
> But,
> 
> > 		  if (ptrace(PTRACE_DETACH, tracee, NULL, NULL)) {
> > 			  nr_ptrace_fails++;
> 
> I assume this can only fail for the same reason if waitid() fails?
> Or there is something else?

Oh yeah, I was just curious about the timing and see how many fail
waitid() but succeed ptrace().

> >  	/*
> >  	 * Reinstate GROUP_STOP_PENDING if group stop is in effect and
> > -	 * @child isn't dead.
> > +	 * @child isn't dead.  This will trigger TRACED -> RUNNING ->
> > +	 * STOPPED transition.  As this transition can affect the next
> > +	 * ptracer if it attaches before the transition completes, set
> > +	 * TRAPPING too.  Read comment in ptrace_attach() for more details.
> >  	 */
> >  	if (!(child->flags & PF_EXITING) &&
> >  	    (child->signal->flags & SIGNAL_STOP_STOPPED ||
> >  	     child->signal->group_stop_count))
> > -		child->group_stop |= GROUP_STOP_PENDING;
> > +		child->group_stop |= GROUP_STOP_PENDING | GROUP_STOP_TRAPPING;
> 
> This doesn't look safe, see below. We do not know what the tracee does,
> it can be even running.

Ah, right.  Setting TRAPPING should probably depend on JOBCTL_TRAPPED
flag which is added later.

> >  static int ptrace_attach(struct task_struct *task)
> >  {
> > -	bool wait_trap = false;
> >  	int retval;
> >
> >  	audit_ptrace(task);
> > @@ -245,7 +247,6 @@ static int ptrace_attach(struct task_str
> >  	if (task_is_stopped(task)) {
> >  		task->group_stop |= GROUP_STOP_PENDING | GROUP_STOP_TRAPPING;
> >  		signal_wake_up(task, 1);
> > -		wait_trap = true;
> >  	}
> >
> >  	spin_unlock(&task->sighand->siglock);
> > @@ -256,9 +257,8 @@ unlock_tasklist:
> >  unlock_creds:
> >  	mutex_unlock(&task->signal->cred_guard_mutex);
> >  out:
> > -	if (wait_trap)
> > -		wait_event(current->signal->wait_chldexit,
> > -			   !(task->group_stop & GROUP_STOP_TRAPPING));
> > +	wait_event(current->signal->wait_chldexit,
> > +		   !(task->group_stop & GROUP_STOP_TRAPPING));
> 
> Suppose that SIGCONT or, worse, SIGKILL comes in between.

Please ignore this one for now.  I'll re-do it after the SEIZE series.

Thanks.

-- 
tejun