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

[Oleg Nesterov]

Hi Tejun,

On 11/09, Tejun Heo wrote:
>
> Hello, Oleg.
>
> > 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...

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.

> > 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.
>
> 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.

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.

--- x/kernel/ptrace.c
+++ x/kernel/ptrace.c
@@ -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);

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