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

[Christian Brauner]

On 2026-07-06 07:30 -0500, Eric W. Biederman wrote:
> Christian Brauner <brauner@xxxxxxxxxx> writes:
> > I agree and that's not an acceptable regression especially given how
> > sensitive this codepath is.
>
> Regression????
>
> The process is already dead.  It has already been sent a fatal signal.
> The fatal signal has already been accepted for delivery.
>
> The process is already in a state where fatal_signal_pending returns
> true.

You say you're happy for a nuanced discussion here and that there is a
use for dealing with SIGKILL in this situation. That is what this is.
It's also the same window you said you'd reexamine earlier in this
thread and as far as I can tell v2 still has it. Oleg's concern seems
real and I care about the vfs side of coredumping quite a lot so I took
the time to trace it.

The state your series creates at send time is "dying with a coredump".
A real SIGKILL means "die without (finishing) the dump". Those are two
observably different things one is an immediate exit the other can be
minutes of I/O. Today SIGKILL is allowed to pick the earliest exit at
any point in time. With your series there's a window where it can't and
kill() still returns 0.

I applied v2 on top of v7.2-rc1 and traced this so let me spell out
the invariant that gets lost. Upstream guarantees that a
process-directed SIGKILL accepted before signal->core_state is
published never loses against the dump:

- before the dump signal is dequeued the SIGKILL's complete_signal()
  sets SIGNAL_GROUP_EXIT and get_signal()'s "marked for death" path
  delivers SIGKILL without dumping

- between dequeue and zap_threads() sees
  SIGNAL_GROUP_EXIT and returns -EAGAIN so vfs_coredump() gives up
  before the dump has started

- once signal->core_state is published prepare_signal() lets SIGKILL
  through and dump_interrupted() aborts the dump.

With the series enqueue_signal() sets SIGNAL_GROUP_EXIT at send time
of the dump signal while core_state is only published once the chosen
thread has been woken, scheduled, and reached get_signal() to call
coredump_begin(). During that whole window prepare_signal() hits

	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;
	}

and the SIGKILL is dropped without leaving a trace. The death path then
does sigdelset(&current->pending.signal, SIGKILL) and coredump_begin()
clears TIF_SIGPENDING. So dump_interrupted() has nothing left to see
and the full dump is written. Every recheck between the drop and the
dump (dump_interrupted() runs before each write) comes up empty. It has
to. The SIGKILL wasn't recorded anywhere.

> The code today does not work by design.  It works by happenstance.  The
> code runs a very strong risk of violating invariants in the code.  With
> the attendant risk of worse problems elsewhere.

I don't think that holds up.  Whatever each line was originally written
for upstream maintains a clean observable invariant. complete_signal()
never takes the group-exit shortcut for coredump signals and
zap_threads() publishes core_state and sets SIGNAL_GROUP_EXIT in a
single siglock section right at the

	/* Allow SIGKILL, see prepare_signal() */

comment. A sender can thus never observe "GROUP_EXIT set, no
core_state, dump pending". every state it can see either delivers the
SIGKILL or lets it abort the dump. And your own commit 06af8679449d
("coredump: Limit what can interrupt coredumps") says the exception is
deliberate:

    The coredump code deliberately supports being interrupted by
    SIGKILL, and depends upon prepare_signal to filter out all other
    signals.

That commit narrowed signal interruption of the dump down to SIGKILL
instead of removing it. Your new coredump_begin() is now reached only
after a wakeup and a schedule with the group already marked for exit.
And prepare_signal() drops SIGKILL for that whole time.

> The window Oleg is talking about is so small I don't know if you can
> actually write a test program to hit it.
>
> Even a process that executes.
> 	pid_t target = xxxx;
> 	kill(target, SIGSEGV);
> 	kill(target, SIGKILL);
> Has a reasonable chance of the coredump starting before the SIGKILL is
> sent.

That example lands the SIGKILL inside the window nearly every time once
the target is sleeping. The two kill()s complete within a couple of
microseconds of each other. The target first has to be woken, scheduled,
and make it into get_signal() before core_state exists.

On upstream no dump happens. The parent sees SIGKILL. With the series
the full dump happens and the parent sees SIGSEGV plus the core flag.

Btw, pin victim and killer to one cpu with the killer at SCHED_FIFO and
the victim provably cannot reach get_signal() between the two kills. A
variant without the rt priority but plain same-CPU affinity would also
work.

And the window isn't inherently small. signal_wake_up() cannot wake a
task in plain TASK_UNINTERRUPTIBLE. Take a task blocked in
sb_start_write() on a frozen filesystem, the SIGSEGV marks the group at
send time, a SIGKILL sent minutes later isn't sent, and after the thaw
the process sits down and writes out a full core. The same holds behind
hung storage.

> The only use for accepting SIGKILL at that point is the very special
> case that a coredump is taking too long and we have no other means of
> aborting the coredump.

That's the use the window breaks. Whoever sends that SIGKILL decides
based on a timeout or an event (systemd's stop timeout, a watchdog
deadline, the OOM killer firing) not based on whether the victim has
reached coredump_begin() yet. So the same SIGKILL either aborts the
dump or is thrown away depending on where it happens to land.

And the conditions that make someone reach for SIGKILL in the first
place are the ones that stretch that window. Frozen filesystems or hung
storage.

> Labeling something a regression when there is no code shown to care
> is inconsistent when the kernel's no-regression rule.

Ignoring changes in the wait status oom cares...

With the changes here __task_will_free_mem() sees SIGNAL_GROUP_EXIT &&
!core_state and reports that the victim will promptly exit and release
memory, so oom_kill_process() takes its fast path. Mark the victim,
queue the oom reaper, and send no SIGKILL at all.

It won't die quickly, it is about to start dumping. And if that fast
path isn't taken the one SIGKILL that __oom_kill_process() does send is
swallowed by prepare_signal(). Either way the oom reaper then reaps the
address space that the dumper is about to read and the victim survives
the oOm kill to spend however long it takes writing out a large,
silently corrupt core.

The core_state check in __task_will_free_mem() exists to prevent exactly
this and doesn't cover the new pre-dump state. Patch 8 rewords that very
comment without noticing.

In userspace the affected pattern is a dump signal followed by SIGKILL,
and that pattern is standard: systemd's watchdog path (WatchdogSignal=
defaults to SIGABRT, SIGKILL follows after TimeoutAbortSec),
crash-handling supervisors, and any admin whose kill -9 landed in that
window (which is reported as delivered and so nothing suggests a retry)
while the machine then sits down and writes a full core over NFS anyway.
Plain SIGTERM-then-SIGKILL stop paths are unaffected, which is the
pre-existing case I come back to below.

As for POSIX: accepting SIGKILL there terminates the process strictly
sooner, and POSIX has nothing to say about aborting coredumps. If
anything, discarding a successfully sent SIGKILL while the process
keeps doing minutes of work is the behaviour that's harder to square
with "cannot be caught, blocked, or ignored". The PTRACE_EVENT_EXIT
case was SIGKILL letting a tracee escape its tracer, and your
9a95f78eab70 explained why dropping it there was fine: killing the
tracer provides another way out. The same commit message says it
outright:

    This differs from the coredump code where there is no other
    mechanism besides honoring SIGKILL to expedite the end of
    coredumping.

That reasoning covers the not-yet-started dump exactly as much as the
running one. It doesn't need to generalize and nobody is asking it to.

Your certainly right for synchronous faults where the window really is
small. The drop branch in prepare_signal() also isn't new and it's fine
for non-dump deaths. And making fatal_signal_pending() return true at
send time of a dump signal is a genuine improvement. None of that
rescues the SIGKILL window though.

> 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.

That's all Oleg and I are asking for and it fits inside your design.
I think that shouldn't be much code and blocks nothing else in the
series. With those fixed I have no objection to the direction.

Short circuit delivery for coredump signals is a clear improvement and
I'd like to see it land but not with needless risk of behavioral
changes.

Christian