svn commit: r331298 - head/sys/dev/syscons

[Bruce Evans]

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 <kostikbel at gmail.com>
>>> 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 <sys/smp.h>
> #include <sys/sysctl.h>
> #include <sys/sysproto.h>
> +#include <sys/taskqueue.h>
> #include <sys/vnode.h>
> #include <sys/watchdog.h>
>
> @@ -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  <CTL><ALT><DEL> 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