On 28/06/18(Thu) 14:53, Visa Hankala wrote: > 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?
Thanks for your analyze. So this is a regression introduced by the fix for the previous TOCTOU race. The kernel is currently grabbing the KERNEL_LOCK() in userret() to serialize access to `ps_sigact'. In the future we'll want to use finer locks. So my question is which fix goes in that direction? The one you posted or not grabbing the KERNEL_LOCK() in userret()? If it doesn't matter, then I believe you should commit your fix, it is ok mpi@. > 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; >
