On Mon, Jun 07, 2021 at 11:34:26AM -0600, Theo de Raadt wrote:
> > When I came up with this diff in 2017, it was not considered the
> > correct fix. Syncing file system and some UVM logic was changed.
> > This improved the situation, but never fixed it.
>
> I guess this got started after my hibernate/unmount change?
It startet here
kern/kern_sig.c
revision 1.167
date: 2014/06/21 20:58:30; author: guenther; state: Exp; lines: +12 -1;
commitid: IvlsVYNsU5F7UWHE;
If the kernel generates a deadly trap signal (SEGV, BUS, etc) for
an untraced process but finds it blocking or ignoring it, just kill
the process instead of looping. It's undefined behavor in POSIX but
quite annoying when encountered in practice.
improvements from kettenis@
ok matthew@
You hibernate/unmount changes came later. One idea to fix was to
mount / read only instead of unmounting. But that opened a terrible
can of worms.
I think that running OpenBSD especially in virtual machines creates
races that trigger it.
> Also, isn't this incorrect about having unmapped pages. I think what
> happens is it is prevented from backing a new mapping, because the storage
> is halted.
If I remember correctly, init gets a signal during shutdown. Then
it goes to the signal handler. The handler code is not mapped and
it cannot be mapped as the VFS has been shut down. So it gets a
signal again. It is ignored, as we are already in the handler. So
the conditions in this code block are fulfilled and init dies with
sigexit().
> Is this more correct?
>
> "After vfs_shutdown(9), init(8) may cannot receive signals because new
> code pages cannot be mapped against halted storage".
This comment describes the situation.
> > if ((pr->ps_flags & PS_TRACED) == 0 &&
> > (sigprop[signum] & SA_KILL) &&
> > - ((p->p_sigmask & mask) || (ps->ps_sigignore & mask)))
> > + ((p->p_sigmask & mask) || (ps->ps_sigignore & mask)) &&
> > + pr->ps_pid != 1)
> > sigexit(p, signum);
> > ptsignal(p, signum, STHREAD);
> > }
>
> OK, so if the sigexit() doesn't happen, what happens to the process instead?
> The signal simply disappears?
It loops until the kernel has halted the machine. init process
jumps between signal handler and page fault trap.
> Doesn't this signal also disappear when vfs_shutdown hasn't been called?
> I worry about a weird change in the signal disposition for init. Maybe
> there is a global variable from vfs_subr.c we can add into the condition?
I did not find a suitable variable, but we can add one. Not sure
this is necessary. It catches the case if there is a fatal bug in
the signal handler. Then it allows the kernel to panic.
So we are talking about forcing a kernel panic if this code in init
signal handler has a bug that causes segfault.
void
disaster(int sig)
{
emergency("fatal signal: %s", strsignal(sig));
sleep(STALL_TIMEOUT);
_exit(sig); /* reboot */
}
I don't think this justifies to add another global variable.
bluhm
Index: kern/kern_sig.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_sig.c,v
retrieving revision 1.281
diff -u -p -r1.281 kern_sig.c
--- kern/kern_sig.c 10 May 2021 18:01:24 -0000 1.281
+++ kern/kern_sig.c 7 Jun 2021 20:33:16 -0000
@@ -842,10 +842,14 @@ trapsignal(struct proc *p, int signum, u
* generated by the kernel, be ignorable or blockable.
* If it is and we're not being traced, then just kill
* the process.
+ * After vfs_shutdown(9), init(8) cannot receive signals
+ * because new code pages cannot be mapped against halted
+ * storage. init(8) may not die or the kernel panics.
*/
if ((pr->ps_flags & PS_TRACED) == 0 &&
(sigprop[signum] & SA_KILL) &&
- ((p->p_sigmask & mask) || (ps->ps_sigignore & mask)))
+ ((p->p_sigmask & mask) || (ps->ps_sigignore & mask)) &&
+ (pr->ps_pid != 1 || !vfs_down))
sigexit(p, signum);
ptsignal(p, signum, STHREAD);
}
Index: kern/vfs_subr.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/vfs_subr.c,v
retrieving revision 1.305
diff -u -p -r1.305 vfs_subr.c
--- kern/vfs_subr.c 28 Apr 2021 09:53:53 -0000 1.305
+++ kern/vfs_subr.c 7 Jun 2021 20:16:01 -0000
@@ -1740,6 +1740,8 @@ vfs_unmountall(void)
}
}
+int vfs_down;
+
/*
* Sync and unmount file systems before shutting down.
*/
@@ -1750,6 +1752,7 @@ vfs_shutdown(struct proc *p)
acct_shutdown();
#endif
+ vfs_down = 1;
printf("syncing disks...");
if (panicstr == 0) {
Index: sys/mount.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/sys/mount.h,v
retrieving revision 1.148
diff -u -p -r1.148 mount.h
--- sys/mount.h 6 Apr 2021 14:17:35 -0000 1.148
+++ sys/mount.h 7 Jun 2021 20:24:36 -0000
@@ -496,6 +496,7 @@ extern int bufcachepercent;
extern void bufadjust(int);
struct uvm_constraint_range;
extern int bufbackoff(struct uvm_constraint_range*, long);
+extern int vfs_down;
/*
* Operations supported on mounted file system.
