diff mbox

ppc_set_hwdebug vs ptrace_set_debugreg

Message ID 20101214125427.GA2443@in.ibm.com (mailing list archive)
State Rejected
Delegated to: Benjamin Herrenschmidt
Headers show

Commit Message

K.Prasad Dec. 14, 2010, 12:54 p.m. UTC 
On Mon, Dec 13, 2010 at 08:05:36PM +0100, Andreas Schwab wrote:
> "K.Prasad" <prasad@linux.vnet.ibm.com> writes:
> 
> > +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> > +	/* Create a new breakpoint request if one doesn't exist already */
> > +	hw_breakpoint_init(&attr);
> > +	attr.bp_addr = bp_info->addr & ~HW_BREAKPOINT_ALIGN;
> > +	arch_bp_generic_fields(bp_info->addr &
> > +				(DABR_DATA_WRITE | DABR_DATA_READ),
> > +							&attr.bp_type);
> > +
> > +	bp = register_user_hw_breakpoint(&attr, ptrace_triggered, task);
> > +	if (IS_ERR(bp))
> > +		return PTR_ERR(bp);
> > +
> > +	child->thread.ptrace_bps[0] = bp;
> > +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
> > +
> >  	child->thread.dabr = (unsigned long)bp_info->addr;
> 
> That cannot work, see
> <http://permalink.gmane.org/gmane.linux.ports.ppc64.devel/71418>.
>

Ok. The above patch makes it a bit easy.

How about the revised patch below? It is only compile-tested; have you
got a quick test case that I can run?

Enable PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG to use the generic
hardware breakpoint interfaces. This helps prevent conflict for the use of
DABR register in the absence of CONFIG_PPC_ADV_DEBUG_REGS and when
PTRACE_SET_DEBUGREG/PTRACE_GET_DEBUGREG flags are used by ptrace.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/ptrace.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Andreas Schwab Dec. 14, 2010, 6:59 p.m. UTC | #1 
"K.Prasad" <prasad@linux.vnet.ibm.com> writes:

> How about the revised patch below? It is only compile-tested; have you
> got a quick test case that I can run?

Try the watchpoint tests in gdb.

Andreas.
Andreas Schwab Dec. 16, 2010, 5:07 p.m. UTC | #2 
"K.Prasad" <prasad@linux.vnet.ibm.com> writes:

> How about the revised patch below? It is only compile-tested; have you
> got a quick test case that I can run?

It crashes the kernel when running the watch-vfork test.

Andreas.
K.Prasad Jan. 2, 2011, 12:54 p.m. UTC | #3 
On Thu, Dec 16, 2010 at 06:07:47PM +0100, Andreas Schwab wrote:
> "K.Prasad" <prasad@linux.vnet.ibm.com> writes:
> 
> > How about the revised patch below? It is only compile-tested; have you
> > got a quick test case that I can run?
> 
> It crashes the kernel when running the watch-vfork test.
> 
> Andreas.
>

Hi Andreas,
	I tried running it multiple times but saw no crash (or error
messages in dmesg). Can you send me the crash logs? What's the behaviour
when the testcase is run on an unpatched kernel?

The watch-vfork test actually fails on my system (4 unexpected failures)
irrespective of the kernel containing the patch or not.

Thanks,
K.Prasad
P.S.: I'd been on vacation and couldn't look at this issue during then.
Andreas Schwab Jan. 2, 2011, 2:58 p.m. UTC | #4 
"K.Prasad" <prasad@linux.vnet.ibm.com> writes:

> The watch-vfork test actually fails on my system (4 unexpected failures)

It should pass all four tests.  If gdb cannot even set a watchpoint it
cannot trigger the crash, of course.

Andreas.
diff mbox

Patch

Index: linux-2.6.set_hwdebug/arch/powerpc/kernel/ptrace.c
===================================================================
--- linux-2.6.set_hwdebug.orig/arch/powerpc/kernel/ptrace.c
+++ linux-2.6.set_hwdebug/arch/powerpc/kernel/ptrace.c
@@ -1316,6 +1316,10 @@  static int set_dac_range(struct task_str
 static long ppc_set_hwdebug(struct task_struct *child,
 		     struct ppc_hw_breakpoint *bp_info)
 {
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	struct perf_event *bp;
+	struct perf_event_attr attr;
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
 #ifndef CONFIG_PPC_ADV_DEBUG_REGS
 	unsigned long dabr;
 #endif
@@ -1365,6 +1369,10 @@  static long ppc_set_hwdebug(struct task_
 
 	if (child->thread.dabr)
 		return -ENOSPC;
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	if (child->thread.ptrace_bps[0])
+		return -ENOSPC;
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
 
 	if ((unsigned long)bp_info->addr >= TASK_SIZE)
 		return -EIO;
@@ -1376,6 +1384,20 @@  static long ppc_set_hwdebug(struct task_
 	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
 		dabr |= DABR_DATA_WRITE;
 
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	/* Create a new breakpoint request if one doesn't exist already */
+	hw_breakpoint_init(&attr);
+	attr.bp_addr = dabr & ~HW_BREAKPOINT_ALIGN;
+	arch_bp_generic_fields(dabr & (DABR_DATA_WRITE | DABR_DATA_READ),
+							&attr.bp_type);
+
+	bp = register_user_hw_breakpoint(&attr, ptrace_triggered, child);
+	if (IS_ERR(bp))
+		return PTR_ERR(bp);
+
+	child->thread.ptrace_bps[0] = bp;
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
+
 	child->thread.dabr = dabr;
 
 	return 1;
@@ -1405,6 +1427,16 @@  static long ppc_del_hwdebug(struct task_
 		return -EINVAL;
 	if (child->thread.dabr == 0)
 		return -ENOENT;
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	/*
+	 * There is no way by which address in ptrace_bps[0] and thread.dabr
+	 * can be different. So we don't explicitly check if they're the same
+	 */
+	if (child->thread.ptrace_bps[0]) {
+		unregister_hw_breakpoint(child->thread.ptrace_bps[0]);
+		child->thread.ptrace_bps[0] = NULL;
+	}
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
 
 	child->thread.dabr = 0;