1abbee
From 64e21697bdefe0a37edc8557fd110daea2667771 Mon Sep 17 00:00:00 2001
1abbee
From: David Herrmann <dh.herrmann@gmail.com>
1abbee
Date: Wed, 28 Oct 2015 19:11:36 +0100
1abbee
Subject: [PATCH] core: fix priority ordering in notify-handling
1abbee
1abbee
Currently, we dispatch NOTIFY messages in a tight loop. Regardless how
1abbee
much data is incoming, we always dispatch everything that is queued.
1abbee
This, however, completely breaks priority event-handling of sd-event.
1abbee
When dispatching one NOTIFY event, another completely different event
1abbee
might fire, or might be queued by the NOTIFY handling. However, this
1abbee
event will not get dispatched until all other further NOTIFY messages are
1abbee
handled. Those might even arrive _after_ the other event fired, and as
1abbee
such completely break priority ordering of sd-event (which several code
1abbee
paths rely on).
1abbee
1abbee
Break this by never dispatching multiple messages. Just return after each
1abbee
message that was read and let sd-event handle everything else.
1abbee
1abbee
(The patch looks scarier that it is. It basically just drops the for(;;)
1abbee
 loop and re-indents the loop-content.)
1abbee
1abbee
(cherry picked from commit b215b0ede11c0dda90009c8412609d2416150075)
1abbee
Related: #1267707
1abbee
---
23b3cf
 src/core/manager.c | 158 ++++++++++++++++++++++-----------------------
1abbee
 1 file changed, 78 insertions(+), 80 deletions(-)
1abbee
1abbee
diff --git a/src/core/manager.c b/src/core/manager.c
Pablo Greco 48fc63
index 5da8365938..c5021993e5 100644
1abbee
--- a/src/core/manager.c
1abbee
+++ b/src/core/manager.c
1abbee
@@ -1635,9 +1635,33 @@ static void manager_invoke_notify_message(Manager *m, Unit *u, pid_t pid, char *
1abbee
 }
1abbee
 
1abbee
 static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t revents, void *userdata) {
1abbee
+        _cleanup_fdset_free_ FDSet *fds = NULL;
1abbee
         Manager *m = userdata;
1abbee
+
1abbee
+        char buf[NOTIFY_BUFFER_MAX+1];
1abbee
+        struct iovec iovec = {
1abbee
+                .iov_base = buf,
1abbee
+                .iov_len = sizeof(buf)-1,
1abbee
+        };
1abbee
+        union {
1abbee
+                struct cmsghdr cmsghdr;
1abbee
+                uint8_t buf[CMSG_SPACE(sizeof(struct ucred)) +
1abbee
+                            CMSG_SPACE(sizeof(int) * NOTIFY_FD_MAX)];
1abbee
+        } control = {};
1abbee
+        struct msghdr msghdr = {
1abbee
+                .msg_iov = &iovec,
1abbee
+                .msg_iovlen = 1,
1abbee
+                .msg_control = &control,
1abbee
+                .msg_controllen = sizeof(control),
1abbee
+        };
1abbee
+
1abbee
+        struct cmsghdr *cmsg;
1abbee
+        struct ucred *ucred = NULL;
1abbee
+        bool found = false;
1abbee
+        Unit *u1, *u2, *u3;
1abbee
+        int r, *fd_array = NULL;
1abbee
+        unsigned n_fds = 0;
1abbee
         ssize_t n;
1abbee
-        int r;
1abbee
 
1abbee
         assert(m);
1abbee
         assert(m->notify_fd == fd);
1abbee
@@ -1647,108 +1671,82 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t
1abbee
                 return 0;
1abbee
         }
1abbee
 
1abbee
-        for (;;) {
1abbee
-                _cleanup_fdset_free_ FDSet *fds = NULL;
1abbee
-                char buf[NOTIFY_BUFFER_MAX+1];
1abbee
-                struct iovec iovec = {
1abbee
-                        .iov_base = buf,
1abbee
-                        .iov_len = sizeof(buf)-1,
1abbee
-                };
1abbee
-                union {
1abbee
-                        struct cmsghdr cmsghdr;
1abbee
-                        uint8_t buf[CMSG_SPACE(sizeof(struct ucred)) +
1abbee
-                                    CMSG_SPACE(sizeof(int) * NOTIFY_FD_MAX)];
1abbee
-                } control = {};
1abbee
-                struct msghdr msghdr = {
1abbee
-                        .msg_iov = &iovec,
1abbee
-                        .msg_iovlen = 1,
1abbee
-                        .msg_control = &control,
1abbee
-                        .msg_controllen = sizeof(control),
1abbee
-                };
1abbee
-                struct cmsghdr *cmsg;
1abbee
-                struct ucred *ucred = NULL;
1abbee
-                bool found = false;
1abbee
-                Unit *u1, *u2, *u3;
1abbee
-                int *fd_array = NULL;
1abbee
-                unsigned n_fds = 0;
1abbee
-
1abbee
-                n = recvmsg(m->notify_fd, &msghdr, MSG_DONTWAIT|MSG_CMSG_CLOEXEC);
1abbee
-                if (n < 0) {
1abbee
-                        if (errno == EAGAIN || errno == EINTR)
1abbee
-                                break;
1abbee
+        n = recvmsg(m->notify_fd, &msghdr, MSG_DONTWAIT|MSG_CMSG_CLOEXEC);
1abbee
+        if (n < 0) {
1abbee
+                if (errno == EAGAIN || errno == EINTR)
1abbee
+                        return 0;
1abbee
 
1abbee
-                        return -errno;
1abbee
-                }
1abbee
+                return -errno;
1abbee
+        }
1abbee
 
1abbee
-                for (cmsg = CMSG_FIRSTHDR(&msghdr); cmsg; cmsg = CMSG_NXTHDR(&msghdr, cmsg)) {
1abbee
-                        if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS) {
1abbee
+        for (cmsg = CMSG_FIRSTHDR(&msghdr); cmsg; cmsg = CMSG_NXTHDR(&msghdr, cmsg)) {
1abbee
+                if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS) {
1abbee
 
1abbee
-                                fd_array = (int*) CMSG_DATA(cmsg);
1abbee
-                                n_fds = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int);
1abbee
+                        fd_array = (int*) CMSG_DATA(cmsg);
1abbee
+                        n_fds = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int);
1abbee
 
1abbee
-                        } else if (cmsg->cmsg_level == SOL_SOCKET &&
1abbee
-                                   cmsg->cmsg_type == SCM_CREDENTIALS &&
1abbee
-                                   cmsg->cmsg_len == CMSG_LEN(sizeof(struct ucred))) {
1abbee
+                } else if (cmsg->cmsg_level == SOL_SOCKET &&
1abbee
+                           cmsg->cmsg_type == SCM_CREDENTIALS &&
1abbee
+                           cmsg->cmsg_len == CMSG_LEN(sizeof(struct ucred))) {
1abbee
 
1abbee
-                                ucred = (struct ucred*) CMSG_DATA(cmsg);
1abbee
-                        }
1abbee
+                        ucred = (struct ucred*) CMSG_DATA(cmsg);
1abbee
                 }
1abbee
+        }
1abbee
 
1abbee
-                if (n_fds > 0) {
1abbee
-                        assert(fd_array);
1abbee
+        if (n_fds > 0) {
1abbee
+                assert(fd_array);
1abbee
 
1abbee
-                        r = fdset_new_array(&fds, fd_array, n_fds);
1abbee
-                        if (r < 0) {
1abbee
-                                close_many(fd_array, n_fds);
1abbee
-                                return log_oom();
1abbee
-                        }
1abbee
+                r = fdset_new_array(&fds, fd_array, n_fds);
1abbee
+                if (r < 0) {
1abbee
+                        close_many(fd_array, n_fds);
1abbee
+                        return log_oom();
1abbee
                 }
1abbee
+        }
1abbee
 
1abbee
-                if (!ucred || ucred->pid <= 0) {
1abbee
-                        log_warning("Received notify message without valid credentials. Ignoring.");
1abbee
-                        continue;
1abbee
-                }
1abbee
+        if (!ucred || ucred->pid <= 0) {
1abbee
+                log_warning("Received notify message without valid credentials. Ignoring.");
1abbee
+                return 0;
1abbee
+        }
1abbee
 
1abbee
-                if ((size_t) n >= sizeof(buf)) {
1abbee
-                        log_warning("Received notify message exceeded maximum size. Ignoring.");
1abbee
-                        continue;
1abbee
-                }
1abbee
+        if ((size_t) n >= sizeof(buf)) {
1abbee
+                log_warning("Received notify message exceeded maximum size. Ignoring.");
1abbee
+                return 0;
1abbee
+        }
1abbee
 
1abbee
-                buf[n] = 0;
1abbee
+        buf[n] = 0;
1abbee
 
1abbee
-                /* Notify every unit that might be interested, but try
1abbee
-                 * to avoid notifying the same one multiple times. */
1abbee
-                u1 = manager_get_unit_by_pid(m, ucred->pid);
1abbee
-                if (u1) {
1abbee
-                        manager_invoke_notify_message(m, u1, ucred->pid, buf, n, fds);
1abbee
-                        found = true;
1abbee
-                }
1abbee
+        /* Notify every unit that might be interested, but try
1abbee
+         * to avoid notifying the same one multiple times. */
1abbee
+        u1 = manager_get_unit_by_pid(m, ucred->pid);
1abbee
+        if (u1) {
1abbee
+                manager_invoke_notify_message(m, u1, ucred->pid, buf, n, fds);
1abbee
+                found = true;
1abbee
+        }
1abbee
 
1abbee
-                u2 = hashmap_get(m->watch_pids1, LONG_TO_PTR(ucred->pid));
1abbee
-                if (u2 && u2 != u1) {
1abbee
-                        manager_invoke_notify_message(m, u2, ucred->pid, buf, n, fds);
1abbee
-                        found = true;
1abbee
-                }
1abbee
+        u2 = hashmap_get(m->watch_pids1, LONG_TO_PTR(ucred->pid));
1abbee
+        if (u2 && u2 != u1) {
1abbee
+                manager_invoke_notify_message(m, u2, ucred->pid, buf, n, fds);
1abbee
+                found = true;
1abbee
+        }
1abbee
 
1abbee
-                u3 = hashmap_get(m->watch_pids2, LONG_TO_PTR(ucred->pid));
1abbee
-                if (u3 && u3 != u2 && u3 != u1) {
1abbee
-                        manager_invoke_notify_message(m, u3, ucred->pid, buf, n, fds);
1abbee
-                        found = true;
1abbee
-                }
1abbee
+        u3 = hashmap_get(m->watch_pids2, LONG_TO_PTR(ucred->pid));
1abbee
+        if (u3 && u3 != u2 && u3 != u1) {
1abbee
+                manager_invoke_notify_message(m, u3, ucred->pid, buf, n, fds);
1abbee
+                found = true;
1abbee
+        }
1abbee
 
1abbee
-                if (!found)
1abbee
-                        log_warning("Cannot find unit for notify message of PID "PID_FMT".", ucred->pid);
1abbee
+        if (!found)
1abbee
+                log_warning("Cannot find unit for notify message of PID "PID_FMT".", ucred->pid);
1abbee
 
1abbee
-                if (fdset_size(fds) > 0)
1abbee
-                        log_warning("Got auxiliary fds with notification message, closing all.");
1abbee
-        }
1abbee
+        if (fdset_size(fds) > 0)
1abbee
+                log_warning("Got auxiliary fds with notification message, closing all.");
1abbee
 
1abbee
         return 0;
1abbee
 }
1abbee
 
1abbee
 static void invoke_sigchld_event(Manager *m, Unit *u, siginfo_t *si) {
1abbee
         uint64_t iteration;
1abbee
-        
1abbee
+
1abbee
         assert(m);
1abbee
         assert(u);
1abbee
         assert(si);