From: Lennart Poettering <lennart@poettering.net>
Date: Wed, 10 Feb 2021 16:59:46 +0000 (+0100)
Subject: sd-netlink: revamp message serial handling
X-Git-Tag: v248-rc1~156^2~3
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b522c4b92a1a8999e008002f0a30acbaf58b55e4;p=thirdparty%2Fsystemd.git

sd-netlink: revamp message serial handling

Let's use uint32_t everywhere to maintain the seqno, since that's what
the kernel does. Prviously in the reply_callback logic we used 64bit,
for no apparent reason.

Using 32bit also provides us with the benefit that we can avoid using
uint64_hash_ops, and can use trivial_hash_ops instead for the reply
hashmap, so that we can store the seqno in the key pointer directly.

While we are at it, let's make sure we never run into serial collisions
internally (32bit is a lot, but not that much), and let's put a limit on
outstanding serials, to catch programming errors.
---

diff --git a/src/libsystemd/sd-netlink/netlink-internal.h b/src/libsystemd/sd-netlink/netlink-internal.h
index b433bd21dc0..fd7f07a6c1f 100644
--- a/src/libsystemd/sd-netlink/netlink-internal.h
+++ b/src/libsystemd/sd-netlink/netlink-internal.h
@@ -19,7 +19,7 @@
 struct reply_callback {
         sd_netlink_message_handler_t callback;
         usec_t timeout;
-        uint64_t serial;
+        uint32_t serial;
         unsigned prioq_idx;
 };
 
diff --git a/src/libsystemd/sd-netlink/sd-netlink.c b/src/libsystemd/sd-netlink/sd-netlink.c
index db9fcd79dd0..6220798b76f 100644
--- a/src/libsystemd/sd-netlink/sd-netlink.c
+++ b/src/libsystemd/sd-netlink/sd-netlink.c
@@ -17,6 +17,9 @@
 #include "string-util.h"
 #include "util.h"
 
+/* Some really high limit, to catch programming errors */
+#define REPLY_CALLBACKS_MAX UINT16_MAX
+
 static int sd_netlink_new(sd_netlink **ret) {
         _cleanup_(sd_netlink_unrefp) sd_netlink *rtnl = NULL;
 
@@ -190,18 +193,25 @@ static sd_netlink *netlink_free(sd_netlink *rtnl) {
 DEFINE_TRIVIAL_REF_UNREF_FUNC(sd_netlink, sd_netlink, netlink_free);
 
 static void rtnl_seal_message(sd_netlink *rtnl, sd_netlink_message *m) {
+        uint32_t picked;
+
         assert(rtnl);
         assert(!rtnl_pid_changed(rtnl));
         assert(m);
         assert(m->hdr);
 
-        /* don't use seq == 0, as that is used for broadcasts, so we
-           would get confused by replies to such messages */
-        m->hdr->nlmsg_seq = rtnl->serial++ ? : rtnl->serial++;
+        /* Avoid collisions with outstanding requests */
+        do {
+                picked = rtnl->serial;
 
-        rtnl_message_seal(m);
+                /* Don't use seq == 0, as that is used for broadcasts, so we would get confused by replies to
+                   such messages */
+                rtnl->serial = rtnl->serial == UINT32_MAX ? 1 : rtnl->serial + 1;
+
+        } while (hashmap_contains(rtnl->reply_callbacks, UINT32_TO_PTR(picked)));
 
-        return;
+        m->hdr->nlmsg_seq = picked;
+        rtnl_message_seal(m);
 }
 
 int sd_netlink_send(sd_netlink *nl,
@@ -339,7 +349,7 @@ static int process_timeout(sd_netlink *rtnl) {
 
         assert_se(prioq_pop(rtnl->reply_callbacks_prioq) == c);
         c->timeout = 0;
-        hashmap_remove(rtnl->reply_callbacks, &c->serial);
+        hashmap_remove(rtnl->reply_callbacks, UINT32_TO_PTR(c->serial));
 
         slot = container_of(c, sd_netlink_slot, reply_callback);
 
@@ -359,7 +369,7 @@ static int process_timeout(sd_netlink *rtnl) {
 static int process_reply(sd_netlink *rtnl, sd_netlink_message *m) {
         struct reply_callback *c;
         sd_netlink_slot *slot;
-        uint64_t serial;
+        uint32_t serial;
         uint16_t type;
         int r;
 
@@ -367,7 +377,7 @@ static int process_reply(sd_netlink *rtnl, sd_netlink_message *m) {
         assert(m);
 
         serial = rtnl_message_get_serial(m);
-        c = hashmap_remove(rtnl->reply_callbacks, &serial);
+        c = hashmap_remove(rtnl->reply_callbacks, UINT32_TO_PTR(serial));
         if (!c)
                 return 0;
 
@@ -568,7 +578,6 @@ int sd_netlink_call_async(
                 uint64_t usec,
                 const char *description) {
         _cleanup_free_ sd_netlink_slot *slot = NULL;
-        uint32_t s;
         int r, k;
 
         assert_return(nl, -EINVAL);
@@ -576,7 +585,10 @@ int sd_netlink_call_async(
         assert_return(callback, -EINVAL);
         assert_return(!rtnl_pid_changed(nl), -ECHILD);
 
-        r = hashmap_ensure_allocated(&nl->reply_callbacks, &uint64_hash_ops);
+        if (hashmap_size(nl->reply_callbacks) >= REPLY_CALLBACKS_MAX)
+                return -ERANGE;
+
+        r = hashmap_ensure_allocated(&nl->reply_callbacks, &trivial_hash_ops);
         if (r < 0)
                 return r;
 
@@ -593,20 +605,18 @@ int sd_netlink_call_async(
         slot->reply_callback.callback = callback;
         slot->reply_callback.timeout = calc_elapse(usec);
 
-        k = sd_netlink_send(nl, m, &s);
+        k = sd_netlink_send(nl, m, &slot->reply_callback.serial);
         if (k < 0)
                 return k;
 
-        slot->reply_callback.serial = s;
-
-        r = hashmap_put(nl->reply_callbacks, &slot->reply_callback.serial, &slot->reply_callback);
+        r = hashmap_put(nl->reply_callbacks, UINT32_TO_PTR(slot->reply_callback.serial), &slot->reply_callback);
         if (r < 0)
                 return r;
 
         if (slot->reply_callback.timeout != 0) {
                 r = prioq_put(nl->reply_callbacks_prioq, &slot->reply_callback, &slot->reply_callback.prioq_idx);
                 if (r < 0) {
-                        (void) hashmap_remove(nl->reply_callbacks, &slot->reply_callback.serial);
+                        (void) hashmap_remove(nl->reply_callbacks, UINT32_TO_PTR(slot->reply_callback.serial));
                         return r;
                 }
         }