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

[Eric W. Biederman]

Oleg Nesterov <oleg@xxxxxxxxxx> writes:

> On 06/29, Eric W. Biederman wrote:
>>
>> "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx> writes:
>>
>> > Oleg Nesterov <oleg@xxxxxxxxxx> writes:
>> >
>> >> 	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.
>
> Hmm. I disagree...
>
>> 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.
>
> Yes, the coredumping process is not dead. Yet. It can do a lot of activity
> and use a lot of resources.

It is semantically dead.  Pragmatically I completely agree.

>> Do you know of something where userspace actually depends upon
>> killing a coredump before it even starts?
>
> Well. I think a user has all rights to assume that SIGKILL must always
> terminate the process asap, the process killed by SIGKILL must not start
> the coredumping.

If we arrange things so that semantically SIGKILL is delivered before
the signal that triggers the coredump, we can do that without semantic
complications.  The window is tiny enough I am not certain it matters.

There are other issues with that part of code as well.

Can we please look at all of these issues after I post the next version
of my patchset (later today)?  I don't think anything else depends upon them.

>> 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.
>
> Yes... and do_sigacttion() between send and delivery...

HANDLER_EXIT implies the signal is fatal so there will be no userspace
delivery.

SA_IMMUTABLE only exists to ensure that when siglock is dropped that
userspace won't change the way get_signal delivers the signal.

If we can perform short circuited delivery of the fatal signal in
all cases then SA_IMMUTABLE becomes unnecessary.

Earlier you mentioned that force_sig_info_to_task combined with
dequeue_synchronous signal could do a lot better.  I looked back
at my proof of concept branch and you are quite right.  Especially
once we can get short circuit delivery for the coredump signals.

Eric