From 6f3b6308bb5705fdcadec3f4cac105211bc0a3e2 Mon Sep 17 00:00:00 2001

From: Daan De Meyer <daan.j.demeyer@gmail.com>

Date: Tue, 24 Aug 2021 16:46:47 +0100

Subject: [PATCH] core: Check unit start rate limiting earlier

Fixes #17433. Currently, if any of the validations we do before we

check start rate limiting fail, we can still enter a busy loop as

no rate limiting gets applied. A common occurence of this scenario

is path units triggering a service that fails a condition check.

To fix the issue, we simply move up start rate limiting checks to

be the first thing we do when starting a unit. To achieve this,

we add a new method to the unit vtable and implement it for the

relevant unit types so that we can do the start rate limit checks

earlier on.

(cherry picked from commit 9727f2427ff6b2e1f4ab927cc57ad8e888f04e95)

Related: #2037395

[msekleta: I've deleted part of the original commit that adds test for

issue #17433. This was necessary because upstream commit assumes newer

organization of the test code which we don't have in RHEL-8 tree. As

a consequence we must add explicit test for this in the internal

test-suite.]

---

 src/core/automount.c | 23 +++++++++++++++++------

 src/core/mount.c     | 23 +++++++++++++++++------

 src/core/path.c      | 23 +++++++++++++++++------

 src/core/service.c   | 25 ++++++++++++++++++-------

 src/core/socket.c    | 23 +++++++++++++++++------

 src/core/swap.c      | 23 +++++++++++++++++------

 src/core/timer.c     | 22 ++++++++++++++++------

 src/core/unit.c      | 14 ++++++++++----

 src/core/unit.h      |  4 ++++

 9 files changed, 133 insertions(+), 47 deletions(-)

diff --git a/src/core/automount.c b/src/core/automount.c

index 2bc160cb07..5e16adabb5 100644

--- a/src/core/automount.c

+++ b/src/core/automount.c

@@ -808,12 +808,6 @@ static int automount_start(Unit *u) {

                 return -ENOENT;

         }

-        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;

@@ -1077,6 +1071,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",

@@ -1135,4 +1144,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 4cb49d845d..619f64d5b7 100644

--- a/src/core/mount.c

+++ b/src/core/mount.c

@@ -1065,12 +1065,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;

@@ -1953,6 +1947,21 @@ static int mount_control_pid(Unit *u) {

         return m->control_pid;

 }

+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",

@@ -2044,4 +2053,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 4bccc0396b..1e69a1f05f 100644

--- a/src/core/path.c

+++ b/src/core/path.c

@@ -565,12 +565,6 @@ static int path_start(Unit *u) {

                 return -ENOENT;

         }

-        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;

@@ -730,6 +724,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",

@@ -782,4 +791,6 @@ const UnitVTable path_vtable = {

         .bus_vtable = bus_path_vtable,

         .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 01cb1a375e..a21d98ab8f 100644

--- a/src/core/service.c

+++ b/src/core/service.c

@@ -2356,13 +2356,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;

@@ -4050,6 +4043,22 @@ static bool service_needs_console(Unit *u) {

                       SERVICE_FINAL_SIGKILL);

 }

+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",

@@ -4191,4 +4200,6 @@ const UnitVTable service_vtable = {

                         [JOB_FAILED]     = "Stopped (with error) %s.",

                 },

         },

+

+        .test_start_limit = service_test_start_limit,

 };

diff --git a/src/core/socket.c b/src/core/socket.c

index 09491c6677..36d2e4f823 100644

--- a/src/core/socket.c

+++ b/src/core/socket.c

@@ -2469,12 +2469,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;

@@ -3267,6 +3261,21 @@ static int socket_control_pid(Unit *u) {

         return s->control_pid;

 }

+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",

@@ -3359,4 +3368,6 @@ const UnitVTable socket_vtable = {

                         [JOB_TIMEOUT]    = "Timed out stopping %s.",

                 },

         },

+

+        .test_start_limit = socket_test_start_limit,

 };

diff --git a/src/core/swap.c b/src/core/swap.c

index 823699699e..90fcd69300 100644

--- a/src/core/swap.c

+++ b/src/core/swap.c

@@ -851,12 +851,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;

@@ -1458,6 +1452,21 @@ static int swap_control_pid(Unit *u) {

         return s->control_pid;

 }

+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",

@@ -1547,4 +1556,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 be16321296..fb9ae17990 100644

--- a/src/core/timer.c

+++ b/src/core/timer.c

@@ -599,12 +599,6 @@ static int timer_start(Unit *u) {

                 return -ENOENT;

         }

-        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;

@@ -829,6 +823,21 @@ static void timer_timezone_change(Unit *u) {

         timer_enter_waiting(t, false);

 }

+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",

@@ -884,4 +893,5 @@ const UnitVTable timer_vtable = {

         .bus_set_property = bus_timer_set_property,

         .can_transient = true,

+        .test_start_limit = timer_test_start_limit,

 };

diff --git a/src/core/unit.c b/src/core/unit.c

index fa94a67fdd..9005f79df3 100644

--- a/src/core/unit.c

+++ b/src/core/unit.c

@@ -1726,10 +1726,16 @@ int unit_start(Unit *u) {

         assert(u);

-        /* 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. */

+        /* 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. */

         state = unit_active_state(u);

         if (UNIT_IS_ACTIVE_OR_RELOADING(state))

                 return -EALREADY;

diff --git a/src/core/unit.h b/src/core/unit.h

index a8bc350b66..9e6f1bcf81 100644

--- a/src/core/unit.h

+++ b/src/core/unit.h

@@ -567,6 +567,10 @@ typedef struct UnitVTable {

         /* The bus vtable */

         const sd_bus_vtable *bus_vtable;

+        /* 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;