From 3b2321f6eafad36b7bbb1e53b12b161fb2d9f955 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 31 May 2023 15:59:11 +0200 Subject: [PATCH 1/6] units/systemd-vconsole-setup.service: improve title "Setup" is a noun, and the expected order is " ". ("Set up" is the verb. But we want a noun here, so that we can say e.g. "Starting Virtual Console Setup".) --- units/systemd-vconsole-setup.service.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/units/systemd-vconsole-setup.service.in b/units/systemd-vconsole-setup.service.in index 0604ed0173c..a75f72e3aef 100644 --- a/units/systemd-vconsole-setup.service.in +++ b/units/systemd-vconsole-setup.service.in @@ -8,7 +8,7 @@ # (at your option) any later version. [Unit] -Description=Setup Virtual Console +Description=Virtual Console Setup Documentation=man:systemd-vconsole-setup.service(8) man:vconsole.conf(5) ConditionPathExists=/dev/tty0 From 6cfb3ebc60ae40c044142c4f484703f7bbe2b2c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 12 Jul 2023 15:51:07 +0200 Subject: [PATCH 2/6] units/systemd-firstboot: start the service after systemd-vconsole-setup.service This way, we don't start user interaction before (or while) the configured fonts are loading. Tweak the comments a bit while at it. --- man/systemd-vconsole-setup.service.xml | 19 +++++++++++-------- units/systemd-firstboot.service | 9 ++++++++- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/man/systemd-vconsole-setup.service.xml b/man/systemd-vconsole-setup.service.xml index e462ef8f603..d156c482db9 100644 --- a/man/systemd-vconsole-setup.service.xml +++ b/man/systemd-vconsole-setup.service.xml @@ -32,14 +32,17 @@ Description - systemd-vconsole-setup sets up and configures either all virtual consoles, or — if the - optional TTY parameter is provided — a specific one. When the system is booting up, it's - called by systemd-udevd8 during - VT console subsystem initialization. Also, - systemd-localed.service8 invokes - it as needed when language or console changes are made. Internally, this program calls loadkeys1 and setfont8. + systemd-vconsole-setup sets up and configures either all virtual consoles, or + — if the optional TTY parameter is provided — a specific one. When the system + is booting up, systemd-vconsole-setup.service is called by + systemd-udevd8 during + VT console subsystem initialization. Also, + systemd-localed.service8 + invokes it as needed when language or console changes are made. + Internally, this program calls + loadkeys1 + and + setfont8. Execute systemctl restart systemd-vconsole-setup.service in order to apply any manual diff --git a/units/systemd-firstboot.service b/units/systemd-firstboot.service index 0c8d574ef8b..78a408708b2 100644 --- a/units/systemd-firstboot.service +++ b/units/systemd-firstboot.service @@ -15,7 +15,14 @@ ConditionPathIsReadWrite=/etc ConditionFirstBoot=yes DefaultDependencies=no -After=systemd-remount-fs.service systemd-sysusers.service systemd-tmpfiles-setup.service +# This service may need to write to the file system: +After=systemd-remount-fs.service +# Both systemd-sysusers and systemd-tmpfiles may create the root account +# (via factory files or credentials), obviating the need for us to do that: +After=systemd-sysusers.service systemd-tmpfiles-setup.service +# Let vconsole-setup do its setup before starting user interaction: +After=systemd-vconsole-setup.service + Wants=first-boot-complete.target Before=first-boot-complete.target sysinit.target Conflicts=shutdown.target From 84214541fa294a5d8a5394405918d5434bd4d01d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 12 Jul 2023 14:32:54 +0200 Subject: [PATCH 3/6] Revert "pid1: order units using TTYVHangup= after vconsole setup" This reverts commit e019ea738d63d5f7803f378f8bd3e074d66be08f. In the new approach, a lock on /dev/console will be used. This lock will solve the issue for services which run in early boot. Services which run later are ordered after sysinit.target, so they'll run much later anyway so this automatic dependency is not useful. Let's remove it again to make the code simpler. --- man/systemd.exec.xml | 8 ++------ src/basic/special.h | 1 - src/core/execute.c | 10 ---------- src/core/execute.h | 1 - src/core/mount.c | 2 +- src/core/service.c | 7 +------ src/core/socket.c | 6 +----- src/core/swap.c | 6 +----- 8 files changed, 6 insertions(+), 35 deletions(-) diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml index e0774073677..c3df4166d55 100644 --- a/man/systemd.exec.xml +++ b/man/systemd.exec.xml @@ -70,12 +70,8 @@ Units whose standard output or error output is connected to or (or their combinations with console output, see below) automatically acquire - dependencies of type After= on systemd-journald.socket. - - - Units using the terminal (standard input, output, or error are connected to a terminal - or TTYPath= is used) automatically acquire an After= dependency - on systemd-vconsole-setup.service. + dependencies of type After= on + systemd-journald.socket. Units using LogNamespace= will automatically gain ordering and requirement dependencies on the two socket units associated with diff --git a/src/basic/special.h b/src/basic/special.h index bc9c9eb011c..b7760079a27 100644 --- a/src/basic/special.h +++ b/src/basic/special.h @@ -86,7 +86,6 @@ #define SPECIAL_QUOTACHECK_SERVICE "systemd-quotacheck.service" #define SPECIAL_QUOTAON_SERVICE "quotaon.service" #define SPECIAL_REMOUNT_FS_SERVICE "systemd-remount-fs.service" -#define SPECIAL_VCONSOLE_SETUP_SERVICE "systemd-vconsole-setup.service" #define SPECIAL_VOLATILE_ROOT_SERVICE "systemd-volatile-root.service" #define SPECIAL_UDEVD_SERVICE "systemd-udevd.service" #define SPECIAL_GROWFS_SERVICE "systemd-growfs@.service" diff --git a/src/core/execute.c b/src/core/execute.c index 445983bfb62..3a0d289fa55 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -7219,16 +7219,6 @@ bool exec_context_has_encrypted_credentials(ExecContext *c) { return false; } -int exec_context_add_default_dependencies(Unit *u, const ExecContext *c) { - assert(u); - assert(u->default_dependencies); - - if (c && exec_context_needs_term(c)) - return unit_add_dependency_by_name(u, UNIT_AFTER, SPECIAL_VCONSOLE_SETUP_SERVICE, - /* add_reference= */ true, UNIT_DEPENDENCY_DEFAULT); - return 0; -} - void exec_status_start(ExecStatus *s, pid_t pid) { assert(s); diff --git a/src/core/execute.h b/src/core/execute.h index 09f007bb4e9..f784c1a5652 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -501,7 +501,6 @@ void exec_context_revert_tty(ExecContext *c); int exec_context_get_clean_directories(ExecContext *c, char **prefix, ExecCleanMask mask, char ***ret); int exec_context_get_clean_mask(ExecContext *c, ExecCleanMask *ret); -int exec_context_add_default_dependencies(Unit *u, const ExecContext *c); void exec_status_start(ExecStatus *s, pid_t pid); void exec_status_exit(ExecStatus *s, const ExecContext *context, pid_t pid, int code, int status); diff --git a/src/core/mount.c b/src/core/mount.c index 765c9899ef6..7bab18b4919 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -608,7 +608,7 @@ static int mount_add_default_dependencies(Mount *m) { if (r < 0) return r; - return exec_context_add_default_dependencies(UNIT(m), &m->exec_context); + return 0; } static int mount_verify(Mount *m) { diff --git a/src/core/service.c b/src/core/service.c index c6178cf2cd7..0574a909e16 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -747,12 +747,7 @@ static int service_add_default_dependencies(Service *s) { return r; /* Third, add us in for normal shutdown. */ - r = unit_add_two_dependencies_by_name(UNIT(s), UNIT_BEFORE, UNIT_CONFLICTS, SPECIAL_SHUTDOWN_TARGET, true, UNIT_DEPENDENCY_DEFAULT); - if (r < 0) - return r; - - /* Fourth, add generic dependencies */ - return exec_context_add_default_dependencies(UNIT(s), &s->exec_context); + return unit_add_two_dependencies_by_name(UNIT(s), UNIT_BEFORE, UNIT_CONFLICTS, SPECIAL_SHUTDOWN_TARGET, true, UNIT_DEPENDENCY_DEFAULT); } static void service_fix_stdio(Service *s) { diff --git a/src/core/socket.c b/src/core/socket.c index e4c1f5a793f..d72194f20b5 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -283,11 +283,7 @@ static int socket_add_default_dependencies(Socket *s) { return r; } - r = unit_add_two_dependencies_by_name(UNIT(s), UNIT_BEFORE, UNIT_CONFLICTS, SPECIAL_SHUTDOWN_TARGET, true, UNIT_DEPENDENCY_DEFAULT); - if (r < 0) - return r; - - return exec_context_add_default_dependencies(UNIT(s), &s->exec_context); + return unit_add_two_dependencies_by_name(UNIT(s), UNIT_BEFORE, UNIT_CONFLICTS, SPECIAL_SHUTDOWN_TARGET, true, UNIT_DEPENDENCY_DEFAULT); } _pure_ static bool socket_has_exec(Socket *s) { diff --git a/src/core/swap.c b/src/core/swap.c index 73cf320bfd2..fff75ff8040 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -270,11 +270,7 @@ static int swap_add_default_dependencies(Swap *s) { if (r < 0) return r; - r = unit_add_two_dependencies_by_name(UNIT(s), UNIT_BEFORE, UNIT_CONFLICTS, SPECIAL_UMOUNT_TARGET, true, UNIT_DEPENDENCY_DEFAULT); - if (r < 0) - return r; - - return exec_context_add_default_dependencies(UNIT(s), &s->exec_context); + return unit_add_two_dependencies_by_name(UNIT(s), UNIT_BEFORE, UNIT_CONFLICTS, SPECIAL_UMOUNT_TARGET, true, UNIT_DEPENDENCY_DEFAULT); } static int swap_verify(Swap *s) { From a0043bfa51281c2374878e2a98cf2a3ee10fd92c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 31 May 2023 16:26:14 +0200 Subject: [PATCH 4/6] pid1,vconsole-setup: take a lock for the console device When systemd-firstboot (or any other unit which uses the tty) is started, systemd will reset the terminal. If systemd-vconsole-setup happens to be running at that time, it'll error out when it tries to use the vconsole fd and gets an EIO from ioctl. e019ea738d63d5f7803f378f8bd3e074d66be08f was the first fix. It added an implicit ordering between units using the tty and systemd-vconsole-setup. (The commit title is wrong. The approach was generalized, but the commit title wasn't updated.) Then cea32691c313b2dab91cef986d08f309edeb4a40 was added to restart systemd-vconsole-setup.service from systemd-firstboot.service. This was OK, with the ordering in place, systemd-vconsole-setup.service would wait until systemd-firstboot.service exited. But this wasn't enough, because we want the key mappings to be loaded immediately after systemd-firstboot writes the config. 8eb668b9ab2f7627a89c95ffd61350ee9d416da1 implemented that, but actually reintroduced the original issue. I had to drop the ordering between the two units because otherwise we'd deadlock, waiting from firstboot for vconsole-setup which wouldn't start while firstboot was running. Restarting vconsole-setup.service from systemd-firstboot.service works just fine, but when vconsole-setup.service is started earlier, it may be interrupted by systemd-firstboot.service. To resolve the issue, let's take a lock around the tty device. The reset is performed after fork, so the (short) delay should not matter too much. In xopenat_lock() the assert on is dropped so that we can call xopenat(fd, NULL) to get a copy of the original fd. Fixes #26908. --- src/basic/fs-util.c | 1 - src/core/execute.c | 39 +++++++++++++++++++++-------------- src/vconsole/vconsole-setup.c | 9 ++++++++ 3 files changed, 33 insertions(+), 16 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 94c0dfd3de5..804440ef2a5 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -1172,7 +1172,6 @@ int xopenat_lock( int r; assert(dir_fd >= 0 || dir_fd == AT_FDCWD); - assert(path); assert(IN_SET(operation & ~LOCK_NB, LOCK_EX, LOCK_SH)); /* POSIX/UNPOSIX locks don't work on directories (errno is set to -EBADF so let's return early with diff --git a/src/core/execute.c b/src/core/execute.c index 3a0d289fa55..246c151ebce 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -268,25 +268,34 @@ static int exec_context_tty_size(const ExecContext *context, unsigned *ret_rows, } static void exec_context_tty_reset(const ExecContext *context, const ExecParameters *p) { - const char *path; + _cleanup_close_ int fd = -EBADF; + const char *path = exec_context_tty_path(ASSERT_PTR(context)); - assert(context); + /* Take a lock around the device for the duration of the setup that we do here. + * systemd-vconsole-setup.service also takes the lock to avoid being interrupted. + * We open a new fd that will be closed automatically, and operate on it for convenience. + */ - path = exec_context_tty_path(context); + if (p && p->stdin_fd >= 0) { + fd = xopenat_lock(p->stdin_fd, NULL, + O_RDONLY|O_CLOEXEC|O_NONBLOCK|O_NOCTTY, 0, 0, LOCK_BSD, LOCK_EX); + if (fd < 0) + return; + } else if (path) { + fd = open_terminal(path, O_RDWR|O_NOCTTY|O_CLOEXEC|O_NONBLOCK); + if (fd < 0) + return; - if (context->tty_vhangup) { - if (p && p->stdin_fd >= 0) - (void) terminal_vhangup_fd(p->stdin_fd); - else if (path) - (void) terminal_vhangup(path); - } + if (lock_generic(fd, LOCK_BSD, LOCK_EX) < 0) + return; + } else + return; /* nothing to do */ - if (context->tty_reset) { - if (p && p->stdin_fd >= 0) - (void) reset_terminal_fd(p->stdin_fd, true); - else if (path) - (void) reset_terminal(path); - } + if (context->tty_vhangup) + (void) terminal_vhangup_fd(fd); + + if (context->tty_reset) + (void) reset_terminal_fd(fd, true); if (p && p->stdin_fd >= 0) { unsigned rows = context->tty_rows, cols = context->tty_cols; diff --git a/src/vconsole/vconsole-setup.c b/src/vconsole/vconsole-setup.c index 4d9915cbbaa..f2987412b99 100644 --- a/src/vconsole/vconsole-setup.c +++ b/src/vconsole/vconsole-setup.c @@ -26,6 +26,7 @@ #include "fileio.h" #include "io-util.h" #include "locale-util.h" +#include "lock-util.h" #include "log.h" #include "proc-cmdline.h" #include "process-util.h" @@ -589,6 +590,14 @@ int main(int argc, char **argv) { context_load_config(&c); + /* Take lock around the remaining operation to avoid being interrupted by a tty reset operation + * performed for services with TTYVHangup=yes. */ + r = lock_generic(fd, LOCK_BSD, LOCK_EX); + if (r < 0) { + log_error_errno(r, "Failed to lock console: %m"); + return EXIT_FAILURE; + } + (void) toggle_utf8_sysfs(utf8); (void) toggle_utf8_vc(vc, fd, utf8); From a8425c53eb218bd47530dc812143f2ad7faeb64e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 13 Jul 2023 09:58:03 +0200 Subject: [PATCH 5/6] core: adjust indentation One of the two conditionals in the function had different indentation than the other. --- src/shared/exit-status.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/shared/exit-status.c b/src/shared/exit-status.c index 623adda89e7..0ac688b7635 100644 --- a/src/shared/exit-status.c +++ b/src/shared/exit-status.c @@ -145,12 +145,11 @@ bool is_clean_exit(int code, int status, ExitClean clean, const ExitStatusSet *s bitmap_isset(&success_status->status, status)); /* If a daemon does not implement handlers for some of the signals, we do not consider this an - unclean shutdown */ + * unclean shutdown */ if (code == CLD_KILLED) - return - (clean == EXIT_CLEAN_DAEMON && IN_SET(status, SIGHUP, SIGINT, SIGTERM, SIGPIPE)) || - (success_status && - bitmap_isset(&success_status->signal, status)); + return (clean == EXIT_CLEAN_DAEMON && IN_SET(status, SIGHUP, SIGINT, SIGTERM, SIGPIPE)) || + (success_status && + bitmap_isset(&success_status->signal, status)); return false; } From 8623dab880a969fa2eab47614d7a496796f227a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 13 Jul 2023 10:22:54 +0200 Subject: [PATCH 6/6] units/systemd-vconsole-setup: suppress error when service is restarted The service has Type=oneshot, which means that the default value of SuccessExitStatus=0. When multiple vtcon devices are detected, udev will restart the service after each one. If this happens quickly enough, the old instance will get SIGTERM while it is still running: [ 5.357341] (udev-worker)[593]: vtcon1: /usr/lib/udev/rules.d/90-vconsole.rules:12 RUN '/usr/bin/systemctl --no-block restart systemd-vconsole-setup.service [ 5.357439] (udev-worker)[593]: vtcon1: Running command "/usr/bin/systemctl --no-block restart systemd-vconsole-setup.service" [ 5.357485] (udev-worker)[593]: vtcon1: Starting '/usr/bin/systemctl --no-block restart systemd-vconsole-setup.service' [ 5.357537] (udev-worker)[609]: vtcon0: /usr/lib/udev/rules.d/90-vconsole.rules:12 RUN '/usr/bin/systemctl --no-block restart systemd-vconsole-setup.service [ 5.357587] (udev-worker)[609]: vtcon0: Running command "/usr/bin/systemctl --no-block restart systemd-vconsole-setup.service" [ 5.357634] (udev-worker)[609]: vtcon0: Starting '/usr/bin/systemctl --no-block restart systemd-vconsole-setup.service' ... [ 5.680529] systemd[1]: systemd-vconsole-setup.service: Trying to enqueue job systemd-vconsole-setup.service/restart/replace [ 5.680565] systemd[1]: systemd-vconsole-setup.service: Merged into running job, re-running: systemd-vconsole-setup.service/restart as 557 [ 5.680600] systemd[1]: systemd-vconsole-setup.service: Enqueued job systemd-vconsole-setup.service/restart as 557 ... [ 5.682334] systemd[1]: Received SIGCHLD from PID 744 ((le-setup)). [ 5.682377] systemd[1]: Child 744 ((le-setup)) died (code=killed, status=15/TERM) [ 5.682407] systemd[1]: systemd-vconsole-setup.service: Child 744 belongs to systemd-vconsole-setup.service. [ 5.682436] systemd[1]: systemd-vconsole-setup.service: Main process exited, code=killed, status=15/TERM [ 5.682471] systemd[1]: systemd-vconsole-setup.service: Failed with result 'signal'. [ 5.682518] systemd[1]: systemd-vconsole-setup.service: Service will not restart (manual stop) [ 5.682552] systemd[1]: systemd-vconsole-setup.service: Changed stop-sigterm -> failed This is expected and not a problem. Let's treat SIGTERM as success so we don't get this spurious "failure". --- units/systemd-vconsole-setup.service.in | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/units/systemd-vconsole-setup.service.in b/units/systemd-vconsole-setup.service.in index a75f72e3aef..96417e19bda 100644 --- a/units/systemd-vconsole-setup.service.in +++ b/units/systemd-vconsole-setup.service.in @@ -19,6 +19,12 @@ Before=initrd-switch-root.target shutdown.target [Service] Type=oneshot +# This service will be restarted by udev whenever a new vtcon device appears. +# If the previous instance is still running, it shall be interrupted without +# error. +SuccessExitStatus=SIGTERM RemainAfterExit=yes + ExecStart={{ROOTLIBEXECDIR}}/systemd-vconsole-setup + ImportCredential=vconsole.*