From e73bf3425c0b5a6339388a3a459ce2bac497308b Mon Sep 17 00:00:00 2001 From: Michal Sekletar Date: Fri, 27 May 2022 21:11:37 +0200 Subject: [PATCH 1/3] logind: don't start user@UID.service instance for background sessions We have had background session class for a long time (since commit e2acb67baa), but so far the only difference in handling of background sessions was logging, i.e. we log some messages with LOG_DEBUG for such sessions. Previously there were complains [1] about excessive logging for each time cron session is started. We used to advise user to enable lingering for users if they want to avoid these log messages. However, on servers with a lot of users the extra processes that result from lingering just adds too much overhead. Hence I think that our current handling of background sessions is not ideal and we should make better use of this attribute. This commit introduces a change in default behavior of logind. Logind is now not going to start user instance of systemd when background session is created and that should address excessive logging problem for cron where background class is used by default. When the same user actually logs in normally then user instance will be started as previously. Also note that PAM_TTY variable is now always set to some value for PAM sessions started via PAMName= option. Otherwise we would categorize such sessions as "background" and user manager won't be started. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1825942 --- src/core/execute.c | 12 +++++++----- src/login/logind-dbus.c | 2 +- src/login/logind-session.c | 6 +++--- src/login/logind-user.c | 5 +++-- src/login/logind-user.h | 2 +- src/login/logind.c | 5 +---- src/login/pam_systemd.c | 5 ++++- 7 files changed, 20 insertions(+), 17 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 05fc00ca1c..962edc1eee 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -1213,13 +1213,15 @@ static int setup_pam( if (getttyname_malloc(STDIN_FILENO, &q) >= 0) tty = strjoina("/dev/", q); + else + /* If everything else failed then let's just use value "systemd". This will cause that session + * isn't going to be marked as "background" and user manager will be started. */ + tty = "systemd"; } - if (tty) { - pam_code = pam_set_item(handle, PAM_TTY, tty); - if (pam_code != PAM_SUCCESS) - goto fail; - } + pam_code = pam_set_item(handle, PAM_TTY, tty); + if (pam_code != PAM_SUCCESS) + goto fail; STRV_FOREACH(nv, *env) { pam_code = pam_putenv(handle, *nv); diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index a426842bbb..e308e52af3 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -1305,7 +1305,7 @@ static int method_set_user_linger(sd_bus_message *message, void *userdata, sd_bu return r; if (manager_add_user_by_uid(m, uid, &u) >= 0) - user_start(u); + user_start(u, /* want_user_instance= */ true); } else { User *u; diff --git a/src/login/logind-session.c b/src/login/logind-session.c index 22bb4c3714..5808630503 100644 --- a/src/login/logind-session.c +++ b/src/login/logind-session.c @@ -664,12 +664,12 @@ static int session_start_scope(Session *s, sd_bus_message *properties, sd_bus_er description, /* These two have StopWhenUnneeded= set, hence add a dep towards them */ STRV_MAKE(s->user->runtime_dir_service, - s->user->service), + s->class != SESSION_BACKGROUND ? s->user->service : NULL), /* And order us after some more */ STRV_MAKE("systemd-logind.service", "systemd-user-sessions.service", s->user->runtime_dir_service, - s->user->service), + s->class != SESSION_BACKGROUND ? s->user->service : NULL), user_record_home_directory(s->user->user_record), properties, error, @@ -700,7 +700,7 @@ int session_start(Session *s, sd_bus_message *properties, sd_bus_error *error) { if (s->started) return 0; - r = user_start(s->user); + r = user_start(s->user, /* want_user_instance= */ s->class != SESSION_BACKGROUND); if (r < 0) return r; diff --git a/src/login/logind-user.c b/src/login/logind-user.c index 3f1b8f610b..5f6cd590ce 100644 --- a/src/login/logind-user.c +++ b/src/login/logind-user.c @@ -442,7 +442,7 @@ static int user_update_slice(User *u) { return 0; } -int user_start(User *u) { +int user_start(User *u, bool want_user_instance) { assert(u); if (u->started && !u->stopping) @@ -465,7 +465,8 @@ int user_start(User *u) { (void) user_update_slice(u); /* Start user@UID.service */ - user_start_service(u); + if (want_user_instance) + user_start_service(u); if (!u->started) { if (!dual_timestamp_is_set(&u->timestamp)) diff --git a/src/login/logind-user.h b/src/login/logind-user.h index 21b9f8f348..a20775ae70 100644 --- a/src/login/logind-user.h +++ b/src/login/logind-user.h @@ -57,7 +57,7 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(User *, user_free); bool user_may_gc(User *u, bool drop_not_started); void user_add_to_gc_queue(User *u); -int user_start(User *u); +int user_start(User *u, bool need_user_instance); int user_stop(User *u, bool force); int user_finalize(User *u); UserState user_get_state(User *u); diff --git a/src/login/logind.c b/src/login/logind.c index d14a17274b..2fdcd1d55e 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -1041,7 +1041,6 @@ static int manager_startup(Manager *m) { int r; Seat *seat; Session *session; - User *user; Button *button; Inhibitor *inhibitor; @@ -1118,9 +1117,7 @@ static int manager_startup(Manager *m) { HASHMAP_FOREACH(seat, m->seats) (void) seat_start(seat); - HASHMAP_FOREACH(user, m->users) - (void) user_start(user); - + /* Users are started by respective sessions */ HASHMAP_FOREACH(session, m->sessions) (void) session_start(session, NULL, NULL); diff --git a/src/login/pam_systemd.c b/src/login/pam_systemd.c index cb6a6fb514..677a014078 100644 --- a/src/login/pam_systemd.c +++ b/src/login/pam_systemd.c @@ -790,7 +790,10 @@ _public_ PAM_EXTERN int pam_sm_open_session( * does the PAM session registration early for new connections, and registers a pty only * much later (this is because it doesn't know yet if it needs one at all, as whether to * register a pty or not is negotiated much later in the protocol). */ - + } else if (streq(tty, "systemd")) { + if (isempty(class)) + class = "user"; + tty = NULL; } else /* Chop off leading /dev prefix that some clients specify, but others do not. */ tty = skip_dev_prefix(tty); From bf40417c7cbe0afb185eda97ea46395b1bb74bdf Mon Sep 17 00:00:00 2001 From: Michal Sekletar Date: Tue, 21 Jun 2022 18:41:46 +0200 Subject: [PATCH 2/3] tests: add test for handling of background sessions --- test/test-functions | 1 + test/units/testsuite-35.sh | 64 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/test/test-functions b/test/test-functions index 19559fc071..974fa4384b 100644 --- a/test/test-functions +++ b/test/test-functions @@ -1736,6 +1736,7 @@ install_basic_tools() { # in Debian ldconfig is just a shell script wrapper around ldconfig.real image_install -o ldconfig.real # for TEST-35-LOGIN + image_install -o evemu-device evemu-event crond crontab image_install -o evemu-device evemu-event if command -v expect >/dev/null && command -v tclsh >/dev/null ; then # shellcheck disable=SC2016 diff --git a/test/units/testsuite-35.sh b/test/units/testsuite-35.sh index aa1a6af2fb..98283cad25 100755 --- a/test/units/testsuite-35.sh +++ b/test/units/testsuite-35.sh @@ -443,6 +443,69 @@ EOF rm -f dbus.log logind.log } +setup_cron() { + # Setup test user and cron + useradd test || : + crond -s -n & + # Install crontab for the test user that runs sleep every minute. But let's sleep for + # 65 seconds to make sure there is overlap between two consecutive runs, i.e. we have + # always a cron session running. + crontab -u test - <&2 "Skipping test for background cron sessions because cron is missing." + return + fi + + trap teardown_cron EXIT + setup_cron + + if [[ $(loginctl --no-legend list-sessions | grep -c test) -lt 1 ]]; then + echo >&2 '"test" user should have at least one session' + loginctl list-sessions + return 1 + fi + + # Check that all sessions of test user have class=background and no user instance was started + # for the test user. + while read -r s _; do + local class + + class=$(loginctl --property Class --value show-session "$s") + if [[ "$class" != "background" ]]; then + echo >&2 "Session has incorrect class, expected \"background\", got \"$class\"." + return 1 + fi + done < <(loginctl --no-legend list-sessions | grep test) + + state=$(systemctl --property ActiveState --value show user@"$(id -u test)".service) + if [[ "$state" != "inactive" ]]; then + echo >&2 "User instance state is unexpected, expected \"inactive\", got \"$state\"" + return 1 + fi + + state=$(systemctl --property SubState --value show user@"$(id -u test)".service) + if [[ "$state" != "dead" ]]; then + echo >&2 "User instance state is unexpected, expected \"dead\", got \"$state\"" + return 1 + fi +} + : >/failed test_enable_debug @@ -452,6 +515,7 @@ test_suspend_on_lid test_shutdown test_session test_lock_idle_action +test_no_user_instance_for_cron touch /testok rm /failed From 1611606fd91648f78137e823d5cc3ac68767e4a2 Mon Sep 17 00:00:00 2001 From: Michal Sekletar Date: Mon, 30 May 2022 15:13:13 +0200 Subject: [PATCH 3/3] NEWS: mention change in default behavior of background sessions --- NEWS | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/NEWS b/NEWS index e01ab3e2d3..6476c0b308 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,13 @@ systemd System and Service Manager +CHANGES WITH 252: + Backwards-incompatible changes: + + * systemd-logind's background session class, which is by default used + for sessions started via cron, is no longer going to start systemd + --user instance. Note that because of that the user dbus instance + will not start either. Other session types remain unaffected. + CHANGES WITH 251: Backwards-incompatible changes: