On Wed, Jun 27, 2018 at 08:46:04PM +0200, Landry Breuil wrote:
> On Wed, Jun 27, 2018 at 05:37:54PM +0100, Laurence Tratt wrote:
> > >Synopsis: kernel_lock not locked
> > >Category: kernel
> > >Environment:
> > System : OpenBSD 6.3
> > Details : OpenBSD 6.3-current (GENERIC.MP) #55: Mon Jun 25 23:01:52
> > MDT 2018
> >
> > [email protected]:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> >
> > Architecture: OpenBSD.amd64
> > Machine : amd64
> > >Description:
> > I just hit the following kernel panic (a locking error in sched_bsd.c):
> >
> > https://imagebin.ca/v/46kV6Tfqe1sc
> >
> > I can hit this repeatedly by gdb'ing the new quodlibet 4.1.0 update that
> > Stuart just pushed to ports. It crashes at load; exactly at the point I
> > quit gdb the kernel panics. Here's the userland trace I get just before
> > the kernel panic occurs:
>
> Fwiw, i've hit a similar panic (kernel_lock not locked) this weekend (on an up
> to date kernel) when using egdb on ... firefox, of course.
There is a locking bug that gets triggered when a traced and stopped
multithreaded process is forced to exit. When the bug hits, a thread
calls exit1() with the kernel locked recursively:
sched_exit
exit1
single_thread_check
single_thread_set
issignal <-- KERNEL_LOCK()
userret <-- KERNEL_LOCK()
syscall
Xsyscall_untramp
sched_exit() assumes that a single KERNEL_UNLOCK() releases the lock
completely. However, the assumption is wrong in the above case.
sched_exit() switches to the CPU's idle thread, which in turn calls
mi_switch(). Then, mi_switch() tries to release the kernel lock (which
is bound to the CPU, and which should not be locked in the first place).
That causes a panic with WITNESS because WITNESS had associated the lock
with the exiting thread and the lock is not found in the idle thread's
lock list. That is why the panic's stack trace looks peculiar:
panic
witness_unlock
___mp_release_all
mi_switch
sched_idle
Without WITNESS, the system would hang soon instead.
The bug can be fixed by making sched_exit() release the kernel lock
completely. That would also make exit1() more agnostic with regard to
the state of the lock. As an alternative, issignal() could avoid the
recursive locking.
Comments? OK?
Index: kern/kern_sched.c
===================================================================
RCS file: src/sys/kern/kern_sched.c,v
retrieving revision 1.48
diff -u -p -r1.48 kern_sched.c
--- kern/kern_sched.c 19 Jun 2018 19:29:52 -0000 1.48
+++ kern/kern_sched.c 28 Jun 2018 13:47:28 -0000
@@ -218,8 +218,11 @@ sched_exit(struct proc *p)
LIST_INSERT_HEAD(&spc->spc_deadproc, p, p_hash);
+#ifdef MULTIPROCESSOR
/* This process no longer needs to hold the kernel lock. */
- KERNEL_UNLOCK();
+ KERNEL_ASSERT_LOCKED();
+ __mp_release_all(&kernel_lock);
+#endif
SCHED_LOCK(s);
idle = spc->spc_idleproc;
