[PATCH] arm64: hw_breakpoint: use target state to determine ABI behaviour

[PATCH] arm64: hw_breakpoint: use target state to determine ABI behaviour

[Will Deacon]

The arm64 hw_breakpoint interface is slightly less flexible than its
32-bit counterpart, thanks to some changes in the architecture rendering
unaligned watchpoint addresses obselete for AArch64.

However, in a multi-arch environment (i.e. debugging a 32-bit target
with a 64-bit GDB under a 64-bit kernel), we need to provide a feature
compatible interface to GDB in order for debugging to function correctly.

This patch adds a new helper, is_compat_bp,  to our hw_breakpoint
implementation which changes the interface behaviour based on the
architecture of the debug target as opposed to the debugger itself.
This allows debugged to function as expected for multi-arch
configurations without relying on deprecated architectural behaviours
when debugging native applications.

Cc: Yao Qi <yao.qi@arm.com>
Cc: Catalin Marinas <Catalin.Marinas@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/hw_breakpoint.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index bba85c8f8037..2926c3dd6479 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -163,6 +163,12 @@ enum hw_breakpoint_ops {
 	HW_BREAKPOINT_RESTORE
 };
 
+static int is_compat_bp(struct perf_event *bp)
+{
+	struct task_struct *tsk = bp->hw.target;
+	return tsk && is_compat_thread(task_thread_info(tsk));
+}
+
 /**
  * hw_breakpoint_slot_setup - Find and setup a perf slot according to
  *			      operations
@@ -420,7 +426,7 @@ static int arch_build_bp_info(struct perf_event *bp)
 	 * Watchpoints can be of length 1, 2, 4 or 8 bytes.
 	 */
 	if (info->ctrl.type == ARM_BREAKPOINT_EXECUTE) {
-		if (is_compat_task()) {
+		if (is_compat_bp(bp)) {
 			if (info->ctrl.len != ARM_BREAKPOINT_LEN_2 &&
 			    info->ctrl.len != ARM_BREAKPOINT_LEN_4)
 				return -EINVAL;
@@ -477,7 +483,7 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 	 * AArch32 tasks expect some simple alignment fixups, so emulate
 	 * that here.
 	 */
-	if (is_compat_task()) {
+	if (is_compat_bp(bp)) {
 		if (info->ctrl.len == ARM_BREAKPOINT_LEN_8)
 			alignment_mask = 0x7;
 		else
-- 
2.1.4

[PATCH] arm64: hw_breakpoint: use target state to determine ABI behaviour

[Catalin Marinas]

On Tue, Sep 15, 2015 at 05:16:13PM +0100, Will Deacon wrote:
> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> index bba85c8f8037..2926c3dd6479 100644
> --- a/arch/arm64/kernel/hw_breakpoint.c
> +++ b/arch/arm64/kernel/hw_breakpoint.c
> @@ -163,6 +163,12 @@ enum hw_breakpoint_ops {
>  	HW_BREAKPOINT_RESTORE
>  };
>  
> +static int is_compat_bp(struct perf_event *bp)
> +{
> +	struct task_struct *tsk = bp->hw.target;
> +	return tsk && is_compat_thread(task_thread_info(tsk));
> +}

What are the conditions for tsk == NULL?

-- 
Catalin

[PATCH] arm64: hw_breakpoint: use target state to determine ABI behaviour

[Will Deacon]

On Tue, Sep 15, 2015 at 05:43:01PM +0100, Catalin Marinas wrote:
> On Tue, Sep 15, 2015 at 05:16:13PM +0100, Will Deacon wrote:
> > diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> > index bba85c8f8037..2926c3dd6479 100644
> > --- a/arch/arm64/kernel/hw_breakpoint.c
> > +++ b/arch/arm64/kernel/hw_breakpoint.c
> > @@ -163,6 +163,12 @@ enum hw_breakpoint_ops {
> >  	HW_BREAKPOINT_RESTORE
> >  };
> >  
> > +static int is_compat_bp(struct perf_event *bp)
> > +{
> > +	struct task_struct *tsk = bp->hw.target;
> > +	return tsk && is_compat_thread(task_thread_info(tsk));
> > +}
> 
> What are the conditions for tsk == NULL?

If you create a per-cpu breakpoint (i.e. a breakpoint that isn't affine
to a task), then the target is NULL. These can only be created by the
perf_event_open syscall, not from ptrace.

Will

[PATCH] arm64: hw_breakpoint: use target state to determine ABI behaviour

[Catalin Marinas]

On Tue, Sep 15, 2015 at 05:55:29PM +0100, Will Deacon wrote:
> On Tue, Sep 15, 2015 at 05:43:01PM +0100, Catalin Marinas wrote:
> > On Tue, Sep 15, 2015 at 05:16:13PM +0100, Will Deacon wrote:
> > > diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> > > index bba85c8f8037..2926c3dd6479 100644
> > > --- a/arch/arm64/kernel/hw_breakpoint.c
> > > +++ b/arch/arm64/kernel/hw_breakpoint.c
> > > @@ -163,6 +163,12 @@ enum hw_breakpoint_ops {
> > >  	HW_BREAKPOINT_RESTORE
> > >  };
> > >  
> > > +static int is_compat_bp(struct perf_event *bp)
> > > +{
> > > +	struct task_struct *tsk = bp->hw.target;
> > > +	return tsk && is_compat_thread(task_thread_info(tsk));
> > > +}
> > 
> > What are the conditions for tsk == NULL?
> 
> If you create a per-cpu breakpoint (i.e. a breakpoint that isn't affine
> to a task), then the target is NULL. These can only be created by the
> perf_event_open syscall, not from ptrace.

OK, so can sys_perf_event_open() be called from a compat task? If that's
the case, is_compat_bp() should do:

	if (tsk)
		return is_compat_thread(task_thread_info(tsk));
	else
		return is_compat_task();

and a comment on why this is (not) needed. Your patch changes the
current code behaviour when tsk == NULL.

-- 
Catalin