diff options
author | Yu Watanabe <watanabe.yu+github@gmail.com> | 2021-08-26 06:04:40 +0900 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-08-26 06:04:40 +0900 |
commit | ebab417cfbb45fcdaa24d1fc01ad5a5a04818785 (patch) | |
tree | e05990948b96f6b61f1bd7998ea01fe631589bc6 | |
parent | e447ffe4daca1d0beb57242f079125669e4e1c3c (diff) | |
parent | 9727f2427ff6b2e1f4ab927cc57ad8e888f04e95 (diff) | |
download | systemd-ebab417cfbb45fcdaa24d1fc01ad5a5a04818785.tar.gz |
Merge pull request #20531 from DaanDeMeyer/fix-17433
core: Check unit start rate limiting earlier
-rw-r--r-- | src/core/automount.c | 23 | ||||
-rw-r--r-- | src/core/mount.c | 23 | ||||
-rw-r--r-- | src/core/path.c | 23 | ||||
-rw-r--r-- | src/core/service.c | 25 | ||||
-rw-r--r-- | src/core/socket.c | 24 | ||||
-rw-r--r-- | src/core/socket.h | 1 | ||||
-rw-r--r-- | src/core/swap.c | 23 | ||||
-rw-r--r-- | src/core/timer.c | 23 | ||||
-rw-r--r-- | src/core/unit.c | 7 | ||||
-rw-r--r-- | src/core/unit.h | 4 | ||||
l--------- | test/TEST-63-ISSUE-17433/Makefile | 1 | ||||
-rwxr-xr-x | test/TEST-63-ISSUE-17433/test.sh | 9 | ||||
-rw-r--r-- | test/meson.build | 2 | ||||
-rw-r--r-- | test/testsuite-10.units/test10.service | 3 | ||||
-rw-r--r-- | test/testsuite-63.units/test63.path | 2 | ||||
-rw-r--r-- | test/testsuite-63.units/test63.service | 5 | ||||
-rw-r--r-- | test/units/testsuite-63.service | 16 |
17 files changed, 170 insertions, 44 deletions
diff --git a/src/core/automount.c b/src/core/automount.c index 30226b9bde..11eb352b9b 100644 --- a/src/core/automount.c +++ b/src/core/automount.c @@ -813,12 +813,6 @@ static int automount_start(Unit *u) { if (r < 0) return r; - r = unit_test_start_limit(u); - if (r < 0) { - automount_enter_dead(a, AUTOMOUNT_FAILURE_START_LIMIT_HIT); - return r; - } - r = unit_acquire_invocation_id(u); if (r < 0) return r; @@ -1064,6 +1058,21 @@ static bool automount_supported(void) { return supported; } +static int automount_test_start_limit(Unit *u) { + Automount *a = AUTOMOUNT(u); + int r; + + assert(a); + + r = unit_test_start_limit(u); + if (r < 0) { + automount_enter_dead(a, AUTOMOUNT_FAILURE_START_LIMIT_HIT); + return r; + } + + return 0; +} + static const char* const automount_result_table[_AUTOMOUNT_RESULT_MAX] = { [AUTOMOUNT_SUCCESS] = "success", [AUTOMOUNT_FAILURE_RESOURCES] = "resources", @@ -1126,4 +1135,6 @@ const UnitVTable automount_vtable = { [JOB_FAILED] = "Failed to unset automount %s.", }, }, + + .test_start_limit = automount_test_start_limit, }; diff --git a/src/core/mount.c b/src/core/mount.c index fb8f72e257..35b56426d4 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -1167,12 +1167,6 @@ static int mount_start(Unit *u) { assert(IN_SET(m->state, MOUNT_DEAD, MOUNT_FAILED)); - r = unit_test_start_limit(u); - if (r < 0) { - mount_enter_dead(m, MOUNT_FAILURE_START_LIMIT_HIT); - return r; - } - r = unit_acquire_invocation_id(u); if (r < 0) return r; @@ -2138,6 +2132,21 @@ static int mount_can_clean(Unit *u, ExecCleanMask *ret) { return exec_context_get_clean_mask(&m->exec_context, ret); } +static int mount_test_start_limit(Unit *u) { + Mount *m = MOUNT(u); + int r; + + assert(m); + + r = unit_test_start_limit(u); + if (r < 0) { + mount_enter_dead(m, MOUNT_FAILURE_START_LIMIT_HIT); + return r; + } + + return 0; +} + static const char* const mount_exec_command_table[_MOUNT_EXEC_COMMAND_MAX] = { [MOUNT_EXEC_MOUNT] = "ExecMount", [MOUNT_EXEC_UNMOUNT] = "ExecUnmount", @@ -2235,4 +2244,6 @@ const UnitVTable mount_vtable = { [JOB_TIMEOUT] = "Timed out unmounting %s.", }, }, + + .test_start_limit = mount_test_start_limit, }; diff --git a/src/core/path.c b/src/core/path.c index 800524a308..693636b0ee 100644 --- a/src/core/path.c +++ b/src/core/path.c @@ -590,12 +590,6 @@ static int path_start(Unit *u) { if (r < 0) return r; - r = unit_test_start_limit(u); - if (r < 0) { - path_enter_dead(p, PATH_FAILURE_START_LIMIT_HIT); - return r; - } - r = unit_acquire_invocation_id(u); if (r < 0) return r; @@ -812,6 +806,21 @@ static void path_reset_failed(Unit *u) { p->result = PATH_SUCCESS; } +static int path_test_start_limit(Unit *u) { + Path *p = PATH(u); + int r; + + assert(p); + + r = unit_test_start_limit(u); + if (r < 0) { + path_enter_dead(p, PATH_FAILURE_START_LIMIT_HIT); + return r; + } + + return 0; +} + static const char* const path_type_table[_PATH_TYPE_MAX] = { [PATH_EXISTS] = "PathExists", [PATH_EXISTS_GLOB] = "PathExistsGlob", @@ -866,4 +875,6 @@ const UnitVTable path_vtable = { .reset_failed = path_reset_failed, .bus_set_property = bus_path_set_property, + + .test_start_limit = path_test_start_limit, }; diff --git a/src/core/service.c b/src/core/service.c index c55304d170..9d8eef1f74 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -2436,13 +2436,6 @@ static int service_start(Unit *u) { assert(IN_SET(s->state, SERVICE_DEAD, SERVICE_FAILED)); - /* Make sure we don't enter a busy loop of some kind. */ - r = unit_test_start_limit(u); - if (r < 0) { - service_enter_dead(s, SERVICE_FAILURE_START_LIMIT_HIT, false); - return r; - } - r = unit_acquire_invocation_id(u); if (r < 0) return r; @@ -4445,6 +4438,22 @@ static const char *service_finished_job(Unit *u, JobType t, JobResult result) { return NULL; } +static int service_test_start_limit(Unit *u) { + Service *s = SERVICE(u); + int r; + + assert(s); + + /* Make sure we don't enter a busy loop of some kind. */ + r = unit_test_start_limit(u); + if (r < 0) { + service_enter_dead(s, SERVICE_FAILURE_START_LIMIT_HIT, false); + return r; + } + + return 0; +} + static const char* const service_restart_table[_SERVICE_RESTART_MAX] = { [SERVICE_RESTART_NO] = "no", [SERVICE_RESTART_ON_SUCCESS] = "on-success", @@ -4608,4 +4617,6 @@ const UnitVTable service_vtable = { }, .finished_job = service_finished_job, }, + + .test_start_limit = service_test_start_limit, }; diff --git a/src/core/socket.c b/src/core/socket.c index 675ad3c025..177068eed4 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -34,6 +34,7 @@ #include "process-util.h" #include "selinux-util.h" #include "serialize.h" +#include "service.h" #include "signal-util.h" #include "smack-util.h" #include "socket.h" @@ -2512,12 +2513,6 @@ static int socket_start(Unit *u) { assert(IN_SET(s->state, SOCKET_DEAD, SOCKET_FAILED)); - r = unit_test_start_limit(u); - if (r < 0) { - socket_enter_dead(s, SOCKET_FAILURE_START_LIMIT_HIT); - return r; - } - r = unit_acquire_invocation_id(u); if (r < 0) return r; @@ -3424,6 +3419,21 @@ static int socket_can_clean(Unit *u, ExecCleanMask *ret) { return exec_context_get_clean_mask(&s->exec_context, ret); } +static int socket_test_start_limit(Unit *u) { + Socket *s = SOCKET(u); + int r; + + assert(s); + + r = unit_test_start_limit(u); + if (r < 0) { + socket_enter_dead(s, SOCKET_FAILURE_START_LIMIT_HIT); + return r; + } + + return 0; +} + static const char* const socket_exec_command_table[_SOCKET_EXEC_COMMAND_MAX] = { [SOCKET_EXEC_START_PRE] = "ExecStartPre", [SOCKET_EXEC_START_CHOWN] = "ExecStartChown", @@ -3550,4 +3560,6 @@ const UnitVTable socket_vtable = { [JOB_TIMEOUT] = "Timed out stopping %s.", }, }, + + .test_start_limit = socket_test_start_limit, }; diff --git a/src/core/socket.h b/src/core/socket.h index a65195f2aa..6813bdcf8c 100644 --- a/src/core/socket.h +++ b/src/core/socket.h @@ -5,7 +5,6 @@ typedef struct Socket Socket; typedef struct SocketPeer SocketPeer; #include "mount.h" -#include "service.h" #include "socket-util.h" #include "unit.h" diff --git a/src/core/swap.c b/src/core/swap.c index 48ba5c7664..29c63118ac 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -932,12 +932,6 @@ static int swap_start(Unit *u) { if (UNIT(other)->job && UNIT(other)->job->state == JOB_RUNNING) return -EAGAIN; - r = unit_test_start_limit(u); - if (r < 0) { - swap_enter_dead(s, SWAP_FAILURE_START_LIMIT_HIT); - return r; - } - r = unit_acquire_invocation_id(u); if (r < 0) return r; @@ -1588,6 +1582,21 @@ static int swap_can_clean(Unit *u, ExecCleanMask *ret) { return exec_context_get_clean_mask(&s->exec_context, ret); } +static int swap_test_start_limit(Unit *u) { + Swap *s = SWAP(u); + int r; + + assert(s); + + r = unit_test_start_limit(u); + if (r < 0) { + swap_enter_dead(s, SWAP_FAILURE_START_LIMIT_HIT); + return r; + } + + return 0; +} + static const char* const swap_exec_command_table[_SWAP_EXEC_COMMAND_MAX] = { [SWAP_EXEC_ACTIVATE] = "ExecActivate", [SWAP_EXEC_DEACTIVATE] = "ExecDeactivate", @@ -1683,4 +1692,6 @@ const UnitVTable swap_vtable = { [JOB_TIMEOUT] = "Timed out deactivating swap %s.", }, }, + + .test_start_limit = swap_test_start_limit, }; diff --git a/src/core/timer.c b/src/core/timer.c index 12515a6a75..8853121c00 100644 --- a/src/core/timer.c +++ b/src/core/timer.c @@ -627,12 +627,6 @@ static int timer_start(Unit *u) { if (r < 0) return r; - r = unit_test_start_limit(u); - if (r < 0) { - timer_enter_dead(t, TIMER_FAILURE_START_LIMIT_HIT); - return r; - } - r = unit_acquire_invocation_id(u); if (r < 0) return r; @@ -890,6 +884,21 @@ static int timer_can_clean(Unit *u, ExecCleanMask *ret) { return 0; } +static int timer_test_start_limit(Unit *u) { + Timer *t = TIMER(u); + int r; + + assert(t); + + r = unit_test_start_limit(u); + if (r < 0) { + timer_enter_dead(t, TIMER_FAILURE_START_LIMIT_HIT); + return r; + } + + return 0; +} + static const char* const timer_base_table[_TIMER_BASE_MAX] = { [TIMER_ACTIVE] = "OnActiveSec", [TIMER_BOOT] = "OnBootSec", @@ -949,4 +958,6 @@ const UnitVTable timer_vtable = { .timezone_change = timer_timezone_change, .bus_set_property = bus_timer_set_property, + + .test_start_limit = timer_test_start_limit, }; diff --git a/src/core/unit.c b/src/core/unit.c index 7d72dfa864..48e7b95e56 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -1857,6 +1857,13 @@ int unit_start(Unit *u) { assert(u); + /* Check start rate limiting early so that failure conditions don't cause us to enter a busy loop. */ + if (UNIT_VTABLE(u)->test_start_limit) { + int r = UNIT_VTABLE(u)->test_start_limit(u); + if (r < 0) + return r; + } + /* If this is already started, then this will succeed. Note that this will even succeed if this unit * is not startable by the user. This is relied on to detect when we need to wait for units and when * waiting is finished. */ diff --git a/src/core/unit.h b/src/core/unit.h index b3e9c2106f..b689f29f8f 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -658,6 +658,10 @@ typedef struct UnitVTable { * of this type will immediately fail. */ bool (*supported)(void); + /* If this function is set, it's invoked first as part of starting a unit to allow start rate + * limiting checks to occur before we do anything else. */ + int (*test_start_limit)(Unit *u); + /* The strings to print in status messages */ UnitStatusMessageFormats status_message_formats; diff --git a/test/TEST-63-ISSUE-17433/Makefile b/test/TEST-63-ISSUE-17433/Makefile new file mode 120000 index 0000000000..e9f93b1104 --- /dev/null +++ b/test/TEST-63-ISSUE-17433/Makefile @@ -0,0 +1 @@ +../TEST-01-BASIC/Makefile
\ No newline at end of file diff --git a/test/TEST-63-ISSUE-17433/test.sh b/test/TEST-63-ISSUE-17433/test.sh new file mode 100755 index 0000000000..c595a9f2de --- /dev/null +++ b/test/TEST-63-ISSUE-17433/test.sh @@ -0,0 +1,9 @@ +#!/usr/bin/env bash +set -e + +TEST_DESCRIPTION="https://github.com/systemd/systemd/issues/17433" + +# shellcheck source=test/test-functions +. "${TEST_BASE_DIR:?}/test-functions" + +do_test "$@" diff --git a/test/meson.build b/test/meson.build index a21230a4a8..b8335fb50f 100644 --- a/test/meson.build +++ b/test/meson.build @@ -33,6 +33,8 @@ if install_tests install_dir : testdata_dir) install_subdir('testsuite-52.units', install_dir : testdata_dir) + install_subdir('testsuite-63.units', + install_dir : testdata_dir) testsuite08_dir = testdata_dir + '/testsuite-08.units' install_data('testsuite-08.units/-.mount', diff --git a/test/testsuite-10.units/test10.service b/test/testsuite-10.units/test10.service index d0be786b01..2fb476b986 100644 --- a/test/testsuite-10.units/test10.service +++ b/test/testsuite-10.units/test10.service @@ -1,6 +1,9 @@ [Unit] Requires=test10.socket ConditionPathExistsGlob=/tmp/nonexistent +# Make sure we hit the socket trigger limit in the test and not the service start limit. +StartLimitInterval=1000 +StartLimitBurst=1000 [Service] ExecStart=true diff --git a/test/testsuite-63.units/test63.path b/test/testsuite-63.units/test63.path new file mode 100644 index 0000000000..a6573bda0a --- /dev/null +++ b/test/testsuite-63.units/test63.path @@ -0,0 +1,2 @@ +[Path] +PathExists=/tmp/test63 diff --git a/test/testsuite-63.units/test63.service b/test/testsuite-63.units/test63.service new file mode 100644 index 0000000000..c83801874d --- /dev/null +++ b/test/testsuite-63.units/test63.service @@ -0,0 +1,5 @@ +[Unit] +ConditionPathExists=!/tmp/nonexistent + +[Service] +ExecStart=true diff --git a/test/units/testsuite-63.service b/test/units/testsuite-63.service new file mode 100644 index 0000000000..04122723d4 --- /dev/null +++ b/test/units/testsuite-63.service @@ -0,0 +1,16 @@ +[Unit] +Description=TEST-63-ISSUE-17433 + +[Service] +ExecStartPre=rm -f /failed /testok +Type=oneshot +ExecStart=rm -f /tmp/nonexistent +ExecStart=systemctl start test63.path +ExecStart=touch /tmp/test63 +# Make sure systemd has sufficient time to hit the start limit for test63.service. +ExecStart=sleep 2 +ExecStart=sh -x -c 'test "$(systemctl show test63.service -P ActiveState)" = failed' +ExecStart=sh -x -c 'test "$(systemctl show test63.service -P Result)" = start-limit-hit' +ExecStart=sh -x -c 'test "$(systemctl show test63.path -P ActiveState)" = failed' +ExecStart=sh -x -c 'test "$(systemctl show test63.path -P Result)" = unit-start-limit-hit' +ExecStart=sh -x -c 'echo OK >/testok' |