naccyde / rpms / systemd

Forked from rpms/systemd a year ago
Clone
661545
From 7aff4320eab8b12b19a8800a13f6573c0cdda946 Mon Sep 17 00:00:00 2001
661545
From: Franck Bui <fbui@suse.com>
661545
Date: Mon, 18 Mar 2019 20:59:36 +0100
661545
Subject: [PATCH] core: reduce the number of stalled PIDs from the watched
661545
 processes list when possible
661545
661545
Some PIDs can remain in the watched list even though their processes have
661545
exited since a long time. It can easily happen if the main process of a forking
661545
service manages to spawn a child before the control process exits for example.
661545
661545
However when a pid is about to be mapped to a unit by calling unit_watch_pid(),
661545
the caller usually knows if the pid should belong to this unit exclusively: if
661545
we just forked() off a child, then we can be sure that its PID is otherwise
661545
unused. In this case we take this opportunity to remove any stalled PIDs from
661545
the watched process list.
661545
661545
If we learnt about a PID in any other form (for example via PID file, via
661545
searching, MAINPID= and so on), then we can't assume anything.
661545
661545
(cherry picked from commit f75f613d259e9332be8e9657fa37e73f7aabcb8a)
661545
661545
Resolves: #1501796
661545
---
661545
 src/core/busname.c    |  4 ++--
661545
 src/core/cgroup.c     |  8 +++-----
661545
 src/core/dbus-scope.c |  2 +-
661545
 src/core/mount.c      |  4 ++--
661545
 src/core/service.c    | 24 ++++++++++--------------
661545
 src/core/socket.c     |  6 +++---
661545
 src/core/swap.c       |  4 ++--
661545
 src/core/unit.c       | 25 ++++++++++++++++++++++---
661545
 src/core/unit.h       |  2 +-
661545
 src/shared/util.c     | 14 ++++++++++++++
661545
 src/shared/util.h     |  1 +
661545
 11 files changed, 61 insertions(+), 33 deletions(-)
661545
661545
diff --git a/src/core/busname.c b/src/core/busname.c
661545
index f5553e5418..8d149aae4e 100644
661545
--- a/src/core/busname.c
661545
+++ b/src/core/busname.c
661545
@@ -350,7 +350,7 @@ static int busname_coldplug(Unit *u, Hashmap *deferred_work) {
661545
                 if (n->control_pid <= 0)
661545
                         return -EBADMSG;
661545
 
661545
-                r = unit_watch_pid(UNIT(n), n->control_pid);
661545
+                r = unit_watch_pid(UNIT(n), n->control_pid, false);
661545
                 if (r < 0)
661545
                         return r;
661545
 
661545
@@ -413,7 +413,7 @@ static int busname_make_starter(BusName *n, pid_t *_pid) {
661545
                 _exit(ret);
661545
         }
661545
 
661545
-        r = unit_watch_pid(UNIT(n), pid);
661545
+        r = unit_watch_pid(UNIT(n), pid, true);
661545
         if (r < 0)
661545
                 goto fail;
661545
 
661545
diff --git a/src/core/cgroup.c b/src/core/cgroup.c
661545
index 0779fa5552..0ce265dbf4 100644
661545
--- a/src/core/cgroup.c
661545
+++ b/src/core/cgroup.c
661545
@@ -26,6 +26,7 @@
661545
 #include "special.h"
661545
 #include "cgroup-util.h"
661545
 #include "cgroup.h"
661545
+#include "util.h"
661545
 
661545
 #define CGROUP_CPU_QUOTA_PERIOD_USEC ((usec_t) 100 * USEC_PER_MSEC)
661545
 
661545
@@ -881,7 +882,7 @@ void unit_destroy_cgroup_if_empty(Unit *u) {
661545
 
661545
 pid_t unit_search_main_pid(Unit *u) {
661545
         _cleanup_fclose_ FILE *f = NULL;
661545
-        pid_t pid = 0, npid, mypid;
661545
+        pid_t pid = 0, npid;
661545
 
661545
         assert(u);
661545
 
661545
@@ -891,15 +892,12 @@ pid_t unit_search_main_pid(Unit *u) {
661545
         if (cg_enumerate_processes(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, &f) < 0)
661545
                 return 0;
661545
 
661545
-        mypid = getpid();
661545
         while (cg_read_pid(f, &npid) > 0)  {
661545
-                pid_t ppid;
661545
-
661545
                 if (npid == pid)
661545
                         continue;
661545
 
661545
                 /* Ignore processes that aren't our kids */
661545
-                if (get_parent_of_pid(npid, &ppid) >= 0 && ppid != mypid)
661545
+                if (pid_is_my_child(npid) == 0)
661545
                         continue;
661545
 
661545
                 if (pid != 0) {
661545
diff --git a/src/core/dbus-scope.c b/src/core/dbus-scope.c
661545
index 60215a1935..f31360ad56 100644
661545
--- a/src/core/dbus-scope.c
661545
+++ b/src/core/dbus-scope.c
661545
@@ -93,7 +93,7 @@ static int bus_scope_set_transient_property(
661545
                                 return -EINVAL;
661545
 
661545
                         if (mode != UNIT_CHECK) {
661545
-                                r = unit_watch_pid(UNIT(s), pid);
661545
+                                r = unit_watch_pid(UNIT(s), pid, false);
661545
                                 if (r < 0 && r != -EEXIST)
661545
                                         return r;
661545
                         }
661545
diff --git a/src/core/mount.c b/src/core/mount.c
661545
index bfbfc10731..3167bd6bb1 100644
661545
--- a/src/core/mount.c
661545
+++ b/src/core/mount.c
661545
@@ -659,7 +659,7 @@ static int mount_coldplug(Unit *u, Hashmap *deferred_work) {
661545
                 if (m->control_pid <= 0)
661545
                         return -EBADMSG;
661545
 
661545
-                r = unit_watch_pid(UNIT(m), m->control_pid);
661545
+                r = unit_watch_pid(UNIT(m), m->control_pid, false);
661545
                 if (r < 0)
661545
                         return r;
661545
 
661545
@@ -751,7 +751,7 @@ static int mount_spawn(Mount *m, ExecCommand *c, pid_t *_pid) {
661545
         if (r < 0)
661545
                 goto fail;
661545
 
661545
-        r = unit_watch_pid(UNIT(m), pid);
661545
+        r = unit_watch_pid(UNIT(m), pid, true);
661545
         if (r < 0)
661545
                 /* FIXME: we need to do something here */
661545
                 goto fail;
661545
diff --git a/src/core/service.c b/src/core/service.c
661545
index f7b859d076..1ad154e41f 100644
661545
--- a/src/core/service.c
661545
+++ b/src/core/service.c
661545
@@ -145,8 +145,6 @@ static void service_unwatch_pid_file(Service *s) {
661545
 }
661545
 
661545
 static int service_set_main_pid(Service *s, pid_t pid) {
661545
-        pid_t ppid;
661545
-
661545
         assert(s);
661545
 
661545
         if (pid <= 1)
661545
@@ -165,12 +163,10 @@ static int service_set_main_pid(Service *s, pid_t pid) {
661545
 
661545
         s->main_pid = pid;
661545
         s->main_pid_known = true;
661545
+        s->main_pid_alien = pid_is_my_child(pid) == 0;
661545
 
661545
-        if (get_parent_of_pid(pid, &ppid) >= 0 && ppid != getpid()) {
661545
+        if (s->main_pid_alien)
661545
                 log_unit_warning(UNIT(s)->id, "%s: Supervising process "PID_FMT" which is not our child. We'll most likely not notice when it exits.", UNIT(s)->id, pid);
661545
-                s->main_pid_alien = true;
661545
-        } else
661545
-                s->main_pid_alien = false;
661545
 
661545
         return 0;
661545
 }
661545
@@ -809,7 +805,7 @@ static int service_load_pid_file(Service *s, bool may_warn) {
661545
         if (r < 0)
661545
                 return r;
661545
 
661545
-        r = unit_watch_pid(UNIT(s), pid);
661545
+        r = unit_watch_pid(UNIT(s), pid, false);
661545
         if (r < 0) {
661545
                 /* FIXME: we need to do something here */
661545
                 log_unit_warning(UNIT(s)->id, "Failed to watch PID "PID_FMT" from service %s", pid, UNIT(s)->id);
661545
@@ -844,7 +840,7 @@ static int service_search_main_pid(Service *s) {
661545
         if (r < 0)
661545
                 return r;
661545
 
661545
-        r = unit_watch_pid(UNIT(s), pid);
661545
+        r = unit_watch_pid(UNIT(s), pid, false);
661545
         if (r < 0) {
661545
                 /* FIXME: we need to do something here */
661545
                 log_unit_warning(UNIT(s)->id, "Failed to watch PID "PID_FMT" from service %s", pid, UNIT(s)->id);
661545
@@ -989,7 +985,7 @@ static int service_coldplug(Unit *u, Hashmap *deferred_work) {
661545
                             SERVICE_STOP, SERVICE_STOP_SIGTERM, SERVICE_STOP_SIGKILL,
661545
                             SERVICE_STOP_SIGABRT, SERVICE_STOP_POST,
661545
                             SERVICE_FINAL_SIGTERM, SERVICE_FINAL_SIGKILL))) {
661545
-                        r = unit_watch_pid(UNIT(s), s->main_pid);
661545
+                        r = unit_watch_pid(UNIT(s), s->main_pid, false);
661545
                         if (r < 0)
661545
                                 return r;
661545
                 }
661545
@@ -1001,7 +997,7 @@ static int service_coldplug(Unit *u, Hashmap *deferred_work) {
661545
                            SERVICE_STOP, SERVICE_STOP_SIGTERM, SERVICE_STOP_SIGKILL,
661545
                            SERVICE_STOP_SIGABRT, SERVICE_STOP_POST,
661545
                            SERVICE_FINAL_SIGTERM, SERVICE_FINAL_SIGKILL)) {
661545
-                        r = unit_watch_pid(UNIT(s), s->control_pid);
661545
+                        r = unit_watch_pid(UNIT(s), s->control_pid, false);
661545
                         if (r < 0)
661545
                                 return r;
661545
                 }
661545
@@ -1271,7 +1267,7 @@ static int service_spawn(
661545
         if (r < 0)
661545
                 goto fail;
661545
 
661545
-        r = unit_watch_pid(UNIT(s), pid);
661545
+        r = unit_watch_pid(UNIT(s), pid, true);
661545
         if (r < 0)
661545
                 /* FIXME: we need to do something here */
661545
                 goto fail;
661545
@@ -2325,7 +2321,7 @@ static int service_deserialize_item(Unit *u, const char *key, const char *value,
661545
                         log_unit_debug(u->id, "Failed to parse main-pid value %s", value);
661545
                 else {
661545
                         service_set_main_pid(s, pid);
661545
-                        unit_watch_pid(UNIT(s), pid);
661545
+                        unit_watch_pid(UNIT(s), pid, false);
661545
                 }
661545
         } else if (streq(key, "main-pid-known")) {
661545
                 int b;
661545
@@ -3085,7 +3081,7 @@ static void service_notify_message(
661545
                         }
661545
                         if (r > 0) {
661545
                                 service_set_main_pid(s, new_main_pid);
661545
-                                unit_watch_pid(UNIT(s), new_main_pid);
661545
+                                unit_watch_pid(UNIT(s), new_main_pid, false);
661545
                                 notify_dbus = true;
661545
                         }
661545
                 }
661545
@@ -3265,7 +3261,7 @@ static void service_bus_name_owner_change(
661545
                         log_unit_debug(u->id, "%s's D-Bus name %s is now owned by process %u", u->id, name, (unsigned) pid);
661545
 
661545
                         service_set_main_pid(s, pid);
661545
-                        unit_watch_pid(UNIT(s), pid);
661545
+                        unit_watch_pid(UNIT(s), pid, false);
661545
                 }
661545
         }
661545
 }
661545
diff --git a/src/core/socket.c b/src/core/socket.c
661545
index 1e8ae0a6e5..a5f610cd77 100644
661545
--- a/src/core/socket.c
661545
+++ b/src/core/socket.c
661545
@@ -1350,7 +1350,7 @@ static int socket_coldplug(Unit *u, Hashmap *deferred_work) {
661545
                 if (s->control_pid <= 0)
661545
                         return -EBADMSG;
661545
 
661545
-                r = unit_watch_pid(UNIT(s), s->control_pid);
661545
+                r = unit_watch_pid(UNIT(s), s->control_pid, false);
661545
                 if (r < 0)
661545
                         return r;
661545
 
661545
@@ -1427,7 +1427,7 @@ static int socket_spawn(Socket *s, ExecCommand *c, pid_t *_pid) {
661545
         if (r < 0)
661545
                 goto fail;
661545
 
661545
-        r = unit_watch_pid(UNIT(s), pid);
661545
+        r = unit_watch_pid(UNIT(s), pid, true);
661545
         if (r < 0)
661545
                 /* FIXME: we need to do something here */
661545
                 goto fail;
661545
@@ -1512,7 +1512,7 @@ static int socket_chown(Socket *s, pid_t *_pid) {
661545
                 _exit(ret);
661545
         }
661545
 
661545
-        r = unit_watch_pid(UNIT(s), pid);
661545
+        r = unit_watch_pid(UNIT(s), pid, true);
661545
         if (r < 0)
661545
                 goto fail;
661545
 
661545
diff --git a/src/core/swap.c b/src/core/swap.c
661545
index 757a8d45c5..4a5e882332 100644
661545
--- a/src/core/swap.c
661545
+++ b/src/core/swap.c
661545
@@ -548,7 +548,7 @@ static int swap_coldplug(Unit *u, Hashmap *deferred_work) {
661545
                 if (s->control_pid <= 0)
661545
                         return -EBADMSG;
661545
 
661545
-                r = unit_watch_pid(UNIT(s), s->control_pid);
661545
+                r = unit_watch_pid(UNIT(s), s->control_pid, false);
661545
                 if (r < 0)
661545
                         return r;
661545
 
661545
@@ -645,7 +645,7 @@ static int swap_spawn(Swap *s, ExecCommand *c, pid_t *_pid) {
661545
         if (r < 0)
661545
                 goto fail;
661545
 
661545
-        r = unit_watch_pid(UNIT(s), pid);
661545
+        r = unit_watch_pid(UNIT(s), pid, true);
661545
         if (r < 0)
661545
                 /* FIXME: we need to do something here */
661545
                 goto fail;
661545
diff --git a/src/core/unit.c b/src/core/unit.c
661545
index def36a0930..cdac192eb6 100644
661545
--- a/src/core/unit.c
661545
+++ b/src/core/unit.c
661545
@@ -52,6 +52,7 @@
661545
 #include "execute.h"
661545
 #include "virt.h"
661545
 #include "dropin.h"
661545
+#include "util.h"
661545
 
661545
 const UnitVTable * const unit_vtable[_UNIT_TYPE_MAX] = {
661545
         [UNIT_SERVICE] = &service_vtable,
661545
@@ -1961,7 +1962,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su
661545
         unit_add_to_gc_queue(u);
661545
 }
661545
 
661545
-int unit_watch_pid(Unit *u, pid_t pid) {
661545
+int unit_watch_pid(Unit *u, pid_t pid, bool exclusive) {
661545
         int q, r;
661545
 
661545
         assert(u);
661545
@@ -1970,6 +1971,15 @@ int unit_watch_pid(Unit *u, pid_t pid) {
661545
         /* Watch a specific PID. We only support one or two units
661545
          * watching each PID for now, not more. */
661545
 
661545
+        /* Caller might be sure that this PID belongs to this unit only. Let's take this
661545
+         * opportunity to remove any stalled references to this PID as they can be created
661545
+         * easily (when watching a process which is not our direct child). */
661545
+        if (exclusive) {
661545
+                log_unit_debug(u->id, "Cleaning "PID_FMT" from watches.", pid);
661545
+                hashmap_remove2(u->manager->watch_pids1, LONG_TO_PTR(pid), NULL);
661545
+                hashmap_remove2(u->manager->watch_pids2, LONG_TO_PTR(pid), NULL);
661545
+        }
661545
+
661545
         r = set_ensure_allocated(&u->pids, NULL);
661545
         if (r < 0)
661545
                 return r;
661545
@@ -1985,7 +1995,12 @@ int unit_watch_pid(Unit *u, pid_t pid) {
661545
                         return r;
661545
 
661545
                 r = hashmap_put(u->manager->watch_pids2, LONG_TO_PTR(pid), u);
661545
-        }
661545
+                if (r >= 0)
661545
+                        log_unit_debug(u->id, "Watching "PID_FMT" through watch_pids2.", pid);
661545
+                else if (r == -EEXIST)
661545
+                        log_unit_warning(u->id, "Cannot watch "PID_FMT", PID is already watched twice.", pid);
661545
+        } else if (r >= 0)
661545
+                log_unit_debug(u->id, "Watching "PID_FMT" through watch_pids1.", pid);
661545
 
661545
         q = set_put(u->pids, LONG_TO_PTR(pid));
661545
         if (q < 0)
661545
@@ -1998,6 +2013,8 @@ void unit_unwatch_pid(Unit *u, pid_t pid) {
661545
         assert(u);
661545
         assert(pid >= 1);
661545
 
661545
+        log_unit_debug(u->id, "Unwatching "PID_FMT".", pid);
661545
+
661545
         hashmap_remove_value(u->manager->watch_pids1, LONG_TO_PTR(pid), u);
661545
         hashmap_remove_value(u->manager->watch_pids2, LONG_TO_PTR(pid), u);
661545
         set_remove(u->pids, LONG_TO_PTR(pid));
661545
@@ -2028,7 +2045,9 @@ static int unit_watch_pids_in_path(Unit *u, const char *path) {
661545
                 pid_t pid;
661545
 
661545
                 while ((r = cg_read_pid(f, &pid)) > 0) {
661545
-                        r = unit_watch_pid(u, pid);
661545
+                        if (pid_is_my_child(pid) == 0)
661545
+                                log_unit_debug(u->id, "Watching non detached "PID_FMT".", pid);
661545
+                        r = unit_watch_pid(u, pid, false);
661545
                         if (r < 0 && ret >= 0)
661545
                                 ret = r;
661545
                 }
661545
diff --git a/src/core/unit.h b/src/core/unit.h
661545
index a6e21d60ce..4a8bd79052 100644
661545
--- a/src/core/unit.h
661545
+++ b/src/core/unit.h
661545
@@ -537,7 +537,7 @@ int unit_kill_common(Unit *u, KillWho who, int signo, pid_t main_pid, pid_t cont
661545
 
661545
 void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_success);
661545
 
661545
-int unit_watch_pid(Unit *u, pid_t pid);
661545
+int unit_watch_pid(Unit *u, pid_t pid, bool exclusive);
661545
 void unit_unwatch_pid(Unit *u, pid_t pid);
661545
 int unit_watch_all_pids(Unit *u);
661545
 void unit_unwatch_all_pids(Unit *u);
661545
diff --git a/src/shared/util.c b/src/shared/util.c
661545
index 82c8e433dd..127a64c3c6 100644
661545
--- a/src/shared/util.c
661545
+++ b/src/shared/util.c
661545
@@ -641,6 +641,20 @@ int get_parent_of_pid(pid_t pid, pid_t *_ppid) {
661545
         return 0;
661545
 }
661545
 
661545
+int pid_is_my_child(pid_t pid) {
661545
+        pid_t ppid;
661545
+        int r;
661545
+
661545
+        if (pid <= 1)
661545
+                return false;
661545
+
661545
+        r = get_parent_of_pid(pid, &ppid);
661545
+        if (r < 0)
661545
+                return r;
661545
+
661545
+        return ppid == getpid();
661545
+}
661545
+
661545
 int fchmod_umask(int fd, mode_t m) {
661545
         mode_t u;
661545
         int r;
661545
diff --git a/src/shared/util.h b/src/shared/util.h
661545
index 538ca4be2d..6b885d7533 100644
661545
--- a/src/shared/util.h
661545
+++ b/src/shared/util.h
661545
@@ -279,6 +279,7 @@ const char* split(const char **state, size_t *l, const char *separator, bool quo
661545
         for ((state) = (s), (word) = split(&(state), &(length), (separator), (quoted)); (word); (word) = split(&(state), &(length), (separator), (quoted)))
661545
 
661545
 pid_t get_parent_of_pid(pid_t pid, pid_t *ppid);
661545
+int pid_is_my_child(pid_t pid);
661545
 
661545
 char *strappend(const char *s, const char *suffix);
661545
 char *strnappend(const char *s, const char *suffix, size_t length);