Date:	Wed, 7 Sep 2011 19:15:04 +0200
From:	"Indan Zupancic" <indan@....nu>
To:	"Denys Vlasenko" <vda.linux@...glemail.com>
Cc:	"Denys Vlasenko" <dvlasenk@...hat.com>,
	"Oleg Nesterov" <oleg@...hat.com>, "Tejun Heo" <tj@...nel.org>,
	linux-kernel@...r.kernel.org
Subject: Re: RFC: PTRACE_SEIZE needs API cleanup?

Hello,

The end of this thread is in sight.

On Wed, September 7, 2011 04:34, Denys Vlasenko wrote:
> On Tuesday 06 September 2011 19:08, Indan Zupancic wrote:
>> >> > Problem. Now we interfere with SIGSTRAPs. Yes, there are users who want
>> >> > to be able to see real SIGTRAPs they send to the program,
>> >> > or ones generated by, say, int3 instruction on x86.
>> >>
>> >> But SIGTRAP is ours, ptrace already sends SIGTRAPS at execve.
>> >
>> > ...whcih causes problems, and therefore we have PTRACE_O_TRACEEXEC to suppress
>> > this idiotic post-execve SIGTRAP.
>>
>> The SIGTRAP is still there, there is just an extra bit to distinguish
>> it from normal SIGTRAPs.
>
> No. SIGTRAP is no longer 'there'. The PTRACE_EVENT_EXEC happens in a different
> place: after syscall-entry, but before syscall-exit. Also, it can't be blocked
> by signal mask. Old-style post-execve SIGTRAP happens after syscall exit,
> and can be blocked by signal mask.

That would be inconvenient, to say the least.

> IOW: PTRACE_EVENT_EXEC is not a signal at all. It just happens to have
> WSTOPSIG(status) == SIGTRAP.

Is the new fake SIGTRAP ever passed on to the tracee? Or is "data" ignored
for PTRACE_EVENT_EXEC? Or is SIGTRAP passed on if set in data? Just curious,
it doesn't really matter.

More generic question is if setting data to anything can generate a new
signal, or if it is only used to block, pass or change existing signals?

>> It doesn't really matter if it's SIGSTOP with an extra flag, or SIGTRAP
>> with an extra flag set. But SIGTRAP is already used this way for fork
>> and exec,
>
> SIGTRAP is used for fork? I don't think so.

Well, the thing that looks like a SIGTRAP but isn't, which you get when
setting PTRACE_O_TRACEFORK. The manpage is confusing because it says the
child gets the SIGTRAP | PTRACE_EVENT_FORK << 8, but it means the parent
pracee that does the fork. The child tracee gets a SIGSTOP. It would be
more consistent if the child would get a SIGTRAP | PTRACE_EVENT_FORK << 8
or variant too. Otherwise the tracer can't know for sure whether to pass
on the SIGSTOP or not. (Now it doesn't matter, but it does if group stops
get fixed.)

>> so using it always instead of sometimes SIGSTOP would be more
>> consistent. It's also clearer what to do with the SIGTRAP as tracer: Block
>> it so the tracee doesn't see it. With SIGSTOP it's less clear what to do.
>> In current trace it doesn't matter what you do, in practice, and that's also
>> what the manpage says. So using SIGTRAP with an extra bit set is cleaner.
>
>
>
>> >> Tracers that want to get group stop notifications will set WSTOPPED and get
>> >> that information that way. But as ptrace won't generate any SIGSTOPs, they
>> >> don't have to use GETSIGINFO to know if it came from ptrace or not: It never
>> >> does.
>> >
>> > Drats. In your proposal, if I'd set WSTOPPED, I will get group-stops, right?
>>
>> Group stop notifications, yes, that was the idea.
>>
>> > How in your proposed solution can I "restart and cancel group-stop" after
>> > group-stop? And how can I "restart and wait for a SIGCONT"?
>>
>> Not sure what you mean with restart, just send another SIGSTOP?
>>
>> Blocking a group stop happens by blocking the SIGSTOP delivery to the tracee.
>
> Here we go again.
>
> I will write it in bold.
>
> YOU MUST NOT BLOCK DELIVERY OF STOPPING SIGNALS! IT'S ***WRONG***!

Sorry, relax! I meant the SIGSTOP that isn't a SIGSTOP, but looks like a
SIGSTOP the tracer gets as group notification. Not the SIGSTOP causing
the group stop. I hope. Either that, or I should have read your whole
email before replying, or going back to this bit to fix it, when I finally
understood the problem of trying to manage group stops at SIGSTOP time.
I can't remember, it's getting a bit fuzzy.

> I already told you numerous times that SIGTSTP, SISTTIN and SIGTTOU
> can have non-default handlers. Do you understand what this means?
> I guess you don't, since you never replied with the explanation how you
> propose to with this problem.

I thought I admitted that using SIGSTOP wasn't sufficient and that group
stop notifications are indeed often necessary, exactly because of these
issues. But perhaps I should have said explicitly, that yes, I get it:
Using anything else than group stop notifications is unreliable and
doesn't always work.

Keep in mind that I never proposed to get rid of group stop, only that
programs should have an option to not get them, so group stops and
continuations just work for them.

> I also told you that SIGSTOP/SIGCONT are *process-wide*. Meaning
> that if you stop on SIGSTOP delivery to one thread of a multithreaded process
> and will not restart it via PTRACE_CONT, then THIS PARTICULAL THREAD
> will stop running, yes, but all other threads WILL NOT STOP.

Okay, now it's my time to use caps:

THAT DOES NOT MATTER _IF_ ALL THREADS GET A SIGSTOP/SIGCONT.

If that would be the case, then it doesn't matter if it's process-wide or
not, because the tracer would get a chance to grab all threads. But sadly,
this isn't the case, as I mistakenly thought with my sloppy testing.

>> Each tracee gets a SIGSTOP ptrace event, it's not like only one gets it and
>> that controls the state of the whole group.
>
> It's exactly the opposite. SIGSTOP signal-delivery-stop happens on *one* thread,
> and if tracer does not suppress it, then all threads go into group-stop.
>
>> Same for SIGCONT. I'll have to
>> double check this, but I think this is how it works.
>
> Please do double-check.

I did. You are right, I confused the SIGSTOP after fork/clone my program gets
because it sets PTRACE_O_TRACEFORK/CLONE for a normal SIGSTOP notification,
so I always saw one SIGSTOP for every thread and forked task (I tested both).

And in that case, yes of course it's idiotic to try to control group stop
my misguided way.

>> It's probably best to not change the current model for group stop notifications
>> (when requested via WSTOPPED), as that is confusing, even if the new behaviour
>> is better (which it probably isn't).
>>
>> You're right that if a tracee requests group stop notifications, it probably
>> wants to be able to poke around too sometimes, and then a ptrace slightly
>> entwined with group stops is preferred. But it's not elegant. :-/
>
> How kernel knows that tracer is done with poking around, and now it's ok to
> wake up on SIGCONT?

Well, if you don't change the model, the same as now, of course.

But if I'd ignore backward compatibility and would create a new API,
I'd probably make ptrace trap any task when doing a ptrace operation
on it, so you could issue a ptrace at any time instead of only on
specific moments. Once grabbed, you have to let it go with PTRACE_CONT.
So to answer this hypotetical question, in that case, if no ptrace
operation was done, no action would be needed. For the kernel there
would be a clear distinction between group-stopped and ptrace-trapped.
It's too late for any such changes now though.

>> Any reason why this can't work?
>
> Gosh......... I already told you why it won't work.

Keep in mind I saw SIGSTOPs for all threads in my tests, so thinking
that all threads get a SIGCONT too (when stopped) isn't that stupid.

> SIGCONT is magic. It wakes all group-stopped threads, and this effect
> can't be blocked *even via ptrace*. This means that restarting only one
> thread via SIGCONT is impossible.

Magic indeed. I guess I got sucked too much into "a thread is just a process"
thing. I assumed the group effect was implemented by automatically propagating
group-wide signals when needed in a semi-hidden way.

>> Letting PTRACE_CONT
>> not resume stopped tasks would fix normal group stops without any changes.
>
> Exactly our line of thought before gdb people shot down our idea.

Sad, and I'm running out of alternatives. I've only two left.

>> Only programs that want to resume single threads while holding the rest
>> need to change their code slightly to let the tasks hang in SIGCONT instead
>> of group stop notification.
>
>> >> Switching off group-stop notifications makes it
>> >> only easier to use ptrace when not specifically interested in group stops.
>> >
>> > Solution in the search of a problem.
>>
>> You're probably right that it's not worth the trouble. I'm fine if tracers
>> always get a group stop notification, but if so, there should be an option
>> that makes PTRACE_CONT not continue group stopped tasks.
>
> Yes. Exactly. That "option" is called PTRACE_LISTEN.

Hm, but PTRACE_LISTEN isn't a fire and forget thing, it seems too complicated
and stateful to use easily. I'll take another look at it.

I want to put the burden on gdb and other users who want to mess with group
stop state.

>
>> What I want to avoid is adding extra specific code in the tracer when all
>> it wants is to be transparent to group stops.
>
> You mean, you want to avoid this atrocious bloat? -
>
> -	ptrace(PTRACE_CONT, pid, 0, sig);
> +	int is_stopped = .... determine whether it's a group_stop...
> +	if (is_stopped) ptrace(PTRACE_LISTEN, pid, 0, 0);
> +	else            ptrace(PTRACE_CONT, pid, 0, sig);
>
> Somehow this doesn't look like big problem to me.

If that was all, then yes, it would be fine.

Okay, silly question:

If SIGCONT on one thread continues all, then you have no guarantee that
the thread you did PTRACE_LISTEN on will get the SIGCONT. Hence you have
no chance to do a PTRACE_SYSCALL or other variant of PTRACE_CONT before
the thread runs.

But reading the patch description it seems PTRACE_LISTEN is more subtle,
it doesn't really run a task, it puts it in quasi running stat, and the
tracer is supposed to keep track of all tasks in the group and continue
the PTRACE_LISTENed task later on or something?!

If the thread runs after a SIGCONT on another task, then PTRACE_LISTEN
is broken. If the task doesn't run ("quasi running" state), then what
is needed to make it run and how the hell does the tracer know when to
press the magic button? (Keep in mind that the task getting the SIGCONT
may not be traced.) As far as I know there are no group continuation
notification events. If there are, then those could be used instead of
SIGCONT in my previous silly proposal, and unlike those, it would work.

So all in all it seems that instead of PTRACE_LISTEN, an extra argument
for PTRACE_CONT/PTRACE_SYSCALL/PTRACE_EMU/etc is much better. This flag
would always work, so no need for if checks and so on. The flag would
say to not continue a group stopped task, e.g. PTRACE_MAYSTOP. This would
probably have to bet set in "addr".

Then it would become:

-	ptrace(PTRACE_CONT, pid, 0, sig);
+	ptrace(PTRACE_CONT, pid, PTRACE_MAYSTOP, sig);

Advantage is that this would be truly the only code change needed, no
messing around with state, except if you want to take full control
like gdb needs sometimes. (But not more than currently.)

>
>> One way is to not get the group stop notifications, so the tracer doesn't
>> accidentally continues the stopped tasks. To me this seems the simplest
>> solution, but it has the problem that it only works for tracers not
>> interested in group stop. To also make it work with group stops, my idea
>> was to let PTRACE_CONT not continue stopped tasks. Instead of trapping
>> tasks at the beginning of group stop, they can trap the tasks at group
>> stop exit time, which is much more natural anyway (because it is group
>> continuation that gdb wants to prevent, not group stop).
>>
>> With PTRACE_LISTEN the tracee has to handle group stops specially, then,
>> instead of doing nothing for group stop notifications, it has to use
>> PTRACE_LISTEN to emulate group stop and continuation for the tracee. No
>> one is going to bother to add code for this when they're not interested
>> in group stops, especially not if the code is portable at all.
>
> Portable code using ptrace? I pity those who need to maintain that...

Tell me about it, I saw strace's code. That's why I limit my ptrace code to
only Linux, and that's hard enough already. (Idea being that a 2.6 compiled
binary works on a 2.4 kernel.)

>
>> It's just not worth the trouble. The behaviour is too strange and different with
>> PTRACE_LISTEN.
>
> I disagree. See code fragment above. It's simple.

It seems more complicated than that, but I hope I'm missing something again.

Actually, what I'm missing is WCONTINUED. Why the hell is gdb not using that
to control thread running? If it did, it won't have to ask the user what
to do at group stop time, it could automatically do the right thing at group
continuation time. Or is the tracee not in trapped state when WCONTINUED
happens, but it is when WSTOPPED happens? If gdb got WCONTINUED, I don't see
a reason why gdb would ever need to control group stop state via PTRACE_CONT.

What was the gdb's people argument to shoot down this idea?

>
>> Compare that to:
>>
>>  "PTRACE_O_FOO makes PTRACE_CONT/PTRACE_SYSCALL/etc. not automatically
>>   continue a group stopped process, so that SIGSTOP and SIGCONT work for
>>   ptraced processes. Tracers that want to control thread continuation
>>   can do that at SIGCONT time."
>
> This doesn't allow to make a decision whether to honor or ignore group-stop
> *after* it happened.

Gdb also wants to control group stop beginning? Does it ever wants to block
group stop from happening, and should it have the right if it doesn't trace
all tasks in the group? SIGSTOP was supposed to be not blockable.

Currently the tracer gets a group stop notification, which means, I assume,
that a group stop happened. How can gdb prevent a group stop from happening?

I think all it can do is either end the group stop or let the tasks hang in
trapped state. With the new options it would also be able to let them go into
a real group stop.

So all in all I propose the following:

- Add the PTRACE_MAYSTOP argument to 'addr' for continuating operations like
  PTRACE_CONT/PTRACE_SYSCALL/etc. When set, ptrace won't end a group stop
  and the task can go from trapped into stopped state.

This has the same function as PTRACE_LISTEN, except it's much simpler to use
and explain. If 'addr' is currently ignored, then programs can set this argument
without worrying about ptrace returning an error and it's backward compatible.

An alternative is to add an option to make PTRACE_MAYSTOP the default. And add
a PTRACE_DO_CONT instead, if WCONTINUED isn't good enough for gdb. Or say that
a SIGCONT passed at group notification time restores the old behaviour.

What do you think?

Greetings,

Indan


--
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