ptrace() hangs on attempt to seize/attach stopped & frozen task

Hi,

So, the ptrace() hangs if we try to attach to stopped task from freezing cgroup.
It seems this was introduced by 5d8f72b55c2756("freezer: change ptrace_stop/do_signal_stop to use freezable_schedule()"). See below for the exact scenario and small script to reproduce this.

Tracee: Tracer:
static bool do_signal_stop(int signr)

	__set_current_state(TASK_STOPPED);
	freezable_schedule();
		freezer_do_not_count();
		schedule(); /* waiting for wake up */

									ptrace_attach()
										if (task_is_stopped(task) &&
										    task_set_jobctl_pending(task, JOBCTL_TRAP_STOP | JOBCTL_TRAPPING))
											signal_wake_up_state(task, __TASK_STOPPED);

		/* woken up by ptrace_attach() */
		freezer_count();
			__refrigerator()
										/* And here we will hang, because tracee is now frozen in __refrigerator() */
										wait_on_bit(&task->jobctl, JOBCTL_TRAPPING_BIT,
												TASK_UNINTERRUPTIBLE);

Reproduecer:
-----------
#!/bin/bash

sleep 100 &
pid=$!
kill -SIGSTOP $pid
mkdir -p /sys/fs/cgroup/freezer/test
echo $pid > /sys/fs/cgroup/freezer/test/tasks echo FROZEN > /sys/fs/cgroup/freezer/test/freezer.state

echo '#include <stdlib.h>
#include <sys/ptrace.h>

int main(int argc, char **argv)
{
int pid = atoi(argv[1]);
return ptrace(PTRACE_SEIZE, pid, NULL, 0);
}
' | gcc -x c -
./a.out $pid

Hi,

quite possible...

Thanks. All I can say I never liked this wait_on_bit() ;)

I need to think, but *at first glance* we can move this wait-for-stopped- traced-transition into do_wait() path, and this way clear_jobctl_trapping() can use __wake_up_parent(). Perhaps we just need to modify task_stopped_code() to take JOBCTL_TRAPPING into account...

Sure, debugger will block in sys_wait() after PTRACE_ATTACH/SEIZE. But this does not really differ from the case when the tracee was already frozen; SIGSTOP sent by ATTACH or PTRACE_INTERRUPT, so debugger will equally block in do_wait() until the tracee is unfrozen.

Oleg.

Hello, Oleg.

We simply need to reimplement cgroup freezer so that its userland visible state is well defined (most likely jobctl stop). Right now, it's allowing userland to trigger "stuck somewhere in the kernel" condition, so interactions with frozen tasks are naturally broken.

Thanks.

tejun

Not really, this was broken before that commit. Note that try_to_freeze() was called under "relock:" label, so we had the same problem: the tracee doesn't do do_jobctl_trap() after do_signal_stop() without try_to_freeze() in between. Not that it matters, but still.

Oleg.

Hi Tejun,

It seems that we can simply kill JOBCTL_TRAPPING, no other changes are needed.

But afaics task_stopped_code()->task_is_stopped_or_traced() looks confusing, it should not see a TASK_STOPPED task if ptrace, task_is_traced() makes more sense. This is off-topic.

I agree, the freezer is not perfect, and it needs changes.

Still I think this needs a fix in ptrace code. At least we should not wait in TASK_UNINTERRUPTIBLE state.

And perhaps we can simply remove this logic? I forgot why do we hide this STOPPED -> RUNNING -> TRACED transition from the attaching thread. But the vague feeling tells me that we discussed this before and perhaps it was me who suggested to avoid the user-visible change when you introduced this transition...

Anyway, now I do not understand why do we want to hide it. Lets consider the following "test-case",

	void test(int pid)
	{
		kill(pid, SIGSTOP);
		waitpid(pid, NULL, WSTOPPED);

		ptrace(PTRACE_ATTACH-OR-PTRACE_SEIZE, pid, 0,0);

		assert(ptrace(PTRACE_DETACH, pid, 0,0) == 0);
	}

Yes, it will fail if we remove JOBCTL_TRAPPING. But it can equally fail if SIGCONT comes before ATTACH, so perhaps we do not really care?

Jan, Pedro, do you think the patch below can break gdb somehow? With this patch you can never assume that waitpid(WNOHANG) or ptrace(WHATEVER) will succeed right after PTRACE_ATTACH/PTRACE_SEIZE, even if you know that the tracee was TASK_STOPPED before attach.

Tejun, do you see any reason to keep JOBCTL_TRAPPING?

Oleg.

@@ -338,21 +338,13 @@ static int ptrace_attach(struct task_struct *task, long request,
 	 * 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.  We'll wait
-	 * for the transition to complete before returning from this
-	 * function.
-	 *
-	 * This hides STOPPED -> RUNNING -> TRACED transition from the
-	 * attaching thread but a different thread in the same group can
-	 * still observe the transient RUNNING state.  IOW, if another
-	 * thread's WNOHANG wait(2) on the stopped tracee races against
-	 * ATTACH, the wait(2) may fail due to the transient RUNNING.
+	 * event which clears the group stop states happens.
 	 *
 	 * The following task_is_stopped() test is safe as both transitions
 	 * in and out of STOPPED are protected by siglock.
 	 */
 	if (task_is_stopped(task) &&
-	    task_set_jobctl_pending(task, JOBCTL_TRAP_STOP | JOBCTL_TRAPPING))
+	    task_set_jobctl_pending(task, JOBCTL_TRAP_STOP))
 		signal_wake_up_state(task, __TASK_STOPPED);
 
 	spin_unlock(&task->sighand->siglock);

Hello, Oleg.

Sorry about the delay.

Heh, it was too long ago for me to remember much. :)

Hmmm... It's nasty tho. We're breaking a guaranteed userland behavior to mask a deficiency (IMHO it's an outright bug) in a different subsystem. The problem here is that cgroup-frozen threads become un-runnable on a running system and it doesn't make sense to me to work around that from all the affected places rather than fixing it at the source especially if that involves breaking a known supported userland behavior. This isn't different from the frozen processes failing to respond to SIGKILL. I'd be a lot more comfortable stating that cgroup freezer is currently broken rather than diddling with subtle ptrace semantics.

Thanks.

tejun

Hmm, [email protected] doesn't like you. But I dared to open and nothing happened so far. although perhaps you already own my machine.

Same here...

Perhaps you are right, but I am wondering if it was ever guaranteed.

What actually annoys me is that now I am almost sure that it was me who asked you to hide this from user-space, and today I see no reason for this hack.

OK, lets keep this JOBCTL_TRAPPING_BIT.

But still I would like to know what Pedro thinks...

Anyway, wait_on_bit(TASK_UNINTERRUPTIBLE) doesn't look good. Do you see any problem with the change below? Yes, the comment is not clear, it should be updated, the tracee can clear this bit too.

And perhaps we can change get_task_state() until freezer gets another state,

	--- x/fs/proc/array.c
	+++ x/fs/proc/array.c
	@@ -126,6 +126,9 @@ static inline const char *get_task_state
	 {
		unsigned int state = (tsk->state | tsk->exit_state) & TASK_REPORT;
	 
	+	if (tsk->flags & PF_FROZEN)
	+		return "D (frozen)";
	+
		BUILD_BUG_ON(1 + ilog2(TASK_REPORT) != ARRAY_SIZE(task_state_array)-1);
	 
		return task_state_array[fls(state)];

?

Oleg.

@@ -364,8 +364,13 @@ unlock_creds:
 	mutex_unlock(&task->signal->cred_guard_mutex);
 out:
 	if (!retval) {
-		wait_on_bit(&task->jobctl, JOBCTL_TRAPPING_BIT,
-			    TASK_UNINTERRUPTIBLE);
+		if (wait_on_bit(&task->jobctl, JOBCTL_TRAPPING_BIT,
+				TASK_KILLABLE))
+			/*
+			 * We will clear JOBCTL_TRAPPING in __ptrace_unlink(),
+			 * until then nobody can trace this task anyway.
+			 */
+			retval = -EINTR;
 		proc_ptrace_connector(task, PTRACE_ATTACH);
 	}
 

Hey, Oleg.

lol no idea what's going on there but dude you gotta clean up the browsing history.

Hmm... the only nit is that we'll eventually want to share "T (stopped)" or do "T (frozen)" and switching down the road could be a bit confusing. It shouldn't be a big deal tho. I think I'm mostly reluctant to accomodate the broken behavior of cgroup freezer.

tejun

Not sure, because I don't think I fully understand that proposed change.

Both GDB and gdbserver have special processing for attaching to already-stopped processes. (and neither use PTRACE_SEIZE yet.)

Here's the gdbserver version:

https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/gdbserver/linux-low.c;h=41ab510fa4ac5654f101f08efb68e26b5bc5dbd7;hb=HEAD#l903

Copied here for convenience:

907 linux_attach_lwp (ptid_t ptid)
908 {
909 struct lwp_info *new_lwp;
910 int lwpid = ptid_get_lwp (ptid);
911
912 if (ptrace (PTRACE_ATTACH, lwpid, (PTRACE_TYPE_ARG3) 0, (PTRACE_TYPE_ARG4) 0)
913 != 0)
914 return errno;
915
916 new_lwp = add_lwp (ptid);
917
918 /* We need to wait for SIGSTOP before being able to make the next
919 ptrace call on this LWP. */
920 new_lwp->must_set_ptrace_flags = 1;
921
922 if (linux_proc_pid_is_stopped (lwpid))
923 {
924 if (debug_threads)
925 debug_printf ("Attached to a stopped process\n");
926
927 /* The process is definitely stopped. It is in a job control

 928          stop, unless the kernel predates the TASK_STOPPED /
 929          TASK_TRACED distinction, in which case it might be in a
 930          ptrace stop.  Make sure it is in a ptrace stop; from there we
 931          can kill it, signal it, et cetera.
 932
 933          First make sure there is a pending SIGSTOP.  Since we are
 934          already attached, the process can not transition from stopped
 935          to running without a PTRACE_CONT; so we know this signal will
 936          go into the queue.  The SIGSTOP generated by PTRACE_ATTACH is
 937          probably already in the queue (unless this kernel is old
 938          enough to use TASK_STOPPED for ptrace stops); but since
 939          SIGSTOP is not an RT signal, it can only be queued once.  */

940 kill_lwp (lwpid, SIGSTOP);
941
942 /* Finally, resume the stopped process. This will deliver the
943 SIGSTOP (or a higher priority signal, just like normal
944 PTRACE_ATTACH), which we'll catch later on. */
945 ptrace (PTRACE_CONT, lwpid, (PTRACE_TYPE_ARG3) 0, (PTRACE_TYPE_ARG4) 0);
946 }
947
948 /* The next time we wait for this LWP we'll see a SIGSTOP as PTRACE_ATTACH
949 brings it to a halt.
950

linux_proc_pid_is_stopped checks whether the state in /proc/pid/status is "T (stopped)".

Here's the equivalent in gdb:

https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/linux-nat.c;h=841ec3949c37438dfba924d8db6b37ffc416dd29;hb=HEAD#l974

This queuing of a SIGSTOP + PTRACE_CONT was necessary because otherwise when gdb attaches to a job stopped process, gdb would hang in the waitpid after PTRACE_ATTACH, waiting for the initial SIGSTOP which would never arrive.

If the proposed change makes it so that a new intermediate state can be observed right after PTRACE_ATTACH, and so linux_proc_pid_is_stopped can return false, then there's potential for breakage. But maybe not, if we're sure that that when that happens, waitpid returns for the initial PTRACE_ATTACH-induced SIGSTOP.

Thanks,
Pedro Alves

Thanks Pedro for your email,

I'll recheck tomorrow, but at first glance:

Yes, I am starting to recall that I have looked at this code years ago ;)

This can't happen today. Starting from v3.0 at least.

Yes, because its exit code could be already cleared iirc. This was fixed even before.

See above,

Yes. Just you can't assume that watpid(WNOHANG) will succeed. Is it OK?

Oleg.

Eh, interesting. So right after PTRACE_ATTACH, we either observe "running" or "ptrace-stopped", but never "job stopped". Correct?

I've actually just now tried this:

index 841ec39..42f2b0d 100644
@@ -981,6 +981,7 @@ linux_nat_post_attach_wait (ptid_t ptid, int first, int *cloned,
   pid_t new_pid, pid = ptid_get_lwp (ptid);
   int status;
 
+#if 0
   if (linux_proc_pid_is_stopped (pid))
     {
       if (debug_linux_nat)

@@ -1006,6 +1007,7 @@ linux_nat_post_attach_wait (ptid_t ptid, int first, int *cloned,
         (or a higher priority signal, just like normal PTRACE_ATTACH).  */
       ptrace (PTRACE_CONT, pid, 0, 0);
     }
+#endif
 
   /* Make sure the initial process is stopped.  The user-level threads
      layer might want to poke around in the inferior, and that won't

and sure enough, gdb's test that covers that use case still passes, on Fedora 20 (Linux 3.19.8).

And given that my Thunderbird crashed while writing this, I had sufficient time to be sure that a full test run passes cleanly too. :-P :-)

Yes, assuming the ptracer is guaranteed to get a SIGCHLD to wake it up.

Thanks,
Pedro Alves

OK, lets make it killable at least, TASK_KILLABLE looks better in any case.

I am starting to think again that we should remove JOBCTL_TRAPPING, but this needs a bit more discussion and probably testing.

Anyway I agree with Tejun, the freezer doesn't look nice too.

Hmm. I need to re-check but it seems we have another problem... ptrace_stop() does task_participate_group_stop() and this (in particular) clears JOBCTL_STOP_PENDING. This is only correct if the task is still traced, but this is not necessarily true and we can in fact clear JOBCTL_STOP_PENDING set by __ptrace_unlink() called by the exiting tracer.

Oleg.

ptrace_attach() can hang waiting for STOPPED -> TRACED transition if the tracee gets frozen in between, change wait_on_bit() to use TASK_KILLABLE.

This doesn't really solve the problem(s) and we probably need to fix the freezer. In particular, note that this means that pm freezer will fail if it races attach-to-stopped-task.

And otoh perhaps we can just remove JOBCTL_TRAPPING_BIT altogether, it is not clear if we really need to hide this transition from debugger, WNOHANG after PTRACE_ATTACH can fail anyway if it races with SIGCONT.

---
 kernel/ptrace.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

index c8e0e05..80b3604 100644
@@ -364,8 +364,14 @@ unlock_creds:
 	mutex_unlock(&task->signal->cred_guard_mutex);
 out:
 	if (!retval) {
-		wait_on_bit(&task->jobctl, JOBCTL_TRAPPING_BIT,
-			    TASK_UNINTERRUPTIBLE);
+		/*
+		 * We do not bother to change retval or clear JOBCTL_TRAPPING
+		 * if wait_on_bit() was interrupted by SIGKILL. The tracer will
+		 * not return to user-mode, it will exit and clear this bit in
+		 * __ptrace_unlink() if it wasn't already cleared by the tracee;
+		 * and until then nobody can ptrace this task.
+		 */
+		wait_on_bit(&task->jobctl, JOBCTL_TRAPPING_BIT, TASK_KILLABLE);
 		proc_ptrace_connector(task, PTRACE_ATTACH);
 	}
 

1.5.5.1

task_stopped_code()->task_is_stopped_or_traced() doesn't look right, the traced task must never be TASK_STOPPED.

We can not add WARN_ON(task_is_stopped(p)), but this is only because do_wait() can race with PTRACE_ATTACH from another thread.

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

index c090738..fc7b4ec 100644
@@ -1127,7 +1127,7 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
 static int *task_stopped_code(struct task_struct *p, bool ptrace)
 {
 	if (ptrace) {
-		if (task_is_stopped_or_traced(p) &&
+		if (task_is_traced(p) &&
 		    !(p->jobctl & JOBCTL_LISTENING))
 			return &p->exit_code;
 	} else {

1.5.5.1

Thanks.

tejun

I think it's a left over from before. We used to leave STOPPED tasks alone during ptrace attach.

Thanks.

tejun