From fbcc7f4168827bf7c956bd83eaa42e1f449e95bd Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Wed, 26 Apr 2017 18:18:03 +0200 Subject: [PATCH 1/5] parse-util: introduce parse_dev() helper --- src/basic/parse-util.c | 15 +++++++++++++++ src/basic/parse-util.h | 1 + src/test/test-parse-util.c | 14 ++++++++++++++ 3 files changed, 30 insertions(+) diff --git a/src/basic/parse-util.c b/src/basic/parse-util.c index d86700736d..4532f222c8 100644 --- a/src/basic/parse-util.c +++ b/src/basic/parse-util.c @@ -589,3 +589,18 @@ int parse_ip_port(const char *s, uint16_t *ret) { return 0; } + +int parse_dev(const char *s, dev_t *ret) { + unsigned x, y; + dev_t d; + + if (sscanf(s, "%u:%u", &x, &y) != 2) + return -EINVAL; + + d = makedev(x, y); + if ((unsigned) major(d) != x || (unsigned) minor(d) != y) + return -EINVAL; + + *ret = d; + return 0; +} diff --git a/src/basic/parse-util.h b/src/basic/parse-util.h index 4d132f0de5..dc09782ca8 100644 --- a/src/basic/parse-util.h +++ b/src/basic/parse-util.h @@ -30,6 +30,7 @@ #define MODE_INVALID ((mode_t) -1) int parse_boolean(const char *v) _pure_; +int parse_dev(const char *s, dev_t *ret); int parse_pid(const char *s, pid_t* ret_pid); int parse_mode(const char *s, mode_t *ret); int parse_ifindex(const char *s, int *ret); diff --git a/src/test/test-parse-util.c b/src/test/test-parse-util.c index d08014100b..099c8e88f5 100644 --- a/src/test/test-parse-util.c +++ b/src/test/test-parse-util.c @@ -526,6 +526,19 @@ static void test_parse_nice(void) { assert_se(parse_nice("+20", &n) == -ERANGE); } +static void test_parse_dev(void) { + dev_t dev; + + assert_se(parse_dev("0", &dev) == -EINVAL); + assert_se(parse_dev("5", &dev) == -EINVAL); + assert_se(parse_dev("5:", &dev) == -EINVAL); + assert_se(parse_dev(":5", &dev) == -EINVAL); +#if SIZEOF_DEV_T < 8 + assert_se(parse_dev("4294967295:4294967295", &dev) == -EINVAL); +#endif + assert_se(parse_dev("8:11", &dev) >= 0 && major(dev) == 8 && minor(dev) == 11); +} + int main(int argc, char *argv[]) { log_parse_environment(); log_open(); @@ -542,6 +555,7 @@ int main(int argc, char *argv[]) { test_parse_percent(); test_parse_percent_unbounded(); test_parse_nice(); + test_parse_dev(); return 0; } From 9b1419111aee0f4dca07593bd8e6db4ed94ce141 Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Fri, 12 May 2017 11:32:53 +0200 Subject: [PATCH 2/5] core: only apply NonBlocking= to fds passed via socket activation Make sure to only apply the O_NONBLOCK flag to the fds passed via socket activation. Previously the flag was also applied to the fds which came from the fd store but this was incorrect since services, after being restarted, expect that these passed fds have their flags unchanged and can be reused as before. The documentation was a bit unclear about this so clarify it. --- man/systemd.service.xml | 17 ++++++++--------- src/core/execute.c | 21 ++++++++++++++------- src/core/execute.h | 1 + src/core/service.c | 14 ++++++++++---- 4 files changed, 33 insertions(+), 20 deletions(-) diff --git a/man/systemd.service.xml b/man/systemd.service.xml index 9a0b72aca9..48c1ea15b1 100644 --- a/man/systemd.service.xml +++ b/man/systemd.service.xml @@ -782,15 +782,14 @@ NonBlocking= - Set the O_NONBLOCK flag - for all file descriptors passed via socket-based activation. - If true, all file descriptors >= 3 (i.e. all except stdin, - stdout, and stderr) will have the - O_NONBLOCK flag set and hence are in - non-blocking mode. This option is only useful in conjunction - with a socket unit, as described in - systemd.socket5. - Defaults to false. + Set the O_NONBLOCK flag for all file descriptors passed via socket-based + activation. If true, all file descriptors >= 3 (i.e. all except stdin, stdout, stderr), excluding those passed + in via the file descriptor storage logic (see FileDescriptorStoreMax= for details), will + have the O_NONBLOCK flag set and hence are in non-blocking mode. This option is only + useful in conjunction with a socket unit, as described in + systemd.socket5 and has no + effect on file descriptors which were previously saved in the file-descriptor store for example. Defaults to + false. diff --git a/src/core/execute.c b/src/core/execute.c index e97651105e..7d9fc037b9 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -157,7 +157,7 @@ static int shift_fds(int fds[], unsigned n_fds) { return 0; } -static int flags_fds(const int fds[], unsigned n_fds, bool nonblock) { +static int flags_fds(const int fds[], unsigned n_fds, unsigned n_socket_fds, bool nonblock) { unsigned i; int r; @@ -166,13 +166,16 @@ static int flags_fds(const int fds[], unsigned n_fds, bool nonblock) { assert(fds); - /* Drops/Sets O_NONBLOCK and FD_CLOEXEC from the file flags */ + /* Drops/Sets O_NONBLOCK and FD_CLOEXEC from the file flags. + * O_NONBLOCK only applies to socket activation though. */ for (i = 0; i < n_fds; i++) { - r = fd_nonblock(fds[i], nonblock); - if (r < 0) - return r; + if (i < n_socket_fds) { + r = fd_nonblock(fds[i], nonblock); + if (r < 0) + return r; + } /* We unconditionally drop FD_CLOEXEC from the fds, * since after all we want to pass these fds to our @@ -2242,6 +2245,7 @@ static int exec_child( int socket_fd, int named_iofds[3], int *fds, unsigned n_fds, + unsigned n_socket_fds, char **files_env, int user_lookup_fd, int *exit_status, @@ -2669,7 +2673,7 @@ static int exec_child( if (r >= 0) r = shift_fds(fds, n_fds); if (r >= 0) - r = flags_fds(fds, n_fds, context->non_blocking); + r = flags_fds(fds, n_fds, n_socket_fds, context->non_blocking); if (r < 0) { *exit_status = EXIT_FDS; return r; @@ -2909,7 +2913,8 @@ int exec_spawn(Unit *unit, pid_t *ret) { _cleanup_strv_free_ char **files_env = NULL; - int *fds = NULL; unsigned n_fds = 0; + int *fds = NULL; + unsigned n_fds = 0, n_socket_fds = 0; _cleanup_free_ char *line = NULL; int socket_fd, r; int named_iofds[3] = { -1, -1, -1 }; @@ -2942,6 +2947,7 @@ int exec_spawn(Unit *unit, socket_fd = -1; fds = params->fds; n_fds = params->n_fds; + n_socket_fds = params->n_socket_fds; } r = exec_context_named_iofds(unit, context, params, named_iofds); @@ -2980,6 +2986,7 @@ int exec_spawn(Unit *unit, socket_fd, named_iofds, fds, n_fds, + n_socket_fds, files_env, unit->manager->user_lookup_fds[1], &exit_status, diff --git a/src/core/execute.h b/src/core/execute.h index 9f2b6fd39e..dee7af1423 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -247,6 +247,7 @@ struct ExecParameters { int *fds; char **fd_names; unsigned n_fds; + unsigned n_socket_fds; ExecFlags flags; bool selinux_context_net:1; diff --git a/src/core/service.c b/src/core/service.c index b45929e535..370a6316e7 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1068,14 +1068,16 @@ static int service_coldplug(Unit *u) { return 0; } -static int service_collect_fds(Service *s, int **fds, char ***fd_names) { +static int service_collect_fds(Service *s, int **fds, char ***fd_names, unsigned *n_socket_fds) { _cleanup_strv_free_ char **rfd_names = NULL; _cleanup_free_ int *rfds = NULL; - int rn_fds = 0, r; + unsigned rn_socket_fds = 0; + int rn_fds = 0, r; assert(s); assert(fds); assert(fd_names); + assert(n_socket_fds); if (s->socket_fd >= 0) { @@ -1138,6 +1140,8 @@ static int service_collect_fds(Service *s, int **fds, char ***fd_names) { } } + rn_socket_fds = rn_fds; + if (s->n_fd_store > 0) { ServiceFDStore *fs; char **nl; @@ -1169,6 +1173,7 @@ static int service_collect_fds(Service *s, int **fds, char ***fd_names) { *fds = rfds; *fd_names = rfd_names; + *n_socket_fds = rn_socket_fds; rfds = NULL; rfd_names = NULL; @@ -1204,7 +1209,7 @@ static int service_spawn( _cleanup_strv_free_ char **final_env = NULL, **our_env = NULL, **fd_names = NULL; _cleanup_free_ int *fds = NULL; - unsigned n_fds = 0, n_env = 0; + unsigned n_fds = 0, n_socket_fds = 0, n_env = 0; const char *path; pid_t pid; @@ -1248,7 +1253,7 @@ static int service_spawn( s->exec_context.std_output == EXEC_OUTPUT_SOCKET || s->exec_context.std_error == EXEC_OUTPUT_SOCKET) { - r = service_collect_fds(s, &fds, &fd_names); + r = service_collect_fds(s, &fds, &fd_names, &n_socket_fds); if (r < 0) return r; @@ -1348,6 +1353,7 @@ static int service_spawn( exec_params.fds = fds; exec_params.fd_names = fd_names; exec_params.n_fds = n_fds; + exec_params.n_socket_fds = n_socket_fds; exec_params.confirm_spawn = manager_get_confirm_spawn(UNIT(s)->manager); exec_params.cgroup_supported = UNIT(s)->manager->cgroup_supported; exec_params.cgroup_path = path; From 4c47affcf1e501431f3cb567c516ec252dfc4bbc Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Thu, 8 Jun 2017 15:41:26 +0200 Subject: [PATCH 3/5] core: remove the redundancy of 'n_fds' and 'n_storage_fds' in ExecParameters struct 'n_fds' field in the ExecParameters structure was counting the total number of file descriptors to be passed to a unit. This counter also includes the number of passed socket fds which is counted by 'n_socket_fds' already. This patch removes that redundancy by replacing 'n_fds' with 'n_storage_fds'. The new field only counts the fds passed via the storage store mechanism. That way each fd is counted at one place only. Subsequently the patch makes sure to fix code that used 'n_fds' and also wanted to iterate through all of them by explicitly adding 'n_socket_fds' + 'n_storage_fds'. Suggested by Lennart. --- src/core/execute.c | 30 ++++++++++++++++---------- src/core/execute.h | 2 +- src/core/service.c | 52 ++++++++++++++++++++++++++-------------------- 3 files changed, 49 insertions(+), 35 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 7d9fc037b9..550a8cfd39 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -157,10 +157,11 @@ static int shift_fds(int fds[], unsigned n_fds) { return 0; } -static int flags_fds(const int fds[], unsigned n_fds, unsigned n_socket_fds, bool nonblock) { - unsigned i; +static int flags_fds(const int fds[], unsigned n_storage_fds, unsigned n_socket_fds, bool nonblock) { + unsigned i, n_fds; int r; + n_fds = n_storage_fds + n_socket_fds; if (n_fds <= 0) return 0; @@ -2244,7 +2245,8 @@ static int exec_child( char **argv, int socket_fd, int named_iofds[3], - int *fds, unsigned n_fds, + int *fds, + unsigned n_storage_fds, unsigned n_socket_fds, char **files_env, int user_lookup_fd, @@ -2262,6 +2264,7 @@ static int exec_child( uid_t uid = UID_INVALID; gid_t gid = GID_INVALID; int i, r, ngids = 0; + unsigned n_fds; assert(unit); assert(command); @@ -2302,6 +2305,7 @@ static int exec_child( log_forget_fds(); + n_fds = n_storage_fds + n_socket_fds; r = close_remaining_fds(params, runtime, dcreds, user_lookup_fd, socket_fd, fds, n_fds); if (r < 0) { *exit_status = EXIT_FDS; @@ -2673,7 +2677,7 @@ static int exec_child( if (r >= 0) r = shift_fds(fds, n_fds); if (r >= 0) - r = flags_fds(fds, n_fds, n_socket_fds, context->non_blocking); + r = flags_fds(fds, n_storage_fds, n_socket_fds, context->non_blocking); if (r < 0) { *exit_status = EXIT_FDS; return r; @@ -2914,7 +2918,7 @@ int exec_spawn(Unit *unit, _cleanup_strv_free_ char **files_env = NULL; int *fds = NULL; - unsigned n_fds = 0, n_socket_fds = 0; + unsigned n_storage_fds = 0, n_socket_fds = 0; _cleanup_free_ char *line = NULL; int socket_fd, r; int named_iofds[3] = { -1, -1, -1 }; @@ -2926,18 +2930,18 @@ int exec_spawn(Unit *unit, assert(context); assert(ret); assert(params); - assert(params->fds || params->n_fds <= 0); + assert(params->fds || (params->n_storage_fds + params->n_socket_fds <= 0)); if (context->std_input == EXEC_INPUT_SOCKET || context->std_output == EXEC_OUTPUT_SOCKET || context->std_error == EXEC_OUTPUT_SOCKET) { - if (params->n_fds > 1) { + if (params->n_socket_fds > 1) { log_unit_error(unit, "Got more than one socket."); return -EINVAL; } - if (params->n_fds == 0) { + if (params->n_socket_fds == 0) { log_unit_error(unit, "Got no socket."); return -EINVAL; } @@ -2946,7 +2950,7 @@ int exec_spawn(Unit *unit, } else { socket_fd = -1; fds = params->fds; - n_fds = params->n_fds; + n_storage_fds = params->n_storage_fds; n_socket_fds = params->n_socket_fds; } @@ -2985,7 +2989,8 @@ int exec_spawn(Unit *unit, argv, socket_fd, named_iofds, - fds, n_fds, + fds, + n_storage_fds, n_socket_fds, files_env, unit->manager->user_lookup_fds[1], @@ -3195,6 +3200,7 @@ const char* exec_context_fdname(const ExecContext *c, int fd_index) { int exec_context_named_iofds(Unit *unit, const ExecContext *c, const ExecParameters *p, int named_iofds[3]) { unsigned i, targets; const char* stdio_fdname[3]; + unsigned n_fds; assert(c); assert(p); @@ -3206,7 +3212,9 @@ int exec_context_named_iofds(Unit *unit, const ExecContext *c, const ExecParamet for (i = 0; i < 3; i++) stdio_fdname[i] = exec_context_fdname(c, i); - for (i = 0; i < p->n_fds && targets > 0; i++) + n_fds = p->n_storage_fds + p->n_socket_fds; + + for (i = 0; i < n_fds && targets > 0; i++) if (named_iofds[STDIN_FILENO] < 0 && c->std_input == EXEC_INPUT_NAMED_FD && stdio_fdname[STDIN_FILENO] && diff --git a/src/core/execute.h b/src/core/execute.h index dee7af1423..136319359c 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -246,7 +246,7 @@ struct ExecParameters { int *fds; char **fd_names; - unsigned n_fds; + unsigned n_storage_fds; unsigned n_socket_fds; ExecFlags flags; diff --git a/src/core/service.c b/src/core/service.c index 370a6316e7..df7f1f3053 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1068,11 +1068,16 @@ static int service_coldplug(Unit *u) { return 0; } -static int service_collect_fds(Service *s, int **fds, char ***fd_names, unsigned *n_socket_fds) { +static int service_collect_fds(Service *s, + int **fds, + char ***fd_names, + unsigned *n_storage_fds, + unsigned *n_socket_fds) { + _cleanup_strv_free_ char **rfd_names = NULL; _cleanup_free_ int *rfds = NULL; - unsigned rn_socket_fds = 0; - int rn_fds = 0, r; + unsigned rn_socket_fds = 0, rn_storage_fds = 0; + int r; assert(s); assert(fds); @@ -1092,7 +1097,7 @@ static int service_collect_fds(Service *s, int **fds, char ***fd_names, unsigned if (!rfd_names) return -ENOMEM; - rn_fds = 1; + rn_socket_fds = 1; } else { Iterator i; Unit *u; @@ -1118,20 +1123,20 @@ static int service_collect_fds(Service *s, int **fds, char ***fd_names, unsigned if (!rfds) { rfds = cfds; - rn_fds = cn_fds; + rn_socket_fds = cn_fds; cfds = NULL; } else { int *t; - t = realloc(rfds, (rn_fds + cn_fds) * sizeof(int)); + t = realloc(rfds, (rn_socket_fds + cn_fds) * sizeof(int)); if (!t) return -ENOMEM; - memcpy(t + rn_fds, cfds, cn_fds * sizeof(int)); + memcpy(t + rn_socket_fds, cfds, cn_fds * sizeof(int)); rfds = t; - rn_fds += cn_fds; + rn_socket_fds += cn_fds; } r = strv_extend_n(&rfd_names, socket_fdname(sock), cn_fds); @@ -1140,45 +1145,47 @@ static int service_collect_fds(Service *s, int **fds, char ***fd_names, unsigned } } - rn_socket_fds = rn_fds; - if (s->n_fd_store > 0) { ServiceFDStore *fs; + unsigned n_fds; char **nl; int *t; - t = realloc(rfds, (rn_fds + s->n_fd_store) * sizeof(int)); + t = realloc(rfds, (rn_socket_fds + s->n_fd_store) * sizeof(int)); if (!t) return -ENOMEM; rfds = t; - nl = realloc(rfd_names, (rn_fds + s->n_fd_store + 1) * sizeof(char*)); + nl = realloc(rfd_names, (rn_socket_fds + s->n_fd_store + 1) * sizeof(char*)); if (!nl) return -ENOMEM; rfd_names = nl; + n_fds = rn_socket_fds; LIST_FOREACH(fd_store, fs, s->fd_store) { - rfds[rn_fds] = fs->fd; - rfd_names[rn_fds] = strdup(strempty(fs->fdname)); - if (!rfd_names[rn_fds]) + rfds[n_fds] = fs->fd; + rfd_names[n_fds] = strdup(strempty(fs->fdname)); + if (!rfd_names[n_fds]) return -ENOMEM; - rn_fds++; + rn_storage_fds++; + n_fds++; } - rfd_names[rn_fds] = NULL; + rfd_names[n_fds] = NULL; } *fds = rfds; *fd_names = rfd_names; *n_socket_fds = rn_socket_fds; + *n_storage_fds = rn_storage_fds; rfds = NULL; rfd_names = NULL; - return rn_fds; + return 0; } static bool service_exec_needs_notify_socket(Service *s, ExecFlags flags) { @@ -1209,7 +1216,7 @@ static int service_spawn( _cleanup_strv_free_ char **final_env = NULL, **our_env = NULL, **fd_names = NULL; _cleanup_free_ int *fds = NULL; - unsigned n_fds = 0, n_socket_fds = 0, n_env = 0; + unsigned n_storage_fds = 0, n_socket_fds = 0, n_env = 0; const char *path; pid_t pid; @@ -1253,12 +1260,11 @@ static int service_spawn( s->exec_context.std_output == EXEC_OUTPUT_SOCKET || s->exec_context.std_error == EXEC_OUTPUT_SOCKET) { - r = service_collect_fds(s, &fds, &fd_names, &n_socket_fds); + r = service_collect_fds(s, &fds, &fd_names, &n_storage_fds, &n_socket_fds); if (r < 0) return r; - n_fds = r; - log_unit_debug(UNIT(s), "Passing %i fds to service", n_fds); + log_unit_debug(UNIT(s), "Passing %i fds to service", n_storage_fds + n_socket_fds); } r = service_arm_timer(s, usec_add(now(CLOCK_MONOTONIC), timeout)); @@ -1352,7 +1358,7 @@ static int service_spawn( exec_params.environment = final_env; exec_params.fds = fds; exec_params.fd_names = fd_names; - exec_params.n_fds = n_fds; + exec_params.n_storage_fds = n_storage_fds; exec_params.n_socket_fds = n_socket_fds; exec_params.confirm_spawn = manager_get_confirm_spawn(UNIT(s)->manager); exec_params.cgroup_supported = UNIT(s)->manager->cgroup_supported; From dc6284e9efb0a6801dd7a951d39ebb7d9a279676 Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Wed, 26 Apr 2017 14:20:41 +0200 Subject: [PATCH 4/5] logind: when setting a new controller, don't prepare the VT if logind is restarted When assigning a new session controller to a session, the VT is prepared so the controller can expect the VT to be in a good default state. However when logind is restarted and a session controller already took control of a session, there's no need to prepare th VT otherwise logind may screw up the VT state set by the controller. This patch prevents the preparation of the VT in this case. --- src/login/logind-session-dbus.c | 2 +- src/login/logind-session.c | 17 ++++++++++------- src/login/logind-session.h | 2 +- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/login/logind-session-dbus.c b/src/login/logind-session-dbus.c index 22dea5db1f..412efcd958 100644 --- a/src/login/logind-session-dbus.c +++ b/src/login/logind-session-dbus.c @@ -396,7 +396,7 @@ static int method_take_control(sd_bus_message *message, void *userdata, sd_bus_e if (uid != 0 && (force || uid != s->user->uid)) return sd_bus_error_setf(error, SD_BUS_ERROR_ACCESS_DENIED, "Only owner of session may take control"); - r = session_set_controller(s, sd_bus_message_get_sender(message), force); + r = session_set_controller(s, sd_bus_message_get_sender(message), force, true); if (r < 0) return r; diff --git a/src/login/logind-session.c b/src/login/logind-session.c index 4a168906d6..ae05877a68 100644 --- a/src/login/logind-session.c +++ b/src/login/logind-session.c @@ -449,7 +449,7 @@ int session_load(Session *s) { if (controller) { if (bus_name_has_owner(s->manager->bus, controller, NULL) > 0) - session_set_controller(s, controller, false); + session_set_controller(s, controller, false, false); else session_restore_vt(s); } @@ -1170,7 +1170,7 @@ static int on_bus_track(sd_bus_track *track, void *userdata) { return 0; } -int session_set_controller(Session *s, const char *sender, bool force) { +int session_set_controller(Session *s, const char *sender, bool force, bool prepare) { _cleanup_free_ char *name = NULL; int r; @@ -1202,11 +1202,14 @@ int session_set_controller(Session *s, const char *sender, bool force) { * Note that we reset the VT on ReleaseControl() and if the controller * exits. * If logind crashes/restarts, we restore the controller during restart - * or reset the VT in case it crashed/exited, too. */ - r = session_prepare_vt(s); - if (r < 0) { - s->track = sd_bus_track_unref(s->track); - return r; + * (without preparing the VT since the controller has probably overridden + * VT state by now) or reset the VT in case it crashed/exited, too. */ + if (prepare) { + r = session_prepare_vt(s); + if (r < 0) { + s->track = sd_bus_track_unref(s->track); + return r; + } } session_release_controller(s, true); diff --git a/src/login/logind-session.h b/src/login/logind-session.h index ffb7cd2d41..c8a3152acb 100644 --- a/src/login/logind-session.h +++ b/src/login/logind-session.h @@ -176,7 +176,7 @@ void session_restore_vt(Session *s); void session_leave_vt(Session *s); bool session_is_controller(Session *s, const char *sender); -int session_set_controller(Session *s, const char *sender, bool force); +int session_set_controller(Session *s, const char *sender, bool force, bool prepare); void session_drop_controller(Session *s); int bus_session_method_activate(sd_bus_message *message, void *userdata, sd_bus_error *error); From aed24c4cd7641da6f530853d10637568c13c8f35 Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Thu, 16 Mar 2017 11:17:09 +0100 Subject: [PATCH 5/5] logind: save/restore session devices and their respective file descriptors This patch ensures that session devices are saved for each session. In order to make the revokation logic work when logind is restarted, the session devices are now saved in the session state files and their respective file descriptors sent to PID1's fdstore in order to keep them open accross restart. This is mandatory in order to keep the revokation logic working. Indeed in case of input-devices, the same file descriptors must be shared by logind and a given session controller in order EVIOCREVOKE to work otherwise multiple sessions can have device access in parallel. This should be the only remaining and missing piece for making logind fully restartable. Fixes: #1163 --- src/login/logind-session-dbus.c | 15 +++++- src/login/logind-session-device.c | 81 +++++++++++++++++++++++-------- src/login/logind-session-device.h | 5 +- src/login/logind-session.c | 70 ++++++++++++++++++++++++-- src/login/logind-session.h | 2 + src/login/logind.c | 71 +++++++++++++++++++++++++-- units/systemd-logind.service.in | 1 + 7 files changed, 215 insertions(+), 30 deletions(-) diff --git a/src/login/logind-session-dbus.c b/src/login/logind-session-dbus.c index 412efcd958..22e5349a67 100644 --- a/src/login/logind-session-dbus.c +++ b/src/login/logind-session-dbus.c @@ -444,14 +444,23 @@ static int method_take_device(sd_bus_message *message, void *userdata, sd_bus_er * equivalent). */ return sd_bus_error_setf(error, BUS_ERROR_DEVICE_IS_TAKEN, "Device already taken"); - r = session_device_new(s, dev, &sd); + r = session_device_new(s, dev, true, &sd); if (r < 0) return r; + r = session_device_save(sd); + if (r < 0) + goto error; + r = sd_bus_reply_method_return(message, "hb", sd->fd, !sd->active); if (r < 0) - session_device_free(sd); + goto error; + session_save(s); + return 0; + +error: + session_device_free(sd); return r; } @@ -478,6 +487,8 @@ static int method_release_device(sd_bus_message *message, void *userdata, sd_bus return sd_bus_error_setf(error, BUS_ERROR_DEVICE_NOT_TAKEN, "Device not taken"); session_device_free(sd); + session_save(s); + return sd_bus_reply_method_return(message, NULL); } diff --git a/src/login/logind-session-device.c b/src/login/logind-session-device.c index 4055a23277..05af2045d2 100644 --- a/src/login/logind-session-device.c +++ b/src/login/logind-session-device.c @@ -30,6 +30,8 @@ #include "fd-util.h" #include "logind-session-device.h" #include "missing.h" +#include "parse-util.h" +#include "sd-daemon.h" #include "util.h" enum SessionDeviceNotifications { @@ -206,7 +208,10 @@ static int session_device_start(SessionDevice *sd) { r = session_device_open(sd, true); if (r < 0) return r; - close_nointr(sd->fd); + /* For evdev devices, the file descriptor might be left + * uninitialized. This might happen while resuming into a + * session and logind has been restarted right before. */ + safe_close(sd->fd); sd->fd = r; break; case DEVICE_TYPE_UNKNOWN: @@ -341,7 +346,7 @@ err_dev: return r; } -int session_device_new(Session *s, dev_t dev, SessionDevice **out) { +int session_device_new(Session *s, dev_t dev, bool open_device, SessionDevice **out) { SessionDevice *sd; int r; @@ -370,22 +375,24 @@ int session_device_new(Session *s, dev_t dev, SessionDevice **out) { goto error; } - /* Open the device for the first time. We need a valid fd to pass back - * to the caller. If the session is not active, this _might_ immediately - * revoke access and thus invalidate the fd. But this is still needed - * to pass a valid fd back. */ - sd->active = session_is_active(s); - r = session_device_open(sd, sd->active); - if (r < 0) { - /* EINVAL _may_ mean a master is active; retry inactive */ - if (sd->active && r == -EINVAL) { - sd->active = false; - r = session_device_open(sd, false); + if (open_device) { + /* Open the device for the first time. We need a valid fd to pass back + * to the caller. If the session is not active, this _might_ immediately + * revoke access and thus invalidate the fd. But this is still needed + * to pass a valid fd back. */ + sd->active = session_is_active(s); + r = session_device_open(sd, sd->active); + if (r < 0) { + /* EINVAL _may_ mean a master is active; retry inactive */ + if (sd->active && r == -EINVAL) { + sd->active = false; + r = session_device_open(sd, false); + } + if (r < 0) + goto error; } - if (r < 0) - goto error; + sd->fd = r; } - sd->fd = r; LIST_PREPEND(sd_by_device, sd->device->session_devices, sd); @@ -435,15 +442,16 @@ void session_device_complete_pause(SessionDevice *sd) { void session_device_resume_all(Session *s) { SessionDevice *sd; Iterator i; - int r; assert(s); HASHMAP_FOREACH(sd, s->devices, i) { if (!sd->active) { - r = session_device_start(sd); - if (!r) - session_device_notify(sd, SESSION_DEVICE_RESUME); + if (session_device_start(sd) < 0) + continue; + if (session_device_save(sd) < 0) + continue; + session_device_notify(sd, SESSION_DEVICE_RESUME); } } } @@ -478,3 +486,36 @@ unsigned int session_device_try_pause_all(Session *s) { return num_pending; } + +int session_device_save(SessionDevice *sd) { + _cleanup_free_ char *state = NULL; + int r; + + assert(sd); + + /* Store device fd in PID1. It will send it back to us on + * restart so revocation will continue to work. To make things + * simple, send fds for all type of devices even if they don't + * support the revocation mechanism so we don't have to handle + * them differently later. + * + * Note: for device supporting revocation, PID1 will drop a + * stored fd automatically if the corresponding device is + * revoked. */ + r = asprintf(&state, "FDSTORE=1\n" + "FDNAME=session-%s", sd->session->id); + if (r < 0) + return -ENOMEM; + + return sd_pid_notify_with_fds(0, false, state, &sd->fd, 1); +} + +void session_device_attach_fd(SessionDevice *sd, int fd, bool active) { + assert(fd > 0); + assert(sd); + assert(sd->fd < 0); + assert(!sd->active); + + sd->fd = fd; + sd->active = active; +} diff --git a/src/login/logind-session-device.h b/src/login/logind-session-device.h index 7c8503583f..83aef1e18c 100644 --- a/src/login/logind-session-device.h +++ b/src/login/logind-session-device.h @@ -44,10 +44,13 @@ struct SessionDevice { LIST_FIELDS(struct SessionDevice, sd_by_device); }; -int session_device_new(Session *s, dev_t dev, SessionDevice **out); +int session_device_new(Session *s, dev_t dev, bool open_device, SessionDevice **out); void session_device_free(SessionDevice *sd); void session_device_complete_pause(SessionDevice *sd); void session_device_resume_all(Session *s); void session_device_pause_all(Session *s); unsigned int session_device_try_pause_all(Session *s); + +int session_device_save(SessionDevice *sd); +void session_device_attach_fd(SessionDevice *sd, int fd, bool active); diff --git a/src/login/logind-session.c b/src/login/logind-session.c index ae05877a68..42dfecaffb 100644 --- a/src/login/logind-session.c +++ b/src/login/logind-session.c @@ -152,6 +152,18 @@ void session_set_user(Session *s, User *u) { LIST_PREPEND(sessions_by_user, u->sessions, s); } +static void session_save_devices(Session *s, FILE *f) { + SessionDevice *sd; + Iterator i; + + if (!hashmap_isempty(s->devices)) { + fprintf(f, "DEVICES="); + HASHMAP_FOREACH(sd, s->devices, i) + fprintf(f, "%u:%u ", major(sd->dev), minor(sd->dev)); + fprintf(f, "\n"); + } +} + int session_save(Session *s) { _cleanup_free_ char *temp_path = NULL; _cleanup_fclose_ FILE *f = NULL; @@ -281,8 +293,10 @@ int session_save(Session *s) { s->timestamp.realtime, s->timestamp.monotonic); - if (s->controller) + if (s->controller) { fprintf(f, "CONTROLLER=%s\n", s->controller); + session_save_devices(s, f); + } r = fflush_and_check(f); if (r < 0) @@ -304,6 +318,43 @@ fail: return log_error_errno(r, "Failed to save session data %s: %m", s->state_file); } +static int session_load_devices(Session *s, const char *devices) { + const char *p; + int r = 0; + + assert(s); + + for (p = devices;;) { + _cleanup_free_ char *word = NULL; + SessionDevice *sd; + dev_t dev; + int k; + + k = extract_first_word(&p, &word, NULL, 0); + if (k == 0) + break; + if (k < 0) { + r = k; + break; + } + + k = parse_dev(word, &dev); + if (k < 0) { + r = k; + continue; + } + + /* The file descriptors for loaded devices will be reattached later. */ + k = session_device_new(s, dev, false, &sd); + if (k < 0) + r = k; + } + + if (r < 0) + log_error_errno(r, "Loading session devices for session %s failed: %m", s->id); + + return r; +} int session_load(Session *s) { _cleanup_free_ char *remote = NULL, @@ -317,7 +368,9 @@ int session_load(Session *s) { *uid = NULL, *realtime = NULL, *monotonic = NULL, - *controller = NULL; + *controller = NULL, + *active = NULL, + *devices = NULL; int k, r; @@ -345,6 +398,8 @@ int session_load(Session *s) { "REALTIME", &realtime, "MONOTONIC", &monotonic, "CONTROLLER", &controller, + "ACTIVE", &active, + "DEVICES", &devices, NULL); if (r < 0) @@ -447,10 +502,17 @@ int session_load(Session *s) { if (monotonic) timestamp_deserialize(monotonic, &s->timestamp.monotonic); + if (active) { + k = parse_boolean(active); + if (k >= 0) + s->was_active = k; + } + if (controller) { - if (bus_name_has_owner(s->manager->bus, controller, NULL) > 0) + if (bus_name_has_owner(s->manager->bus, controller, NULL) > 0) { session_set_controller(s, controller, false, false); - else + session_load_devices(s, devices); + } else session_restore_vt(s); } diff --git a/src/login/logind-session.h b/src/login/logind-session.h index c8a3152acb..12b9d86f55 100644 --- a/src/login/logind-session.h +++ b/src/login/logind-session.h @@ -111,6 +111,8 @@ struct Session { bool started:1; bool stopping:1; + bool was_active:1; + sd_bus_message *create_message; sd_event_source *timer_event_source; diff --git a/src/login/logind.c b/src/login/logind.c index 1e2acc838b..fb70972b80 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -409,10 +409,71 @@ static int manager_enumerate_users(Manager *m) { return r; } +static int manager_attach_fds(Manager *m) { + _cleanup_strv_free_ char **fdnames = NULL; + int n, i, fd; + + /* Upon restart, PID1 will send us back all fds of session devices + * that we previously opened. Each file descriptor is associated + * with a given session. The session ids are passed through FDNAMES. */ + + n = sd_listen_fds_with_names(true, &fdnames); + if (n <= 0) + return n; + + for (i = 0; i < n; i++) { + struct stat st; + SessionDevice *sd; + Session *s; + char *id; + + fd = SD_LISTEN_FDS_START + i; + + id = startswith(fdnames[i], "session-"); + if (!id) + continue; + + s = hashmap_get(m->sessions, id); + if (!s) { + /* If the session doesn't exist anymore, the associated session + * device attached to this fd doesn't either. Let's simply close + * this fd. */ + log_debug("Failed to attach fd for unknown session: %s", id); + close_nointr(fd); + continue; + } + + if (fstat(fd, &st) < 0) { + /* The device is allowed to go away at a random point, in which + * case fstat failing is expected. */ + log_debug_errno(errno, "Failed to stat device fd for session %s: %m", id); + close_nointr(fd); + continue; + } + + sd = hashmap_get(s->devices, &st.st_rdev); + if (!sd) { + /* Weird we got an fd for a session device which wasn't + * recorded in the session state file... */ + log_warning("Got fd for missing session device [%u:%u] in session %s", + major(st.st_rdev), minor(st.st_rdev), s->id); + close_nointr(fd); + continue; + } + + log_debug("Attaching fd to session device [%u:%u] for session %s", + major(st.st_rdev), minor(st.st_rdev), s->id); + + session_device_attach_fd(sd, fd, s->was_active); + } + + return 0; +} + static int manager_enumerate_sessions(Manager *m) { _cleanup_closedir_ DIR *d = NULL; struct dirent *de; - int r = 0; + int r = 0, k; assert(m); @@ -427,7 +488,6 @@ static int manager_enumerate_sessions(Manager *m) { FOREACH_DIRENT(de, d, return -errno) { struct Session *s; - int k; if (!dirent_is_file(de)) continue; @@ -441,7 +501,6 @@ static int manager_enumerate_sessions(Manager *m) { k = manager_add_session(m, de->d_name, &s); if (k < 0) { log_error_errno(k, "Failed to add session by file name %s: %m", de->d_name); - r = k; continue; } @@ -453,6 +512,12 @@ static int manager_enumerate_sessions(Manager *m) { r = k; } + /* We might be restarted and PID1 could have sent us back the + * session device fds we previously saved. */ + k = manager_attach_fds(m); + if (k < 0) + log_warning_errno(k, "Failed to reattach session device fds: %m"); + return r; } diff --git a/units/systemd-logind.service.in b/units/systemd-logind.service.in index e20a3ad057..22d74f51d9 100644 --- a/units/systemd-logind.service.in +++ b/units/systemd-logind.service.in @@ -31,6 +31,7 @@ RestrictNamespaces=yes RestrictAddressFamilies=AF_UNIX AF_NETLINK AF_INET AF_INET6 SystemCallFilter=~@clock @cpu-emulation @debug @keyring @module @obsolete @raw-io @reboot @swap SystemCallArchitectures=native +FileDescriptorStoreMax=512 # Increase the default a bit in order to allow many simultaneous # logins since we keep one fd open per session.