[PATCH] ptrace: copy_thread() should clear TIF_SINGLESTEP and X86_EFLAGS_TF

[PATCH] ptrace: copy_thread() should clear TIF_SINGLESTEP and X86_EFLAGS_TF

[Oleg Nesterov]

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 <oleg@redhat.com>
---

 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;
 

Re: [PATCH] ptrace: copy_thread() should clear TIF_SINGLESTEP and X86_EFLAGS_TF

[Roland McGrath]

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

Re: [PATCH] ptrace: copy_thread() should clear TIF_SINGLESTEP and X86_EFLAGS_TF

[Oleg Nesterov]

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.

Re: [PATCH] ptrace: copy_thread() should clear TIF_SINGLESTEP and X86_EFLAGS_TF

[Roland McGrath]

> 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

[PATCH] ptrace: copy_process() should disable stepping

[Oleg Nesterov]

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 <oleg@redhat.com>
---

 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

Re: [PATCH] ptrace: copy_process() should disable stepping

[Roland McGrath]

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

Re: [PATCH] ptrace: copy_process() should disable stepping

[Roland McGrath]

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

Re: [PATCH] ptrace: copy_process() should disable stepping

[Oleg Nesterov]

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.

[PATCH v2] ptrace: copy_process() should disable stepping

[Oleg Nesterov]

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 <oleg@redhat.com>
---

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

Re: [PATCH v2] ptrace: copy_process() should disable stepping

[Roland McGrath]

Acked-by: Roland McGrath <roland@redhat.com>