[PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers
Wei-min Pan
weimin.pan@oracle.com
Thu Feb 7 21:39:00 GMT 2019
On 2/7/2019 4:49 AM, Alan Hayward wrote:
>
>> 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.
OK, thanks.
>
>
> 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"
>>>>>> + }
>>>>>> +}
More information about the Gdb-patches
mailing list