Re: [RFA] Factorize killing the children in linux-ptrace.c, and fix a 'process leak'.

[Philippe Waroquiers <philippe dot waroquiers at skynet dot be>]

On Sat, 2018-11-10 at 18:29 +0000, Pedro Alves wrote:
> There's some long and ugly history around PTRACE_KILL vs SIGKILL.
> See gdb/linux-nat.c kill_wait_one_lwp and kill_one_lwp, and the comments
> within those functions.  I don't know whether the kernels those were for
> are relevant any more.  Likely using git blame and finding the corresponding
> patch posts would shed some more light.  Whatever we do, it'd be nice to
> make gdb/linux-nat.c use the unified code as well.

I have done some digging in the source code and searched some
more info using git blame.
=> I found 7 different linux logics related to kill(SIGKILL)
and ptrace(PTRACE_KILL).
I found only one identification of kernels giving problems.
ChangeLogs/commit msg were not really giving much
background information : these are telling what is being changed,
not why it is being changed.
linux-low.c has the most interesting comments explaining
the kill related problems.

Below is some updated text with the found info (sorry, you
will have to re-read some parts already in the RFA).

I have to admit that a 'linux-ptrace.c only factorization'
as suggested in the RFA (at least as a first step, till we have
validated with it that a simpler kill logic is ok on various platforms)
is somewhat less frightening me than reducing all 7 different
logics to a single one.
(there is a table at the end of this mail that summarises all
7 logics).

I must say that all this is killing me :).
Maybe I would feel better if I would just have added my own
local new 8th kill logic where I need it :).

Philippe


nat/linux-ptrace.c has 3 different logics to kill a child process.
So, this patch factorizes killing a child in the function kill_child.

The 3 different logics are:
* linux_ptrace_test_ret_to_nx is calling both kill (child, SIGKILL)
  and ptrace (PTRACE_KILL, child, ...), and then is calling once
  waitpid.
* linux_check_ptrace_features is calling ptrace (PTRACE_KILL, child, ...)
  + my_waitpid in a loop, as long as the waitpid status was WIFSTOPPED.
* linux_test_for_tracefork is calling once ptrace (PTRACE_KILL, child, ...)
  + my_waitpid.

The linux ptrace documentation indicates that PTRACE_KILL is deprecated,
and tells to not use it, as it might return success but not kill the tracee.
The documentation indicates to send SIGKILL directly.

I suspect that linux_ptrace_test_ret_to_nx calls both kill and ptrace just
to be sure ...
git blame indicates that the call to kill(SIGKILL) was added in addition to the
pre-existing ptrace PTRACE_KILL. The commit log also indicates to ignore
PTRACE_KILL errors. But there is no explanation why adding the SIGKILL call
was needed, and why PTRACE_KILL was kept.

I suspect that linux_check_ptrace_features calls ptrace in a loop
to bypass the PTRACE_KILL limitation.
git blame indicates that this function was heavily based on
linux-nat.c:linux_test_for_tracefork (which was moved to linux-ptrace.c,
see next paragraph). I found no log/comments about the killing subtilities.

linux_test_for_tracefork seems to not handle the PTRACE_KILL
limitation/has not been updated like linux_check_ptrace_features
(which has a loop).

Also, 2 of the 3 logics are calling my_waitpid, which seems better,
as this is protecting the waitpid syscall against EINTR.

We also find a bunch of other kill logics in gdbserver/linux-low.c and
linux-nat.c and linux-fork.c.

gdbserver/linux-low.c has linux_kill_one_lwp, that has an extensive
comment explaining why it uses both PTRACE_KILLME and SIGKILL
Note however that the gdbserver code does the SIGKILL using
'syscall (__NR_tkill', not using 'kill (... SIGKILL).
gdbserver/linux-low.c does a loop calling both PTRACE_KILLME and SIGKILL,
but has a comment telling that the loop is most likely unnecessary.

Also, gdbservers/linux-low.c handles the case of waiting for "clone" children,
indicating that __WALL depends on SIGCHLD, and killing a stopped process
does not generate this signal.
It also indicates it supports old kernels not providing clone(CLONE_THREAD),
by sending a SIGKILL for each thread (not only one for the process).
There is also a comment telling that only PTRACE_KILL was used for years,
and that paranoia tells to do both SIGKILL and PTRACE_KILL, in case
PTRACE_KILL might work better on old kernels (the comment says it is dubious
there is such an old kernel, but explains that this is the paranoia).
linux-low.c also has a special logic to kill the thread that has its
lwpid == pid, because killing the parent before the children can
cause zombies on kernels at least 2.6.0-test7 ... 2.6.8-rc4.

linux-nat.c first uses SIGKILL, and then PTRACE_KILL. A comment (from 2011)
is telling that PTRACE_KILL may resume the inferior (so the need for SIGKILL).
Another comment (also 2011) tells that some kernels ignore even SIGKILL
for processes under ptrace (so the reason for keeping the PTRACE_KILL).
Note that linux-nat.c also loops (like linux-low.c), but indicates
the loop is needed as the linux kernel sometimes fails to kill a thread
after PTRACE_KILL.
To the contrary of linux-low.c, linux-nat.c uses waitpid (__WALL) rather
than do 2 successive waitpid calls like linux-low.c.
linux-nat.c also stops all threads before killing them, with a comment
explaining that this is needed to have PTRACE_KILL working.

linux-fork.c has also its own duplicated kill logics.
One uses (only) SIGKILL (with a comment telling this always works,
contrary to PTRACE_KILL).
The other logic (related to killing checkpoints) only uses PTRACE_KILL.
This logic does not call waitpid, instead it does an inferior call
to have waitpid called by an inferior (no idea why, must be something
special related to checkpoint implementation).

Below is a table summarising what has been found.
loop&stopcond is '-' if the logic at 'where' does not loop, otherwise
it details when the loop continues. SIGKILL and PTRACE_KILL column contains
a YES if the logic uses them, otherwise '-' indicates
absence of call. A 'V' indicates the return status is verified,
otherwise errors are ignored. TK indicates SIGKILL is sent with __NR_tkill,
not with the usual kill(SIGKILL).

waitpid gives some details about how the waitpid is done:
  'MY' indicates my_waitpid is called (to protect against EINTR),
  otherwise, it is a direct call to waitpid.
  Note that the return status of waitpid is not systematically checked,
  or is not checked the same way or errors are not necessarily reported.
  Flags are 0, or the specifically used flags. 2 set of flags separated
  by a , means there are 2 calls to waitpid.
  

where                        loop&cond        SIGKILL PTRACE_KILL waitpid
--------------------         -------------    ------- ----------- -------
linux-ptrace.c
 linux_ptrace_test_ret_to_nx -                YES     YES         0
 linux_check_ptrace_features WIFSTOPPED       -       YES V       MY 0
 linux_test_for_tracefork    -                -       YES V       MY 0
 
linux-low.c                  WIFSTOPPED       YES TK  YES         MY 0,__WCLONE

linux-nat.c                  waitpid == pid   YES TK  YES         MY __WALL

linux-fork.c
 linux_fork_killall          WIFSTOPPED       YES     -           0
 delete_checkpoint_command   -                -       YES V       (infcall) 0