This patch fixes x86, other machines need the similar fix. Hopefully
maintainers can help.
If the tracee calls fork() after PTRACE_SINGLESTEP, the forked child starts
with TIF_SINGLESTEP/X86_EFLAGS_TF bits copied from ptraced parent. This is
not right, especially when the new child is not auto-attaced: in this case
it is killed by SIGTRAP.
Test-case:
#include <stdio.h>
#include <unistd.h>
#include <signal.h>
#include <sys/ptrace.h>
#include <sys/wait.h>
#include <assert.h>
int main(void)
{
int pid, status;
if (!(pid = fork())) {
assert(ptrace(PTRACE_TRACEME) == 0);
kill(getpid(), SIGSTOP);
if (!fork()) {
/* kernel bug: this child will be killed by SIGTRAP */
printf("Hello world\n");
return 43;
}
wait(&status);
return WEXITSTATUS(status);
}
for (;;) {
assert(pid == wait(&status));
if (WIFEXITED(status))
break;
assert(ptrace(PTRACE_SINGLESTEP, pid, 0,0) == 0);
}
assert(WEXITSTATUS(status) == 43);
return 0;
}
Tested on x86_64, hopefully the change in process_32.c is right too.
Signed-off-by: Oleg Nesterov <[email protected]>
---
arch/x86/kernel/process_32.c | 3 +++
arch/x86/kernel/process_64.c | 3 +++
2 files changed, 6 insertions(+)
--- V1/arch/x86/kernel/process_32.c~FORK_CLEAR_TIF_SINGLESTEP 2009-09-15 08:45:40.000000000 +0200
+++ V1/arch/x86/kernel/process_32.c 2009-11-06 21:51:57.000000000 +0100
@@ -252,6 +252,8 @@ int copy_thread(unsigned long clone_flag
childregs->ax = 0;
childregs->sp = sp;
+ childregs->flags &= ~X86_EFLAGS_TF;
+
p->thread.sp = (unsigned long) childregs;
p->thread.sp0 = (unsigned long) (childregs+1);
@@ -287,6 +289,7 @@ int copy_thread(unsigned long clone_flag
clear_tsk_thread_flag(p, TIF_DS_AREA_MSR);
p->thread.ds_ctx = NULL;
+ clear_tsk_thread_flag(p, TIF_SINGLESTEP);
clear_tsk_thread_flag(p, TIF_DEBUGCTLMSR);
p->thread.debugctlmsr = 0;
--- V1/arch/x86/kernel/process_64.c~FORK_CLEAR_TIF_SINGLESTEP 2009-09-15 08:45:40.000000000 +0200
+++ V1/arch/x86/kernel/process_64.c 2009-11-06 21:45:16.000000000 +0100
@@ -289,6 +289,8 @@ int copy_thread(unsigned long clone_flag
if (sp == ~0UL)
childregs->sp = (unsigned long)childregs;
+ childregs->flags &= ~X86_EFLAGS_TF;
+
p->thread.sp = (unsigned long) childregs;
p->thread.sp0 = (unsigned long) (childregs+1);
p->thread.usersp = me->thread.usersp;
@@ -332,6 +334,7 @@ int copy_thread(unsigned long clone_flag
clear_tsk_thread_flag(p, TIF_DS_AREA_MSR);
p->thread.ds_ctx = NULL;
+ clear_tsk_thread_flag(p, TIF_SINGLESTEP);
clear_tsk_thread_flag(p, TIF_DEBUGCTLMSR);
p->thread.debugctlmsr = 0;
There is also the TIF_FORCED_TF logic to check. Probably it should just
call user_disable_single_step() instead. Perhaps copy_process() should
just call that and then no per-arch changes would be required.
Thanks,
Roland
On 11/06, Roland McGrath wrote:
>
> There is also the TIF_FORCED_TF logic to check.
I thought this flag has no effect without TIF_SINGLESTEP, and it is
always updated by enable_single_step().
> Probably it should just
> call user_disable_single_step() instead. Perhaps copy_process() should
> just call that and then no per-arch changes would be required.
OK, thanks, will resend tomorrow.
user_disable_single_step() is very much arch-dependent, I am worried
if it is safe to call it from copy_process(), when the new task_struct,
thread_info, etc are not properly initialized yet...
Oleg.
> I thought this flag has no effect without TIF_SINGLESTEP, and it is
> always updated by enable_single_step().
Right, but to be anal about the semantics you shouldn't clear TF if it's
not set, etc.
> user_disable_single_step() is very much arch-dependent, I am worried
> if it is safe to call it from copy_process(), when the new task_struct,
> thread_info, etc are not properly initialized yet...
Well, arch people can weigh in about if it is problematical.
Skimming over the existing arch definitions, it looks OK to me.
Thanks,
Roland
If the tracee calls fork() after PTRACE_SINGLESTEP, the forked child starts
with TIF_SINGLESTEP/X86_EFLAGS_TF bits copied from ptraced parent. This is
not right, especially when the new child is not auto-attaced: in this case
it is killed by SIGTRAP.
Change copy_process() to call user_disable_single_step() if TIF_SINGLESTEP
is set. Tested on x86, hopefully this is correct on any architecture.
Test-case:
#include <stdio.h>
#include <unistd.h>
#include <signal.h>
#include <sys/ptrace.h>
#include <sys/wait.h>
#include <assert.h>
int main(void)
{
int pid, status;
if (!(pid = fork())) {
assert(ptrace(PTRACE_TRACEME) == 0);
kill(getpid(), SIGSTOP);
if (!fork()) {
/* kernel bug: this child will be killed by SIGTRAP */
printf("Hello world\n");
return 43;
}
wait(&status);
return WEXITSTATUS(status);
}
for (;;) {
assert(pid == wait(&status));
if (WIFEXITED(status))
break;
assert(ptrace(PTRACE_SINGLESTEP, pid, 0,0) == 0);
}
assert(WEXITSTATUS(status) == 43);
return 0;
}
Signed-off-by: Oleg Nesterov <[email protected]>
---
kernel/fork.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
--- V1/kernel/fork.c~FORK_DISABLE_STEP 2009-10-09 19:52:23.000000000 +0200
+++ V1/kernel/fork.c 2009-11-07 22:15:15.000000000 +0100
@@ -1199,10 +1199,14 @@ static struct task_struct *copy_process(
p->sas_ss_sp = p->sas_ss_size = 0;
/*
- * Syscall tracing should be turned off in the child regardless
- * of CLONE_PTRACE.
+ * Syscall tracing and stepping hould be turned off in the
+ * child regardless of CLONE_PTRACE.
*/
clear_tsk_thread_flag(p, TIF_SYSCALL_TRACE);
+#ifdef TIF_SINGLESTEP
+ if (unlikely(test_tsk_thread_flag(p, TIF_SINGLESTEP)))
+ user_disable_single_step(p);
+#endif
#ifdef TIF_SYSCALL_EMU
clear_tsk_thread_flag(p, TIF_SYSCALL_EMU);
#endif
That is inappropriate use of arch details in generic code. It might
happen to be harmless fritter in practice on the arch's we have but it
is certainly not the correct way to go about things. You should just
call user_disable_single_step() unconditionally. Even on an arch with
no such machinery at all that should be defined safely as a no-op (see
linux/ptrace.h). If there is some reason not to do that, please
explain it.
Thanks,
Roland
On 11/08, Roland McGrath wrote:
>
> That is inappropriate use of arch details in generic code. It might
> happen to be harmless fritter in practice on the arch's we have but it
> is certainly not the correct way to go about things. You should just
> call user_disable_single_step() unconditionally. Even on an arch with
> no such machinery at all that should be defined safely as a no-op (see
> linux/ptrace.h). If there is some reason not to do that, please
> explain it.
Yes, we have arch_has_single_step.
I added test_tsk_thread_flag(TIF_SINGLESTEP) check for 2 reasons: to
optimize out user_disable_single_step() in the likely case, and because
I wasn't sure it is safe to call user_disable_single_step() unconditionally.
OK, will resend, thanks.
Oleg.
If the tracee calls fork() after PTRACE_SINGLESTEP, the forked child starts
with TIF_SINGLESTEP/X86_EFLAGS_TF bits copied from ptraced parent. This is
not right, especially when the new child is not auto-attaced: in this case
it is killed by SIGTRAP.
Change copy_process() to call user_disable_single_step(). Tested on x86.
Test-case:
#include <stdio.h>
#include <unistd.h>
#include <signal.h>
#include <sys/ptrace.h>
#include <sys/wait.h>
#include <assert.h>
int main(void)
{
int pid, status;
if (!(pid = fork())) {
assert(ptrace(PTRACE_TRACEME) == 0);
kill(getpid(), SIGSTOP);
if (!fork()) {
/* kernel bug: this child will be killed by SIGTRAP */
printf("Hello world\n");
return 43;
}
wait(&status);
return WEXITSTATUS(status);
}
for (;;) {
assert(pid == wait(&status));
if (WIFEXITED(status))
break;
assert(ptrace(PTRACE_SINGLESTEP, pid, 0,0) == 0);
}
assert(WEXITSTATUS(status) == 43);
return 0;
}
Signed-off-by: Oleg Nesterov <[email protected]>
---
kernel/fork.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
--- V1/kernel/fork.c~FORK_DISABLE_STEP 2009-10-09 19:52:23.000000000 +0200
+++ V1/kernel/fork.c 2009-11-10 00:45:12.000000000 +0100
@@ -1199,9 +1199,10 @@ static struct task_struct *copy_process(
p->sas_ss_sp = p->sas_ss_size = 0;
/*
- * Syscall tracing should be turned off in the child regardless
- * of CLONE_PTRACE.
+ * Syscall tracing and stepping should be turned off in the
+ * child regardless of CLONE_PTRACE.
*/
+ user_disable_single_step(p);
clear_tsk_thread_flag(p, TIF_SYSCALL_TRACE);
#ifdef TIF_SYSCALL_EMU
clear_tsk_thread_flag(p, TIF_SYSCALL_EMU);
Acked-by: Roland McGrath <[email protected]>