[PATCH] arm64: ptrace: hw_break_set take into account hardware breakpoints number

[PATCH] arm64: ptrace: hw_break_set take into account hardware breakpoints number

[Victor Kamensky]

Hi Folks,

I've run into the issue where I failed to debug multithreaded
process on real ARMV8 h/w. GDB was failing with 
"Unexpected error setting hardware debug registers" error.
and on strace of gdb it showed
"ptrace(PTRACE_SETREGSET, 447, 0x403 /* NT_??? */, [{0x7fc1ba8cf8, 264}]) = -1 ENOSPC (No space left on device)"

Initially I thought it is "cdc27c2 arm64: ptrace: avoid 
using HW_BREAKPOINT_EMPTY for disabled events" but I had
it in my tree already. It turns out it is something
different.

Failure happens because current code does not take into
account number of available h/w breakpoints. It works
with default settings in fastmodels where by default it
has 16 h/w breakpoints. And after understanding root cause
I was able to reproduce the issue with fastmodels when I
passed "-C cluster0.cpu0.number-of-breakpoints=0x4 ...
-C cluster1.cpu3.number-of-breakpoints=0x4" options 
restricting number of available h/w breakpoints.

The issue that ENOSPC returned by  __reserve_bp_slot function
called with the following backtrace:

#0 __reserve_bp_slot( bp = (struct perf_event*) 0xFFFFFFC07B72A400 ) at hw_breakpoint.c:281
#1 reserve_bp_slot( bp = (struct perf_event*) 0xFFFFFFC07B72A400 ) at hw_breakpoint.c:320
#2 register_perf_hw_breakpoint( bp = (struct perf_event*) 0xFFFFFFC07B72A400 ) at hw_breakpoint.c:396
#3 hw_breakpoint_event_init( bp = <Value not available : Undefined value in stack frame for register X0> ) at hw_breakpoint.c:572
#4 perf_init_event( event = (struct perf_event*) 0xFFFFFFC07B72A400 ) at core.c:6733
#5 perf_event_alloc( attr = <Value not available : Undefined value in stack frame for register X0>, cpu = <Value not available : Undefined value in stack frame for register X1>, task = <Value not available : Undefined value in stack frame for register X2>, group_leader = (struct perf_event*) 0xFFFFFFC07B72A400, parent_event = <Value not available : Undefined value in stack frame for register X4>, overflow_handler = <Value not available : Undefined value in stack frame for register X5>, context = <Value not available : Undefined value in stack frame for register X6> ) at core.c:6885
#6 perf_event_create_kernel_counter( attr = <Value currently has no location>, cpu = -1, task = (struct task_struct*) 0xFFFFFFC07B4A1600, overflow_handler = <Value currently has no location>, context = <Value currently has no location> ) at core.c:7395
#7 register_user_hw_breakpoint( attr = <Value currently has no location>, triggered = <Value currently has no location>, context = <Value currently has no location>, tsk = <Value currently has no location> ) at hw_breakpoint.c:423
#8 ptrace_hbp_create( idx = <Value optimised away by compiler>, tsk = <Value optimised away by compiler>, note_type = <Value optimised away by compiler> ) at ptrace.c:208
#9 ptrace_hbp_set_addr( note_type = <Value currently has no location>, tsk = <Value currently has no location>, idx = <Value currently has no location>, addr = 0 ) at ptrace.c:350
#10 hw_break_set( target = <Value not available : Undefined value in stack frame for register X0>, regset = <Value currently has no location>, pos = 16, count = 248, kbuf = (const void*) 0x0, ubuf = <Value currently has no location> ) at ptrace.c:450
#11 ptrace_regset( task = (struct task_struct*) 0xFFFFFFC07B4A1600, req = <Value currently has no location>, type = <Value currently has no location>, kiov = (struct iovec*) 0xFFFFFFC07AC1FE00 ) at ptrace.c:787
#12 ptrace_request( child = (struct task_struct*) 0xFFFFFFC07B4A1600, request = <Value currently has no location>, addr = <Value currently has no location>, data = <Value currently has no location> ) at ptrace.c:999
#13 arch_ptrace( child = <Value currently has no location>, request = <Value currently has no location>, addr = <Value currently has no location>, data = <Value currently has no location> ) at ptrace.c:1086
#14 SYSC_ptrace( data = <Value optimised away by compiler>, addr = <Value optimised away by compiler>, pid = <Value optimised away by compiler>, request = <Value optimised away by compiler> ) at ptrace.c:1065
#15 [/wd1/linaro/linux-build/_le_64_linus_302/vmlinux EL1N:0xFFFFFFC00008429C ]

In __reserve_bp_slot function it fails on this snippet:

>	if (slots.pinned + (!!slots.flexible) > nr_slots[type])
>		return -ENOSPC;

basically when in hw_break_set function code iterates
over 16 entries of 'struct user_hwdebug_state' it tries to
reserve real entries in __reserve_bp_slot with above
call stack - ptrace_hbp_set_addr calls ptrace_hbp_get_initialised_bp
which in turns calls ptrace_hbp_create if it cannot find
existing bp. The issue is that on real h/w with limited
number of h/w breakpoints, less than 16, it fails.

My proposed fix, that follows this cover latter, just reads
actual number of available h/w breakpoint/wathcpoint slots
and it iterates over 'struct user_hwdebug_state' only upto
that number of entries or size of 'struct user_hwdebug_state'. 
Assumption here is that all relevant entries passed in
PTRACE_SETREGSET are in first entries of 'struct
user_hwdebug_state' array.

Patch was tested on mustang. I did test multithreaded process
debugging, also hbreak and watchpoint functionality. Mustang
supports and could do up to 4 h/w breakpoint and 4 h/w 
watchpoints for given executable.

Thanks,
Victor

Apendix 1 Test case to reproduce the issue
------------------------------------------

Illustrate issue of failure to debug multithread process

root at mustang:~# cat threads.c 
#include <pthread.h>
#include <stdio.h>
#include <unistd.h>

#define NUM_THREADS 4

void
threads_sleep1 (void)
{
    sleep(1);
}

void
threads_sleep2 (void)
{
    sleep(1);
}

void
threads_func1 (void)
{
}

void
threads_func2 (void)
{
    threads_func1();
}

static void *
test_thread (void *arg)
{
    int i;
    for (i = 0; i < 200000; i++) {
        threads_func2();
        if ((i % 10000) == 0) {
            threads_sleep2();
         }
    }

    return NULL;
}

int
main (void)
{
    int i;
    pthread_t threads[NUM_THREADS];
    
    for (i = 0; i < NUM_THREADS; i++) {
        pthread_create(threads + i,
                       NULL,
                       test_thread,
                       NULL);
    }

    test_thread(NULL);

    for (i = 0; i < NUM_THREADS; i++) {
        pthread_join(threads[i], NULL);
    }
    
    return 0;
}
root at mustang:~# gcc -g -o threads threads.c -lpthread
root@mustang:~# gdb threads
GNU gdb (Linaro GDB) 7.6.1-2013.10
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "aarch64-poky-linux".
For bug reporting instructions, please see:
<http://bugs.launchpad.net/gdb-linaro/>...
Reading symbols from /home/root/threads...done.
(gdb) run
Starting program: /home/root/threads 
warning: Could not load shared library symbols for linux-vdso.so.1.
Do you need "set solib-search-path" or "set sysroot"?
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/libthread_db.so.1".
Unexpected error setting hardware debug registers
(gdb)

Victor Kamensky (1):
  arm64: ptrace: hw_break_set take into account hardware breakpoints
    number

 arch/arm64/kernel/ptrace.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

-- 
1.8.1.4

[PATCH] arm64: ptrace: hw_break_set take into account hardware breakpoints number

[Victor Kamensky]

hw_break_set function that performs ptrace_regset for hardware
breakpoints and watchpoints needs to take into account actual
number of hardware breakpoints and watchpoints available in CPU.

Current code iterates over all 16 entries of 'struct user_hwdebug_state'
and tries to reserve hardware breakpoint for each index, which fails
if CPU supports less than 16 hardware breakpoints. One manifestation of
the issue is that gdb fails to debug multithreaded user land application
and exits with 'Unexpected error setting hardware debug registers'
error - ptrace system call for hardware breakpoints regset fails with
ENOSPC.

Solution is to read number of available hardware breakpoints and
watchpoints available in CPU and process only that number of first
entries in passed 'struct user_hwdebug_state' regset. Code assumes
that ptrace caller uses only first entries in 'struct
user_hwdebug_state' up to number of hardware breakpoints and watchpoints
supported by CPU. I.e there are no "ignore me, empty" entries in
the middle of 'struct user_hwdebug_state' entries array.

Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
 arch/arm64/kernel/ptrace.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index fe63ac5..4178ba1 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -248,22 +248,32 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type,
 	return 0;
 }
 
-static int ptrace_hbp_get_resource_info(unsigned int note_type, u32 *info)
+static int ptrace_hbp_get_num_slots(unsigned int note_type, u8 *num)
 {
-	u8 num;
-	u32 reg = 0;
-
 	switch (note_type) {
 	case NT_ARM_HW_BREAK:
-		num = hw_breakpoint_slots(TYPE_INST);
+		*num = hw_breakpoint_slots(TYPE_INST);
 		break;
 	case NT_ARM_HW_WATCH:
-		num = hw_breakpoint_slots(TYPE_DATA);
+		*num = hw_breakpoint_slots(TYPE_DATA);
 		break;
 	default:
 		return -EINVAL;
 	}
 
+	return 0;
+}
+
+static int ptrace_hbp_get_resource_info(unsigned int note_type, u32 *info)
+{
+	u8 num;
+	u32 reg = 0;
+	int err;
+
+	err = ptrace_hbp_get_num_slots(note_type, &num);
+	if (err)
+		return err;
+
 	reg |= debug_monitors_arch();
 	reg <<= 8;
 	reg |= num;
@@ -432,6 +442,11 @@ static int hw_break_set(struct task_struct *target,
 	int ret, idx = 0, offset, limit;
 	u32 ctrl;
 	u64 addr;
+	u8 num_slots;
+
+	ret = ptrace_hbp_get_num_slots(note_type, &num_slots);
+	if (ret)
+		return ret;
 
 	/* Resource info and pad */
 	offset = offsetof(struct user_hwdebug_state, dbg_regs);
@@ -441,7 +456,7 @@ static int hw_break_set(struct task_struct *target,
 
 	/* (address, ctrl) registers */
 	limit = regset->n * regset->size;
-	while (count && offset < limit) {
+	while ((count && offset < limit) && (idx < num_slots)) {
 		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &addr,
 					 offset, offset + PTRACE_HBP_ADDR_SZ);
 		if (ret)
-- 
1.8.1.4

[PATCH] arm64: ptrace: hw_break_set take into account hardware breakpoints number

[Will Deacon]

Hi Victor,

On Mon, Sep 29, 2014 at 09:04:02AM +0100, Victor Kamensky wrote:
> hw_break_set function that performs ptrace_regset for hardware
> breakpoints and watchpoints needs to take into account actual
> number of hardware breakpoints and watchpoints available in CPU.
> 
> Current code iterates over all 16 entries of 'struct user_hwdebug_state'
> and tries to reserve hardware breakpoint for each index, which fails
> if CPU supports less than 16 hardware breakpoints. One manifestation of
> the issue is that gdb fails to debug multithreaded user land application
> and exits with 'Unexpected error setting hardware debug registers'
> error - ptrace system call for hardware breakpoints regset fails with
> ENOSPC.

When does this happen? hw_break_set is driven by userspace, so if GDB is
asking for more registers than we actually have, then this is a GDB bug and
the kernel is doing the right thing.

Have you reproduced this with the latest version of GDB?

Will

[PATCH] arm64: ptrace: hw_break_set take into account hardware breakpoints number

[Victor Kamensky]

Hi Will,

On 29 September 2014 03:16, Will Deacon <will.deacon@arm.com> wrote:
> Hi Victor,
>
> On Mon, Sep 29, 2014 at 09:04:02AM +0100, Victor Kamensky wrote:
>> hw_break_set function that performs ptrace_regset for hardware
>> breakpoints and watchpoints needs to take into account actual
>> number of hardware breakpoints and watchpoints available in CPU.
>>
>> Current code iterates over all 16 entries of 'struct user_hwdebug_state'
>> and tries to reserve hardware breakpoint for each index, which fails
>> if CPU supports less than 16 hardware breakpoints. One manifestation of
>> the issue is that gdb fails to debug multithreaded user land application
>> and exits with 'Unexpected error setting hardware debug registers'
>> error - ptrace system call for hardware breakpoints regset fails with
>> ENOSPC.
>
> When does this happen? hw_break_set is driven by userspace, so if GDB is
> asking for more registers than we actually have, then this is a GDB bug and
> the kernel is doing the right thing.
>
> Have you reproduced this with the latest version of GDB?

Thank you for the reply. Just checked latest version of GDB, indeed it
does not have my original issue. It turns out my gdb is based on some
old linaro gdb version that unconditionally passes sizeof(user_hwdebug_state)
as iov.len to PTRACE_SETREGSET with note type NT_ARM_HW_BREAK.
Latest gdb adjusts iov.len based on available number of hardware breakpoints.
I should have checked latest gdb before posting this. Naturally previously
suggested patch is withdrawn.

Question is there any place where variable payload nature of
PTRACE_SETREGSET with note type NT_ARM_HW_BREAK and
NT_ARM_HW_WATCH documented? I've tried kernel Documentation
directory, aarch64 abi document, http://infocenter.arm.com search, none
of them mention NT_ARM_HW_BREAK. Just curious is there any place
for that sort of information to look in the future. If answer that it is
documented by existing kernel/gdb code :), please feel free to ignore
this question.

Thanks,
Victor

> Will

[PATCH] arm64: ptrace: hw_break_set take into account hardware breakpoints number

[Christopher Covington]

Hi Victor,

On 09/29/2014 01:49 PM, Victor Kamensky wrote:
> Hi Will,
> 
> On 29 September 2014 03:16, Will Deacon <will.deacon@arm.com> wrote:
>> Hi Victor,
>>
>> On Mon, Sep 29, 2014 at 09:04:02AM +0100, Victor Kamensky wrote:
>>> hw_break_set function that performs ptrace_regset for hardware
>>> breakpoints and watchpoints needs to take into account actual
>>> number of hardware breakpoints and watchpoints available in CPU.
>>>
>>> Current code iterates over all 16 entries of 'struct user_hwdebug_state'
>>> and tries to reserve hardware breakpoint for each index, which fails
>>> if CPU supports less than 16 hardware breakpoints. One manifestation of
>>> the issue is that gdb fails to debug multithreaded user land application
>>> and exits with 'Unexpected error setting hardware debug registers'
>>> error - ptrace system call for hardware breakpoints regset fails with
>>> ENOSPC.
>>
>> When does this happen? hw_break_set is driven by userspace, so if GDB is
>> asking for more registers than we actually have, then this is a GDB bug and
>> the kernel is doing the right thing.
>>
>> Have you reproduced this with the latest version of GDB?
> 
> Thank you for the reply. Just checked latest version of GDB, indeed it
> does not have my original issue. It turns out my gdb is based on some
> old linaro gdb version that unconditionally passes sizeof(user_hwdebug_state)
> as iov.len to PTRACE_SETREGSET with note type NT_ARM_HW_BREAK.
> Latest gdb adjusts iov.len based on available number of hardware breakpoints.
> I should have checked latest gdb before posting this. Naturally previously
> suggested patch is withdrawn.
> 
> Question is there any place where variable payload nature of
> PTRACE_SETREGSET with note type NT_ARM_HW_BREAK and
> NT_ARM_HW_WATCH documented? I've tried kernel Documentation
> directory, aarch64 abi document, http://infocenter.arm.com search, none
> of them mention NT_ARM_HW_BREAK. Just curious is there any place
> for that sort of information to look in the future. If answer that it is
> documented by existing kernel/gdb code :), please feel free to ignore
> this question.

While it doesn't address those specific note types (yet):

http://man7.org/linux/man-pages/man2/ptrace.2.html

       PTRACE_GETREGSET (since Linux 2.6.34)
              Read the tracee's registers.  addr specifies, in an
              architecture-dependent way, the type of registers to be read.
              NT_PRSTATUS (with numerical value 1) usually results in
              reading of general-purpose registers.  If the CPU has, for
              example, floating-point and/or vector registers, they can be
              retrieved by setting addr to the corresponding NT_foo
              constant.  data points to a struct iovec, which describes the
              destination buffer's location and length.  On return, the
              kernel modifies iov.len to indicate the actual number of bytes
              returned.

       PTRACE_SETREGSET (since Linux 2.6.34)
              Modify the tracee's registers.  The meaning of addr and data
              is analogous to PTRACE_GETREGSET.

Perhaps this could be expanded upon.

https://www.kernel.org/doc/man-pages/patches.html

Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.