Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1270466 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: ptrace() hangs on attempt to seize/attach stopped & frozen task Tejun Heo <tj@kernel.org> - 2015-11-16 19:50 +0100
    Re: ptrace() hangs on attempt to seize/attach stopped & frozen task Oleg Nesterov <oleg@redhat.com> - 2015-11-17 19:40 +0100
      Re: ptrace() hangs on attempt to seize/attach stopped & frozen task Tejun Heo <tj@kernel.org> - 2015-11-17 20:00 +0100
      Re: ptrace() hangs on attempt to seize/attach stopped & frozen task Pedro Alves <palves@redhat.com> - 2015-11-19 18:00 +0100
        Re: ptrace() hangs on attempt to seize/attach stopped & frozen task Oleg Nesterov <oleg@redhat.com> - 2015-11-19 18:50 +0100
          Re: ptrace() hangs on attempt to seize/attach stopped & frozen task Pedro Alves <palves@redhat.com> - 2015-11-19 19:10 +0100

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

Hello, Oleg.

Sorry about the delay.

On Tue, Nov 10, 2015 at 09:20:17PM +0100, Oleg Nesterov wrote:
> > 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...

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

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

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

[toc] | [next] | [standalone]

#1271537

On 11/16, Tejun Heo wrote:
>
> *** WARNING: THE ATTACHED DOCUMENT(S) CONTAIN MACROS ***
> *** MACROS MAY CONTAIN MALICIOUS CODE ***
> *** Open only if you can verify and trust the sender ***
> *** Please contact infosec@redhat.com if you have questions or concerns **

Hmm, infosec@redhat.com doesn't like you. But I dared to open and nothing
happened so far. although perhaps you already own my machine.

> > 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...
>
> Heh, it was too long ago for me to remember much. :)

Same here...

> > 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?
>
> Hmmm... It's nasty tho.  We're breaking a guaranteed userland behavior

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.

> I'd be a lot more comfortable stating
> that cgroup freezer is currently broken rather than diddling with
> subtle ptrace semantics.

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.

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

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

[toc] | [prev] | [next] | [standalone]

#1271573

Hey, Oleg.

On Tue, Nov 17, 2015 at 08:34:19PM +0100, Oleg Nesterov wrote:
> On 11/16, Tejun Heo wrote:
> >
> > *** WARNING: THE ATTACHED DOCUMENT(S) CONTAIN MACROS ***
> > *** MACROS MAY CONTAIN MALICIOUS CODE ***
> > *** Open only if you can verify and trust the sender ***
> > *** Please contact infosec@redhat.com if you have questions or concerns **
> 
> Hmm, infosec@redhat.com doesn't like you. But I dared to open and nothing
> happened so far. although perhaps you already own my machine.

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

> > Hmmm... It's nasty tho.  We're breaking a guaranteed userland behavior
> 
> 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.
> 
> > I'd be a lot more comfortable stating
> > that cgroup freezer is currently broken rather than diddling with
> > subtle ptrace semantics.
> 
> 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)];

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.

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

Yeah, this looks good to me.

Thanks.

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

[toc] | [prev] | [next] | [standalone]

#1273288

On 11/17/2015 07:34 PM, Oleg Nesterov wrote:
> On 11/16, Tejun Heo wrote:

>>> 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...
>>
>> Heh, it was too long ago for me to remember much. :)
> 
> Same here...
> 
>>> 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.

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

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

[toc] | [prev] | [next] | [standalone]

#1273305

Thanks Pedro for your email,

I'll recheck tomorrow, but at first glance:

On 11/19, Pedro Alves wrote:
>
> Both GDB and gdbserver have special processing for attaching to already-stopped
> processes.

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

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

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

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

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

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

See above,

> But maybe not, if we're sure that
> that when that happens, waitpid returns for the initial
> PTRACE_ATTACH-induced SIGSTOP.

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

Oleg.

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

[toc] | [prev] | [next] | [standalone]

#1273331

On 11/19/2015 05:47 PM, Oleg Nesterov wrote:
> Thanks Pedro for your email,
> 

>>  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))
> 
> This can't happen today. Starting from v3.0 at least.

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:

diff --git c/gdb/linux-nat.c w/gdb/linux-nat.c
index 841ec39..42f2b0d 100644
--- c/gdb/linux-nat.c
+++ w/gdb/linux-nat.c
@@ -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  :-)

>> But maybe not, if we're sure that
>> that when that happens, waitpid returns for the initial
>> PTRACE_ATTACH-induced SIGSTOP.
> 
> Yes. Just you can't assume that watpid(WNOHANG) will succeed. Is it OK?

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web