[PATCH v2] failsafe: console failsafe shell improvements

Tue Aug 2 13:07:06 PDT 2022

I wrote:
> When running a failsafe shell on a console, job control was unavailable,
> and ^C did not function correctly.
>
> This change invokes console failsafe shells via `setsid`, making them
> session leaders and allowing them to claim controlling terminals, which
> makes job control function properly. To support this, the busybox
> `setsid` utility is enabled. This has a minimal 149-byte size impact on
> a test x86_64 squashfs rootfs image.
>
> ^C was causing console failsafe shell itself to exit, and was being
> ignored in subprocesses of such shells: it was not possible to ^C out of
> a program that would not exit on its own, such as many typical `ping`
> invocations. As job control was unavailable, it was not possible to
> suspend these subprocesses either, causing a hung program to tie up a
> console indefinitely, unless another means to signal the program was
> available. This was caused by SIGINT being placed at disposition SIG_IGN
> by the shell running preinit, which it did because the console shell was
> executed asynchronously with &. That disposition was inherited by the
> console shell and its subprocesses, generally causing ^C to have no
> effect. In the console shell itself, although SIGINT was ignored, ^C
> caused the read loop to return with nothing read, which the shell
> "converted" to SIGINT, causing it to exit.

Is there any interest in or feedback on this patch? It's a significant 
improvement to the failsafe shell's usability on serial consoles.

(Cc chunkeey as the commiter of c9725d4fb620, my previous failsafe 
improvement that this one follows.)

> As there is no way in busybox `ash` to reset the disposition of a signal
> already ignored at shell entry, and no apparent way to avoid SIGINT
> being placed at SIG_IGN when & is used in preinit, an alternative
> construct is needed. Now, `start-stop-daemon` is used to start (-S) the
> console failsafe shell in the background (-b). This approach does not
> alter SIGINT, allowing the console shell to be started with that
> signal's handling intact, and normal ^C processing to occur.
>
> busybox `ash` has some behaviors conditional on SHLVL, and while the
> console shells ought to run at SHLVL=1, they were not by virtue of being
> started by the shell-based preinit system. Additionally, a variety of
> detritus was present in the console shell's environment, carried over
> from preinit. These conditions are corrected by running the console
> shell via `env -i` to clear the environment and establish a minimum and
> correct set of environment variables for operation, in the same manner
> as `login`. HOME is not explicitly set, because it's addressed in
> /etc/profile. For non-failsafe console shells when
> system. at system[0].ttylogin = 0, `login -f root` achieves a similar
> effect. (`login` already started non-failsafe console shells when
> ttylogin = 1 and behaved correctly. This brings the ttylogin = 0 case to
> parity.) Note that even `login -f` is somewhat undesirable for failsafe
> shells because it requires a viable /etc/passwd, hence the `env -i`
> construct in that case.
>
> The TERM environment variable from the preinit environment, with value
> "linux", would rarely be correct for serial consoles. Now, the preinit
> TERM value is preserved (or set to "linux" if unset) only when the
> console is /dev/console or /dev/tty[0-9]*. Otherwise, it will be set to
> a safe default appropriate for serial consoles, "vt102". This change is
> also duplicated for non-failsafe console shells.
>
> This also indicates failsafe mode by showing "- failsafe -" on all
> consoles (not just the last-defined one). It sets a hostname of
> "OpenWrt-failsafe" in failsafe mode which is rendered in the shell's
> prompt as a reminder of the mode during interactive failsafe use.
> Previously, no hostname was set, which resulted in the kernel-default
> hostname, "(none)", appearing in failsafe shell prompts.
>
> Signed-off-by: Mark Mentovai <mark at mentovai.com>
> ---
> .../files/lib/preinit/10_indicate_failsafe     |  7 ++++++-
> .../files/lib/preinit/30_failsafe_wait         |  5 ++++-
> .../files/lib/preinit/99_10_failsafe_login     | 18 +++++++++++++-----
> package/base-files/files/usr/libexec/login.sh  | 14 +++++++++++++-
> package/utils/busybox/Config-defaults.in       |  2 +-
> 5 files changed, 37 insertions(+), 9 deletions(-)
>
> diff --git a/package/base-files/files/lib/preinit/10_indicate_failsafe b/package/base-files/files/lib/preinit/10_indicate_failsafe
> index 7bf5e029e42f..8c950bff74d0 100644
> --- a/package/base-files/files/lib/preinit/10_indicate_failsafe
> +++ b/package/base-files/files/lib/preinit/10_indicate_failsafe
> @@ -9,9 +9,14 @@ indicate_failsafe_led () {
>
> indicate_failsafe() {
> 	[ "$pi_preinit_no_failsafe" = "y" ] && return
> -	echo "- failsafe -"
> +	local consoles="$(cat /sys/class/tty/console/active)"
> +	[ -n "$consoles" ] || consoles=console
> +	for console in $consoles; do
> +		[ -c "/dev/$console" ] && echo "- failsafe -" >"/dev/$console"
> +	done
> 	preinit_net_echo "Entering Failsafe!\n"
> 	indicate_failsafe_led
> +	echo OpenWrt-failsafe > /proc/sys/kernel/hostname
> }
>
> boot_hook_add failsafe indicate_failsafe
> diff --git a/package/base-files/files/lib/preinit/30_failsafe_wait b/package/base-files/files/lib/preinit/30_failsafe_wait
> index 9ab2e8bd4d8b..c792089ece1a 100644
> --- a/package/base-files/files/lib/preinit/30_failsafe_wait
> +++ b/package/base-files/files/lib/preinit/30_failsafe_wait
> @@ -40,7 +40,7 @@ fs_wait_for_key () {
> 		rm -f $keypress_wait
> 	} &
>
> -	local consoles="$(sed -e 's/ /\n/g' /proc/cmdline | grep '^console=' | sed -e 's/^console=//' -e 's/,.*//')"
> +	local consoles="$(cat /sys/class/tty/console/active)"
> 	[ -n "$consoles" ] || consoles=console
> 	for console in $consoles; do
> 		[ -c "/dev/$console" ] || continue
> @@ -78,6 +78,9 @@ fs_wait_for_key () {
> 	keypressed=1
> 	[ "$(cat $keypress_true)" = "true" ] && keypressed=0
>
> +	trap - INT
> +	trap - USR1
> +
> 	rm -f $keypress_true
> 	rm -f $keypress_wait
> 	rm -f $keypress_sec
> diff --git a/package/base-files/files/lib/preinit/99_10_failsafe_login b/package/base-files/files/lib/preinit/99_10_failsafe_login
> index 6f4af3f28b53..f72a35ed5311 100644
> --- a/package/base-files/files/lib/preinit/99_10_failsafe_login
> +++ b/package/base-files/files/lib/preinit/99_10_failsafe_login
> @@ -2,14 +2,22 @@
> # Copyright (C) 2010 Vertical Communications
>
> failsafe_shell() {
> -	local consoles="$(sed -e 's/ /\n/g' /proc/cmdline | grep '^console=' | sed -e 's/^console=//' -e 's/,.*//')"
> +	local consoles="$(cat /sys/class/tty/console/active)"
> 	[ -n "$consoles" ] || consoles=console
> 	for console in $consoles; do
> -		[ -c "/dev/$console" ] && while true; do
> -			ash --login <"/dev/$console" >"/dev/$console" 2>"/dev/$console"
> -			sleep 1
> -		done &
> +		case "$console" in
> +			console|tty[0-9]*)
> +				term=${TERM:-linux}
> +				;;
> +			*)
> +				term=vt102
> +				;;
> +		esac
> +		# Running asynchronously via the shell's & would ignore SIGINT,
> +		# breaking ^C. Use start-stop-daemon instead.
> +		[ -c "/dev/$console" ] && start-stop-daemon -Sb -p /dev/null -- env -i ash -c "while true; do setsid -c env -i USER=root LOGNAME=root SHELL=/bin/ash TERM="$term" ash --login <\"/dev/$console\" >\"/dev/$console\" 2>\"/dev/$console\"; sleep 1; done"
> 	done
> +
> }
>
> boot_hook_add failsafe failsafe_shell
> diff --git a/package/base-files/files/usr/libexec/login.sh b/package/base-files/files/usr/libexec/login.sh
> index 1fff39c6a069..e2f898e850c3 100755
> --- a/package/base-files/files/usr/libexec/login.sh
> +++ b/package/base-files/files/usr/libexec/login.sh
> @@ -1,5 +1,17 @@
> #!/bin/sh
>
> -[ "$(uci -q get system. at system[0].ttylogin)" = 1 ] || exec /bin/ash --login
> +[ -t 0 ] && {
> +	tty_dev=$(readlink /proc/self/fd/0)
> +	case "$tty_dev" in
> +		/dev/console|/dev/tty[0-9]*)
> +			export TERM=${TERM:-linux}
> +			;;
> +		/dev/*)
> +			export TERM=vt102
> +			;;
> +	esac
> +}
> +
> +[ "$(uci -q get system. at system[0].ttylogin)" = 1 ] || exec /bin/login -f root
>
> exec /bin/login
> diff --git a/package/utils/busybox/Config-defaults.in b/package/utils/busybox/Config-defaults.in
> index abe6d5431a70..7d7be5d4ce51 100644
> --- a/package/utils/busybox/Config-defaults.in
> +++ b/package/utils/busybox/Config-defaults.in
> @@ -1780,7 +1780,7 @@ config BUSYBOX_DEFAULT_FEATURE_SETPRIV_CAPABILITY_NAMES
> 	default n
> config BUSYBOX_DEFAULT_SETSID
> 	bool
> -	default n
> +	default y
> config BUSYBOX_DEFAULT_SWAPON
> 	bool
> 	default y
> -- 
> 2.36.1