Re: [PATCH 6/6] ptrace: PTRACE_DETACH should do flush_ptrace_hw_breakpoint(child)

[Frederic Weisbecker <fweisbec@gmail.com>]

On Thu, Apr 18, 2013 at 08:44:25PM +0200, Oleg Nesterov wrote:
> Change ptrace_detach() to call flush_ptrace_hw_breakpoint(child).
> This frees the slots for non-ptrace PERF_TYPE_BREAKPOINT users, and
> this ensures that the tracee won't be killed by SIGTRAP triggered by
> the active breakpoints.
> 
> Test-case:
> 
> 	unsigned long encode_dr7(int drnum, int enable, unsigned int type, unsigned int len)
> 	{
> 		unsigned long dr7;
> 
> 		dr7 = ((len | type) & 0xf)
> 			<< (DR_CONTROL_SHIFT + drnum * DR_CONTROL_SIZE);
> 		if (enable)
> 			dr7 |= (DR_GLOBAL_ENABLE << (drnum * DR_ENABLE_SIZE));
> 
> 		return dr7;
> 	}
> 
> 	int write_dr(int pid, int dr, unsigned long val)
> 	{
> 		return ptrace(PTRACE_POKEUSER, pid,
> 				offsetof (struct user, u_debugreg[dr]),
> 				val);
> 	}
> 
> 	void func(void)
> 	{
> 	}
> 
> 	int main(void)
> 	{
> 		int pid, stat;
> 		unsigned long dr7;
> 
> 		pid = fork();
> 		if (!pid) {
> 			assert(ptrace(PTRACE_TRACEME, 0,0,0) == 0);
> 			kill(getpid(), SIGHUP);
> 
> 			func();
> 			return 0x13;
> 		}
> 
> 		assert(pid == waitpid(-1, &stat, 0));
> 		assert(WSTOPSIG(stat) == SIGHUP);
> 
> 		assert(write_dr(pid, 0, (long)func) == 0);
> 		dr7 = encode_dr7(0, 1, DR_RW_EXECUTE, DR_LEN_1);
> 		assert(write_dr(pid, 7, dr7) == 0);
> 
> 		assert(ptrace(PTRACE_DETACH, pid, 0,0) == 0);
> 		assert(pid == waitpid(-1, &stat, 0));
> 		assert(stat == 0x1300);
> 
> 		return 0;
> 	}
> 
> Before this patch the child is killed after PTRACE_DETACH.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/ptrace.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 776ab3b..33752d9 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -467,6 +467,7 @@ static int ptrace_detach(struct task_struct *child, unsigned int data)
>  	/* Architecture-specific hardware disable .. */
>  	ptrace_disable(child);
>  	clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
> +	flush_ptrace_hw_breakpoint(child);

So I assume the tracee is still guaranteed to be stopped at that time, right? Or already
killed? But it can't be concurrently killed given the patch you did that prevented that?

I'm just asking to make sure we can't have two concurrent calls to flush_ptrace_hw_breakpoint()
at the same time.

Also it seems to be a regression since we brought the breakpoint/perf infrastructure. But
backporting this patch prior to "ptrace: ensure arch_ptrace/ptrace_request can never race with SIGKILL"
might be racy. Hmm...

Thanks.

>  
>  	write_lock_irq(&tasklist_lock);
>  	/*
> -- 
> 1.5.5.1
>