From owner-svn-src-all@freebsd.org Thu Mar 22 14:21:48 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 0D3AFF6B865; Thu, 22 Mar 2018 14:21:48 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 567C576FA5; Thu, 22 Mar 2018 14:21:46 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id 31DC6104B94; Fri, 23 Mar 2018 01:21:44 +1100 (AEDT) Date: Fri, 23 Mar 2018 01:21:43 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Konstantin Belousov cc: Warner Losh , Warner Losh , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r331298 - head/sys/dev/syscons In-Reply-To: <20180322114213.GR76926@kib.kiev.ua> Message-ID: <20180322235735.J3354@besplex.bde.org> References: <201803211447.w2LElDcK091988@repo.freebsd.org> <20180322024846.S4293@besplex.bde.org> <20180321202752.GO76926@kib.kiev.ua> <20180322174025.Q1053@besplex.bde.org> <20180322114213.GR76926@kib.kiev.ua> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=VJytp5HX c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=pGLkceISAAAA:8 a=gcR34HTtTRcHr_MuQ_EA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Mar 2018 14:21:48 -0000 On Thu, 22 Mar 2018, Konstantin Belousov wrote: > On Thu, Mar 22, 2018 at 05:50:57PM +1100, Bruce Evans wrote: >> On Wed, 21 Mar 2018, Warner Losh wrote: >> >>> On Wed, Mar 21, 2018 at 2:27 PM, Konstantin Belousov >>> wrote: >>>> ... >>>> Are you saying that fast interrupt handlers call shutdown_nice() ? This >>>> is the quite serious bug on its own. To fix it, shutdown_nice() should >>>> use a fast taskqueue to schedule the task which would lock the process >>>> and send the signal. >>> >>> Is there some way we know we're in a fast interrupt handler? If so, it >>> should be simple to fix. If not, then there's an API change ahead of us... >> >> There is a td_intr_nesting_level flag that might work. (I invented this, >> but don't like its current use.) > But why do we need to know this ? We can always schedule a task in the > fast taskqueue if init is present and can be scheduled. Not quite always. In my version, fast interrupt handlers are actually fast, so they can interrupt any spin mutex and cannot call any scheduling function. This is enforced by setting the pcpu pointer to NULL. Taskqueues and even SWIs are unavailable for fast interrupt handlers. Scheduling is done by setting a flag that is checked in timeout handlers like it was in FreeBSD-1. > ... >> shutdown_nice() is also called directly from syscons and vt for the reboot, >> halt and poweroff keys. This happens in normal interrupt handler context, >> so there is only a problem when init is not running. > > diff --git a/sys/kern/kern_shutdown.c b/sys/kern/kern_shutdown.c > index e5ea9644ad3..e7c6d4c92b2 100644 > --- a/sys/kern/kern_shutdown.c > +++ b/sys/kern/kern_shutdown.c > @@ -72,6 +72,7 @@ __FBSDID("$FreeBSD$"); > #include > #include > #include > +#include > #include > #include > > @@ -276,6 +277,28 @@ sys_reboot(struct thread *td, struct reboot_args *uap) > return (error); > } > > +static void > +shutdown_nice_task_fn(void *arg, int pending __unused) > +{ > + int howto; > + > + howto = (uintptr_t)arg; > + /* Send a signal to init(8) and have it shutdown the world. */ > + PROC_LOCK(initproc); > + if (howto & RB_POWEROFF) > + kern_psignal(initproc, SIGUSR2); > + else if (howto & RB_POWERCYCLE) > + kern_psignal(initproc, SIGWINCH); > + else if (howto & RB_HALT) > + kern_psignal(initproc, SIGUSR1); > + else > + kern_psignal(initproc, SIGINT); > + PROC_UNLOCK(initproc); > +} > + > +static struct task shutdown_nice_task = TASK_INITIALIZER(0, > + &shutdown_nice_task_fn, NULL); > + I don't like having a whole task for this. The whole thing is just a hack to work around some the upper layers of the tty driver and some lower layers not having any input [escape] sequences to control the kernel. Only the syscons and vt lower layers have such sequences (where they are actually key combinations that are converted to control operations instead of to input [escape] sequences). To work around for hardware ttys, the filter for kdb sequences is abused to implement a non-kdb sequence for rebooting. The tty input methods could check for kernel-control sequences and safely signal init. This is a bit too complicated for syscons and vt since they can more easily check for key combinations, but wouldhave to convert these to standard sequences to get the tty layer to do the same thing. (They have many more kernel-control key combinations and the non-kdb one for rebooting is just a bug for them.) This is a bit complicated for hardware tty drivers too -- some use the tty bulk-input method and this shouldn't check for sequences, but should reduce to bcopy(). Hoever, to detect the kdb sequences, these drivers han to check at a low level anyway. They can be clever about this and only check for the console device[s] which are usually only used for for input at a low rate. > /* > * Called by events that want to shut down.. e.g on a PC > */ > @@ -283,20 +306,14 @@ void > shutdown_nice(int howto) > { > > - if (initproc != NULL) { > - /* Send a signal to init(8) and have it shutdown the world. */ > - PROC_LOCK(initproc); > - if (howto & RB_POWEROFF) > - kern_psignal(initproc, SIGUSR2); > - else if (howto & RB_POWERCYCLE) > - kern_psignal(initproc, SIGWINCH); > - else if (howto & RB_HALT) > - kern_psignal(initproc, SIGUSR1); > - else > - kern_psignal(initproc, SIGINT); > - PROC_UNLOCK(initproc); > + if (initproc != NULL && !SCHEDULER_STOPPED()) { > + shutdown_nice_task.ta_context = (void *)(uintptr_t)howto; > + taskqueue_enqueue(taskqueue_fast, &shutdown_nice_task); > } else { > - /* No init(8) running, so simply reboot. */ > + /* > + * No init(8) running, or scheduler would not allow it > + * to run, so simply reboot. > + */ > kern_reboot(howto | RB_NOSYNC); > } > } Calling kern_reboot() from fast interrupt handlers is still invalid. It is quite likely to deadlock. In particular, it should deadlock in when kern_reboot() prints messages to the console. Most console drivers have races instead of deadlocks by dropping their lock[s] in their fast interrupt handler before calling the buggy alt escape function. Bruce