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

[Eric W. Biederman]

"Eric W. Biederman" <ebiederm@xxxxxxxxxxxx> writes:

> Oleg Nesterov <oleg@xxxxxxxxxx> writes:
>
>> Eric,
>>
>> Please rebase on top of Linus's tree, git am fails at 7/11.
>
> This was built on v7.1.
>
> Now that v7.2-rc1 is out I will be happy to rebase on top of that.
>
>> 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.
>
> I will reexamine that.  I used to have something to deal with this case
> but somehow convinced myself it didn't matter.

I was thinking of another related problem.

In this case loosing SIGKILL before coredump_begin seems fine.  The process
is already dying of a signal.

The only point of supporting SIGKILL at all during a coredump is because
writing the coredump out can be slow.  So SIGKILL in that case just
aborts the coredump.  If the coredump hasn't started an abort seems
pointless.

I can think of ways to tweak the logic but I can't imagine anything
reliably working until the code reaches coredump_begin, and
TIF_SIGPENDING is cleared.

Do you know of something where userspace actually depends upon
killing a coredump before it even starts?

If not I don't imagine it is worth spending any time on this corner case.

...

There is another corner case I just noticed.

When force_sig_info_to_task is passed HANDLER_EXIT today get_signal
skips ptrace stops when SA_IMMUTABLE is set.  My change did not short
circuit deliver those signals when the process was ptraced so my last
change removing SA_IMMUTABLE was premature.

Eric