Before this change debuggers needed to jump through hoops
if they want to detach from a running tracee.
Typically they just try PTRACE_DETACH, and if it fails
with ESRCH, they make tracee enter a ptrace-stop.
Before introduction of PTRACE_INTERRUPT, the only way to do that
was to send a signal (which raced with real signals),
then repeatedly call waitpid looking for (this particular)
signal to be delivered, and finally call PTRACE_DETACH.
With PTRACE_INTERRUPT it is somewhat less ugly,
and not racy wrt signals.
This does not make much sense, especially that kernel code
for detach operation already supports implicit,
asynchronous detach which happens if tracer process exits.
This change simply removes the requirement that tracee is
in ptrace-stop for PTRACE_DETACH to work.
This is a user-visible behavior change.
Do we really have to introduce a separate
PTRACE_NOT_STUPID_DETACH? I hope not.
If this behavior existed from the start, strace
would not need a large, ugly chunk of code it currently has
to handle this quirk.
CCing Jan to hear his comments from gdb side.
Signed-off-by: Denys Vlasenko <[email protected]>
CC: Jan Kratochvil <[email protected]>
CC: Dmitry V. Levin <[email protected]>
CC: Oleg Nesterov <[email protected]>
---
kernel/ptrace.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index fc07650..2e294bf 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -56,9 +56,7 @@ void __ptrace_link(struct task_struct *child, struct task_struct *new_parent)
* state.
*
* Unlinking can happen via two paths - explicit PTRACE_DETACH or ptracer
- * exiting. For PTRACE_DETACH, unless the ptracee has been killed between
- * ptrace_check_attach() and here, it's guaranteed to be in TASK_TRACED.
- * If the ptracer is exiting, the ptracee can be in any state.
+ * exiting. The ptracee is not necessarily ptrace-stopped.
*
* After detach, the ptracee should be in a state which conforms to the
* group stop. If the group is stopped or in the process of stopping, the
@@ -1062,7 +1060,8 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
}
ret = ptrace_check_attach(child, request == PTRACE_KILL ||
- request == PTRACE_INTERRUPT);
+ request == PTRACE_INTERRUPT ||
+ request == PTRACE_DETACH);
if (ret < 0)
goto out_put_task_struct;
@@ -1207,7 +1206,8 @@ asmlinkage long compat_sys_ptrace(compat_long_t request, compat_long_t pid,
}
ret = ptrace_check_attach(child, request == PTRACE_KILL ||
- request == PTRACE_INTERRUPT);
+ request == PTRACE_INTERRUPT ||
+ request == PTRACE_DETACH);
if (!ret) {
ret = compat_arch_ptrace(child, request, addr, data);
if (ret || request != PTRACE_DETACH)
--
1.8.1.4
On Wed, 19 Jun 2013 17:15:36 +0200, Denys Vlasenko wrote:
> CCing Jan to hear his comments from gdb side.
GDB never calls PTRACE_DETACH without having the inferior already stopped.
Jan
On 06/19, Denys Vlasenko wrote:
>
> This is a user-visible behavior change.
> Do we really have to introduce a separate
> PTRACE_NOT_STUPID_DETACH? I hope not.
Oh, I think yes.
> @@ -1062,7 +1060,8 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
> }
>
> ret = ptrace_check_attach(child, request == PTRACE_KILL ||
> - request == PTRACE_INTERRUPT);
> + request == PTRACE_INTERRUPT ||
> + request == PTRACE_DETACH);
There doesn't look right.
For example ptrace_disable(). See the comment set_task_blockstep().
And flush_ptrace_hw_breakpoint() can race with the exiting task.
And the setting of ->exit_code is racy too.
And this makes the ptrace_unfreeze_traced() logic more confusing...
but probably this is fine.
Oleg.
On 06/19/2013 05:09 PM, Jan Kratochvil wrote:
> On Wed, 19 Jun 2013 17:15:36 +0200, Denys Vlasenko wrote:
>> CCing Jan to hear his comments from gdb side.
PTRACE_DETACH takes a signal number in the data parameter.
What happens to if the tracer passes a non-zero signal?
--
Pedro Alves
On 06/19, Pedro Alves wrote:
>
> On 06/19/2013 05:09 PM, Jan Kratochvil wrote:
> > On Wed, 19 Jun 2013 17:15:36 +0200, Denys Vlasenko wrote:
> >> CCing Jan to hear his comments from gdb side.
>
> PTRACE_DETACH takes a signal number in the data parameter.
> What happens to if the tracer passes a non-zero signal?
non-zero doesn't matter, zero is equally bad.
ptrace_detach() simply does child->exit_code = data, assuming
that child will look at it after resume. This is just wrong if
the child is not stopped.
Oleg.
On 06/19/2013 06:32 PM, Oleg Nesterov wrote:
> On 06/19, Denys Vlasenko wrote:
>>
>> This is a user-visible behavior change.
>> Do we really have to introduce a separate
>> PTRACE_NOT_STUPID_DETACH? I hope not.
>
> Oh, I think yes.
>
>> @@ -1062,7 +1060,8 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
>> }
>>
>> ret = ptrace_check_attach(child, request == PTRACE_KILL ||
>> - request == PTRACE_INTERRUPT);
>> + request == PTRACE_INTERRUPT ||
>> + request == PTRACE_DETACH);
>
> There doesn't look right.
>
> For example ptrace_disable(). See the comment set_task_blockstep().
I see the comment. I think it implies that TF-induced debug
interrupt may happen on the running task after it is detached,
which will result in SIGTRAP being sent to it.
Correct me if I'm wrong.
If so, do we have the same problem if tracer exits
and implicit detach is performed?
--
vda
On 06/20, Denys Vlasenko wrote:
>
> On 06/19/2013 06:32 PM, Oleg Nesterov wrote:
> > On 06/19, Denys Vlasenko wrote:
> >>
> >> This is a user-visible behavior change.
> >> Do we really have to introduce a separate
> >> PTRACE_NOT_STUPID_DETACH? I hope not.
> >
> > Oh, I think yes.
> >
> >> @@ -1062,7 +1060,8 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
> >> }
> >>
> >> ret = ptrace_check_attach(child, request == PTRACE_KILL ||
> >> - request == PTRACE_INTERRUPT);
> >> + request == PTRACE_INTERRUPT ||
> >> + request == PTRACE_DETACH);
> >
> > There doesn't look right.
> >
> > For example ptrace_disable(). See the comment set_task_blockstep().
>
> I see the comment. I think it implies that TF-induced debug
> interrupt may happen on the running task after it is detached,
> which will result in SIGTRAP being sent to it.
No. The comment means that set/clear of TIF_BLOCKSTEP is not safe unless
the tracee can't run. If we race with __switch_to() we can set the wrong
debugctlmsr.
> If so, do we have the same problem if tracer exits
> and implicit detach is performed?
No. If the tracer exits it doesn't do the "cleanups" like ptrace_disable().
That is why this potentially leaves the tracee in the inconsistent state.
Oleg.