hw_breakpoint fixes
Subsystems (6)
Ingo,
Please pull the perf/urgent branch that can be found at:
git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
perf/urgent
I couldn't get Oleg's review/ack for the generic ptrace part but I think he was fine with the general solution when it was a simple draft. Few things have changed since then, I essentially addressed his review.
Thanks,
Frederic
---
Frederic Weisbecker (5):
ptrace: Prepare to fix racy accesses on task breakpoints x86, hw_breakpoints: Fix racy access to ptrace breakpoints powerpc, hw_breakpoints: Fix racy access to ptrace breakpoints arm, hw_breakpoints: Fix racy access to ptrace breakpoints sh, hw_breakpoints: Fix racy access to ptrace breakpoints
arch/arm/kernel/ptrace.c | 8 ++++++++ arch/powerpc/kernel/ptrace.c | 3 +++ arch/sh/kernel/ptrace_32.c | 4 ++++ arch/x86/kernel/ptrace.c | 36 ++++++++++++++++++++++++++---------- include/linux/ptrace.h | 13 ++++++++++++- include/linux/sched.h | 3 +++ kernel/exit.c | 2 +- kernel/ptrace.c | 17 +++++++++++++++++ 8 files changed, 74 insertions(+), 12 deletions(-)
PATCH 1/5 ptrace: Prepare to fix racy accesses on task breakpoints
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.
--- 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(-)
include/linux/ptrace.h +12 -1
@@ -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
include/linux/sched.h +3 -0
@@ -1537,6 +1537,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. */
kernel/exit.c +1 -1
@@ -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
kernel/ptrace.c +17 -0
@@ -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 */
Signature · 1 line
1.7.3.2
Re: [PATCH 1/5] ptrace: Prepare to fix racy accesses on task breakpoints
(Linus and Andrew Cc:-ed as well)
Quoted 8 lines · Frederic Weisbecker
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.
Quoted 8 lines
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 <[email protected]>Signed-off-by Frederic Weisbecker <[email protected]>
Ok, this series looks a bit scary - and this ptrace.h change does not have Oleg's Acked-by. (the arch bits all have maintaner Acked-by's)
The changes look a bit ugly as well: beyond the ugly ifdeffery, we have ptrace.h::ptrace_init_task(), which is only used in tracehook.h::tracehook_finish_clone() which is only used in kernel/fork.c::copy_process().
That's two levels of obfuscation to do something rather simple - i think we should get rid of the tracehook.h redirections, it did not work out in the end as a method of capturing events - ftrace TRACE_EVENT() seems better structured and more maintainable.
But i guess we could live with this fix for v2.6.39, if neither Oleg nor Linus and Andrew are hating this further complication of the ptrace mess enough to NAK it. Thoughts?
Plus, i'd really love it if you did some stress-testing of this change of a mixed ptrace breakpoints and perf breakpoints workload, on some sufficiently SMP box. gdb's hbreak is a very low freq way of testing thus such regressions take ages to be reported.
Thanks,
Ingo
Re: [PATCH 1/5] ptrace: Prepare to fix racy accesses on task breakpoints
Quoted 35 lines · Ingo Molnar
(Linus and Andrew Cc:-ed as well)
Quoted 8 lines · Frederic Weisbecker
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.Quoted 8 lines
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 <[email protected]>Signed-off-by Frederic Weisbecker <[email protected]>Ok, this series looks a bit scary - and this
ptrace.hchange does not have Oleg's Acked-by. (the arch bits all have maintaner Acked-by's)The changes look a bit ugly as well: beyond the ugly ifdeffery, we have
ptrace.h::ptrace_init_task(), which is only used intracehook.h::tracehook_finish_clone()which is only used in kernel/fork.c::copy_process().That's two levels of obfuscation to do something rather simple - i think we should get rid of the
tracehook.hredirections, it did not work out in the end as a method of capturing events - ftraceTRACE_EVENT()seems better structured and more maintainable.
Quoted 8 lines
But i guess we could live with this fix for v2.6.39, if neither Oleg nor Linus and Andrew are hating this further complication of the ptrace mess enough to NAK it. Thoughts?
Plus, i'd really love it if you did some stress-testing of this change of a mixed ptrace breakpoints and perf breakpoints workload, on some sufficiently SMP box. gdb's hbreak is a very low freq way of testing thus such regressions take ages to be reported.
But the perf breakpoints (those created using perf syscall) are not touched at all by this patch. Only the ptrace ones.
What I can stress test is trying some ptrace breakpoint request and at the same time SIGKILL the child, which is the only way to reproduce the bug supposed to be fixed. And run that in a loop for one night. I'll try that.
PATCH 2/5 x86, hw_breakpoints: Fix racy access to ptrace breakpoints
In reply to Frederic WeisbeckerWhile the tracer accesses ptrace breakpoints, the child task may concurrently exit due to a SIGKILL and thus release its breakpoints at the same time. We can then dereference some freed pointers.
To fix this, hold a reference on the child breakpoints before manipulating them.
--- arch/x86/kernel/ptrace.c | 36 ++++++++++++++++++++++++++---------- 1 files changed, 26 insertions(+), 10 deletions(-)
arch/x86/kernel/ptrace.c +26 -10
@@ -608,6 +608,9 @@ static int ptrace_write_dr7(struct task_struct *tsk, unsigned long data)unsigned len, type;struct perf_event *bp; +if (ptrace_get_breakpoints (tsk) <0 ) +return -ESRCH ; + data &= ~DR_CONTROL_RESERVED ; old_dr7 =ptrace_get_dr7 (thread->ptrace_bps); restore:
@@ -655,6 +658,9 @@ restore: } goto restore; } + +ptrace_put_breakpoints (tsk); +return ((orig_ret <0 ) ? orig_ret : rc); }
@@ -668,10 +674,17 @@ static unsigned long ptrace_get_debugreg(struct task_struct *tsk, int n)if (n <HBP_NUM ) {struct perf_event *bp; + +if (ptrace_get_breakpoints (tsk) <0 ) +return -ESRCH ; + bp = thread->ptrace_bps[n];if (!bp) -return 0 ; - val = bp->hw.info.address; + val =0 ; +else + val = bp->hw.info.address; + +ptrace_put_breakpoints (tsk); }else if (n ==6 ) { val = thread->debugreg6; }else if (n ==7 ) {
@@ -686,6 +699,10 @@ static int ptrace_set_breakpoint_addr(struct task_struct *tsk, int nr,struct perf_event *bp;struct thread_struct *t = &tsk->thread;struct perf_event_attr attr; +int err =0 ; + +if (ptrace_get_breakpoints (tsk) <0 ) +return -ESRCH ;if (!t->ptrace_bps[nr]) {ptrace_breakpoint_init (&attr);
@@ -709,24 +726,23 @@ static int ptrace_set_breakpoint_addr(struct task_struct *tsk, int nr,* writing for the user. And anyway this is the previous * behaviour. */ -if (IS_ERR (bp)) -return PTR_ERR (bp); +if (IS_ERR (bp)) { + err =PTR_ERR (bp); + goto put; + } t->ptrace_bps[nr] = bp; }else { -int err; - bp = t->ptrace_bps[nr]; attr = bp->attr; attr.bp_addr = addr; err =modify_user_hw_breakpoint (bp, &attr); -if (err) -return err; } - -return 0 ; +put: +ptrace_put_breakpoints (tsk); +return err; }/*
Signature · 1 line
1.7.3.2
PATCH 3/5 powerpc, hw_breakpoints: Fix racy access to ptrace breakpoints
In reply to Frederic WeisbeckerWhile the tracer accesses ptrace breakpoints, the child task may concurrently exit due to a SIGKILL and thus release its breakpoints at the same time. We can then dereference some freed pointers.
To fix this, hold a reference on the child breakpoints before manipulating them.
--- arch/powerpc/kernel/ptrace.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
arch/powerpc/kernel/ptrace.c +3 -0
@@ -1591,7 +1591,10 @@ long arch_ptrace(struct task_struct *child, long request, }case PTRACE_SET_DEBUGREG : +if (ptrace_get_breakpoints (child) <0 ) +return -ESRCH ; ret =ptrace_set_debugreg (child, addr, data); +ptrace_put_breakpoints (child);break ;#ifdef CONFIG_PPC64
Signature · 1 line
1.7.3.2
PATCH 4/5 arm, hw_breakpoints: Fix racy access to ptrace breakpoints
In reply to Frederic WeisbeckerWhile the tracer accesses ptrace breakpoints, the child task may concurrently exit due to a SIGKILL and thus release its breakpoints at the same time. We can then dereference some freed pointers.
To fix this, hold a reference on the child breakpoints before manipulating them.
--- arch/arm/kernel/ptrace.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)
arch/arm/kernel/ptrace.c +8 -0
@@ -767,12 +767,20 @@ long arch_ptrace(struct task_struct *child, long request,#ifdef CONFIG_HAVE_HW_BREAKPOINT case PTRACE_GETHBPREGS : +if (ptrace_get_breakpoints (child) <0 ) +return -ESRCH ; + ret =ptrace_gethbpregs (child, addr, (unsigned long __user *)data); +ptrace_put_breakpoints (child);break ;case PTRACE_SETHBPREGS : +if (ptrace_get_breakpoints (child) <0 ) +return -ESRCH ; + ret =ptrace_sethbpregs (child, addr, (unsigned long __user *)data); +ptrace_put_breakpoints (child);break ;#endif
Signature · 1 line
1.7.3.2
PATCH 5/5 sh, hw_breakpoints: Fix racy access to ptrace breakpoints
In reply to Frederic WeisbeckerWhile the tracer accesses ptrace breakpoints, the child task may concurrently exit due to a SIGKILL and thus release its breakpoints at the same time. We can then dereference some freed pointers.
To fix this, hold a reference on the child breakpoints before manipulating them.
--- arch/sh/kernel/ptrace_32.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
arch/sh/kernel/ptrace_32.c +4 -0
@@ -117,7 +117,11 @@ void user_enable_single_step(struct task_struct *child)set_tsk_thread_flag (child,TIF_SINGLESTEP ); +if (ptrace_get_breakpoints (child) <0 ) +return ; +set_single_step (child, pc); +ptrace_put_breakpoints (child); }void user_disable_single_step (struct task_struct *child)
Signature · 1 line
1.7.3.2