Re: [PATCH 0/11] Short circuit delivery for coredump signals

[Oleg Nesterov]

Eric,

Please rebase on top of Linus's tree, git am fails at 7/11.

So far I didnt' try to read the individual patches, I've applied
the whole series on top of 25fe708bbc59 to avoid the conflicts, and
after the very quick glance I seem to see some problems.

Please correct me.

-------------------------------------------------------------------------
complete_signal() does:

	if (sig_fatal(p, sig) && !sigismember(&t->real_blocked, sig) &&
	    (sig == SIGKILL || !p->ptrace)) {
		/*
		 * This signal will be fatal to the whole group.
		 *
		 * Start a group exit and wake everybody up.
		 * This way we don't have other threads
		 * running and doing things after a slower
		 * thread has the fatal signal pending.
		 */
		signal->flags = SIGNAL_GROUP_EXIT | SIGNAL_EXIT_DEQUEUE;
		signal->group_exit_code = sig;
		... kill the thread group ...

However, prepare_signal() still does:

	if (signal->flags & SIGNAL_GROUP_EXIT) {
		if (signal->core_state)
			return sig == SIGKILL;
		/*
		 * The process is in the middle of dying, drop the signal.
		 */
		return false;

This means that if SIGKILL comes before coredump_begin() sets signal->core_state,
it will be lost.

-------------------------------------------------------------------------
dequeue_exit_signal:

	if (signal->flags & SIGNAL_EXIT_DEQUEUE) {
		struct sigpending *pending = NULL;
		struct sigqueue *timer_sigq;
		int signr = exit_code;

		signal->flags &= ~SIGNAL_EXIT_DEQUEUE;

		pending = sigismember(&tsk->pending.signal, signr) ?
			&tsk->pending : &signal->shared_pending;

		collect_signal(signr, pending, info, &timer_sigq);

This looks obviously wrong. 2 threads, T1 and T2. SIGSEGV is sent to T1.
T2 calls get_signal(), clears SIGNAL_EXIT_DEQUEUE and returns SIGSEGV.
But collect_signal() won't find SIGSEGV, *info will be bogus.

T2 calls coredump_begin() and initiates the coredump. The core dump will
be written with wrong dumper thread, bogus siginfo (si_addr/etc are lost).
Even the filename is wrong if core_pattern includes "%i".

Oleg.



On 06/26, Eric W. Biederman wrote:
>
> Oleg's recent patchset tweaking how force_sig_info works has inspired me
> to finally push through and update the signal handling to have proper
> short circuit deliver for coredump signals.  Everything is just simpler
> when coredumps are not such a large special case.
>
> What makes this tricky is coredumps have had their own process
> shoot-down logic similar to but separate and different from everything
> else in the kernel.  The bulk of this set of changes is merging the
> process shoot-down logic that is used for signals and the logic for
> coredumps.  So the same process shoot-down logic can be shared.
>
> With the shoot-down logic sorted the rest is quite straight forward.
>
> Who should pick up these changes?  Historically I would put it in my own
> tree but unfortunately I just have a little bit of time here and there,
> and I can't predict when I will have time to work on things.
>
> Eric W. Biederman (11):
>       signal: Compute the exit_code in get_signal
>       signal: In get_signal call do_exit when it is unnecessary to shoot down threads
>       signal: Bring down all threads when handling a non-coredump fatal signal
>       signal: Move stopping for the coredump from do_exit into get_signal
>       signal: Move audit_core_dumps from do_coredump into get_signal
>       coredump: In zap_threads complete startup if there is no need to wait
>       signal: Use the thread killing in get_signal for coredumps
>       exit: Make do_group_exit static
>       signal: Dequeue fatal signals
>       signal: Short circuit deliver coredump signals
>       signal: Remove SA_IMMUTABLE
>
>  fs/coredump.c                          | 161 +++++++++++++++++----------------
>  include/linux/coredump.h               |   4 +
>  include/linux/sched/signal.h           |   2 +
>  include/linux/sched/task.h             |   1 -
>  include/linux/signal_types.h           |   3 -
>  include/uapi/asm-generic/signal-defs.h |   1 -
>  kernel/exit.c                          |  41 ++-------
>  kernel/signal.c                        | 119 +++++++++++++++---------
>  mm/oom_kill.c                          |   2 +-
>  9 files changed, 171 insertions(+), 163 deletions(-)
>