Messages in this thread | | | From | John Blackwood <> | Subject | Re: [PATCH] ARM64: Clear out any singlestep state on a ptrace detach operation | Date | Fri, 4 Dec 2015 15:42:57 -0600 |
| |
> Subject: Re: [PATCH] ARM64: Clear out any singlestep state on a ptrace detach operation > From: Will Deacon <will.deacon@arm.com> > Date: 12/04/2015 04:03 AM > To: "Blackwood, John" <John.Blackwood@ccur.com> > CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "oleg@redhat.com" <oleg@redhat.com>, "fweisbec@gmail.com" <fweisbec@gmail.com> > > On Thu, Dec 03, 2015 at 02:05:31PM -0600, John Blackwood wrote: > > Hello Will, > > Hi John, > > Thanks for the patch. > > > I have a patch for a ptrace(2) issue that we encountered on arm64 kernels. > > If a debugger singlesteps a ptraced task, and then does a ptrace(2) > > PTRACE_DETACH command, the task will not resume successfully. It seems > > that clearing out the singlestep state, as something like a ptrace(2) > > PTRACE_CONT does, gets this working. > > > > Thank you for your time and considerations. > > I think you're correct that we should be doing this, but actually, why > isn't this done by the core code? It looks to me like this should be > part of ptrace_detach, just like it is part of ptrace_resume when a > PTRACE_CONT request is handled. That would also be a step towards making > ptrace_disable optional (since x86 and powerpc are doing stuff that could > be done in core code too). > > I hacked up a quick patch below (not even compile-tested), but I'm not > sure what to do about hardware {break,watch}points. Some architectures > explicitly clear those on detach, whereas others appear to leave them > alone. Thoughts?
Hi Will,
Thanks for looking into this issue.
I can see your point about possibly having the common code handle the singlestep cleanup on detach. However, I'm a newbie to arm64, and also not knowledgeable at all in any of the other architectures, so I wouldn't be able to comment on whether you patch is a good fit for all other architectures.
I took a look at the hardware breakpoints on x86, and indeed, it is up to the debugger to clear out any current hardware breakpoints (and software breakpoints for that matter) before doing a PTRACE_DETACH. Otherwise, the previously ptraced task(s) may hit leftover breakpoints and SIGTRAP and die.
I wrote an x86 test that sets a hw breakpoint and then detaches and the ptraced task will/can hit the breakpoint and die with a SIGTRAP.
The only clearing of hw breakpoints is when a task execs or when a task forks and the new task will not inherit the task->ptrace_bps[] values. The kernel's perf event support is used to context switch in and out a task's hw breakpoint state and unregistering this perf event is only done in flush_thread() during execs.
However, unlike the situation with detaching after an arm64 singlestep operation occurred, the debugger does have the opportunity to remove the hw/sw breakpoints before detaching if it wants the task to be able to continue on successfully.
Just in case you are interested, I have below a simple test that shows the arm64 singlestep/detach sequence issue. When the singlestep state is not cleaned up, then the 'PASSED' echo will not show up.
Thanks again for your time and considerations.
----- /* * Singlestep detach test * If sucessful, 'PASSED' should be seen when executing this test. * Checks for a kernel bug in arm64 where detaching after a singlestep * would cause the previously ptraced task to send itself a SIGTRAP * signal and die. */ #include <unistd.h> #include <stdio.h> #include <errno.h> #include <sys/ptrace.h> #include <sys/types.h> #include <sys/wait.h> #include <string.h>
static int inferior_wait (int inferior) { int wait_status; int status = waitpid(inferior, &wait_status, 0);
if (status == -1) { perror("waitpid"); return 1; } printf("pid: %d ", status); if (WIFEXITED(wait_status)) { printf("exited with status %d", WEXITSTATUS(wait_status)); } else if (WIFSIGNALED(wait_status)) { printf("terminated with signal %d %s", WTERMSIG(wait_status), strsignal(WTERMSIG(wait_status))); } else if (WIFSTOPPED(wait_status)) { printf("stopped with signal %d %s", WSTOPSIG(wait_status), strsignal(WSTOPSIG(wait_status))); } else { printf("don't understand wait status"); } printf("\n"); return 0; }
#define unused __attribute__((unused))
int main(int unused argc, unused char **argv) { int status, inferior;
inferior = fork(); if (inferior == -1) { perror("fork"); return 1; } if (inferior == 0) { status = ptrace(PTRACE_TRACEME, 0, 0, 0); if (status == -1) { perror("inferior ptrace(PTRACE_TRACEME,...)"); return 1; } execl("/bin/echo", "/bin/echo", "PASSED", NULL); perror("inferior execl"); return 1; } /* Parent (debugger) from here on */ status = inferior_wait(inferior); if (status != 0) return status; status = ptrace(PTRACE_SINGLESTEP, inferior, 0, 0); if (status == -1) { perror("ptrace(PTRACE_SINGLE_STEP,...)"); return 1; } status = inferior_wait(inferior); if (status != 0) return status; status = ptrace(PTRACE_DETACH, inferior, 0, 0); if (status == -1) { perror("ptrace(PTRACE_DETACH,...)"); return 1; } return 0; }
| |