[PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[Denys Vlasenko]

Make PTRACE_SEIZE set ptrace options specified in 'data' parameter
    
This can be used to close a few corner cases in strace where we get
unwanted racy behavior after attach, but before we have a chance
to set options (the notorious post-execve SIGTRAP comes to mind),
and removes the need to track "did we set opts for this task" state
in strace internals.
    
While we are at it:
    
Make it possible to extend SEIZE in the future with more functionality
by passing non-zero 'addr' parameter.
To that end, error out if 'addr' is non-zero.
PTRACE_ATTACH did not (and still does not) have such check,
and users (strace) do pass garbage there... let's avoid repeating
this mistake with SEIZE.
    
Set all task->ptrace bits in one operation - before this change,
we were adding PT_SEIZED and PT_PTRACE_CAP with task->ptrace |= BIT ops.
This was probably ok (not a bug), but let's be on a safer side.

Changes since v2: use (unsigned long) casts instead of (long) ones,
move PTRACE_SEIZE_DEVEL-related code to separate lines of code.

    
Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
---

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index dbc4a3e..166de95 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -212,6 +212,7 @@ bool ptrace_may_access(struct task_struct *task, unsigned int mode)
 }
 
 static int ptrace_attach(struct task_struct *task, long request,
+			 unsigned long addr,
 			 unsigned long flags)
 {
 	bool seize = (request == PTRACE_SEIZE);
@@ -219,19 +220,29 @@ static int ptrace_attach(struct task_struct *task, long request,
 
 	/*
 	 * SEIZE will enable new ptrace behaviors which will be implemented
-	 * gradually.  SEIZE_DEVEL is used to prevent applications
+	 * gradually.  SEIZE_DEVEL bit is used to prevent applications
 	 * expecting full SEIZE behaviors trapping on kernel commits which
 	 * are still in the process of implementing them.
 	 *
 	 * Only test programs for new ptrace behaviors being implemented
 	 * should set SEIZE_DEVEL.  If unset, SEIZE will fail with -EIO.
 	 *
-	 * Once SEIZE behaviors are completely implemented, this flag and
-	 * the following test will be removed.
+	 * Once SEIZE behaviors are completely implemented, this flag
+	 * will be removed.
 	 */
 	retval = -EIO;
-	if (seize && !(flags & PTRACE_SEIZE_DEVEL))
-		goto out;
+	if (seize) {
+		if (addr != 0)
+			goto out;
+		if (!(flags & PTRACE_SEIZE_DEVEL))
+			goto out;
+		flags &= ~(unsigned long)PTRACE_SEIZE_DEVEL;
+		if (flags & ~(unsigned long)PTRACE_O_MASK)
+			goto out;
+		flags = PT_PTRACED | PT_SEIZED | (flags << PT_OPT_FLAG_SHIFT);
+	} else {
+		flags = PT_PTRACED;
+	}
 
 	audit_ptrace(task);
 
@@ -263,11 +274,10 @@ static int ptrace_attach(struct task_struct *task, long request,
 	if (task->ptrace)
 		goto unlock_tasklist;
 
-	task->ptrace = PT_PTRACED;
-	if (seize)
-		task->ptrace |= PT_SEIZED;
 	if (task_ns_capable(task, CAP_SYS_PTRACE))
-		task->ptrace |= PT_PTRACE_CAP;
+		flags |= PT_PTRACE_CAP;
+
+	task->ptrace = flags;
 
 	__ptrace_link(task, current);
 
@@ -865,7 +875,7 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
 	}
 
 	if (request == PTRACE_ATTACH || request == PTRACE_SEIZE) {
-		ret = ptrace_attach(child, request, data);
+		ret = ptrace_attach(child, request, addr, data);
 		/*
 		 * Some architectures need to do book-keeping after
 		 * a ptrace attach.

Re: [PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[Oleg Nesterov]

On 09/08, Denys Vlasenko wrote:
>
> Make PTRACE_SEIZE set ptrace options specified in 'data' parameter

Thanks, looks good. But we need the correct "Simplify PTRACE_foo..."
first.

> Make it possible to extend SEIZE in the future with more functionality
> by passing non-zero 'addr' parameter.

Agreed, this makes sense.

> Set all task->ptrace bits in one operation - before this change,
> we were adding PT_SEIZED and PT_PTRACE_CAP with task->ptrace |= BIT ops.
> This was probably ok (not a bug), but let's be on a safer side.

PT_PTRACE_CAP is fine, but PT_SEIZED is not really, afaics.

Oleg.

Re: [PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[Pedro Alves]

On Thursday 08 September 2011 19:22:04, Denys Vlasenko wrote:
> Make PTRACE_SEIZE set ptrace options specified in 'data' parameter
>     
> This can be used to close a few corner cases in strace where we get
> unwanted racy behavior after attach, but before we have a chance
> to set options (the notorious post-execve SIGTRAP comes to mind),

I'm still confused on why you're raising the SIGTRAP argument.  Did you see

 https://lkml.org/lkml/2011/9/8/7

?

-- 
Pedro Alves

Re: [PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[Denys Vlasenko]

On Fri, 2011-09-09 at 12:12 +0100, Pedro Alves wrote:
> On Thursday 08 September 2011 19:22:04, Denys Vlasenko wrote:
> > Make PTRACE_SEIZE set ptrace options specified in 'data' parameter
> >     
> > This can be used to close a few corner cases in strace where we get
> > unwanted racy behavior after attach, but before we have a chance
> > to set options (the notorious post-execve SIGTRAP comes to mind),
> 
> I'm still confused on why you're raising the SIGTRAP argument.  Did you see
> 
>  https://lkml.org/lkml/2011/9/8/7
> 
> From previous discussions, I understood that PTRACE_SEIZE _always_
> disables
> the post-execve SIGTRAP, so I don't believe that race actually exists.
> Or is that not the case?


I believe it is not the case. And I object to making it the case.
My sense of taste says the approach "you need to use SEIZE to affect
feature <foo>" for various unrelated <foo> makes ptrace API ugly.
Especially that in this case, we already have a method in API
to suppress post-execve SIGTRAP.

-- 
vda

Re: [PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[Pedro Alves]

On Friday 09 September 2011 13:28:55, Denys Vlasenko wrote:
> On Fri, 2011-09-09 at 12:12 +0100, Pedro Alves wrote:
> > On Thursday 08 September 2011 19:22:04, Denys Vlasenko wrote:
> > > Make PTRACE_SEIZE set ptrace options specified in 'data' parameter
> > >     
> > > This can be used to close a few corner cases in strace where we get
> > > unwanted racy behavior after attach, but before we have a chance
> > > to set options (the notorious post-execve SIGTRAP comes to mind),
> > 
> > I'm still confused on why you're raising the SIGTRAP argument.  Did you see
> > 
> >  https://lkml.org/lkml/2011/9/8/7
> > 
> > From previous discussions, I understood that PTRACE_SEIZE _always_
> > disables
> > the post-execve SIGTRAP, so I don't believe that race actually exists.
> > Or is that not the case?
> 
> 
> I believe it is not the case. And I object to making it the case.

Well, if you'll remember, back in <https://lkml.org/lkml/2011/5/19/704> I
raised this exact problem with that pesky racy post-execve SIGTRAP showing
through on SEIZE, and Tejun a few replies later mentioned that the SIGTRAP
was to be removed on SEIZE.   I'm sure it was said before even, but I'm
not finding the emails now.

> My sense of taste says the approach "you need to use SEIZE to affect
> feature <foo>" for various unrelated <foo> makes ptrace API ugly.

Yes, very much agreed!

> Especially that in this case, we already have a method in API
> to suppress post-execve SIGTRAP.

Right, but we end up with no way to make the tracee _not ever
stop_ at execve if the tracer wants to, like you can make
the tracee not ever stop on forks or syscalls, by not enabling
the corresponding PTRACE_O_FOO or not PTRACE_SYSCALL.  Not
specifying PTRACE_O_TRACEEXEC coupled with `SEIZE not stopping
tracees for that magic SIGTRAP' got you that.  In a way, it looked to
me to make the API a bit less ugly.  That said I'm not seeing GDB
_not_ using PTRACE_O_TRACEEXEC...

Anyway, could you please check (or Tejun please confirm)
that that magic SIGTRAP is really still there on SEIZE?

-- 
Pedro Alves

Re: [PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[Oleg Nesterov]

On 09/09, Pedro Alves wrote:
>
> Right, but we end up with no way to make the tracee _not ever
> stop_ at execve if the tracer wants to, like you can make
> the tracee not ever stop on forks or syscalls, by not enabling
> the corresponding PTRACE_O_FOO or not PTRACE_SYSCALL.  Not
> specifying PTRACE_O_TRACEEXEC coupled with `SEIZE not stopping
> tracees for that magic SIGTRAP' got you that.  In a way, it looked to
> me to make the API a bit less ugly.

Agreed, perhaps we should do this.

> Anyway, could you please check (or Tejun please confirm)
> that that magic SIGTRAP is really still there on SEIZE?

It is.

Oleg.

Re: [PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[Denys Vlasenko]

On Fri, 2011-09-09 at 14:15 +0100, Pedro Alves wrote:
> On Friday 09 September 2011 13:28:55, Denys Vlasenko wrote:
> > On Fri, 2011-09-09 at 12:12 +0100, Pedro Alves wrote:
> > > On Thursday 08 September 2011 19:22:04, Denys Vlasenko wrote:
> > > > Make PTRACE_SEIZE set ptrace options specified in 'data' parameter
> > > >     
> > > > This can be used to close a few corner cases in strace where we get
> > > > unwanted racy behavior after attach, but before we have a chance
> > > > to set options (the notorious post-execve SIGTRAP comes to mind),
> > > 
> > > I'm still confused on why you're raising the SIGTRAP argument.  Did you see
> > > 
> > >  https://lkml.org/lkml/2011/9/8/7
> > > 
> > > From previous discussions, I understood that PTRACE_SEIZE _always_
> > > disables
> > > the post-execve SIGTRAP, so I don't believe that race actually exists.
> > > Or is that not the case?
> > 
> > 
> > I believe it is not the case. And I object to making it the case.
> 
> Well, if you'll remember, back in <https://lkml.org/lkml/2011/5/19/704> I
> raised this exact problem with that pesky racy post-execve SIGTRAP showing
> through on SEIZE, and Tejun a few replies later mentioned that the SIGTRAP
> was to be removed on SEIZE.   I'm sure it was said before even, but I'm
> not finding the emails now.
> 
> > My sense of taste says the approach "you need to use SEIZE to affect
> > feature <foo>" for various unrelated <foo> makes ptrace API ugly.
> 
> Yes, very much agreed!
> 
> > Especially that in this case, we already have a method in API
> > to suppress post-execve SIGTRAP.
> 
> Right, but we end up with no way to make the tracee _not ever
> stop_ at execve if the tracer wants to, like you can make
> the tracee not ever stop on forks or syscalls, by not enabling
> the corresponding PTRACE_O_FOO or not PTRACE_SYSCALL.  Not
> specifying PTRACE_O_TRACEEXEC coupled with `SEIZE not stopping
> tracees for that magic SIGTRAP' got you that.  In a way, it looked to
> me to make the API a bit less ugly.

This would be a _very_ minor improvement, so tiny it's not worth
bothering with. Let me show you the real-world code (part of strace
source) which skips over unneeded PTRACE_EVENT_EXEC:

                if ((status >> 16) != 0)
                        /* Ptrace event (we ignore all of them for now) */
                        goto restart_tracee_with_sig_0;

Yes. That is all.
It probably compiles into just two assembly instructions.

-- 
vda

Re: [PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[Pedro Alves]

On Friday 09 September 2011 17:55:41, Denys Vlasenko wrote:
> On Fri, 2011-09-09 at 14:15 +0100, Pedro Alves wrote:
> > On Friday 09 September 2011 13:28:55, Denys Vlasenko wrote:
> > > On Fri, 2011-09-09 at 12:12 +0100, Pedro Alves wrote:
> > > > On Thursday 08 September 2011 19:22:04, Denys Vlasenko wrote:
> > > > > Make PTRACE_SEIZE set ptrace options specified in 'data' parameter
> > > > >     
> > > > > This can be used to close a few corner cases in strace where we get
> > > > > unwanted racy behavior after attach, but before we have a chance
> > > > > to set options (the notorious post-execve SIGTRAP comes to mind),
> > > > 
> > > > I'm still confused on why you're raising the SIGTRAP argument.  Did you see
> > > > 
> > > >  https://lkml.org/lkml/2011/9/8/7
> > > > 
> > > > From previous discussions, I understood that PTRACE_SEIZE _always_
> > > > disables
> > > > the post-execve SIGTRAP, so I don't believe that race actually exists.
> > > > Or is that not the case?
> > > 
> > > 
> > > I believe it is not the case. And I object to making it the case.
> > 
> > Well, if you'll remember, back in <https://lkml.org/lkml/2011/5/19/704> I
> > raised this exact problem with that pesky racy post-execve SIGTRAP showing
> > through on SEIZE, and Tejun a few replies later mentioned that the SIGTRAP
> > was to be removed on SEIZE.   I'm sure it was said before even, but I'm
> > not finding the emails now.
> > 
> > > My sense of taste says the approach "you need to use SEIZE to affect
> > > feature <foo>" for various unrelated <foo> makes ptrace API ugly.
> > 
> > Yes, very much agreed!
> > 
> > > Especially that in this case, we already have a method in API
> > > to suppress post-execve SIGTRAP.
> > 
> > Right, but we end up with no way to make the tracee _not ever
> > stop_ at execve if the tracer wants to, like you can make
> > the tracee not ever stop on forks or syscalls, by not enabling
> > the corresponding PTRACE_O_FOO or not PTRACE_SYSCALL.  Not
> > specifying PTRACE_O_TRACEEXEC coupled with `SEIZE not stopping
> > tracees for that magic SIGTRAP' got you that.  In a way, it looked to
> > me to make the API a bit less ugly.
> 
> This would be a _very_ minor improvement, so tiny it's not worth
> bothering with. Let me show you the real-world code (part of strace
> source) which skips over unneeded PTRACE_EVENT_EXEC:
> 
>                 if ((status >> 16) != 0)
>                         /* Ptrace event (we ignore all of them for now) */
>                         goto restart_tracee_with_sig_0;
> 
> Yes. That is all.
> It probably compiles into just two assembly instructions.

WTH?  I'm talking about _not forcing the tracee to stop_.  Let
me repeat.  NOT FORCING THE TRACEE TO STOP.  And about not
needing to handle the magic unadorned SIGTRAP.
The magic unadorned post-exec SIGTRAP does not have `status & 0xff00'
set, it is not a ptrace event!

If we don't disable the magic SIGTRAP, there's no way for a
tracer to do a very non-invasive SEIZE, say, a GDB mode that
only cares to let the tracer run free to catch SIGSEGVs
in some child, while later on during the run, the user remembers
to set a breakpoint.  At that point the tracer needs to catch
exec events, so it'd enable TRACE_O_EVENTEXEC.  Getting rid of
the SIGTRAP gets rid of the spurious stops when TRACE_O_EVENTEXEC
is not enabled.

-- 
Pedro Alves

Re: [PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[Oleg Nesterov]

On 09/09, Pedro Alves wrote:
>
> WTH?  I'm talking about _not forcing the tracee to stop_.

The patch is trivial.

And personally I agree, this looks like a cleanup to me.

Oleg.

--- x/include/linux/ptrace.h
+++ x/include/linux/ptrace.h
@@ -199,7 +199,8 @@ static inline void ptrace_event(int even
 		ptrace_notify((event << 8) | SIGTRAP);
 	} else if (event == PTRACE_EVENT_EXEC && unlikely(current->ptrace)) {
 		/* legacy EXEC report via SIGTRAP */
-		send_sig(SIGTRAP, current, 0);
+		if (!(current->ptrace & PT_SEIZED))
+			send_sig(SIGTRAP, current, 0);
 	}
 }
 

Re: [PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[Denys Vlasenko]

On Friday 09 September 2011 19:09, Pedro Alves wrote:
> > This would be a _very_ minor improvement, so tiny it's not worth
> > bothering with. Let me show you the real-world code (part of strace
> > source) which skips over unneeded PTRACE_EVENT_EXEC:
> > 
> >                 if ((status >> 16) != 0)
> >                         /* Ptrace event (we ignore all of them for now) */
> >                         goto restart_tracee_with_sig_0;
> > 
> > Yes. That is all.
> > It probably compiles into just two assembly instructions.
> 
> WTH?  I'm talking about _not forcing the tracee to stop_.  Let
> me repeat.  NOT FORCING THE TRACEE TO STOP.

No need to shout.

execve is such a rare syscall the one extra stop on it is not
going to be a problem.

> And about not needing to handle the magic unadorned SIGTRAP.
> The magic unadorned post-exec SIGTRAP does not have `status & 0xff00'
> set, it is not a ptrace event!

What SIGTRAP? With PTRACE_O_TRACEEXEC, there is no SIGTRAP.

> If we don't disable the magic SIGTRAP, there's no way for a
> tracer to do a very non-invasive SEIZE, say, a GDB mode that
> only cares to let the tracer run free to catch SIGSEGVs
> in some child, while later on during the run, the user remembers
> to set a breakpoint.  At that point the tracer needs to catch
> exec events, so it'd enable TRACE_O_EVENTEXEC.  Getting rid of
> the SIGTRAP gets rid of the spurious stops when TRACE_O_EVENTEXEC
> is not enabled.

This part I don't understand.

(btw, PTRACE_O_TRACEEXEC, not TRACE_O_EVENTEXEC).

-- 
vda

Re: [PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[Pedro Alves]

On Friday 09 September 2011 21:03:10, Denys Vlasenko wrote:
> On Friday 09 September 2011 19:09, Pedro Alves wrote:

> No need to shout.

Sorry.

> execve is such a rare syscall the one extra stop on it is not
> going to be a problem.
> 
> > And about not needing to handle the magic unadorned SIGTRAP.
> > The magic unadorned post-exec SIGTRAP does not have `status & 0xff00'
> > set, it is not a ptrace event!
> 
> What SIGTRAP? With PTRACE_O_TRACEEXEC, there is no SIGTRAP.

But _without_ PTRACE_O_TRACEEXEC there is.  You've raised its
existence as justification for needing to be able to set
options directly on PTRACE_SEIZE.  Point is, if we don't get rid
of the SIGTRAP when PTRACE_O_TRACEEXEC is _not_ in effect, then
_everyone_ will always pass PTRACE_O_TRACEEXEC to SEIZE.
If that is true, you might as well make it default...  But
I'm claiming that a tracer may not want to see exec events at
all, so making it so that when you don't specify
PTRACE_O_TRACEEXEC, then you also don't get the magic SIGTRAP,
is more useful, and eliminates your justification too.  Oleg already
showed it's a super trivial patch too.  If you want to be
able to specify options directly on SEIZE, fine, I can see
that it is useful (of course gdb does the same "I still need to
set options on this child" dance as strace does).

> > If we don't disable the magic SIGTRAP, there's no way for a
> > tracer to do a very non-invasive SEIZE, say, a GDB mode that
> > only cares to let the tracer run free to catch SIGSEGVs
> > in some child, while later on during the run, the user remembers
> > to set a breakpoint.  At that point the tracer needs to catch
> > exec events, so it'd enable TRACE_O_EVENTEXEC.  Getting rid of
> > the SIGTRAP gets rid of the spurious stops when TRACE_O_EVENTEXEC
> > is not enabled.
> 
> This part I don't understand.

Say, you run the whole of gcc's testsuite under gdb, and
let it run until one of the children SIGSEGVs.  You do "gdb make; run".
Currently, all the children stop momentarily for fork/vfork/exec,
which slows down the run significantly (there are thousands of
forks/execs).  We should be able to only SEIZE the shell that runs
"make" (gdb runs the child through the shell, like "sh -c make"),
and let all its children run free, the least invasive way possible.
When a SIGSEGV happens, gdb can sync up about the process that crashed
from /proc.

We can't get rid of the magic SIGTRAP on PTRACE_ATTACH/PTRACE_TRACEME
for backwards compatibility reasons, but SEIZE is new.

> (btw, PTRACE_O_TRACEEXEC, not TRACE_O_EVENTEXEC).

Thanks.

-- 
Pedro Alves

Re: [PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[Denys Vlasenko]

On Saturday 10 September 2011 13:19, Pedro Alves wrote:
> On Friday 09 September 2011 21:03:10, Denys Vlasenko wrote:
> > execve is such a rare syscall the one extra stop on it is not
> > going to be a problem.
> > 
> > > And about not needing to handle the magic unadorned SIGTRAP.
> > > The magic unadorned post-exec SIGTRAP does not have `status & 0xff00'
> > > set, it is not a ptrace event!
> > 
> > What SIGTRAP? With PTRACE_O_TRACEEXEC, there is no SIGTRAP.
> 
> But _without_ PTRACE_O_TRACEEXEC there is.  You've raised its
> existence as justification for needing to be able to set
> options directly on PTRACE_SEIZE.

It was an example. There may be other options with similar
problem of "we want to enable new behavior ASAP, without
waiting fro the first ptrace-stop".

> Point is, if we don't get rid 
> of the SIGTRAP when PTRACE_O_TRACEEXEC is _not_ in effect, then
> _everyone_ will always pass PTRACE_O_TRACEEXEC to SEIZE.

Yes, that's the nature of many options: they are fixing
ptrace quirks, and therefore newer programs which know about
these options will _always_ use them. For example, should we
also unconditionally enable PTRACE_O_TRACESYSGOOD?



> > > If we don't disable the magic SIGTRAP, there's no way for a
> > > tracer to do a very non-invasive SEIZE, say, a GDB mode that
> > > only cares to let the tracer run free to catch SIGSEGVs
> > > in some child, while later on during the run, the user remembers
> > > to set a breakpoint.  At that point the tracer needs to catch
> > > exec events, so it'd enable TRACE_O_EVENTEXEC.  Getting rid of
> > > the SIGTRAP gets rid of the spurious stops when TRACE_O_EVENTEXEC
> > > is not enabled.
> > 
> > This part I don't understand.
> 
> Say, you run the whole of gcc's testsuite under gdb, and
> let it run until one of the children SIGSEGVs.  You do "gdb make; run".
> Currently, all the children stop momentarily for fork/vfork/exec,
> which slows down the run significantly (there are thousands of
> forks/execs).

I doubt about "significantly". fork and exec are heavy syscalls
(they trash entire L1 data cache on today's CPUs), a ptrace stop
on top of that is perhaps 10% slowdown _of the syscall_,
about a few % slowdown overall.


> We should be able to only SEIZE the shell that runs 
> "make" (gdb runs the child through the shell, like "sh -c make"),
> and let all its children run free, the least invasive way possible.

True.

> When a SIGSEGV happens, gdb can sync up about the process that crashed
> from /proc.

It doesn't need to do even that - but probably will, gdb code is said
to be quite complex. I think current code will require auto-attach stops
in forked children anyway (for parent-child accounting and such),
and it will require a serious rewrite to get rid of that requirement.

-- 
vda

Re: [PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[Pedro Alves]

On Saturday 10 September 2011 12:40:16, Denys Vlasenko wrote:
> On Saturday 10 September 2011 13:19, Pedro Alves wrote:
> > On Friday 09 September 2011 21:03:10, Denys Vlasenko wrote:
> > > execve is such a rare syscall the one extra stop on it is not
> > > going to be a problem.
> > > 
> > > > And about not needing to handle the magic unadorned SIGTRAP.
> > > > The magic unadorned post-exec SIGTRAP does not have `status & 0xff00'
> > > > set, it is not a ptrace event!
> > > 
> > > What SIGTRAP? With PTRACE_O_TRACEEXEC, there is no SIGTRAP.
> > 
> > But _without_ PTRACE_O_TRACEEXEC there is.  You've raised its
> > existence as justification for needing to be able to set
> > options directly on PTRACE_SEIZE.
> 
> It was an example. There may be other options with similar
> problem of "we want to enable new behavior ASAP, without
> waiting fro the first ptrace-stop".

As I said, there may be other examples, and I can see that
options-on-SEIZE is useful (it even makes the tracer less invasive,
as it doesn't force an interrup/stop for setting options), but as
I have been saying, that particular example IMO should be fixed
some other way.

(just-options-on-SEIZE scares me in terms of future expansion,
as it assumes only bitflags will ever be necessary.)

> > Point is, if we don't get rid 
> > of the SIGTRAP when PTRACE_O_TRACEEXEC is _not_ in effect, then
> > _everyone_ will always pass PTRACE_O_TRACEEXEC to SEIZE.
> 
> Yes, that's the nature of many options: they are fixing
> ptrace quirks, and therefore newer programs which know about
> these options will _always_ use them. 

I think it's better to work on a case by case basis, and not
claim all of them fix quirks.  In my mind, there are clearly different
sets of ptrace options.  PTRACE_O_TRACECLONE|FORK|VFORK|EXEC
forces _extra_ stops in the tracees that would not happen
if the tracee wasn't being traced in the first place (ignore the
magic exec SIGTRAP).

> For example, should we
> also unconditionally enable PTRACE_O_TRACESYSGOOD?

PTRACE_O_TRACESYSGOOD changes only the status reported by wait
on syscalls.  To get a syscall stop/SIGTRAP, you need to use
PTRACE_SYSCALL in the first place, and you can only do that
when the tracee is already ptrace stopped.  At that point, you
can just do PTRACE_O_TRACESYSGOOD yourself.  Nothing is seriously
lost if we don't enable it by default.  But it's fine with me
to enable it by default.

> I doubt about "significantly". fork and exec are heavy syscalls
> (they trash entire L1 data cache on today's CPUs), a ptrace stop
> on top of that is perhaps 10% slowdown _of the syscall_,
> about a few % slowdown overall.

There's the userland rountrip and context switches you get away
without.  With RT in the picture, it may be super important -- who
knows.

It's super easy to do it right.  IMO, let's just do it.

> > When a SIGSEGV happens, gdb can sync up about the process that crashed
> > from /proc.
> 
> It doesn't need to do even that - but probably will, gdb code is said
> to be quite complex. 

I happen to know gdb's code quite well.  ;-)

> and it will require a serious rewrite to get rid of that requirement.

Maybe, and it'd need a couple extra ptrace features (e.g., like being able
to make the tracer not see stops for signals it is not interested in.
Red Hat discussed some of those desirable ptrace features for making
gdb less invasive on the archer@ list a while ago.) but I'm
claiming it's better to leave the door open.

-- 
Pedro Alves

Re: [PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[Pedro Alves]

On Saturday 10 September 2011 13:12:38, Pedro Alves wrote:

> (just-options-on-SEIZE scares me in terms of future expansion,
> as it assumes only bitflags will ever be necessary.)

The more I think of this, the more I think we could do this
some other way --- why don't we allow setting the default
options _on the ptracer_, and then tracee's inherit those
options from the ptracer-set-of-default-options if their
parent is not currently ptraced by own tracer?  That is, you'd make
the ptracer set options on itself:

options = PTRACE_O_TRACESYSGOOD | PTRACE_O_TRACECLONE|FORK|VFORK|EXEC ...;
ptrace(PTRACE_SETOPTIONS, gettid(), 0, options);

And then, in all of

 ptrace(PTRACE_ATTACH, foo_pid, 0, 0);
 ptrace(PTRACE_TRACEME, 0, 0, 0);
 ptrace(PTRACE_SEIZE, foo_pid, 0, 0);

you'd end up with the tracee already with your chosen options set.

Clones, vforks and forks that are auto-attached would still inherit their
options from their parent, just like today.

Some pro points:

- works for PTRACE_ATTACH AND PTRACE_TRACEME too.

- most ptracers on earth will only need to set options once.

- backwards compatible.  The "default ptrace options" default
to 0, so old tracers on new kernels will still behave the same.

- future proof.  If some other PTRACE_SET_NEAT_OPTION ptrace
command appears in the future, we can make it work the same
way, instead of getting stuck with PTRACE_SEIZE's `data'
already being taken...

- if ptrace(PTRACE_SETOPTIONS, gettid(), 0, 0) works, then
we can stop gdb (and probably other tracers) from forking a new
child just to check if PTRACE_O_TRACEFOO works before actually
attaching/spawning the process of interest.
(see code around linux_supports_tracefork_flag at
<http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/linux-nat.c?rev=1.215&content-type=text/x-cvsweb-markup&cvsroot=src>)

Cons:

- is there any real con?

Note the default ptrace options would _not_ conflict with
the ptracer's own ptrace options in case the ptracer itself
is being ptraced.

WDYT?

-- 
Pedro Alves

Re: [PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[Indan Zupancic]

Hello,

On Sat, September 10, 2011 17:36, Pedro Alves wrote:
> On Saturday 10 September 2011 13:12:38, Pedro Alves wrote:
>
>> (just-options-on-SEIZE scares me in terms of future expansion,
>> as it assumes only bitflags will ever be necessary.)
>
> The more I think of this, the more I think we could do this
> some other way --- why don't we allow setting the default
> options _on the ptracer_, and then tracee's inherit those
> options from the ptracer-set-of-default-options if their
> parent is not currently ptraced by own tracer?  That is, you'd make
> the ptracer set options on itself:
>
> options = PTRACE_O_TRACESYSGOOD | PTRACE_O_TRACECLONE|FORK|VFORK|EXEC ...;
> ptrace(PTRACE_SETOPTIONS, gettid(), 0, options);

Please just use a zero pid to set options on yourself, that's easier for
everyone.


> And then, in all of
>
>  ptrace(PTRACE_ATTACH, foo_pid, 0, 0);
>  ptrace(PTRACE_TRACEME, 0, 0, 0);
>  ptrace(PTRACE_SEIZE, foo_pid, 0, 0);
>
> you'd end up with the tracee already with your chosen options set.
>
> Clones, vforks and forks that are auto-attached would still inherit their
> options from their parent, just like today.
>
> Some pro points:
>
> - works for PTRACE_ATTACH AND PTRACE_TRACEME too.
>
> - most ptracers on earth will only need to set options once.
>
> - backwards compatible.  The "default ptrace options" default
> to 0, so old tracers on new kernels will still behave the same.
>
> - future proof.  If some other PTRACE_SET_NEAT_OPTION ptrace
> command appears in the future, we can make it work the same
> way, instead of getting stuck with PTRACE_SEIZE's `data'
> already being taken...
>
> - if ptrace(PTRACE_SETOPTIONS, gettid(), 0, 0) works, then
> we can stop gdb (and probably other tracers) from forking a new
> child just to check if PTRACE_O_TRACEFOO works before actually
> attaching/spawning the process of interest.
> (see code around linux_supports_tracefork_flag at
> <http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/linux-nat.c?rev=1.215&content-type=text/x-cvsweb-markup&cvsroot=src>)

The forking a new child just to check the option is needed because aparently
some old ptrace version didn't return an error for unknown options (according
to the comment in that code), so trying it out is the only reliable way of
being sure if options are supported or not. Otherwise no extra forking would
be needed anyway.

> Cons:
>
> - is there any real con?
>
> Note the default ptrace options would _not_ conflict with
> the ptracer's own ptrace options in case the ptracer itself
> is being ptraced.
>
> WDYT?

I agree that this would probably be better than setting options at SEIZE
time.

Greetings,

Indan

Re: [PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[Indan Zupancic]

Hello,

On Sat, September 10, 2011 14:12, Pedro Alves wrote:
>
> I happen to know gdb's code quite well.  ;-)

In that case, could you explain why gdb isn't using WCONTINUED
notifications for selective thread wakeup instead of trying to
achieve the same at group stop time?

Background thread: https://lkml.org/lkml/2011/9/7/294

Basically the only argument against making ptrace not automatically
continue group stopped tasks was that gdb uses that to resume one
thread instead of all. But it does this by letting the tasks hang in
trapped state at group stop notification time, breaking normal group
stops. If it would instead use the group continuation notification,
gdb would never need to directly interfere with group stops.

I find PTRACE_LISTEN an ugly solution to this problem and am searching
for something nicer.

Greetings,

Indan

Re: [PATCH v3] Make PTRACE_SEIZE set ptrace options specified in 'data'

[Tejun Heo]

On Thu, Sep 08, 2011 at 08:22:04PM +0200, Denys Vlasenko wrote:
> Make PTRACE_SEIZE set ptrace options specified in 'data' parameter
>     
> This can be used to close a few corner cases in strace where we get
> unwanted racy behavior after attach, but before we have a chance
> to set options (the notorious post-execve SIGTRAP comes to mind),
> and removes the need to track "did we set opts for this task" state
> in strace internals.
>     
> While we are at it:
>     
> Make it possible to extend SEIZE in the future with more functionality
> by passing non-zero 'addr' parameter.
> To that end, error out if 'addr' is non-zero.
> PTRACE_ATTACH did not (and still does not) have such check,
> and users (strace) do pass garbage there... let's avoid repeating
> this mistake with SEIZE.
>     
> Set all task->ptrace bits in one operation - before this change,
> we were adding PT_SEIZED and PT_PTRACE_CAP with task->ptrace |= BIT ops.
> This was probably ok (not a bug), but let's be on a safer side.
> 
> Changes since v2: use (unsigned long) casts instead of (long) ones,
> move PTRACE_SEIZE_DEVEL-related code to separate lines of code.
> 
>     
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun