Merge pull request #22752 from yuwata/udev-ctrl-manage-sender-pids

udev: enable Delegate=

4 files changed, 61 insertions, 76 deletions

diff --git a/src/udev/udev-ctrl.c b/src/udev/udev-ctrl.c
index 2b697c03b3..8adef47732 100644
--- a/src/udev/udev-ctrl.c
+++ b/src/udev/udev-ctrl.c
@@ -12,7 +12,6 @@
 
 #include "alloc-util.h"
 #include "errno-util.h"
-#include "event-util.h"
 #include "fd-util.h"
 #include "format-util.h"
 #include "io-util.h"
@@ -106,13 +105,6 @@ static void udev_ctrl_disconnect(UdevCtrl *uctrl) {
         uctrl->sock_connect = safe_close(uctrl->sock_connect);
 }
 
-int udev_ctrl_is_connected(UdevCtrl *uctrl) {
-        if (!uctrl)
-                return false;
-
-        return event_source_is_enabled(uctrl->event_source_connect);
-}
-
 static UdevCtrl *udev_ctrl_free(UdevCtrl *uctrl) {
         assert(uctrl);
 
@@ -314,8 +306,6 @@ int udev_ctrl_send(UdevCtrl *uctrl, UdevCtrlMessageType type, const void *data)
                 strscpy(ctrl_msg_wire.value.buf, sizeof(ctrl_msg_wire.value.buf), data);
         } else if (IN_SET(type, UDEV_CTRL_SET_LOG_LEVEL, UDEV_CTRL_SET_CHILDREN_MAX))
                 ctrl_msg_wire.value.intval = PTR_TO_INT(data);
-        else if (type == UDEV_CTRL_SENDER_PID)
-                ctrl_msg_wire.value.pid = PTR_TO_PID(data);
 
         if (!uctrl->connected) {
                 if (connect(uctrl->sock, &uctrl->saddr.sa, uctrl->addrlen) < 0)
diff --git a/src/udev/udev-ctrl.h b/src/udev/udev-ctrl.h
index 83ef44391d..11fc0b62de 100644
--- a/src/udev/udev-ctrl.h
+++ b/src/udev/udev-ctrl.h
@@ -4,7 +4,6 @@
 #include "sd-event.h"
 
 #include "macro.h"
-#include "process-util.h"
 #include "time-util.h"
 
 typedef struct UdevCtrl UdevCtrl;
@@ -19,12 +18,10 @@ typedef enum UdevCtrlMessageType {
         UDEV_CTRL_SET_CHILDREN_MAX,
         UDEV_CTRL_PING,
         UDEV_CTRL_EXIT,
-        UDEV_CTRL_SENDER_PID,
 } UdevCtrlMessageType;
 
 typedef union UdevCtrlMessageValue {
         int intval;
-        pid_t pid;
         char buf[256];
 } UdevCtrlMessageValue;
 
@@ -42,7 +39,6 @@ UdevCtrl *udev_ctrl_unref(UdevCtrl *uctrl);
 int udev_ctrl_attach_event(UdevCtrl *uctrl, sd_event *event);
 int udev_ctrl_start(UdevCtrl *uctrl, udev_ctrl_handler_t callback, void *userdata);
 sd_event_source *udev_ctrl_get_event_source(UdevCtrl *uctrl);
-int udev_ctrl_is_connected(UdevCtrl *uctrl);
 
 int udev_ctrl_wait(UdevCtrl *uctrl, usec_t timeout);
 
@@ -79,8 +75,4 @@ static inline int udev_ctrl_send_exit(UdevCtrl *uctrl) {
         return udev_ctrl_send(uctrl, UDEV_CTRL_EXIT, NULL);
 }
 
-static inline int udev_ctrl_send_pid(UdevCtrl *uctrl) {
-        return udev_ctrl_send(uctrl, UDEV_CTRL_SENDER_PID, PID_TO_PTR(getpid_cached()));
-}
-
 DEFINE_TRIVIAL_CLEANUP_FUNC(UdevCtrl*, udev_ctrl_unref);
diff --git a/src/udev/udevadm-control.c b/src/udev/udevadm-control.c
index 720e09a70f..06c61e5c07 100644
--- a/src/udev/udevadm-control.c
+++ b/src/udev/udevadm-control.c
@@ -87,11 +87,6 @@ int control_main(int argc, char *argv[], void *userdata) {
         if (r < 0)
                 return log_error_errno(r, "Failed to initialize udev control: %m");
 
-        /* See comments in on_post() in udevd.c. */
-        r = udev_ctrl_send_pid(uctrl);
-        if (r < 0)
-                return log_error_errno(r, "Failed to send pid of this process: %m");
-
         while ((c = getopt_long(argc, argv, "el:sSRp:m:t:Vh", options, NULL)) >= 0)
                 switch (c) {
                 case 'e':
diff --git a/src/udev/udevd.c b/src/udev/udevd.c
index ccd30eea95..c6f6d945c8 100644
--- a/src/udev/udevd.c
+++ b/src/udev/udevd.c
@@ -28,6 +28,7 @@
 #include "sd-event.h"
 
 #include "alloc-util.h"
+#include "cgroup-setup.h"
 #include "cgroup-util.h"
 #include "cpu-set-util.h"
 #include "dev-setup.h"
@@ -48,6 +49,7 @@
 #include "mkdir.h"
 #include "netlink-util.h"
 #include "parse-util.h"
+#include "path-util.h"
 #include "pretty-print.h"
 #include "proc-cmdline.h"
 #include "process-util.h"
@@ -85,7 +87,7 @@ typedef struct Manager {
         sd_event *event;
         Hashmap *workers;
         LIST_HEAD(Event, events);
-        const char *cgroup;
+        char *cgroup;
         pid_t pid; /* the process that originally allocated the manager object */
         int log_level;
 
@@ -108,8 +110,6 @@ typedef struct Manager {
 
         bool stop_exec_queue;
         bool exit;
-
-        pid_t pid_udevadm; /* pid of 'udevadm control' */
 } Manager;
 
 typedef enum EventState {
@@ -240,6 +240,7 @@ static Manager* manager_free(Manager *manager) {
         safe_close(manager->inotify_fd);
         safe_close_pair(manager->worker_watch);
 
+        free(manager->cgroup);
         return mfree(manager);
 }
 
@@ -1217,10 +1218,6 @@ static int on_ctrl_msg(UdevCtrl *uctrl, UdevCtrlMessageType type, const UdevCtrl
                 log_debug("Received udev control message (EXIT)");
                 manager_exit(manager);
                 break;
-        case UDEV_CTRL_SENDER_PID:
-                log_debug("Received udev control message (SENDER_PID)");
-                manager->pid_udevadm = value->pid;
-                break;
         default:
                 log_debug("Received unknown udev control message, ignoring");
         }
@@ -1496,35 +1493,9 @@ static int on_post(sd_event_source *s, void *userdata) {
         if (manager->exit)
                 return sd_event_exit(manager->event, 0);
 
-        if (!manager->cgroup)
-                return 1;
-
-        /* Cleanup possible left-over processes in our cgroup. But do not kill udevadm called by
-         * ExecReload= in systemd-udevd.service. See #16867. To protect it, the following two ways are
-         * introduced:
-         *
-         * 1. After the connection is accepted, but the PID of udevadm is not received, do not call
-         *    cg_kill(). So, in this period, unwanted process or threads may exist in our cgroup.
-         *    But, it is expected that the PID of the udevadm will be received soon. So, this time
-         *    period should be short enough.
-         * 2. After the PID of udevadm is received, check the process is active or not, and if it is
-         *    still active, set the PID to the deny list for cg_kill(). Why udev_ctrl_is_connected() is
-         *    not enough? Because the udevadm may be still active after the control socket is
-         *    disconnected. If the process is not active, then clear the PID for later connections.
-         */
-
-        if (udev_ctrl_is_connected(manager->ctrl) >= 0 && !pid_is_valid(manager->pid_udevadm))
-                return 1;
-
-        _cleanup_set_free_ Set *pids = NULL;
-        if (pid_is_valid(manager->pid_udevadm)) {
-                if (pid_is_alive(manager->pid_udevadm))
-                        (void) set_ensure_put(&pids, NULL, PID_TO_PTR(manager->pid_udevadm));
-                else
-                        manager->pid_udevadm = 0;
-        }
-
-        (void) cg_kill(SYSTEMD_CGROUP_CONTROLLER, manager->cgroup, SIGKILL, CGROUP_IGNORE_SELF, pids, NULL, NULL);
+        if (manager->cgroup)
+                /* cleanup possible left-over processes in our cgroup */
+                (void) cg_kill(SYSTEMD_CGROUP_CONTROLLER, manager->cgroup, SIGKILL, CGROUP_IGNORE_SELF, NULL, NULL, NULL);
 
         return 1;
 }
@@ -1754,12 +1725,63 @@ static int parse_argv(int argc, char *argv[]) {
         return 1;
 }
 
-static int manager_new(Manager **ret, int fd_ctrl, int fd_uevent, const char *cgroup) {
+static int create_subcgroup(char **ret) {
+        _cleanup_free_ char *cgroup = NULL, *subcgroup = NULL;
+        int r;
+
+        if (getppid() != 1)
+                return log_debug_errno(SYNTHETIC_ERRNO(EOPNOTSUPP), "Not invoked by PID1.");
+
+        r = sd_booted();
+        if (r < 0)
+                return log_debug_errno(r, "Failed to check if systemd is running: %m");
+        if (r == 0)
+                return log_debug_errno(SYNTHETIC_ERRNO(EOPNOTSUPP), "systemd is not running.");
+
+        /* Get our own cgroup, we regularly kill everything udev has left behind.
+         * We only do this on systemd systems, and only if we are directly spawned
+         * by PID1. Otherwise we are not guaranteed to have a dedicated cgroup. */
+
+        r = cg_pid_get_path(SYSTEMD_CGROUP_CONTROLLER, 0, &cgroup);
+        if (r < 0) {
+                if (IN_SET(r, -ENOENT, -ENOMEDIUM))
+                        return log_debug_errno(r, "Dedicated cgroup not found: %m");
+                return log_debug_errno(r, "Failed to get cgroup: %m");
+        }
+
+        r = cg_get_xattr_bool(SYSTEMD_CGROUP_CONTROLLER, cgroup, "trusted.delegate");
+        if (IN_SET(r, 0, -ENODATA))
+                return log_debug_errno(SYNTHETIC_ERRNO(EOPNOTSUPP), "The cgroup %s is not delegated to us.", cgroup);
+        if (r < 0)
+                return log_debug_errno(r, "Failed to read trusted.delegate attribute: %m");
+
+        /* We are invoked with our own delegated cgroup tree, let's move us one level down, so that we
+         * don't collide with the "no processes in inner nodes" rule of cgroups, when the service
+         * manager invokes the ExecReload= job in the .control/ subcgroup. */
+
+        subcgroup = path_join(cgroup, "/udev");
+        if (!subcgroup)
+                return log_oom_debug();
+
+        r = cg_create_and_attach(SYSTEMD_CGROUP_CONTROLLER, subcgroup, 0);
+        if (r < 0)
+                return log_debug_errno(r, "Failed to create %s subcgroup: %m", subcgroup);
+
+        log_debug("Created %s subcgroup.", subcgroup);
+        if (ret)
+                *ret = TAKE_PTR(subcgroup);
+        return 0;
+}
+
+static int manager_new(Manager **ret, int fd_ctrl, int fd_uevent) {
         _cleanup_(manager_freep) Manager *manager = NULL;
+        _cleanup_free_ char *cgroup = NULL;
         int r;
 
         assert(ret);
 
+        (void) create_subcgroup(&cgroup);
+
         manager = new(Manager, 1);
         if (!manager)
                 return log_oom();
@@ -1767,7 +1789,7 @@ static int manager_new(Manager **ret, int fd_ctrl, int fd_uevent, const char *cg
         *manager = (Manager) {
                 .inotify_fd = -1,
                 .worker_watch = { -1, -1 },
-                .cgroup = cgroup,
+                .cgroup = TAKE_PTR(cgroup),
         };
 
         r = udev_ctrl_new_from_fd(&manager->ctrl, fd_ctrl);
@@ -1912,7 +1934,6 @@ static int main_loop(Manager *manager) {
 }
 
 int run_udevd(int argc, char *argv[]) {
-        _cleanup_free_ char *cgroup = NULL;
         _cleanup_(manager_freep) Manager *manager = NULL;
         int fd_ctrl = -1, fd_uevent = -1;
         int r;
@@ -1969,24 +1990,11 @@ int run_udevd(int argc, char *argv[]) {
         if (r < 0 && r != -EEXIST)
                 return log_error_errno(r, "Failed to create /run/udev: %m");
 
-        if (getppid() == 1 && sd_booted() > 0) {
-                /* Get our own cgroup, we regularly kill everything udev has left behind.
-                 * We only do this on systemd systems, and only if we are directly spawned
-                 * by PID1. Otherwise we are not guaranteed to have a dedicated cgroup. */
-                r = cg_pid_get_path(SYSTEMD_CGROUP_CONTROLLER, 0, &cgroup);
-                if (r < 0) {
-                        if (IN_SET(r, -ENOENT, -ENOMEDIUM))
-                                log_debug_errno(r, "Dedicated cgroup not found: %m");
-                        else
-                                log_warning_errno(r, "Failed to get cgroup: %m");
-                }
-        }
-
         r = listen_fds(&fd_ctrl, &fd_uevent);
         if (r < 0)
                 return log_error_errno(r, "Failed to listen on fds: %m");
 
-        r = manager_new(&manager, fd_ctrl, fd_uevent, cgroup);
+        r = manager_new(&manager, fd_ctrl, fd_uevent);
         if (r < 0)
                 return log_error_errno(r, "Failed to create manager: %m");