From: Zbigniew Jędrzejewski-Szmek Date: Tue, 17 Oct 2023 17:43:31 +0000 (+0200) Subject: pid1,vconsole-setup: lock /dev/console instead of the tty device X-Git-Tag: v255-rc1~199 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=af189d7b50823876e71d1be5f5c575281ffb99c7;p=thirdparty%2Fsystemd.git pid1,vconsole-setup: lock /dev/console instead of the tty device As requested in https://github.com/systemd/systemd/pull/27867#pullrequestreview-1567161854. /dev/console, /dev/tty0, and /dev/ttyN are "different" device nodes that may point to a single underlying device. We want to use a single lock so that we don't get a race if different writers are using a different device path, so let's just always lock around /dev/console. This effectively makes the locking less granular. Fixup for a0043bfa51281c2374878e2a98cf2a3ee10fd92c. Fixes https://github.com/systemd/systemd/issues/28721. Maybe fixes https://github.com/systemd/systemd/issues/28778 and https://github.com/systemd/systemd/issues/28634. --- diff --git a/src/core/execute.c b/src/core/execute.c index 46fb8805b3a..0e7e91370ad 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -25,6 +25,7 @@ #include "cgroup-setup.h" #include "constants.h" #include "cpu-set-util.h" +#include "dev-setup.h" #include "env-file.h" #include "env-util.h" #include "errno-list.h" @@ -106,29 +107,28 @@ int exec_context_tty_size(const ExecContext *context, unsigned *ret_rows, unsign } void exec_context_tty_reset(const ExecContext *context, const ExecParameters *p) { - _cleanup_close_ int fd = -EBADF; + _cleanup_close_ int _fd = -EBADF, lock_fd = -EBADF; const char *path = exec_context_tty_path(ASSERT_PTR(context)); + int fd; - /* 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. - */ - - 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 (p && p->stdin_fd >= 0) + fd = p->stdin_fd; + else if (path) { + fd = _fd = open_terminal(path, O_RDWR|O_NOCTTY|O_CLOEXEC|O_NONBLOCK); if (fd < 0) return; - - if (lock_generic(fd, LOCK_BSD, LOCK_EX) < 0) - return; } else return; /* nothing to do */ + /* Take a synchronization lock 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. + */ + lock_fd = lock_dev_console(); + if (lock_fd < 0) + return (void) log_debug_errno(lock_fd, + "Failed to lock /dev/console: %m"); + if (context->tty_vhangup) (void) terminal_vhangup_fd(fd); diff --git a/src/shared/dev-setup.c b/src/shared/dev-setup.c index 7dca6ad7d4e..f7ed16193f1 100644 --- a/src/shared/dev-setup.c +++ b/src/shared/dev-setup.c @@ -6,14 +6,32 @@ #include "alloc-util.h" #include "dev-setup.h" +#include "fd-util.h" #include "label-util.h" +#include "lock-util.h" #include "log.h" #include "mkdir-label.h" #include "nulstr-util.h" #include "path-util.h" +#include "terminal-util.h" #include "umask-util.h" #include "user-util.h" +int lock_dev_console(void) { + _cleanup_close_ int fd = -EBADF; + int r; + + fd = open_terminal("/dev/console", O_RDONLY|O_CLOEXEC|O_NOCTTY|O_NOFOLLOW); + if (fd < 0) + return fd; + + r = lock_generic(fd, LOCK_BSD, LOCK_EX); + if (r < 0) + return log_error_errno(r, "Failed to lock /dev/console: %m"); + + return TAKE_FD(fd); +} + int dev_setup(const char *prefix, uid_t uid, gid_t gid) { static const char symlinks[] = "-/proc/kcore\0" "/dev/core\0" diff --git a/src/shared/dev-setup.h b/src/shared/dev-setup.h index 92ba6cf7640..5339bc4e5e9 100644 --- a/src/shared/dev-setup.h +++ b/src/shared/dev-setup.h @@ -3,6 +3,8 @@ #include +int lock_dev_console(void); + int dev_setup(const char *prefix, uid_t uid, gid_t gid); int make_inaccessible_nodes(const char *parent_dir, uid_t uid, gid_t gid); diff --git a/src/vconsole/vconsole-setup.c b/src/vconsole/vconsole-setup.c index 29018812183..564f1422845 100644 --- a/src/vconsole/vconsole-setup.c +++ b/src/vconsole/vconsole-setup.c @@ -21,13 +21,13 @@ #include "alloc-util.h" #include "creds-util.h" +#include "dev-setup.h" #include "env-file.h" #include "errno-util.h" #include "fd-util.h" #include "fileio.h" #include "io-util.h" #include "locale-util.h" -#include "lock-util.h" #include "log.h" #include "main-func.h" #include "proc-cmdline.h" @@ -574,7 +574,7 @@ static int verify_source_vc(char **ret_path, const char *src_vc) { static int run(int argc, char **argv) { _cleanup_(context_done) Context c = {}; _cleanup_free_ char *vc = NULL; - _cleanup_close_ int fd = -EBADF; + _cleanup_close_ int fd = -EBADF, lock_fd = -EBADF; bool utf8, keyboard_ok; unsigned idx = 0; int r; @@ -596,9 +596,15 @@ static int run(int argc, char **argv) { /* 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) - return log_error_errno(r, "Failed to lock console: %m"); + lock_fd = lock_dev_console(); + if (lock_fd < 0) { + log_full_errno(lock_fd == -ENOENT ? LOG_DEBUG : LOG_ERR, + lock_fd, + "Failed to lock /dev/console%s: %m", + lock_fd == -ENOENT ? ", ignoring" : ""); + if (lock_fd != -ENOENT) + return lock_fd; + } (void) toggle_utf8_sysfs(utf8); (void) toggle_utf8_vc(vc, fd, utf8);