[PATCH 0/5] kill ptrace_{get,put}_breakpoints()

[PATCH 0/5] kill ptrace_{get,put}_breakpoints()

[Oleg Nesterov]

Hello.

Kill ptrace_{get,put}_breakpoints and task_struct->ptrace_bp_refcnt,
9899d11f "ptrace: ensure arch_ptrace/ptrace_request can never race
with SIGKILL" made this all unneeded.

Benjamin, Paul, arch_dup_task_struct()->flush_ptrace_hw_breakpoint(src)
on powerpc looks "obviously wrong". Don't we need

	- flush_ptrace_hw_breakpoint(src);
	+ dst->thread->ptrace_bps[0] = NULL;

?

Oleg.

[PATCH 1/5] ptrace/x86: Revert "hw_breakpoints: Fix racy access to ptrace breakpoints"

[Oleg Nesterov]

This reverts commit 87dc669ba25777b67796d7262c569429e58b1ed4.

The patch was fine but we can no longer race with SIGKILL after
9899d11f "ptrace: ensure arch_ptrace/ptrace_request can never race
with SIGKILL", the __TASK_TRACED tracee can't be woken up and
->ptrace_bps[] can't go away.

The patch only removes ptrace_get_breakpoints/ptrace_put_breakpoints
and does a couple of "while at it" cleanups, it doesn't remove other
changes from the reverted commit.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/ptrace.c |   28 +++++-----------------------
 1 files changed, 5 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 29a8120..7a98b21 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -641,9 +641,6 @@ 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:
@@ -692,9 +689,7 @@ restore:
 		goto restore;
 	}
 
-	ptrace_put_breakpoints(tsk);
-
-	return ((orig_ret < 0) ? orig_ret : rc);
+	return orig_ret < 0 ? orig_ret : rc;
 }
 
 /*
@@ -706,18 +701,10 @@ static unsigned long ptrace_get_debugreg(struct task_struct *tsk, int n)
 	unsigned long val = 0;
 
 	if (n < HBP_NUM) {
-		struct perf_event *bp;
+		struct perf_event *bp = thread->ptrace_bps[n];
 
-		if (ptrace_get_breakpoints(tsk) < 0)
-			return -ESRCH;
-
-		bp = thread->ptrace_bps[n];
-		if (!bp)
-			val = 0;
-		else
+		if (bp)
 			val = bp->hw.info.address;
-
-		ptrace_put_breakpoints(tsk);
 	} else if (n == 6) {
 		val = thread->debugreg6;
 	 } else if (n == 7) {
@@ -734,9 +721,6 @@ static int ptrace_set_breakpoint_addr(struct task_struct *tsk, int nr,
 	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);
 		/*
@@ -762,7 +746,7 @@ static int ptrace_set_breakpoint_addr(struct task_struct *tsk, int nr,
 		 */
 		if (IS_ERR(bp)) {
 			err = PTR_ERR(bp);
-			goto put;
+			goto out;
 		}
 
 		t->ptrace_bps[nr] = bp;
@@ -773,9 +757,7 @@ static int ptrace_set_breakpoint_addr(struct task_struct *tsk, int nr,
 		attr.bp_addr = addr;
 		err = modify_user_hw_breakpoint(bp, &attr);
 	}
-
-put:
-	ptrace_put_breakpoints(tsk);
+out:
 	return err;
 }
 
-- 
1.5.5.1

Re: [PATCH 1/5] ptrace/x86: Revert "hw_breakpoints: Fix racy access to ptrace breakpoints"

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> This reverts commit 87dc669ba25777b67796d7262c569429e58b1ed4.
> 
> The patch was fine but we can no longer race with SIGKILL after
> 9899d11f "ptrace: ensure arch_ptrace/ptrace_request can never race
> with SIGKILL", the __TASK_TRACED tracee can't be woken up and
> ->ptrace_bps[] can't go away.
> 
> The patch only removes ptrace_get_breakpoints/ptrace_put_breakpoints
> and does a couple of "while at it" cleanups, it doesn't remove other
> changes from the reverted commit.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  arch/x86/kernel/ptrace.c |   28 +++++-----------------------
>  1 files changed, 5 insertions(+), 23 deletions(-)

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH 1/5] ptrace/x86: Revert "hw_breakpoints: Fix racy access to ptrace breakpoints"

[Frederic Weisbecker]

On Sun, Apr 14, 2013 at 06:05:26PM +0200, Oleg Nesterov wrote:
> This reverts commit 87dc669ba25777b67796d7262c569429e58b1ed4.
> 
> The patch was fine but we can no longer race with SIGKILL after
> 9899d11f "ptrace: ensure arch_ptrace/ptrace_request can never race
> with SIGKILL", the __TASK_TRACED tracee can't be woken up and
> ->ptrace_bps[] can't go away.
> 
> The patch only removes ptrace_get_breakpoints/ptrace_put_breakpoints
> and does a couple of "while at it" cleanups, it doesn't remove other
> changes from the reverted commit.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

Thanks a lot for removing that horrid cruft!

[PATCH 2/5] ptrace/powerpc: Revert "hw_breakpoints: Fix racy access to ptrace breakpoints"

[Oleg Nesterov]

This reverts commit 07fa7a0a8a586c01a8b416358c7012dcb9dc688d and
removes ptrace_get/put_breakpoints() added by other commits.

The patch was fine but we can no longer race with SIGKILL after
9899d11f "ptrace: ensure arch_ptrace/ptrace_request can never race
with SIGKILL", the __TASK_TRACED tracee can't be woken up and
->ptrace_bps[] can't go away.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
---
 arch/powerpc/kernel/ptrace.c |   20 --------------------
 1 files changed, 0 insertions(+), 20 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index f9b30c6..d278e43 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -969,16 +969,12 @@ int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 	hw_brk.type = (data & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
 	hw_brk.len = 8;
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
-	if (ptrace_get_breakpoints(task) < 0)
-		return -ESRCH;
-
 	bp = thread->ptrace_bps[0];
 	if ((!data) || !(hw_brk.type & HW_BRK_TYPE_RDWR)) {
 		if (bp) {
 			unregister_hw_breakpoint(bp);
 			thread->ptrace_bps[0] = NULL;
 		}
-		ptrace_put_breakpoints(task);
 		return 0;
 	}
 	if (bp) {
@@ -991,11 +987,9 @@ int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 
 		ret =  modify_user_hw_breakpoint(bp, &attr);
 		if (ret) {
-			ptrace_put_breakpoints(task);
 			return ret;
 		}
 		thread->ptrace_bps[0] = bp;
-		ptrace_put_breakpoints(task);
 		thread->hw_brk = hw_brk;
 		return 0;
 	}
@@ -1010,12 +1004,9 @@ int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 					       ptrace_triggered, NULL, task);
 	if (IS_ERR(bp)) {
 		thread->ptrace_bps[0] = NULL;
-		ptrace_put_breakpoints(task);
 		return PTR_ERR(bp);
 	}
 
-	ptrace_put_breakpoints(task);
-
 #endif /* CONFIG_HAVE_HW_BREAKPOINT */
 	task->thread.hw_brk = hw_brk;
 #else /* CONFIG_PPC_ADV_DEBUG_REGS */
@@ -1434,9 +1425,6 @@ static long ppc_set_hwdebug(struct task_struct *child,
 	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
 		brk.type |= HW_BRK_TYPE_WRITE;
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
-	if (ptrace_get_breakpoints(child) < 0)
-		return -ESRCH;
-
 	/*
 	 * Check if the request is for 'range' breakpoints. We can
 	 * support it if range < 8 bytes.
@@ -1444,12 +1432,10 @@ static long ppc_set_hwdebug(struct task_struct *child,
 	if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE) {
 		len = bp_info->addr2 - bp_info->addr;
 	} else if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT) {
-		ptrace_put_breakpoints(child);
 		return -EINVAL;
 	}
 	bp = thread->ptrace_bps[0];
 	if (bp) {
-		ptrace_put_breakpoints(child);
 		return -ENOSPC;
 	}
 
@@ -1463,11 +1449,9 @@ static long ppc_set_hwdebug(struct task_struct *child,
 					       ptrace_triggered, NULL, child);
 	if (IS_ERR(bp)) {
 		thread->ptrace_bps[0] = NULL;
-		ptrace_put_breakpoints(child);
 		return PTR_ERR(bp);
 	}
 
-	ptrace_put_breakpoints(child);
 	return 1;
 #endif /* CONFIG_HAVE_HW_BREAKPOINT */
 
@@ -1511,16 +1495,12 @@ static long ppc_del_hwdebug(struct task_struct *child, long data)
 		return -EINVAL;
 
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
-	if (ptrace_get_breakpoints(child) < 0)
-		return -ESRCH;
-
 	bp = thread->ptrace_bps[0];
 	if (bp) {
 		unregister_hw_breakpoint(bp);
 		thread->ptrace_bps[0] = NULL;
 	} else
 		ret = -ENOENT;
-	ptrace_put_breakpoints(child);
 	return ret;
 #else /* CONFIG_HAVE_HW_BREAKPOINT */
 	if (child->thread.hw_brk.address == 0)
-- 
1.5.5.1

[PATCH 3/5] ptrace/arm: Revert "hw_breakpoints: Fix racy access to ptrace breakpoints"

[Oleg Nesterov]

This reverts commit bf0b8f4b55e591ba417c2dbaff42769e1fc773b0.

The patch was fine but we can no longer race with SIGKILL after
9899d11f "ptrace: ensure arch_ptrace/ptrace_request can never race
with SIGKILL", the __TASK_TRACED tracee can't be woken up and
->ptrace_bps[] can't go away.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/ptrace.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 03deeff..41668e5 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -886,20 +886,12 @@ 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
 
-- 
1.5.5.1

Re: [PATCH 3/5] ptrace/arm: Revert "hw_breakpoints: Fix racy access to ptrace breakpoints"

[Will Deacon]

On Sun, Apr 14, 2013 at 05:05:34PM +0100, Oleg Nesterov wrote:
> This reverts commit bf0b8f4b55e591ba417c2dbaff42769e1fc773b0.
> 
> The patch was fine but we can no longer race with SIGKILL after
> 9899d11f "ptrace: ensure arch_ptrace/ptrace_request can never race
> with SIGKILL", the __TASK_TRACED tracee can't be woken up and
> ->ptrace_bps[] can't go away.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/kernel/ptrace.c |    8 --------
>  1 files changed, 0 insertions(+), 8 deletions(-)

Looks fine to me. Seems as though I missed arch/arm64/, so nothing to change
there.

  Acked-by: Will Deacon <will.deacon@arm.com>

Will

[PATCH 4/5] ptrace/sh: Revert "hw_breakpoints: Fix racy access to ptrace breakpoints"

[Oleg Nesterov]

This reverts commit e0ac8457d020c0289ea566917267da9e5e6d9865.

The patch was fine but we can no longer race with SIGKILL after
9899d11f "ptrace: ensure arch_ptrace/ptrace_request can never race
with SIGKILL", the __TASK_TRACED tracee can't be woken up and
->ptrace_bps[] can't go away.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Cc: Paul Mundt <lethal@linux-sh.org>
---
 arch/sh/kernel/ptrace_32.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c
index 81f999a..668c816 100644
--- a/arch/sh/kernel/ptrace_32.c
+++ b/arch/sh/kernel/ptrace_32.c
@@ -117,11 +117,7 @@ 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)
-- 
1.5.5.1

[PATCH 5/5] ptrace: Revert "Prepare to fix racy accesses on task breakpoints"

[Oleg Nesterov]

This reverts commit bf26c018490c2fce7fe9b629083b96ce0e6ad019.

The patch was fine but we can no longer race with SIGKILL after
9899d11f "ptrace: ensure arch_ptrace/ptrace_request can never race
with SIGKILL", the __TASK_TRACED tracee can't be woken up and
->ptrace_bps[] can't go away.

Now that ptrace_get_breakpoints/ptrace_put_breakpoints have no
callers, we can kill them and remove task->ptrace_bp_refcnt.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/ptrace.h |   10 ----------
 include/linux/sched.h  |    3 ---
 kernel/exit.c          |    2 +-
 kernel/ptrace.c        |   16 ----------------
 4 files changed, 1 insertions(+), 30 deletions(-)

diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 89573a3..07d0df6 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -142,9 +142,6 @@ static inline void ptrace_init_task(struct task_struct *child, bool ptrace)
 {
 	INIT_LIST_HEAD(&child->ptrace_entry);
 	INIT_LIST_HEAD(&child->ptraced);
-#ifdef CONFIG_HAVE_HW_BREAKPOINT
-	atomic_set(&child->ptrace_bp_refcnt, 1);
-#endif
 	child->jobctl = 0;
 	child->ptrace = 0;
 	child->parent = child->real_parent;
@@ -351,11 +348,4 @@ extern int task_current_syscall(struct task_struct *target, long *callno,
 				unsigned long args[6], unsigned int maxargs,
 				unsigned long *sp, unsigned long *pc);
 
-#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
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d35d2b6..89dc3e4 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1570,9 +1570,6 @@ struct task_struct {
 	} memcg_batch;
 	unsigned int memcg_kmem_skip_account;
 #endif
-#ifdef CONFIG_HAVE_HW_BREAKPOINT
-	atomic_t ptrace_bp_refcnt;
-#endif
 #ifdef CONFIG_UPROBES
 	struct uprobe_task *utask;
 #endif
diff --git a/kernel/exit.c b/kernel/exit.c
index 60bc027..0a66f6d 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -819,7 +819,7 @@ void do_exit(long code)
 	/*
 	 * FIXME: do that only when needed, using sched_exit tracepoint
 	 */
-	ptrace_put_breakpoints(tsk);
+	flush_ptrace_hw_breakpoint(tsk);
 
 	exit_notify(tsk, group_dead);
 #ifdef CONFIG_NUMA
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index acbd284..776ab3b 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -1098,19 +1098,3 @@ 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.5.5.1

Re: [PATCH 5/5] ptrace: Revert "Prepare to fix racy accesses on task breakpoints"

[Frederic Weisbecker]

On Sun, Apr 14, 2013 at 06:05:41PM +0200, Oleg Nesterov wrote:
> This reverts commit bf26c018490c2fce7fe9b629083b96ce0e6ad019.
> 
> The patch was fine but we can no longer race with SIGKILL after
> 9899d11f "ptrace: ensure arch_ptrace/ptrace_request can never race
> with SIGKILL", the __TASK_TRACED tracee can't be woken up and
> ->ptrace_bps[] can't go away.
> 
> Now that ptrace_get_breakpoints/ptrace_put_breakpoints have no
> callers, we can kill them and remove task->ptrace_bp_refcnt.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

Thanks a lot!

Re: [PATCH 0/5] kill ptrace_{get,put}_breakpoints()

[Michael Neuling]

Oleg,

> Kill ptrace_{get,put}_breakpoints and task_struct->ptrace_bp_refcnt,
> 9899d11f "ptrace: ensure arch_ptrace/ptrace_request can never race
> with SIGKILL" made this all unneeded.
> 
> Benjamin, Paul, arch_dup_task_struct()->flush_ptrace_hw_breakpoint(src)
> on powerpc looks "obviously wrong". Don't we need
> 
> 	- flush_ptrace_hw_breakpoint(src);
> 	+ dst->thread->ptrace_bps[0] = NULL;

Do you mean the following?


diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 59dd545..559804e 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -911,7 +911,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct tas
        flush_vsx_to_thread(src);
        flush_spe_to_thread(src);
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
-       flush_ptrace_hw_breakpoint(src);
+       dst->thread.ptrace_bps[0] = NULL;
 #endif /* CONFIG_HAVE_HW_BREAKPOINT */
 
        *dst = *src;



If I add that, I can crash the kernel by forking a process with a
hw_breakpoint attached:


Unable to handle kernel paging request for data at address 0x00100108
Faulting instruction address: 0xc00000000014d5e4
cpu 0x0: Vector: 300 (Data Access) at [c00000007e5836a0]
    pc: c00000000014d5e4: .toggle_bp_slot+0x74/0x1c0
    lr: c00000000014dc14: .release_bp_slot+0x44/0x70
    sp: c00000007e583920
   msr: 9000000000009032
   dar: 100108
 dsisr: 42000000
  current = 0xc00000007e560000
  paca    = 0xc00000000fe00000	 softe: 0	 irq_happened: 0x08
    pid   = 1, comm = init
enter ? for help
[c00000007e5839d0] c00000000014dc14 .release_bp_slot+0x44/0x70
[c00000007e583a50] c000000000144bbc .free_event+0x6c/0x1e0
[c00000007e583ad0] c000000000144dc4 .perf_event_release_kernel+0x94/0x110
[c00000007e583b60] c00000000014cf08 .unregister_hw_breakpoint+0x18/0x30
[c00000007e583bd0] c00000000000e5f8 .ptrace_set_debugreg+0x158/0x230
[c00000007e583cd0] c00000000000eb4c .arch_ptrace+0x43c/0x7b0
[c00000007e583d90] c00000000008cbf8 .SyS_ptrace+0x98/0x170
[c00000007e583e30] c000000000009d54 syscall_exit+0x0/0x98
--- Exception: c01 (System Call) at 000000001001d1d4
SP (3fffdf7459f0) is in userspace

The crash seems to happen some time after the fork.  Might be when the
new processes exits or get another ptrace call on it (I'm not sure which
one sorry).

Without your suggestion it doesn't crash this case (ie. mainline passes).

As for the rest of your series, it passes my tests on powerpc, so I'm
good with it.  

Acked-by: Michael Neuling <mikey@neuling.org>

Mikey

Re: [PATCH 0/5] kill ptrace_{get,put}_breakpoints()

[Oleg Nesterov]

On 04/16, Michael Neuling wrote:
>
> > Benjamin, Paul, arch_dup_task_struct()->flush_ptrace_hw_breakpoint(src)
> > on powerpc looks "obviously wrong". Don't we need
> >
> > 	- flush_ptrace_hw_breakpoint(src);
> > 	+ dst->thread->ptrace_bps[0] = NULL;
>
> Do you mean the following?
>
>
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 59dd545..559804e 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -911,7 +911,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct tas
>         flush_vsx_to_thread(src);
>         flush_spe_to_thread(src);
>  #ifdef CONFIG_HAVE_HW_BREAKPOINT
> -       flush_ptrace_hw_breakpoint(src);
> +       dst->thread.ptrace_bps[0] = NULL;
>  #endif /* CONFIG_HAVE_HW_BREAKPOINT */

Almost.

This is what I think we should do, but it is pointless to do this
in arch_dup_task_struct(), setup_thread_stack() will copy ptrace_bps[]
from parent later.

> Unable to handle kernel paging request for data at address 0x00100108
> Faulting instruction address: 0xc00000000014d5e4
> cpu 0x0: Vector: 300 (Data Access) at [c00000007e5836a0]
>     pc: c00000000014d5e4: .toggle_bp_slot+0x74/0x1c0
>     lr: c00000000014dc14: .release_bp_slot+0x44/0x70
>     sp: c00000007e583920
>    msr: 9000000000009032
>    dar: 100108
>  dsisr: 42000000
>   current = 0xc00000007e560000
>   paca    = 0xc00000000fe00000	 softe: 0	 irq_happened: 0x08
>     pid   = 1, comm = init
> enter ? for help
> [c00000007e5839d0] c00000000014dc14 .release_bp_slot+0x44/0x70
> [c00000007e583a50] c000000000144bbc .free_event+0x6c/0x1e0
> [c00000007e583ad0] c000000000144dc4 .perf_event_release_kernel+0x94/0x110
> [c00000007e583b60] c00000000014cf08 .unregister_hw_breakpoint+0x18/0x30
> [c00000007e583bd0] c00000000000e5f8 .ptrace_set_debugreg+0x158/0x230
> [c00000007e583cd0] c00000000000eb4c .arch_ptrace+0x43c/0x7b0
> [c00000007e583d90] c00000000008cbf8 .SyS_ptrace+0x98/0x170
> [c00000007e583e30] c000000000009d54 syscall_exit+0x0/0x98
> --- Exception: c01 (System Call) at 000000001001d1d4
> SP (3fffdf7459f0) is in userspace
>
> The crash seems to happen some time after the fork.  Might be when the
> new processes exits or get another ptrace call on it (I'm not sure which
> one sorry).

Yes, probably because both parent and child have the same ->ptrace_bps[]
pointers.

> Without your suggestion it doesn't crash this case (ie. mainline passes).

This is clear. flush_ptrace_hw_breakpoint() nullifies ->ptrace_bps[], so
setup_thread_stack() copies NULL.

But, unless I missed something, this is wrong. Why should the parent lose
its bps after fork?

IOW, I think we need something like the patch below, but I do not have
a powerpc machine for the testing.

> Acked-by: Michael Neuling <mikey@neuling.org>

Thanks!

Oleg.

[PATCH] ptrace/powerpc: dont flush_ptrace_hw_breakpoint() on fork()

arch_dup_task_struct() does flush_ptrace_hw_breakpoint(src), this
is not what we want. We should clear child->thread.ptrace_bps[]
copied by dup_task_struct().

--- x/arch/powerpc/kernel/process.c
+++ x/arch/powerpc/kernel/process.c
@@ -910,10 +910,6 @@ int arch_dup_task_struct(struct task_str
 	flush_altivec_to_thread(src);
 	flush_vsx_to_thread(src);
 	flush_spe_to_thread(src);
-#ifdef CONFIG_HAVE_HW_BREAKPOINT
-	flush_ptrace_hw_breakpoint(src);
-#endif /* CONFIG_HAVE_HW_BREAKPOINT */
-
 	*dst = *src;
 	return 0;
 }
@@ -984,6 +980,10 @@ int copy_thread(unsigned long clone_flag
 	p->thread.ksp_limit = (unsigned long)task_stack_page(p) +
 				_ALIGN_UP(sizeof(struct thread_info), 16);
 
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	p->thread.ptrace_bps[0] = NULL;
+#endif
+
 #ifdef CONFIG_PPC_STD_MMU_64
 	if (mmu_has_feature(MMU_FTR_SLB)) {
 		unsigned long sp_vsid;

Re: [PATCH 0/5] kill ptrace_{get,put}_breakpoints()

[Michael Neuling]

Oleg Nesterov <oleg@redhat.com> wrote:

> On 04/16, Michael Neuling wrote:
> >
> > > Benjamin, Paul, arch_dup_task_struct()->flush_ptrace_hw_breakpoint(src)
> > > on powerpc looks "obviously wrong". Don't we need
> > >
> > > 	- flush_ptrace_hw_breakpoint(src);
> > > 	+ dst->thread->ptrace_bps[0] = NULL;
> >
> > Do you mean the following?
> >
> >
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index 59dd545..559804e 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -911,7 +911,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct tas
> >         flush_vsx_to_thread(src);
> >         flush_spe_to_thread(src);
> >  #ifdef CONFIG_HAVE_HW_BREAKPOINT
> > -       flush_ptrace_hw_breakpoint(src);
> > +       dst->thread.ptrace_bps[0] = NULL;
> >  #endif /* CONFIG_HAVE_HW_BREAKPOINT */
> 
> Almost.
> 
> This is what I think we should do, but it is pointless to do this
> in arch_dup_task_struct(), setup_thread_stack() will copy ptrace_bps[]
> from parent later.
> 
> > Unable to handle kernel paging request for data at address 0x00100108
> > Faulting instruction address: 0xc00000000014d5e4
> > cpu 0x0: Vector: 300 (Data Access) at [c00000007e5836a0]
> >     pc: c00000000014d5e4: .toggle_bp_slot+0x74/0x1c0
> >     lr: c00000000014dc14: .release_bp_slot+0x44/0x70
> >     sp: c00000007e583920
> >    msr: 9000000000009032
> >    dar: 100108
> >  dsisr: 42000000
> >   current = 0xc00000007e560000
> >   paca    = 0xc00000000fe00000	 softe: 0	 irq_happened: 0x08
> >     pid   = 1, comm = init
> > enter ? for help
> > [c00000007e5839d0] c00000000014dc14 .release_bp_slot+0x44/0x70
> > [c00000007e583a50] c000000000144bbc .free_event+0x6c/0x1e0
> > [c00000007e583ad0] c000000000144dc4 .perf_event_release_kernel+0x94/0x110
> > [c00000007e583b60] c00000000014cf08 .unregister_hw_breakpoint+0x18/0x30
> > [c00000007e583bd0] c00000000000e5f8 .ptrace_set_debugreg+0x158/0x230
> > [c00000007e583cd0] c00000000000eb4c .arch_ptrace+0x43c/0x7b0
> > [c00000007e583d90] c00000000008cbf8 .SyS_ptrace+0x98/0x170
> > [c00000007e583e30] c000000000009d54 syscall_exit+0x0/0x98
> > --- Exception: c01 (System Call) at 000000001001d1d4
> > SP (3fffdf7459f0) is in userspace
> >
> > The crash seems to happen some time after the fork.  Might be when the
> > new processes exits or get another ptrace call on it (I'm not sure which
> > one sorry).
> 
> Yes, probably because both parent and child have the same ->ptrace_bps[]
> pointers.
> 
> > Without your suggestion it doesn't crash this case (ie. mainline passes).
> 
> This is clear. flush_ptrace_hw_breakpoint() nullifies ->ptrace_bps[], so
> setup_thread_stack() copies NULL.
> 
> But, unless I missed something, this is wrong. Why should the parent lose
> its bps after fork?

Agreed, it shouldn't lose it.

> IOW, I think we need something like the patch below, but I do not have
> a powerpc machine for the testing.

OK, the below works for me... no more crashing.  FWIW

Acked-by: Michael Neuling <mikey@neuling.org>

Thanks,
Mikey

> 
> > Acked-by: Michael Neuling <mikey@neuling.org>
> 
> Thanks!
> 
> Oleg.
> 
> [PATCH] ptrace/powerpc: dont flush_ptrace_hw_breakpoint() on fork()
> 
> arch_dup_task_struct() does flush_ptrace_hw_breakpoint(src), this
> is not what we want. We should clear child->thread.ptrace_bps[]
> copied by dup_task_struct().
> 
> --- x/arch/powerpc/kernel/process.c
> +++ x/arch/powerpc/kernel/process.c
> @@ -910,10 +910,6 @@ int arch_dup_task_struct(struct task_str
>  	flush_altivec_to_thread(src);
>  	flush_vsx_to_thread(src);
>  	flush_spe_to_thread(src);
> -#ifdef CONFIG_HAVE_HW_BREAKPOINT
> -	flush_ptrace_hw_breakpoint(src);
> -#endif /* CONFIG_HAVE_HW_BREAKPOINT */
> -
>  	*dst = *src;
>  	return 0;
>  }
> @@ -984,6 +980,10 @@ int copy_thread(unsigned long clone_flag
>  	p->thread.ksp_limit = (unsigned long)task_stack_page(p) +
>  				_ALIGN_UP(sizeof(struct thread_info), 16);
>  
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +	p->thread.ptrace_bps[0] = NULL;
> +#endif
> +
>  #ifdef CONFIG_PPC_STD_MMU_64
>  	if (mmu_has_feature(MMU_FTR_SLB)) {
>  		unsigned long sp_vsid;
>