Re: [PATCH 1/5] ptrace: Prepare to fix racy accesses on taskbreakpoints

From: Frederic Weisbecker
Date: Mon Apr 25 2011 - 13:38:09 EST


Hi Oleg.

I realize you weren't in the Cc list, which wasn't definitly not
intended.

I think you were fine with the change. But to be sure, can I have your ack?

Thanks.

On Fri, Apr 08, 2011 at 07:34:23PM +0200, Frederic Weisbecker wrote:
> When a task is traced and is in a stopped state, the tracer
> may execute a ptrace request to examine the tracee state and
> get its task struct. Right after, the tracee can be killed
> and thus its breakpoints released.
> This can happen concurrently when the tracer is in the middle
> of reading or modifying these breakpoints, leading to dereferencing
> a freed pointer.
>
> Hence, to prepare the fix, create a generic breakpoint reference
> holding API. When a reference on the breakpoints of a task is
> held, the breakpoints won't be released until the last reference
> is dropped. After that, no more ptrace request on the task's
> breakpoints can be serviced for the tracer.
>
> Reported-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> Signed-off-by: Frederic Weisbecker <fweisbec@xxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxx>
> Cc: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>
> Cc: Will Deacon <will.deacon@xxxxxxx>
> Cc: Prasad <prasad@xxxxxxxxxxxxxxxxxx>
> Cc: Paul Mundt <lethal@xxxxxxxxxxxx>
> Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
> Cc: v2.6.33.. <stable@xxxxxxxxxx>
> ---
> include/linux/ptrace.h | 13 ++++++++++++-
> include/linux/sched.h | 3 +++
> kernel/exit.c | 2 +-
> kernel/ptrace.c | 17 +++++++++++++++++
> 4 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
> index a1147e5..9178d5c 100644
> --- a/include/linux/ptrace.h
> +++ b/include/linux/ptrace.h
> @@ -189,6 +189,10 @@ static inline void ptrace_init_task(struct task_struct *child, bool ptrace)
> child->ptrace = current->ptrace;
> __ptrace_link(child, current->parent);
> }
> +
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> + atomic_set(&child->ptrace_bp_refcnt, 1);
> +#endif
> }
>
> /**
> @@ -350,6 +354,13 @@ extern int task_current_syscall(struct task_struct *target, long *callno,
> unsigned long args[6], unsigned int maxargs,
> unsigned long *sp, unsigned long *pc);
>
> -#endif
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +extern int ptrace_get_breakpoints(struct task_struct *tsk);
> +extern void ptrace_put_breakpoints(struct task_struct *tsk);
> +#else
> +static inline void ptrace_put_breakpoints(struct task_struct *tsk) { }
> +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
> +
> +#endif /* __KERNEL */
>
> #endif
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 83bd2e2..15badfa 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1534,6 +1534,9 @@ struct task_struct {
> unsigned long memsw_nr_pages; /* uncharged mem+swap usage */
> } memcg_batch;
> #endif
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> + atomic_t ptrace_bp_refcnt;
> +#endif
> };
>
> /* Future-safe accessor for struct task_struct's cpus_allowed. */
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 6a488ad..437e327 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1016,7 +1016,7 @@ NORET_TYPE void do_exit(long code)
> /*
> * FIXME: do that only when needed, using sched_exit tracepoint
> */
> - flush_ptrace_hw_breakpoint(tsk);
> + ptrace_put_breakpoints(tsk);
>
> exit_notify(tsk, group_dead);
> #ifdef CONFIG_NUMA
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 0fc1eed..dc7ab65 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -22,6 +22,7 @@
> #include <linux/syscalls.h>
> #include <linux/uaccess.h>
> #include <linux/regset.h>
> +#include <linux/hw_breakpoint.h>
>
>
> /*
> @@ -879,3 +880,19 @@ asmlinkage long compat_sys_ptrace(compat_long_t request, compat_long_t pid,
> return ret;
> }
> #endif /* CONFIG_COMPAT */
> +
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +int ptrace_get_breakpoints(struct task_struct *tsk)
> +{
> + if (atomic_inc_not_zero(&tsk->ptrace_bp_refcnt))
> + return 0;
> +
> + return -1;
> +}
> +
> +void ptrace_put_breakpoints(struct task_struct *tsk)
> +{
> + if (atomic_dec_and_test(&tsk->ptrace_bp_refcnt))
> + flush_ptrace_hw_breakpoint(tsk);
> +}
> +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
> --
> 1.7.3.2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/