Merge pull request #8377 from sourcejedi/logind_restart_is_sorely_lacking_in_testing3

login: don't remove all devices from PID1 when only one was removed

src/login/logind-session-device.c

@@ -193,19 +193,16 @@ static int session_device_start(SessionDevice *sd) {
         switch (sd->type) {
 
         case DEVICE_TYPE_DRM:
-
                 if (sd->fd < 0) {
-                        /* Open device if it isn't open yet */
+                        log_error("Failed to re-activate DRM fd, as the fd was lost (maybe logind restart went wrong?)");
-                        sd->fd = session_device_open(sd, true);
+                        return -EBADF;
-                        if (sd->fd < 0)
+                }
-                                return sd->fd;
+
-                } else {
                 /* Device is kept open. Simply call drmSetMaster() and hope there is no-one else. In case it fails, we
                  * keep the device paused. Maybe at some point we have a drmStealMaster(). */
                 r = sd_drmsetmaster(sd->fd);
                 if (r < 0)
                         return r;
-                }
                 break;
 
         case DEVICE_TYPE_EVDEV:
@@ -239,6 +236,11 @@ static void session_device_stop(SessionDevice *sd) {
         switch (sd->type) {
 
         case DEVICE_TYPE_DRM:
+                if (sd->fd < 0) {
+                        log_error("Failed to de-activate DRM fd, as the fd was lost (maybe logind restart went wrong?)");
+                        return;
+                }
+
                 /* On DRM devices we simply drop DRM-Master but keep it open.
                  * This allows the user to keep resources allocated. The
                  * CAP_SYS_ADMIN restriction to DRM-Master prevents users from
@@ -416,14 +418,20 @@ error:
 void session_device_free(SessionDevice *sd) {
         assert(sd);
 
+        /* Make sure to remove the pushed fd. */
         if (sd->pushed_fd) {
-                const char *m;
+                _cleanup_free_ char *m = NULL;
+                const char *id;
+                int r;
 
-                /* Remove the pushed fd again, just in case. */
+                /* Session ID does not contain separators. */
-
+                id = sd->session->id;
-                m = strjoina("FDSTOREREMOVE=1\n"
+                assert(*(id + strcspn(id, "-\n")) == '\0');
-                             "FDNAME=session-", sd->session->id);
 
+                r = asprintf(&m, "FDSTOREREMOVE=1\n"
+                                 "FDNAME=session-%s-device-%u-%u\n",
+                                 id, major(sd->dev), minor(sd->dev));
+                if (r >= 0)
                         (void) sd_notify(false, m);
         }
 
@@ -510,7 +518,8 @@ unsigned int session_device_try_pause_all(Session *s) {
 }
 
 int session_device_save(SessionDevice *sd) {
-        const char *m;
+        _cleanup_free_ char *m = NULL;
+        const char *id;
         int r;
 
         assert(sd);
@@ -525,8 +534,15 @@ int session_device_save(SessionDevice *sd) {
         if (sd->pushed_fd)
                 return 0;
               
-        m = strjoina("FDSTORE=1\n"
+        /* Session ID does not contain separators. */
-                     "FDNAME=session-", sd->session->id);
+        id = sd->session->id;
+        assert(*(id + strcspn(id, "-\n")) == '\0');
+
+        r = asprintf(&m, "FDSTORE=1\n"
+                         "FDNAME=session-%s-device-%u-%u\n",
+                         id, major(sd->dev), minor(sd->dev));
+        if (r < 0)
+                return r;
 
         r = sd_pid_notify_with_fds(0, false, m, &sd->fd, 1);
         if (r < 0)

src/login/logind.c

@@ -37,6 +37,7 @@
 #include "format-util.h"
 #include "fs-util.h"
 #include "logind.h"
+#include "parse-util.h"
 #include "process-util.h"
 #include "selinux-util.h"
 #include "signal-util.h"
@@ -411,6 +412,38 @@ static int manager_enumerate_users(Manager *m) {
         return r;
 }
 
+static int parse_fdname(const char *fdname, char **session_id, dev_t *dev) {
+        _cleanup_strv_free_ char **parts = NULL;
+        _cleanup_free_ char *id = NULL;
+        unsigned int major, minor;
+        int r;
+
+        parts = strv_split(fdname, "-");
+        if (!parts)
+                return -ENOMEM;
+        if (strv_length(parts) != 5)
+                return -EINVAL;
+
+        if (!streq(parts[0], "session"))
+                return -EINVAL;
+        id = strdup(parts[1]);
+        if (!id)
+                return -ENOMEM;
+
+        if (!streq(parts[2], "device"))
+                return -EINVAL;
+        r = safe_atou(parts[3], &major) ||
+            safe_atou(parts[4], &minor);
+        if (r < 0)
+                return r;
+
+        *dev = makedev(major, minor);
+        *session_id = id;
+        id = NULL;
+
+        return 0;
+}
+
 static int manager_attach_fds(Manager *m) {
         _cleanup_strv_free_ char **fdnames = NULL;
         int n, i, fd;
@@ -424,16 +457,21 @@ static int manager_attach_fds(Manager *m) {
                 return n;
 
         for (i = 0; i < n; i++) {
+                _cleanup_free_ char *id = NULL;
+                dev_t dev;
                 struct stat st;
                 SessionDevice *sd;
                 Session *s;
-                char *id;
+                int r;
 
                 fd = SD_LISTEN_FDS_START + i;
 
-                id = startswith(fdnames[i], "session-");
+                r = parse_fdname(fdnames[i], &id, &dev);
-                if (!id)
+                if (r < 0) {
+                        log_debug_errno(r, "Failed to parse fd name %s: %m", fdnames[i]);
+                        close_nointr(fd);
                         continue;
+                }
 
                 s = hashmap_get(m->sessions, id);
                 if (!s) {
@@ -453,24 +491,24 @@ static int manager_attach_fds(Manager *m) {
                         continue;
                 }
 
-                if (!S_ISCHR(st.st_mode) && !S_ISBLK(st.st_mode)) {
+                if (!S_ISCHR(st.st_mode) || st.st_rdev != dev) {
-                        log_debug("Device fd doesn't actually point to device node: %m");
+                        log_debug("Device fd doesn't point to the expected character device node");
                         close_nointr(fd);
                         continue;
                 }
 
-                sd = hashmap_get(s->devices, &st.st_rdev);
+                sd = hashmap_get(s->devices, &dev);
                 if (!sd) {
                         /* Weird, we got an fd for a session device which wasn't
                          * recorded in the session state file... */
                         log_warning("Got fd for missing session device [%u:%u] in session %s",
-                                    major(st.st_rdev), minor(st.st_rdev), s->id);
+                                    major(dev), minor(dev), s->id);
                         close_nointr(fd);
                         continue;
                 }
 
                 log_debug("Attaching fd to session device [%u:%u] for session %s",
-                          major(st.st_rdev), minor(st.st_rdev), s->id);
+                          major(dev), minor(dev), s->id);
 
                 session_device_attach_fd(sd, fd, s->was_active);
         }