From b522c4b92a1a8999e008002f0a30acbaf58b55e4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 10 Feb 2021 17:59:46 +0100 Subject: [PATCH 1/4] 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. --- src/libsystemd/sd-netlink/netlink-internal.h | 2 +- src/libsystemd/sd-netlink/sd-netlink.c | 40 ++++++++++++-------- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/libsystemd/sd-netlink/netlink-internal.h b/src/libsystemd/sd-netlink/netlink-internal.h index b433bd21dc..fd7f07a6c1 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 db9fcd79dd..6220798b76 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; + /* 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))); + + m->hdr->nlmsg_seq = picked; rtnl_message_seal(m); - - return; } 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; } } From 13ec9f103b53ecc555a16048c3901d3320b26ef3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 10 Feb 2021 18:11:20 +0100 Subject: [PATCH 2/4] sd-netlink: use getsockopt_int() where appropriate --- src/libsystemd/sd-netlink/sd-netlink.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/libsystemd/sd-netlink/sd-netlink.c b/src/libsystemd/sd-netlink/sd-netlink.c index 6220798b76..b0a1eb77ec 100644 --- a/src/libsystemd/sd-netlink/sd-netlink.c +++ b/src/libsystemd/sd-netlink/sd-netlink.c @@ -92,9 +92,7 @@ static bool rtnl_pid_changed(const sd_netlink *rtnl) { int sd_netlink_open_fd(sd_netlink **ret, int fd) { _cleanup_(sd_netlink_unrefp) sd_netlink *rtnl = NULL; - int r; - int protocol; - socklen_t l; + int r, protocol; assert_return(ret, -EINVAL); assert_return(fd >= 0, -EBADF); @@ -103,8 +101,7 @@ int sd_netlink_open_fd(sd_netlink **ret, int fd) { if (r < 0) return r; - l = sizeof(protocol); - r = getsockopt(fd, SOL_SOCKET, SO_PROTOCOL, &protocol, &l); + r = getsockopt_int(fd, SOL_SOCKET, SO_PROTOCOL, &protocol); if (r < 0) return r; From baf78f1a51793c3dbbeb2b420783573d627f85a8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 10 Feb 2021 18:12:58 +0100 Subject: [PATCH 3/4] sd-netlink: reduce indentation levels a bit --- src/libsystemd/sd-netlink/sd-netlink.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/libsystemd/sd-netlink/sd-netlink.c b/src/libsystemd/sd-netlink/sd-netlink.c index b0a1eb77ec..aa9442c418 100644 --- a/src/libsystemd/sd-netlink/sd-netlink.c +++ b/src/libsystemd/sd-netlink/sd-netlink.c @@ -419,20 +419,19 @@ static int process_match(sd_netlink *rtnl, sd_netlink_message *m) { return r; LIST_FOREACH(match_callbacks, c, rtnl->match_callbacks) { - if (type == c->type) { - slot = container_of(c, sd_netlink_slot, match_callback); + if (type != c->type) + continue; - r = c->callback(rtnl, m, slot->userdata); - if (r != 0) { - if (r < 0) - log_debug_errno(r, "sd-netlink: match callback %s%s%sfailed: %m", - slot->description ? "'" : "", - strempty(slot->description), - slot->description ? "' " : ""); + slot = container_of(c, sd_netlink_slot, match_callback); - break; - } - } + r = c->callback(rtnl, m, slot->userdata); + if (r < 0) + log_debug_errno(r, "sd-netlink: match callback %s%s%sfailed: %m", + slot->description ? "'" : "", + strempty(slot->description), + slot->description ? "' " : ""); + if (r != 0) + break; } return 1; From ac3bc1b819b66f925145629a5c2ccb0a098f0446 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 10 Feb 2021 23:05:51 +0100 Subject: [PATCH 4/4] sd-netlink: spread out sequence numbers a bit An (imperfect) fix for #14760. This makes collisions unlikely, but still theoretically possible. Fixes: #14760 --- src/libsystemd/sd-netlink/sd-netlink.c | 28 +++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/libsystemd/sd-netlink/sd-netlink.c b/src/libsystemd/sd-netlink/sd-netlink.c index aa9442c418..f754d08ef4 100644 --- a/src/libsystemd/sd-netlink/sd-netlink.c +++ b/src/libsystemd/sd-netlink/sd-netlink.c @@ -36,11 +36,29 @@ static int sd_netlink_new(sd_netlink **ret) { .original_pid = getpid_cached(), .protocol = -1, - /* Change notification responses have sequence 0, so we must - * start our request sequence numbers at 1, or we may confuse our - * responses with notifications from the kernel */ - .serial = 1, - + /* Kernel change notification messages have sequence number 0. We want to avoid that with our + * own serials, in order not to get confused when matching up kernel replies to our earlier + * requests. + * + * Moreover, when using netlink socket activation (i.e. where PID 1 binds an AF_NETLINK + * socket for us and passes it to us across execve()) and we get restarted multiple times + * while the socket sticks around we might get confused by replies from earlier runs coming + * in late — which is pretty likely if we'd start our sequence numbers always from 1. Hence, + * let's start with a value based on the system clock. This should make collisions much less + * likely (though still theoretically possible). We use a 32 bit µs counter starting at boot + * for this (and explicitly exclude the zero, see above). This counter will wrap around after + * a bit more than 1h, but that's hopefully OK as the kernel shouldn't take that long to + * reply to our requests. + * + * We only pick the initial start value this way. For each message we simply increase the + * sequence number by 1. This means we could enqueue 1 netlink message per µs without risking + * collisions, which should be OK. + * + * Note this means the serials will be in the range 1…UINT32_MAX here. + * + * (In an ideal world we'd attach the current serial counter to the netlink socket itself + * somehow, to avoid all this, but I couldn't come up with a nice way to do this) */ + .serial = (uint32_t) (now(CLOCK_MONOTONIC) % UINT32_MAX) + 1, }; /* We guarantee that the read buffer has at least space for