bus: send memfds as payload only on directed messages and for large parts

5 files changed, 103 insertions, 39 deletions

diff --git a/src/libsystemd-bus/bus-kernel.c b/src/libsystemd-bus/bus-kernel.c
index f7759b6fb4..8ef5752b31 100644
--- a/src/libsystemd-bus/bus-kernel.c
+++ b/src/libsystemd-bus/bus-kernel.c
@@ -69,6 +69,10 @@ static void append_payload_vec(struct kdbus_item **d, const void *p, size_t sz)
 
         *d = ALIGN8_PTR(*d);
 
+        /* Note that p can be NULL, which encodes a region full of
+         * zeroes, which is useful to optimize certain padding
+         * conditions */
+
         (*d)->size = offsetof(struct kdbus_item, vec) + sizeof(struct kdbus_vec);
         (*d)->type = KDBUS_MSG_PAYLOAD_VEC;
         (*d)->vec.address = PTR_TO_UINT64(p);
@@ -244,9 +248,12 @@ static int bus_message_setup_kmsg(sd_bus *b, sd_bus_message *m) {
                 sz += ALIGN8(offsetof(struct kdbus_item, fds) + sizeof(int)*m->n_fds);
 
         m->kdbus = memalign(8, sz);
-        if (!m->kdbus)
-                return -ENOMEM;
+        if (!m->kdbus) {
+                r = -ENOMEM;
+                goto fail;
+        }
 
+        m->free_kdbus = true;
         memset(m->kdbus, 0, sz);
 
         m->kdbus->flags =
@@ -269,24 +276,28 @@ static int bus_message_setup_kmsg(sd_bus *b, sd_bus_message *m) {
 
         MESSAGE_FOREACH_PART(part, i, m) {
                 if (part->is_zero) {
+                        /* If this is padding then simply send a
+                         * vector with a NULL data pointer which the
+                         * kernel will just pass through. This is the
+                         * most efficient way to encode zeroes */
+
                         append_payload_vec(&d, NULL, part->size);
                         continue;
                 }
 
-                if (part->memfd >= 0 && part->sealed) {
-                        bus_body_part_unmap(part);
+                if (part->memfd >= 0 && part->sealed && m->destination) {
+                        /* Try to send a memfd, if the part is
+                         * sealed and this is not a broadcast. Since we can only  */
 
-                        if (!part->data) {
-                                append_payload_memfd(&d, part->memfd, part->size);
-                                continue;
-                        }
+                        append_payload_memfd(&d, part->memfd, part->size);
+                        continue;
                 }
 
-                if (part->memfd >= 0) {
-                        r = bus_body_part_map(part);
-                        if (r < 0)
-                                goto fail;
-                }
+                /* Otherwise let's send a vector to the actual data,
+                 * for that we need to map it first. */
+                r = bus_body_part_map(part);
+                if (r < 0)
+                        goto fail;
 
                 append_payload_vec(&d, part->data, part->size);
         }
@@ -306,13 +317,10 @@ static int bus_message_setup_kmsg(sd_bus *b, sd_bus_message *m) {
         m->kdbus->size = (uint8_t*) d - (uint8_t*) m->kdbus;
         assert(m->kdbus->size <= sz);
 
-        m->free_kdbus = true;
-
         return 0;
 
 fail:
-        free(m->kdbus);
-        m->kdbus = NULL;
+        m->poisoned = true;
         return r;
 }
 
diff --git a/src/libsystemd-bus/bus-kernel.h b/src/libsystemd-bus/bus-kernel.h
index ed3f987ccd..1651c1e41d 100644
--- a/src/libsystemd-bus/bus-kernel.h
+++ b/src/libsystemd-bus/bus-kernel.h
@@ -24,7 +24,14 @@
 #include "sd-bus.h"
 
 #define MEMFD_CACHE_MAX 32
-#define MEMFD_CACHE_ITEM_SIZE_MAX (128*1024)
+
+/* When we cache a memfd block for reuse, we will truncate blocks
+ * longer than this in order not to keep too much data around. */
+#define MEMFD_CACHE_ITEM_SIZE_MAX (32*1024)
+
+/* This determines at which minimum size we prefer sending memfds over
+ * sending vectors */
+#define MEMFD_MIN_SIZE (32*1024)
 
 struct memfd_cache {
         int fd;
diff --git a/src/libsystemd-bus/bus-message.c b/src/libsystemd-bus/bus-message.c
index 209fd71c13..ceac15601e 100644
--- a/src/libsystemd-bus/bus-message.c
+++ b/src/libsystemd-bus/bus-message.c
@@ -59,6 +59,8 @@ static void message_free_part(sd_bus_message *m, struct bus_body_part *part) {
         assert(part);
 
         if (part->memfd >= 0) {
+                /* If we can reuse the memfd, try that. For that it
+                 * can't be sealed yet. */
 
                 if (!part->sealed)
                         bus_kernel_push_memfd(m->bus, part->memfd, part->data, part->mapped);
@@ -3239,7 +3241,7 @@ int sd_bus_message_read_array(sd_bus_message *m, char type, const void **ptr, si
                 return align;
 
         r = sd_bus_message_enter_container(m, SD_BUS_TYPE_ARRAY, CHAR_TO_STR(type));
-        if (r < 0)
+        if (r <= 0)
                 return r;
 
         c = message_get_container(m);
@@ -3730,21 +3732,27 @@ int bus_message_seal(sd_bus_message *m, uint64_t serial) {
                         return r;
         }
 
-        /* Add padding at the end, since we know the body
-         * needs to start at an 8 byte alignment. */
-
+        /* Add padding at the end of the fields part, since we know
+         * the body needs to start at an 8 byte alignment. We made
+         * sure we allocated enough space for this, so all we need to
+         * do here is to zero it out. */
         l = BUS_MESSAGE_FIELDS_SIZE(m);
         a = ALIGN8(l) - l;
         if (a > 0)
                 memset((uint8_t*) BUS_MESSAGE_FIELDS(m) + l, 0, a);
 
-        MESSAGE_FOREACH_PART(part, i, m)
-                if (part->memfd >= 0 && !part->sealed) {
-                        bus_body_part_unmap(part);
+        /* If this is something we can send as memfd, then let's seal
+        the memfd now. Note that we can send memfds as payload only
+        for directed messages, and not for broadcasts. */
+        if (m->destination) {
+                MESSAGE_FOREACH_PART(part, i, m)
+                        if (part->memfd >= 0 && !part->sealed && part->size > MEMFD_MIN_SIZE) {
+                                bus_body_part_unmap(part);
 
-                        if (ioctl(part->memfd, KDBUS_CMD_MEMFD_SEAL_SET, 1) >= 0)
-                                part->sealed = true;
-                }
+                                if (ioctl(part->memfd, KDBUS_CMD_MEMFD_SEAL_SET, 1) >= 0)
+                                        part->sealed = true;
+                        }
+        }
 
         m->header->serial = serial;
         m->sealed = true;
diff --git a/src/libsystemd-bus/bus-socket.c b/src/libsystemd-bus/bus-socket.c
index 4635da4b9e..befded7079 100644
--- a/src/libsystemd-bus/bus-socket.c
+++ b/src/libsystemd-bus/bus-socket.c
@@ -88,23 +88,33 @@ static int bus_message_setup_iovec(sd_bus_message *m) {
                 m->iovec = m->iovec_fixed;
         else {
                 m->iovec = new(struct iovec, n);
-                if (!m->iovec)
-                        return -ENOMEM;
+                if (!m->iovec) {
+                        r = -ENOMEM;
+                        goto fail;
+                }
         }
 
         r = append_iovec(m, m->header, BUS_MESSAGE_BODY_BEGIN(m));
         if (r < 0)
-                return r;
+                goto fail;
 
         MESSAGE_FOREACH_PART(part, i, m)  {
+                r = bus_body_part_map(part);
+                if (r < 0)
+                        goto fail;
+
                 r = append_iovec(m, part->data, part->size);
                 if (r < 0)
-                        return r;
+                        goto fail;
         }
 
         assert(n == m->n_iovec);
 
         return 0;
+
+fail:
+        m->poisoned = true;
+        return r;
 }
 
 bool bus_socket_auth_needs_write(sd_bus *b) {
diff --git a/src/libsystemd-bus/test-bus-zero-copy.c b/src/libsystemd-bus/test-bus-zero-copy.c
index 5a9f45489e..63bb921456 100644
--- a/src/libsystemd-bus/test-bus-zero-copy.c
+++ b/src/libsystemd-bus/test-bus-zero-copy.c
@@ -31,14 +31,19 @@
 #include "bus-error.h"
 #include "bus-kernel.h"
 
+#define FIRST_ARRAY 17
+#define SECOND_ARRAY 33
+
 int main(int argc, char *argv[]) {
         _cleanup_free_ char *bus_name = NULL, *address = NULL;
-        void *p;
+        uint8_t *p;
         sd_bus *a, *b;
         int r, bus_ref;
         sd_bus_message *m;
         sd_memfd *f;
         uint64_t sz;
+        uint32_t u32;
+        size_t i, l;
 
         log_set_max_level(LOG_DEBUG);
 
@@ -75,20 +80,20 @@ int main(int argc, char *argv[]) {
         r = sd_bus_message_open_container(m, 'r', "ayay");
         assert_se(r >= 0);
 
-        r = sd_bus_message_append_array_space(m, 'y', 32, &p);
+        r = sd_bus_message_append_array_space(m, 'y', FIRST_ARRAY, (void**) &p);
         assert_se(r >= 0);
 
-        memset(p, 'L', 32);
+        memset(p, 'L', FIRST_ARRAY);
 
-        r = sd_memfd_new_and_map(&f, 17, &p);
+        r = sd_memfd_new_and_map(&f, SECOND_ARRAY, (void**) &p);
         assert_se(r >= 0);
 
-        memset(p, 'P', 17);
-        munmap(p, 17);
+        memset(p, 'P', SECOND_ARRAY);
+        munmap(p, SECOND_ARRAY);
 
         r = sd_memfd_get_size(f, &sz);
         assert_se(r >= 0);
-        assert_se(sz == 17);
+        assert_se(sz == SECOND_ARRAY);
 
         r = sd_bus_message_append_array_memfd(m, 'y', f);
         assert_se(r >= 0);
@@ -115,6 +120,32 @@ int main(int argc, char *argv[]) {
         assert_se(r > 0);
 
         bus_message_dump(m);
+        sd_bus_message_rewind(m, true);
+
+        r = sd_bus_message_enter_container(m, 'r', "ayay");
+        assert_se(r > 0);
+
+        r = sd_bus_message_read_array(m, 'y', (const void**) &p, &l);
+        assert_se(r > 0);
+        assert_se(l == FIRST_ARRAY);
+
+        for (i = 0; i < l; i++)
+                assert_se(p[i] == 'L');
+
+        r = sd_bus_message_read_array(m, 'y', (const void**) &p, &l);
+        assert_se(r > 0);
+        assert_se(l == SECOND_ARRAY);
+
+        for (i = 0; i < l; i++)
+                assert_se(p[i] == 'P');
+
+        r = sd_bus_message_exit_container(m);
+        assert_se(r > 0);
+
+        r = sd_bus_message_read(m, "u", &u32);
+        assert_se(r > 0);
+        assert_se(u32 == 4711);
+
         sd_bus_message_unref(m);
 
         sd_bus_unref(a);