Re: [DTrace-devel] [PATCH 1/4] Tweak self-armouring

[Nick Alcock <nick.alcock@oracle.com>]

On 25 Oct 2024, Kris Van Hees via DTrace-devel spake thusly:

> On Fri, Oct 04, 2024 at 12:43:52AM -0400, eugene.loh@oracle.com wrote:
>> From: Eugene Loh <eugene.loh@oracle.com>
>> 
>> Commit ea592d60 ("proc: improve armouring against self-grabs") has no
>> tests, but it breaks test/unittest/usdt/tst.multitrace.sh.  The patch
>> makes the following change in libdtrace/dt_proc.c dt_proc_control():
>> 
>>         if ((Psystem_daemon(dpr->dpr_pid, dtp->dt_useruid,
>>                     dtp->dt_sysslice) > 0) ||
>>     -       ((tracer_pid != 0) &&
>>     -        (tracer_pid != getpid())) ||
>>     +       (tracer_pid == getpid()) ||
>>     -       (dpr->dpr_pid == getpid()))
>>     +       (tgid == getpid()))
>>                 noninvasive = 2;
>> 
>> We change a
>>         (tracer_pid != getpid())
>> into a
>>         (tracer_pid == getpid())
>> 
>> Changing that == back into a != makes tst.multitrace.sh work again.
>> 
>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>
> Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

It's better than what I was doing (which was definitely wrong), but it
could still be improved.

>> diff --git a/libdtrace/dt_proc.c b/libdtrace/dt_proc.c
>> index a052abbac..7c3eb2a24 100644
>> --- a/libdtrace/dt_proc.c
>> +++ b/libdtrace/dt_proc.c
>> @@ -954,7 +954,7 @@ dt_proc_control(void *arg)
>>  
>>  			if ((Psystem_daemon(dpr->dpr_pid, dtp->dt_useruid,
>>  				    dtp->dt_sysslice) > 0) ||
>> -			    (tracer_pid == getpid()) ||
>> +			    (tracer_pid != getpid()) ||
>>  			    (tgid == getpid()))
>>  				noninvasive = 2;

What the previous code was saying was "if the traced process is a system
daemon, or is being debugged by someone else, or is ourself, do not do
an invasive grab"; in the one case because stopping systemd or the
system logger, even briefly, seems a like a bad idea, and in the other
two cases because you can't be debugged by two people at once and
because stopping yourself is an obvious deadlock.

We want to check if it's ourself in two ways: firstly, if the tracer PID
is our own PID, we are obviously ourself (this is the process-monitoring
thread itself); secondly, if the tracer PID's task group leader is our
own thread's task group leader, this is the main thread asking.

Unfortunately that's not what the original code was doing (the
tracer_pid thing was indeed wrong, we were saying "if we are debugged by
ourself, go noninvasive, but not if we're debugged by someone else"), so
your change was good as far as it goes... but if the process-monitoring
thread ever decides to monitor itself (perfectly possible with
suffiently-wildcardy probes and a ustack() or something), we'll
deadlock -- and unfortunately the tracer_pid is 0 when the process is
*not* being traced, so what that change is doing is going noninvasive
for *everyone, always*. That's what the original's

       ((tracer_pid != 0) &&
        (tracer_pid != getpid())) ||

was all about: if you flip the latter conditional away from ==, you
*must* put the != 0 back in.

Something we're also not covering but should is that we want to fail
when asked to do long-lived grabs of ourself or a process being debugged
by someone else, or for that matter PID 1, *always*: long-lived grabs
mean we rely on getting process death notifications, etc, and
noninvasive grabs can't do that, so we *should* fail.

Also, we should define "ourself" more widely: we should really refuse
ptrace grabs on *any* DTrace thread, because the amount of proxying and
callbacks back and forth makes deadlocks horribly likely if, say, the
main thread gets invasively grabbed.

Fix coming shortly (before you get back and read this, assuming you're
guzzling turkey now like you should be).

-- 
NULL && (void)