From 5518b72ba8e4e3e367f384387e8e22c4ecf1e02c Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 15 Feb 2024 00:43:14 +0800 Subject: [PATCH 1/5] login/logind-session-dbus: some follow-ups for 'user-incomplete' (#30226) We don't usually say ", refusing" in bus error messages. Also, make use of unref_and_replace_full. --- src/login/logind-session-dbus.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/login/logind-session-dbus.c b/src/login/logind-session-dbus.c index bd8618ba50..7c28b37170 100644 --- a/src/login/logind-session-dbus.c +++ b/src/login/logind-session-dbus.c @@ -426,16 +426,17 @@ static int method_set_class(sd_bus_message *message, void *userdata, sd_bus_erro /* For now, we'll allow only upgrades user-incomplete → user */ if (class != SESSION_USER) return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, - "Class may only be set to 'user', refusing."); + "Class may only be set to 'user'"); + if (s->class == SESSION_USER) /* No change, shortcut */ return sd_bus_reply_method_return(message, NULL); if (s->class != SESSION_USER_INCOMPLETE) return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, - "Only sessions with class 'user-incomplete' may change class, refusing."); + "Only sessions with class 'user-incomplete' may change class"); if (s->upgrade_message) return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, - "Set session class operation already in progress, refsuing."); + "Set session class operation already in progress"); r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_EUID, &creds); if (r < 0) @@ -450,8 +451,7 @@ static int method_set_class(sd_bus_message *message, void *userdata, sd_bus_erro session_set_class(s, class); - sd_bus_message_unref(s->upgrade_message); - s->upgrade_message = sd_bus_message_ref(message); + unref_and_replace_full(s->upgrade_message, message, sd_bus_message_ref, sd_bus_message_unref); r = session_send_upgrade_reply(s, /* error= */ NULL); if (r < 0) From e2a42c0c43f6cdd7cb2b89521392248056eeac49 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 14 Jan 2024 02:38:11 +0800 Subject: [PATCH 2/5] logind-user: track user started/stopping state through user-runtime-dir@.service Before #30884, the user state is tied to user@.service (user service manager). However, #30884 introduced sessions that need no manager, and we can no longer rely on that. Consider the following situation: 1. A 'background-light' session '1' is created (i.e. no user service manager is needed) 2. Session '1' scope unit pulls in user-runtime-dir@.service 3. Session '1' exits. A stop job is enqueued for user-runtime-dir@.service due to StopWhenUnneeded=yes 4. At the same time, another session '2' which requires user manager is started. However, session scope units have JobMode=fail, therefore the start job for user-runtime-dir@.service that was pulled in by session '2' scope job is deleted as it conflicts with the stop job. We want session scope units to continue using JobMode=fail, but we still need the dependencies to be started correctly, i.e. explicitly requested by logind beforehand. Therefore, let's stop using StopWhenUnneeded=yes for user-runtime-dir@.service, and track users' `started` and `stopping` state based on that when user@.service is not needed. Then, for every invocation of user_start(), we'll recheck if we need the service manager and start it if so. Also, the dependency type on user-runtime-dir@.service from user@.service is upgraded to `BindsTo=`, in order to ensure that when logind stops the former, the latter is stopped as well. --- src/login/logind-dbus.c | 17 ++- src/login/logind-session-dbus.c | 19 ++- src/login/logind-session.c | 12 +- src/login/logind-user-dbus.c | 2 +- src/login/logind-user.c | 187 +++++++++++++++++++---------- src/login/logind-user.h | 22 ++-- units/user-runtime-dir@.service.in | 1 - units/user@.service.in | 2 +- 8 files changed, 171 insertions(+), 91 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 933b69542a..2f57cadc6d 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -4105,14 +4105,19 @@ int match_job_removed(sd_bus_message *message, void *userdata, sd_bus_error *err user = hashmap_get(m->user_units, unit); if (user) { /* If the user is stopping, we're tracking stop jobs here. So don't send reply. */ - if (!user->stopping && streq_ptr(path, user->service_job)) { - user->service_job = mfree(user->service_job); + if (!user->stopping) { + char **user_job; + FOREACH_ARGUMENT(user_job, &user->runtime_dir_job, &user->service_manager_job) + if (streq_ptr(path, *user_job)) { + *user_job = mfree(*user_job); - LIST_FOREACH(sessions_by_user, s, user->sessions) - /* Don't propagate user service failures to the client */ - session_jobs_reply(s, id, unit, /* error = */ NULL /* don't propagate user service failures to the client */); + LIST_FOREACH(sessions_by_user, s, user->sessions) + /* Don't propagate user service failures to the client */ + session_jobs_reply(s, id, unit, /* error = */ NULL); - user_save(user); + user_save(user); + break; + } } user_add_to_gc_queue(user); diff --git a/src/login/logind-session-dbus.c b/src/login/logind-session-dbus.c index 7c28b37170..ae71e2fc7b 100644 --- a/src/login/logind-session-dbus.c +++ b/src/login/logind-session-dbus.c @@ -868,13 +868,17 @@ int session_send_lock_all(Manager *m, bool lock) { return r; } -static bool session_ready(Session *s) { +static bool session_job_pending(Session *s) { assert(s); + assert(s->user); - /* Returns true when the session is ready, i.e. all jobs we enqueued for it are done (regardless if successful or not) */ + /* Check if we have some jobs enqueued and not finished yet. Each time we get JobRemoved signal about + * relevant units, session_send_create_reply and hence us is called (see match_job_removed). + * Note that we don't care about job result here. */ - return !s->scope_job && - (!SESSION_CLASS_WANTS_SERVICE_MANAGER(s->class) || !s->user->service_job); + return s->scope_job || + s->user->runtime_dir_job || + (SESSION_CLASS_WANTS_SERVICE_MANAGER(s->class) && s->user->service_manager_job); } int session_send_create_reply(Session *s, sd_bus_error *error) { @@ -890,7 +894,9 @@ int session_send_create_reply(Session *s, sd_bus_error *error) { if (!s->create_message) return 0; - if (!sd_bus_error_is_set(error) && !session_ready(s)) + /* If error occurred, return it immediately. Otherwise let's wait for all jobs to finish before + * continuing. */ + if (!sd_bus_error_is_set(error) && session_job_pending(s)) return 0; c = TAKE_PTR(s->create_message); @@ -938,7 +944,8 @@ int session_send_upgrade_reply(Session *s, sd_bus_error *error) { if (!s->upgrade_message) return 0; - if (!sd_bus_error_is_set(error) && !session_ready(s)) + /* See comments in session_send_create_reply */ + if (!sd_bus_error_is_set(error) && session_job_pending(s)) return 0; c = TAKE_PTR(s->upgrade_message); diff --git a/src/login/logind-session.c b/src/login/logind-session.c index 0a2f04b021..cc6e84a7c7 100644 --- a/src/login/logind-session.c +++ b/src/login/logind-session.c @@ -740,8 +740,8 @@ static int session_start_scope(Session *s, sd_bus_message *properties, sd_bus_er description = strjoina("Session ", s->id, " of User ", s->user->user_record->user_name); /* These two have StopWhenUnneeded= set, hence add a dep towards them */ - wants = strv_new(s->user->runtime_dir_service, - SESSION_CLASS_WANTS_SERVICE_MANAGER(s->class) ? s->user->service : STRV_IGNORE); + wants = strv_new(s->user->runtime_dir_unit, + SESSION_CLASS_WANTS_SERVICE_MANAGER(s->class) ? s->user->service_manager_unit : STRV_IGNORE); if (!wants) return log_oom(); @@ -752,9 +752,9 @@ static int session_start_scope(Session *s, sd_bus_message *properties, sd_bus_er * ordering after systemd-user-sessions.service and the user instance is optional we make use * of STRV_IGNORE with strv_new() to skip these order constraints when needed. */ after = strv_new("systemd-logind.service", - s->user->runtime_dir_service, + s->user->runtime_dir_unit, SESSION_CLASS_IS_EARLY(s->class) ? STRV_IGNORE : "systemd-user-sessions.service", - s->user->service); + s->user->service_manager_unit); if (!after) return log_oom(); @@ -1221,8 +1221,8 @@ void session_set_class(Session *s, SessionClass c) { (void) session_save(s); (void) session_send_changed(s, "Class", NULL); - /* This class change might mean we need the per-user session manager now. Try to start it */ - user_start_service_manager(s->user); + /* This class change might mean we need the per-user session manager now. Try to start it. */ + (void) user_start_service_manager(s->user); } int session_set_display(Session *s, const char *display) { diff --git a/src/login/logind-user-dbus.c b/src/login/logind-user-dbus.c index 0763a5b03f..d277aed2a9 100644 --- a/src/login/logind-user-dbus.c +++ b/src/login/logind-user-dbus.c @@ -356,7 +356,7 @@ static const sd_bus_vtable user_vtable[] = { SD_BUS_PROPERTY("Name", "s", property_get_name, 0, SD_BUS_VTABLE_PROPERTY_CONST), BUS_PROPERTY_DUAL_TIMESTAMP("Timestamp", offsetof(User, timestamp), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("RuntimePath", "s", NULL, offsetof(User, runtime_path), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("Service", "s", NULL, offsetof(User, service), SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("Service", "s", NULL, offsetof(User, service_manager_unit), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Slice", "s", NULL, offsetof(User, slice), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Display", "(so)", property_get_display, 0, SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), SD_BUS_PROPERTY("State", "s", property_get_state, 0, 0), diff --git a/src/login/logind-user.c b/src/login/logind-user.c index 10b8be2233..a14c9b7cfb 100644 --- a/src/login/logind-user.c +++ b/src/login/logind-user.c @@ -77,11 +77,11 @@ int user_new(User **ret, if (r < 0) return r; - r = unit_name_build("user", lu, ".service", &u->service); + r = unit_name_build("user-runtime-dir", lu, ".service", &u->runtime_dir_unit); if (r < 0) return r; - r = unit_name_build("user-runtime-dir", lu, ".service", &u->runtime_dir_service); + r = unit_name_build("user", lu, ".service", &u->service_manager_unit); if (r < 0) return r; @@ -93,11 +93,11 @@ int user_new(User **ret, if (r < 0) return r; - r = hashmap_put(m->user_units, u->service, u); + r = hashmap_put(m->user_units, u->runtime_dir_unit, u); if (r < 0) return r; - r = hashmap_put(m->user_units, u->runtime_dir_service, u); + r = hashmap_put(m->user_units, u->service_manager_unit, u); if (r < 0) return r; @@ -115,24 +115,27 @@ User *user_free(User *u) { while (u->sessions) session_free(u->sessions); - if (u->service) - (void) hashmap_remove_value(u->manager->user_units, u->service, u); + sd_event_source_unref(u->timer_event_source); - if (u->runtime_dir_service) - (void) hashmap_remove_value(u->manager->user_units, u->runtime_dir_service, u); + if (u->service_manager_unit) { + (void) hashmap_remove_value(u->manager->user_units, u->service_manager_unit, u); + free(u->service_manager_job); + free(u->service_manager_unit); + } - if (u->slice) + if (u->runtime_dir_unit) { + (void) hashmap_remove_value(u->manager->user_units, u->runtime_dir_unit, u); + free(u->runtime_dir_job); + free(u->runtime_dir_unit); + } + + if (u->slice) { (void) hashmap_remove_value(u->manager->user_units, u->slice, u); + free(u->slice); + } (void) hashmap_remove_value(u->manager->users, UID_TO_PTR(u->user_record->uid), u); - sd_event_source_unref(u->timer_event_source); - - free(u->service_job); - - free(u->service); - free(u->runtime_dir_service); - free(u->slice); free(u->runtime_path); free(u->state_file); @@ -174,8 +177,11 @@ static int user_save_internal(User *u) { if (u->runtime_path) fprintf(f, "RUNTIME=%s\n", u->runtime_path); - if (u->service_job) - fprintf(f, "SERVICE_JOB=%s\n", u->service_job); + if (u->runtime_dir_job) + fprintf(f, "RUNTIME_DIR_JOB=%s\n", u->runtime_dir_job); + + if (u->service_manager_job) + fprintf(f, "SERVICE_JOB=%s\n", u->service_manager_job); if (u->display) fprintf(f, "DISPLAY=%s\n", u->display->id); @@ -311,7 +317,8 @@ int user_load(User *u) { assert(u); r = parse_env_file(NULL, u->state_file, - "SERVICE_JOB", &u->service_job, + "RUNTIME_DIR_JOB", &u->runtime_dir_job, + "SERVICE_JOB", &u->service_manager_job, "STOPPING", &stopping, "REALTIME", &realtime, "MONOTONIC", &monotonic, @@ -326,8 +333,11 @@ int user_load(User *u) { r = parse_boolean(stopping); if (r < 0) log_debug_errno(r, "Failed to parse 'STOPPING' boolean: %s", stopping); - else + else { u->stopping = r; + if (u->stopping && !u->runtime_dir_job) + log_debug("User '%s' is stopping, but no job is being tracked.", u->user_record->user_name); + } } if (realtime) @@ -344,6 +354,26 @@ int user_load(User *u) { return 0; } +static int user_start_runtime_dir(User *u) { + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + int r; + + assert(u); + assert(!u->stopping); + assert(u->manager); + assert(u->runtime_dir_unit); + + u->runtime_dir_job = mfree(u->runtime_dir_job); + + r = manager_start_unit(u->manager, u->runtime_dir_unit, &error, &u->runtime_dir_job); + if (r < 0) + return log_full_errno(sd_bus_error_has_name(&error, BUS_ERROR_UNIT_MASKED) ? LOG_DEBUG : LOG_ERR, + r, "Failed to start user service '%s': %s", + u->runtime_dir_unit, bus_error_message(&error, r)); + + return 0; +} + static bool user_wants_service_manager(User *u) { assert(u); @@ -354,28 +384,34 @@ static bool user_wants_service_manager(User *u) { return false; } -void user_start_service_manager(User *u) { +int user_start_service_manager(User *u) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; int r; assert(u); + assert(!u->stopping); + assert(u->manager); + assert(u->service_manager_unit); - /* Start the service containing the "systemd --user" instance (user@.service). Note that we don't explicitly - * start the per-user slice or the systemd-runtime-dir@.service instance, as those are pulled in both by - * user@.service and the session scopes as dependencies. */ + if (u->service_manager_started) + return 1; - if (u->stopping) /* Don't try to start this if the user is going down */ - return; + /* Only start user service manager if there's at least one session which wants it */ + if (!user_wants_service_manager(u)) + return 0; - if (!user_wants_service_manager(u)) /* Only start user service manager if there's at least one session which wants it */ - return; + u->service_manager_job = mfree(u->service_manager_job); - u->service_job = mfree(u->service_job); + r = manager_start_unit(u->manager, u->service_manager_unit, &error, &u->service_manager_job); + if (r < 0) { + if (sd_bus_error_has_name(&error, BUS_ERROR_UNIT_MASKED)) + return 0; - r = manager_start_unit(u->manager, u->service, &error, &u->service_job); - if (r < 0) - log_full_errno(sd_bus_error_has_name(&error, BUS_ERROR_UNIT_MASKED) ? LOG_DEBUG : LOG_WARNING, r, - "Failed to start user service '%s', ignoring: %s", u->service, bus_error_message(&error, r)); + return log_error_errno(r, "Failed to start user service '%s': %s", + u->service_manager_unit, bus_error_message(&error, r)); + } + + return (u->service_manager_started = true); } static int update_slice_callback(sd_bus_message *m, void *userdata, sd_bus_error *ret_error) { @@ -460,33 +496,47 @@ static int user_update_slice(User *u) { } int user_start(User *u) { + int r; + assert(u); - if (u->started && !u->stopping) + if (u->service_manager_started) { + /* Everything is up. No action needed. */ + assert(u->started && !u->stopping); return 0; + } - /* If u->stopping is set, the user is marked for removal and service stop-jobs are queued. We have to clear - * that flag before queueing the start-jobs again. If they succeed, the user object can be re-used just fine - * (pid1 takes care of job-ordering and proper restart), but if they fail, we want to force another user_stop() - * so possibly pending units are stopped. */ - u->stopping = false; + if (!u->started || u->stopping) { + /* If u->stopping is set, the user is marked for removal and service stop-jobs are queued. + * We have to clear that flag before queueing the start-jobs again. If they succeed, the + * user object can be re-used just fine (pid1 takes care of job-ordering and proper restart), + * but if they fail, we want to force another user_stop() so possibly pending units are + * stopped. */ + u->stopping = false; - if (!u->started) - log_debug("Tracking new user %s.", u->user_record->user_name); + if (!u->started) + log_debug("Tracking new user %s.", u->user_record->user_name); - /* Save the user data so far, because pam_systemd will read the XDG_RUNTIME_DIR out of it while starting up - * systemd --user. We need to do user_save_internal() because we have not "officially" started yet. */ - user_save_internal(u); + /* Save the user data so far, because pam_systemd will read the XDG_RUNTIME_DIR out of it + * while starting up systemd --user. We need to do user_save_internal() because we have not + * "officially" started yet. */ + user_save_internal(u); - /* Set slice parameters */ - (void) user_update_slice(u); + /* Set slice parameters */ + (void) user_update_slice(u); - /* Start user@UID.service */ - user_start_service_manager(u); + (void) user_start_runtime_dir(u); + } + + /* Start user@UID.service if needed. */ + r = user_start_service_manager(u); + if (r < 0) + return r; if (!u->started) { if (!dual_timestamp_is_set(&u->timestamp)) dual_timestamp_now(&u->timestamp); + user_send_signal(u, true); u->started = true; } @@ -502,16 +552,22 @@ static void user_stop_service(User *u, bool force) { int r; assert(u); - assert(u->service); + assert(u->manager); + assert(u->runtime_dir_unit); - /* The reverse of user_start_service(). Note that we only stop user@UID.service here, and let StopWhenUnneeded= - * deal with the slice and the user-runtime-dir@.service instance. */ + /* Note that we only stop user-runtime-dir@.service here, and let BindsTo= deal with the user@.service + * instance. However, we still need to clear service_manager_job here, so that if the stop is + * interrupted, the new sessions won't be confused by leftovers. */ - u->service_job = mfree(u->service_job); + u->service_manager_job = mfree(u->service_manager_job); + u->service_manager_started = false; - r = manager_stop_unit(u->manager, u->service, force ? "replace" : "fail", &error, &u->service_job); + u->runtime_dir_job = mfree(u->runtime_dir_job); + + r = manager_stop_unit(u->manager, u->runtime_dir_unit, force ? "replace" : "fail", &error, &u->runtime_dir_job); if (r < 0) - log_warning_errno(r, "Failed to stop user service '%s', ignoring: %s", u->service, bus_error_message(&error, r)); + log_warning_errno(r, "Failed to stop user service '%s', ignoring: %s", + u->runtime_dir_unit, bus_error_message(&error, r)); } int user_stop(User *u, bool force) { @@ -638,11 +694,11 @@ int user_check_linger_file(User *u) { static bool user_unit_active(User *u) { int r; - assert(u->service); - assert(u->runtime_dir_service); assert(u->slice); + assert(u->runtime_dir_unit); + assert(u->service_manager_unit); - FOREACH_STRING(i, u->service, u->runtime_dir_service, u->slice) { + FOREACH_STRING(i, u->slice, u->runtime_dir_unit, u->service_manager_unit) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; r = manager_unit_is_active(u->manager, i, &error); @@ -722,18 +778,23 @@ bool user_may_gc(User *u, bool drop_not_started) { return false; /* Check if our job is still pending */ - if (u->service_job) { + const char *j; + FOREACH_ARGUMENT(j, u->runtime_dir_job, u->service_manager_job) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - r = manager_job_is_active(u->manager, u->service_job, &error); + if (!j) + continue; + + r = manager_job_is_active(u->manager, j, &error); if (r < 0) - log_debug_errno(r, "Failed to determine whether job '%s' is pending, ignoring: %s", u->service_job, bus_error_message(&error, r)); + log_debug_errno(r, "Failed to determine whether job '%s' is pending, ignoring: %s", + j, bus_error_message(&error, r)); if (r != 0) return false; } - /* Note that we don't care if the three units we manage for each user object are up or not, as we are managing - * their state rather than tracking it. */ + /* Note that we don't care if the three units we manage for each user object are up or not, as we are + * managing their state rather than tracking it. */ return true; } @@ -754,7 +815,7 @@ UserState user_get_state(User *u) { if (u->stopping) return USER_CLOSING; - if (!u->started || u->service_job) + if (!u->started || u->runtime_dir_job) return USER_OPENING; bool any = false, all_closing = true; diff --git a/src/login/logind-user.h b/src/login/logind-user.h index 9bda5dde42..723b66d189 100644 --- a/src/login/logind-user.h +++ b/src/login/logind-user.h @@ -34,11 +34,17 @@ struct User { char *state_file; char *runtime_path; - char *slice; /* user-UID.slice */ - char *service; /* user@UID.service */ - char *runtime_dir_service; /* user-runtime-dir@UID.service */ + /* user-UID.slice */ + char *slice; - char *service_job; + /* user-runtime-dir@UID.service */ + char *runtime_dir_unit; + char *runtime_dir_job; + + /* user@UID.service */ + bool service_manager_started; + char *service_manager_unit; + char *service_manager_job; Session *display; @@ -51,7 +57,8 @@ struct User { UserGCMode gc_mode; bool in_gc_queue:1; - bool started:1; /* Whenever the user being started, has been started or is being stopped again. */ + bool started:1; /* Whenever the user being started, has been started or is being stopped again + (tracked through user-runtime-dir@.service) */ bool stopping:1; /* Whenever the user is being stopped or has been stopped. */ LIST_HEAD(Session, sessions); @@ -63,10 +70,11 @@ User *user_free(User *u); DEFINE_TRIVIAL_CLEANUP_FUNC(User *, user_free); +int user_start_service_manager(User *u); +int user_start(User *u); + bool user_may_gc(User *u, bool drop_not_started); void user_add_to_gc_queue(User *u); -void user_start_service_manager(User *u); -int user_start(User *u); int user_stop(User *u, bool force); int user_finalize(User *u); UserState user_get_state(User *u); diff --git a/units/user-runtime-dir@.service.in b/units/user-runtime-dir@.service.in index 0641dd0b0a..5fb5cad36a 100644 --- a/units/user-runtime-dir@.service.in +++ b/units/user-runtime-dir@.service.in @@ -11,7 +11,6 @@ Description=User Runtime Directory /run/user/%i Documentation=man:user@.service(5) After=dbus.service -StopWhenUnneeded=yes IgnoreOnIsolate=yes [Service] diff --git a/units/user@.service.in b/units/user@.service.in index da5f98c994..5efb12a860 100644 --- a/units/user@.service.in +++ b/units/user@.service.in @@ -10,8 +10,8 @@ [Unit] Description=User Manager for UID %i Documentation=man:user@.service(5) +BindsTo=user-runtime-dir@%i.service After=user-runtime-dir@%i.service dbus.service systemd-oomd.service -Requires=user-runtime-dir@%i.service IgnoreOnIsolate=yes [Service] From 52bcc872b5f97a14a9b4e4e383f45bb3066e1643 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Fri, 12 Jan 2024 21:30:49 +0800 Subject: [PATCH 3/5] logind-session: use Requires= for user{,-runtime-dir}@.service Since we do require these basic user services, let's make the dependency stronger. Note that logind should enqueue start jobs for these already in user_start(), so mostly just paranoia. --- src/login/logind-dbus.c | 22 ++++++----- src/login/logind-dbus.h | 13 ++++++- src/login/logind-session.c | 80 ++++++++++++++++---------------------- 3 files changed, 59 insertions(+), 56 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 2f57cadc6d..b87edb15dc 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -4247,12 +4247,12 @@ int manager_start_scope( const PidRef *pidref, const char *slice, const char *description, - char **wants, - char **after, + const char * const *requires, + const char * const *extra_after, const char *requires_mounts_for, sd_bus_message *more_properties, sd_bus_error *error, - char **job) { + char **ret_job) { _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL, *reply = NULL; int r; @@ -4260,13 +4260,13 @@ int manager_start_scope( assert(manager); assert(scope); assert(pidref_is_set(pidref)); - assert(job); + assert(ret_job); r = bus_message_new_method_call(manager->bus, &m, bus_systemd_mgr, "StartTransientUnit"); if (r < 0) return r; - r = sd_bus_message_append(m, "ss", strempty(scope), "fail"); + r = sd_bus_message_append(m, "ss", scope, "fail"); if (r < 0) return r; @@ -4286,13 +4286,17 @@ int manager_start_scope( return r; } - STRV_FOREACH(i, wants) { - r = sd_bus_message_append(m, "(sv)", "Wants", "as", 1, *i); + STRV_FOREACH(i, requires) { + r = sd_bus_message_append(m, "(sv)", "Requires", "as", 1, *i); + if (r < 0) + return r; + + r = sd_bus_message_append(m, "(sv)", "After", "as", 1, *i); if (r < 0) return r; } - STRV_FOREACH(i, after) { + STRV_FOREACH(i, extra_after) { r = sd_bus_message_append(m, "(sv)", "After", "as", 1, *i); if (r < 0) return r; @@ -4344,7 +4348,7 @@ int manager_start_scope( if (r < 0) return r; - return strdup_job(reply, job); + return strdup_job(reply, ret_job); } int manager_start_unit(Manager *manager, const char *unit, sd_bus_error *error, char **job) { diff --git a/src/login/logind-dbus.h b/src/login/logind-dbus.h index c9d59231d4..61ce035a48 100644 --- a/src/login/logind-dbus.h +++ b/src/login/logind-dbus.h @@ -24,7 +24,18 @@ int match_reloading(sd_bus_message *message, void *userdata, sd_bus_error *error int manager_send_changed(Manager *manager, const char *property, ...) _sentinel_; -int manager_start_scope(Manager *manager, const char *scope, const PidRef *pidref, const char *slice, const char *description, char **wants, char **after, const char *requires_mounts_for, sd_bus_message *more_properties, sd_bus_error *error, char **job); +int manager_start_scope( + Manager *manager, + const char *scope, + const PidRef *pidref, + const char *slice, + const char *description, + const char * const *requires, + const char * const *extra_after, + const char *requires_mounts_for, + sd_bus_message *more_properties, + sd_bus_error *error, + char **ret_job); int manager_start_unit(Manager *manager, const char *unit, sd_bus_error *error, char **job); int manager_stop_unit(Manager *manager, const char *unit, const char *job_mode, sd_bus_error *error, char **job); int manager_abandon_scope(Manager *manager, const char *scope, sd_bus_error *error); diff --git a/src/login/logind-session.c b/src/login/logind-session.c index cc6e84a7c7..787855a7b5 100644 --- a/src/login/logind-session.c +++ b/src/login/logind-session.c @@ -718,6 +718,8 @@ int session_activate(Session *s) { } static int session_start_scope(Session *s, sd_bus_message *properties, sd_bus_error *error) { + _cleanup_free_ char *scope = NULL; + const char *description; int r; assert(s); @@ -726,59 +728,45 @@ static int session_start_scope(Session *s, sd_bus_message *properties, sd_bus_er if (!SESSION_CLASS_WANTS_SCOPE(s->class)) return 0; - if (!s->scope) { - _cleanup_strv_free_ char **wants = NULL, **after = NULL; - _cleanup_free_ char *scope = NULL; - const char *description; + if (s->scope) + goto finish; - s->scope_job = mfree(s->scope_job); + s->scope_job = mfree(s->scope_job); - scope = strjoin("session-", s->id, ".scope"); - if (!scope) - return log_oom(); + scope = strjoin("session-", s->id, ".scope"); + if (!scope) + return log_oom(); - description = strjoina("Session ", s->id, " of User ", s->user->user_record->user_name); + description = strjoina("Session ", s->id, " of User ", s->user->user_record->user_name); - /* These two have StopWhenUnneeded= set, hence add a dep towards them */ - wants = strv_new(s->user->runtime_dir_unit, - SESSION_CLASS_WANTS_SERVICE_MANAGER(s->class) ? s->user->service_manager_unit : STRV_IGNORE); - if (!wants) - return log_oom(); + r = manager_start_scope( + s->manager, + scope, + &s->leader, + s->user->slice, + description, + /* These should have been pulled in explicitly in user_start(). Just to be sure. */ + STRV_MAKE_CONST(s->user->runtime_dir_unit, + SESSION_CLASS_WANTS_SERVICE_MANAGER(s->class) ? s->user->service_manager_unit : NULL), + /* We usually want to order session scopes after systemd-user-sessions.service + * since the unit is used as login session barrier for unprivileged users. However + * the barrier doesn't apply for root as sysadmin should always be able to log in + * (and without waiting for any timeout to expire) in case something goes wrong + * during the boot process. */ + STRV_MAKE_CONST("systemd-logind.service", + SESSION_CLASS_IS_EARLY(s->class) ? NULL : "systemd-user-sessions.service"), + user_record_home_directory(s->user->user_record), + properties, + error, + &s->scope_job); + if (r < 0) + return log_error_errno(r, "Failed to start session scope %s: %s", + scope, bus_error_message(error, r)); - /* We usually want to order session scopes after systemd-user-sessions.service since the - * latter unit is used as login session barrier for unprivileged users. However the barrier - * doesn't apply for root as sysadmin should always be able to log in (and without waiting - * for any timeout to expire) in case something goes wrong during the boot process. Since - * ordering after systemd-user-sessions.service and the user instance is optional we make use - * of STRV_IGNORE with strv_new() to skip these order constraints when needed. */ - after = strv_new("systemd-logind.service", - s->user->runtime_dir_unit, - SESSION_CLASS_IS_EARLY(s->class) ? STRV_IGNORE : "systemd-user-sessions.service", - s->user->service_manager_unit); - if (!after) - return log_oom(); - - r = manager_start_scope( - s->manager, - scope, - &s->leader, - s->user->slice, - description, - wants, - after, - user_record_home_directory(s->user->user_record), - properties, - error, - &s->scope_job); - if (r < 0) - return log_error_errno(r, "Failed to start session scope %s: %s", - scope, bus_error_message(error, r)); - - s->scope = TAKE_PTR(scope); - } + s->scope = TAKE_PTR(scope); +finish: (void) hashmap_put(s->manager->session_units, s->scope, s); - return 0; } From 9128fd553cf531b58606810dea7b2b93ccfd7f65 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 13 Feb 2024 12:59:00 +0800 Subject: [PATCH 4/5] env-util: minor modernization --- src/basic/env-util.c | 16 ++++++---------- src/basic/env-util.h | 5 +++-- src/core/manager.c | 3 +-- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index b96fbdff48..3bb2654657 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -310,19 +310,17 @@ char **strv_env_delete(char **x, size_t n_lists, ...) { return TAKE_PTR(t); } -char **strv_env_unset(char **l, const char *p) { - char **f, **t; +char** strv_env_unset(char **l, const char *p) { + assert(p); if (!l) return NULL; - assert(p); - /* Drops every occurrence of the env var setting p in the * string list. Edits in-place. */ + char **f, **t; for (f = t = l; *f; f++) { - if (env_match(*f, p)) { free(*f); continue; @@ -335,14 +333,13 @@ char **strv_env_unset(char **l, const char *p) { return l; } -char **strv_env_unset_many(char **l, ...) { - char **f, **t; - +char** strv_env_unset_many_internal(char **l, ...) { if (!l) return NULL; /* Like strv_env_unset() but applies many at once. Edits in-place. */ + char **f, **t; for (f = t = l; *f; f++) { bool found = false; const char *p; @@ -350,12 +347,11 @@ char **strv_env_unset_many(char **l, ...) { va_start(ap, l); - while ((p = va_arg(ap, const char*))) { + while ((p = va_arg(ap, const char*))) if (env_match(*f, p)) { found = true; break; } - } va_end(ap); diff --git a/src/basic/env-util.h b/src/basic/env-util.h index 31680b160c..15d867a3d9 100644 --- a/src/basic/env-util.h +++ b/src/basic/env-util.h @@ -43,8 +43,9 @@ char** _strv_env_merge(char **first, ...); #define strv_env_merge(first, ...) _strv_env_merge(first, __VA_ARGS__, POINTER_MAX) char **strv_env_delete(char **x, size_t n_lists, ...); /* New copy */ -char **strv_env_unset(char **l, const char *p); /* In place ... */ -char **strv_env_unset_many(char **l, ...) _sentinel_; +char** strv_env_unset(char **l, const char *p); /* In place ... */ +char** strv_env_unset_many_internal(char **l, ...) _sentinel_; +#define strv_env_unset_many(l, ...) strv_env_unset_many_internal(l, __VA_ARGS__, NULL) int strv_env_replace_consume(char ***l, char *p); /* In place ... */ int strv_env_replace_strdup(char ***l, const char *assignment); int strv_env_replace_strdup_passthrough(char ***l, const char *assignment); diff --git a/src/core/manager.c b/src/core/manager.c index 5014c3e1c8..c17bd5c8df 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -641,8 +641,7 @@ static char** sanitize_environment(char **l) { "TRIGGER_TIMER_REALTIME_USEC", "TRIGGER_UNIT", "WATCHDOG_PID", - "WATCHDOG_USEC", - NULL); + "WATCHDOG_USEC"); /* Let's order the environment alphabetically, just to make it pretty */ return strv_sort(l); From 0ba24952f58f21ff89b726eaf02d847f0fee28d1 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 13 Feb 2024 12:47:53 +0800 Subject: [PATCH 5/5] core/manager: don't propagate manager session env to children Follow-up for 4cb4e6cf6dce2b66dcb59a8534aa6ca885e2f732 Fixes #31287 --- src/core/manager.c | 14 +++++++++++++- src/login/pam_systemd.c | 3 +++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/core/manager.c b/src/core/manager.c index c17bd5c8df..e8c747d96d 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -667,7 +667,9 @@ int manager_default_environment(Manager *m) { /* Import locale variables LC_*= from configuration */ (void) locale_setup(&m->transient_environment); } else { - /* The user manager passes its own environment along to its children, except for $PATH. */ + /* The user manager passes its own environment along to its children, except for $PATH and + * session envs. */ + m->transient_environment = strv_copy(environ); if (!m->transient_environment) return log_oom(); @@ -675,6 +677,16 @@ int manager_default_environment(Manager *m) { r = strv_env_replace_strdup(&m->transient_environment, "PATH=" DEFAULT_USER_PATH); if (r < 0) return log_oom(); + + /* Envvars set for our 'manager' class session are private and should not be propagated + * to children. Also it's likely that the graphical session will set these on their own. */ + strv_env_unset_many(m->transient_environment, + "XDG_SESSION_ID", + "XDG_SESSION_CLASS", + "XDG_SESSION_TYPE", + "XDG_SESSION_DESKTOP", + "XDG_SEAT", + "XDG_VTNR"); } sanitize_environment(m->transient_environment); diff --git a/src/login/pam_systemd.c b/src/login/pam_systemd.c index 0e67d063a4..9aa298c654 100644 --- a/src/login/pam_systemd.c +++ b/src/login/pam_systemd.c @@ -1150,6 +1150,9 @@ _public_ PAM_EXTERN int pam_sm_open_session( "id=%s object_path=%s runtime_path=%s session_fd=%d seat=%s vtnr=%u original_uid=%u", id, object_path, runtime_path, session_fd, seat, vtnr, original_uid); + /* Please update manager_default_environment() in core/manager.c accordingly if more session envvars + * shall be added. */ + r = update_environment(handle, "XDG_SESSION_ID", id); if (r != PAM_SUCCESS) return r;