diff mbox series

[v2] failsafe: console failsafe shell improvements

Message ID 20220622033555.66239-1-mark@mentovai.com
State New
Headers show
Series [v2] failsafe: console failsafe shell improvements | expand

Commit Message

Mark Mentovai June 22, 2022, 3:35 a.m. UTC
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.

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.@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@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(-)

Comments

Mark Mentovai Aug. 2, 2022, 8:07 p.m. UTC | #1
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.@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@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.@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.@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
diff mbox series

Patch

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.@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.@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