Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers

[Alan Hayward <Alan.Hayward@arm.com>]

> On 6 Feb 2019, at 22:36, Wei-min Pan <weimin.pan@oracle.com> wrote:
> 
> On 2/6/2019 4:43 AM, Alan Hayward wrote:
> >
> >
> >> On 6 Feb 2019, at 00:51, Weimin Pan <weimin.pan@oracle.com> wrote:
> >>
> >> Would you please review this aarch64-specific patch? Thanks.
> >
> >
> > Had a few problems applying the patch directly.
> > Also, the changelog entry should be in the body of the email and not
> > the patch.
> 
> Thanks for pointing it out, noted.
> 
> > But, ignoring that for now...
> >
> > I ran the test case with the current HEAD on AArch64, and it passed.
> > I think that your issue might have already been fixed with the following
> > patch:
> >
> > commit 754e31689866524049b9cfc68053ed4e1293cfac
> > Author: Alan Hayward <alan.hayward@arm.com>
> > Date:   9 weeks ago
> >
> >     AArch64: Racy: Don't set empty set of hardware BPs/WPs on new thread
> >
> >
> > My patch is roughly doing the same thing, but in a different way.
> > Could you check this please?
> 
> Yes, your patch does fix the problem.

Excellent!

> 
> >
> > I've not yet read the test case in detail. If the bug is already fixed,
> > then the test might still be useful.
> 
> I can push the test, which is included in the bug report, upstream.

Ok - I'll review the test case either tomorrow or early next week.


Alan.

> 
> Thanks.
> 
>> Alan.
>> 
>> 
>>> On 7/11/2018 5:30 PM, Wei-min Pan wrote:
>>>> On 6/27/2018 6:10 PM, Weimin Pan wrote:
>>>>> At issue here is that ptrace() does not validate either address or size
>>>>> when setting a hardware watchpoint/breakpoint. As a result, watchpoints
>>>>> were set at address 0, the initial value of aarch64_debug_reg_state in
>>>>> aarch64_process_info, and DR_CONTROL_LENGTH (dreg_state.dbg_regs[i].ctrl)
>>>>> got changed to a non-zero default value, when the PTRACE_SETREGSET request
>>>>> was made in aarch64_linux_set_debug_regs(), in preparation for resuming
>>>>> the thread.
>>>>> 
>>>>> I sent out a patch upstream for review last year. Yao Qi commented that
>>>>> he needed to take a look at ptrace implementation to better understand
>>>>> why this was happening. Unfortunately he didn't get a chance to do so.
>>>>> 
>>>>> This is a revised patch which calls ptrace only if any debug register
>>>>> has changed, either address or control containing a non-zero value,
>>>>> in aarch64_linux_set_debug_regs() and is similar to what x86 does -
>>>>> x86_linux_update_debug_registers() checks if a debug register has
>>>>> non-zero reference count before setting it.
>>>>> 
>>>>> Tested on aarch64-linux-gnu. No regressions.
>>>>> ---
>>>>>   gdb/ChangeLog                                     |   6 +
>>>>>   gdb/nat/aarch64-linux-hw-point.c                  |   7 +
>>>>>   gdb/testsuite/ChangeLog                           |  20 ++-
>>>>>   gdb/testsuite/gdb.arch/aarch64-dbreg-contents.c   | 179 ++++++++++++++++++++++
>>>>>   gdb/testsuite/gdb.arch/aarch64-dbreg-contents.exp |  40 +++++
>>>>>   5 files changed, 244 insertions(+), 8 deletions(-)
>>>>>   create mode 100644 gdb/testsuite/gdb.arch/aarch64-dbreg-contents.c
>>>>>   create mode 100644 gdb/testsuite/gdb.arch/aarch64-dbreg-contents.exp
>>>>> 
>>>>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>>>>> index 7b108bf..d5bf15f 100644
>>>>> --- a/gdb/ChangeLog
>>>>> +++ b/gdb/ChangeLog
>>>>> @@ -1,3 +1,9 @@
>>>>> +2018-06-27  Weimin Pan  <weimin.pan@oracle.com>
>>>>> +
>>>>> +    PR gdb/21870
>>>>> +    * nat/aarch64-linux-hw-point.c (aarch64_linux_set_debug_regs):
>>>>> +    Validate debug register contents before calling ptrace.
>>>>> +
>>>>>   2018-06-18  Tom Tromey  <tom@tromey.com>
>>>>>         * solib-aix.c (solib_aix_get_section_offsets): Return
>>>>> diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c
>>>>> index a3931ea..41ac304 100644
>>>>> --- a/gdb/nat/aarch64-linux-hw-point.c
>>>>> +++ b/gdb/nat/aarch64-linux-hw-point.c
>>>>> @@ -694,11 +694,18 @@ aarch64_linux_set_debug_regs (struct aarch64_debug_reg_state *state,
>>>>>     iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs)
>>>>>            + count * sizeof (regs.dbg_regs[0]));
>>>>>   +  int changed_regs = 0;
>>>>>     for (i = 0; i < count; i++)
>>>>>       {
>>>>>         regs.dbg_regs[i].addr = addr[i];
>>>>>         regs.dbg_regs[i].ctrl = ctrl[i];
>>>>> +      /* Check to see if any of these debug registers contains valid data,
>>>>> +         e.g. non-zero, before calling ptrace.  See PR gdb/21870.  */
>>>>> +      if (ctrl[i] || addr[i])
>>>>> +        changed_regs++;
>>>>>       }
>>>>> +  if (changed_regs == 0)
>>>>> +    return;
>>>>>       if (ptrace (PTRACE_SETREGSET, tid,
>>>>>             watchpoint ? NT_ARM_HW_WATCH : NT_ARM_HW_BREAK,
>>>>> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
>>>>> index a812061..0ce2d34 100644
>>>>> --- a/gdb/testsuite/ChangeLog
>>>>> +++ b/gdb/testsuite/ChangeLog
>>>>> @@ -1,4 +1,9 @@
>>>>> +2018-06-27  Weimin Pan  <weimin.pan@oracle.com>
>>>>> +
>>>>> +    PR gdb/21870
>>>>> +    * gdb.arch/aarch64-dbreg-contents.c: New file.
>>>>> +    * gdb.arch/aarch64-dbreg-contents.exp: New file.
>>>>> +
>>>>>   2018-06-18  Tom de Vries  <tdevries@suse.de>
>>>>>         * gdb.ada/bp_inlined_func.exp: Allow 5 breakpoint locations.
>>>>>   diff --git a/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.c b/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.c
>>>>> new file mode 100644
>>>>> index 0000000..85d4a03
>>>>> --- /dev/null
>>>>> +++ b/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.c
>>>>> @@ -0,0 +1,179 @@
>>>>> +/* Test case for setting a memory-write unaligned watchpoint on aarch64.
>>>>> +
>>>>> +  This software is provided 'as-is', without any express or implied
>>>>> +  warranty.  In no event will the authors be held liable for any damages
>>>>> +  arising from the use of this software.
>>>>> +
>>>>> +  Permission is granted to anyone to use this software for any purpose,
>>>>> +  including commercial applications, and to alter it and redistribute it
>>>>> +  freely.  */
>>>>> +
>>>>> +#define _GNU_SOURCE 1
>>>>> +#ifdef __ia64__
>>>>> +#define ia64_fpreg ia64_fpreg_DISABLE
>>>>> +#define pt_all_user_regs pt_all_user_regs_DISABLE
>>>>> +#endif  /* __ia64__ */
>>>>> +#include <sys/ptrace.h>
>>>>> +#ifdef __ia64__
>>>>> +#undef ia64_fpreg
>>>>> +#undef pt_all_user_regs
>>>>> +#endif  /* __ia64__ */
>>>>> +#include <linux/ptrace.h>
>>>>> +#include <sys/types.h>
>>>>> +#include <sys/user.h>
>>>>> +#if defined __i386__ || defined __x86_64__
>>>>> +#include <sys/debugreg.h>
>>>>> +#endif
>>>>> +
>>>>> +#include <assert.h>
>>>>> +#include <unistd.h>
>>>>> +#include <sys/wait.h>
>>>>> +#include <stdio.h>
>>>>> +#include <stdlib.h>
>>>>> +#include <stddef.h>
>>>>> +#include <errno.h>
>>>>> +#include <sys/uio.h>
>>>>> +#include <elf.h>
>>>>> +#include <error.h>
>>>>> +
>>>>> +static __attribute__((unused)) pid_t child;
>>>>> +
>>>>> +static __attribute__((unused)) void
>>>>> +cleanup (void)
>>>>> +{
>>>>> +  if (child > 0)
>>>>> +    kill (child, SIGKILL);
>>>>> +  child = 0;
>>>>> +}
>>>>> +
>>>>> +static __attribute__((unused)) void
>>>>> +handler_fail (int signo)
>>>>> +{
>>>>> +  cleanup ();
>>>>> +  signal (signo, SIG_DFL);
>>>>> +  raise (signo);
>>>>> +}
>>>>> +
>>>>> +#ifdef __aarch64__
>>>>> +
>>>>> +#define SET_WATCHPOINT set_watchpoint
>>>>> +
>>>>> +/* Macros to extract fields from the hardware debug information word.  */
>>>>> +#define AARCH64_DEBUG_NUM_SLOTS(x) ((x) & 0xff)
>>>>> +#define AARCH64_DEBUG_ARCH(x) (((x) >> 8) & 0xff)
>>>>> +/* Macro for the expected version of the ARMv8-A debug architecture.  */
>>>>> +#define AARCH64_DEBUG_ARCH_V8 0x6
>>>>> +#define DR_CONTROL_ENABLED(ctrl)        (((ctrl) & 0x1) == 1)
>>>>> +#define DR_CONTROL_LENGTH(ctrl)         (((ctrl) >> 5) & 0xff)
>>>>> +
>>>>> +static void
>>>>> +set_watchpoint (pid_t pid, volatile void *addr, unsigned len_mask)
>>>>> +{
>>>>> +  struct user_hwdebug_state dreg_state;
>>>>> +  struct iovec iov;
>>>>> +  long l;
>>>>> +
>>>>> +  assert (len_mask >= 0x01);
>>>>> +  assert (len_mask <= 0xff);
>>>>> +
>>>>> +  iov.iov_base = &dreg_state;
>>>>> +  iov.iov_len = sizeof (dreg_state);
>>>>> +  errno = 0;
>>>>> +  l = ptrace (PTRACE_GETREGSET, pid, NT_ARM_HW_WATCH, &iov);
>>>>> +  assert (l == 0);
>>>>> +  assert (AARCH64_DEBUG_ARCH (dreg_state.dbg_info) == AARCH64_DEBUG_ARCH_V8);
>>>>> +  assert (AARCH64_DEBUG_NUM_SLOTS (dreg_state.dbg_info) >= 1);
>>>>> +
>>>>> +  assert (!DR_CONTROL_ENABLED (dreg_state.dbg_regs[0].ctrl));
>>>>> +  dreg_state.dbg_regs[0].ctrl |= 1;
>>>>> +  assert ( DR_CONTROL_ENABLED (dreg_state.dbg_regs[0].ctrl));
>>>>> +
>>>>> +  assert (DR_CONTROL_LENGTH (dreg_state.dbg_regs[0].ctrl) == 0);
>>>>> +  dreg_state.dbg_regs[0].ctrl |= len_mask << 5;
>>>>> +  assert (DR_CONTROL_LENGTH (dreg_state.dbg_regs[0].ctrl) == len_mask);
>>>>> +
>>>>> +  dreg_state.dbg_regs[0].ctrl |= 2 << 3; // write
>>>>> +  dreg_state.dbg_regs[0].ctrl |= 2 << 1; // GDB: ???: enabled at el0
>>>>> +  //printf("ctrl=0x%x\n",dreg_state.dbg_regs[0].ctrl);
>>>>> +  dreg_state.dbg_regs[0].addr = (uintptr_t) addr;
>>>>> +
>>>>> +  iov.iov_base = &dreg_state;
>>>>> +  iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs)
>>>>> +                 + sizeof (dreg_state.dbg_regs[0]));
>>>>> +  errno = 0;
>>>>> +  l = ptrace (PTRACE_SETREGSET, pid, NT_ARM_HW_WATCH, &iov);
>>>>> +  if (errno != 0)
>>>>> +    error (1, errno, "PTRACE_SETREGSET: NT_ARM_HW_WATCH");
>>>>> +  assert (l == 0);
>>>>> +}
>>>>> +
>>>>> +#endif
>>>>> +
>>>>> +#ifndef SET_WATCHPOINT
>>>>> +
>>>>> +int
>>>>> +main (void)
>>>>> +{
>>>>> +  return 77;
>>>>> +}
>>>>> +#else
>>>>> +
>>>>> +static volatile long long check;
>>>>> +
>>>>> +int
>>>>> +main (void)
>>>>> +{
>>>>> +  pid_t got_pid;
>>>>> +  int i, status;
>>>>> +  long l;
>>>>> +
>>>>> +  atexit (cleanup);
>>>>> +  signal (SIGABRT, handler_fail);
>>>>> +  signal (SIGINT, handler_fail);
>>>>> +
>>>>> +  child = fork ();
>>>>> +  assert (child >= 0);
>>>>> +  if (child == 0)
>>>>> +    {
>>>>> +      l = ptrace (PTRACE_TRACEME, 0, NULL, NULL);
>>>>> +      assert (l == 0);
>>>>> +      i = raise (SIGUSR1);
>>>>> +      assert (i == 0);
>>>>> +      check = -1;
>>>>> +      i = raise (SIGUSR2);
>>>>> +      /* NOTREACHED */
>>>>> +      assert (0);
>>>>> +    }
>>>>> +
>>>>> +  got_pid = waitpid (child, &status, 0);
>>>>> +  assert (got_pid == child);
>>>>> +  assert (WIFSTOPPED (status));
>>>>> +  assert (WSTOPSIG (status) == SIGUSR1);
>>>>> +
>>>>> +  // PASS:
>>>>> +  //SET_WATCHPOINT (child, &check, 0xff);
>>>>> +  // FAIL:
>>>>> +  SET_WATCHPOINT (child, &check, 0x02);
>>>>> +
>>>>> +  errno = 0;
>>>>> +  l = ptrace (PTRACE_CONT, child, 0l, 0l);
>>>>> +  assert_perror (errno);
>>>>> +  assert (l == 0);
>>>>> +
>>>>> +  got_pid = waitpid (child, &status, 0);
>>>>> +  assert (got_pid == child);
>>>>> +  assert (WIFSTOPPED (status));
>>>>> +  if (WSTOPSIG (status) == SIGUSR2)
>>>>> +    {
>>>>> +      /* We missed the watchpoint - unsupported by hardware? */
>>>>> +      cleanup ();
>>>>> +      return 2;
>>>>> +    }
>>>>> +  assert (WSTOPSIG (status) == SIGTRAP);
>>>>> +
>>>>> +  cleanup ();
>>>>> +  return 0;
>>>>> +}
>>>>> +
>>>>> +#endif
>>>>> +
>>>>> diff --git a/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.exp b/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.exp
>>>>> new file mode 100644
>>>>> index 0000000..6aa23b0
>>>>> --- /dev/null
>>>>> +++ b/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.exp
>>>>> @@ -0,0 +1,40 @@
>>>>> +# Copyright 2018 Free Software Foundation, Inc.
>>>>> +
>>>>> +# This program is free software; you can redistribute it and/or modify
>>>>> +# it under the terms of the GNU General Public License as published by
>>>>> +# the Free Software Foundation; either version 3 of the License, or
>>>>> +# (at your option) any later version.
>>>>> +#
>>>>> +# This program is distributed in the hope that it will be useful,
>>>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>> +# GNU General Public License for more details.
>>>>> +#
>>>>> +# You should have received a copy of the GNU General Public License
>>>>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>>> +#
>>>>> +# Make sure that the inferior doesn't assert and exits successfully.
>>>>> +
>>>>> +if {![is_aarch64_target]} {
>>>>> +    verbose "Skipping ${gdb_test_file_name}."
>>>>> +    return
>>>>> +}
>>>>> +
>>>>> +standard_testfile .c
>>>>> +
>>>>> +if { [gdb_compile ${srcdir}/${subdir}/${srcfile} ${binfile} executable {debug}] != "" } {
>>>>> +     untested "failed to compile"
>>>>> +     return -1
>>>>> +}
>>>>> +
>>>>> +clean_restart $testfile
>>>>> +
>>>>> +set test "run to exit"
>>>>> +gdb_test_multiple "run" "$test" {
>>>>> +    -re "exited with code 01.*$gdb_prompt $" {
>>>>> +        pass "$test"
>>>>> +    }
>>>>> +    -re "exited normally.*$gdb_prompt $" {
>>>>> +        pass "$test"
>>>>> +    }
>>>>> +}