Message-ID: <566208E1.4050703@ccur.com>
Date:	Fri, 4 Dec 2015 15:42:57 -0600
From:	John Blackwood <john.blackwood@...r.com>
To:	Will Deacon <will.deacon@....com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"oleg@...hat.com" <oleg@...hat.com>,
	"fweisbec@...il.com" <fweisbec@...il.com>
Subject: Re: [PATCH] ARM64: Clear out any singlestep state on a ptrace detach
 operation

> Subject: Re: [PATCH] ARM64: Clear out any singlestep state on a ptrace detach operation
> From: Will Deacon <will.deacon@....com>
> Date: 12/04/2015 04:03 AM
> To: "Blackwood, John" <John.Blackwood@...r.com>
> CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "oleg@...hat.com" <oleg@...hat.com>, "fweisbec@...il.com" <fweisbec@...il.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;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists