From 027193db48e6d2a17e6b483387ee06299b8ab4d9 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 25 Nov 2022 02:34:57 +0900 Subject: [PATCH 01/14] sd-netlink: always initialize return variable on success --- src/libsystemd/sd-netlink/netlink-socket.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/libsystemd/sd-netlink/netlink-socket.c b/src/libsystemd/sd-netlink/netlink-socket.c index 1da459c014..b5ed533db6 100644 --- a/src/libsystemd/sd-netlink/netlink-socket.c +++ b/src/libsystemd/sd-netlink/netlink-socket.c @@ -200,8 +200,11 @@ static int socket_recv_message(int fd, struct iovec *iov, uint32_t *ret_mcast_gr if (n < 0) { if (n == -ENOBUFS) return log_debug_errno(n, "sd-netlink: kernel receive buffer overrun"); - if (ERRNO_IS_TRANSIENT(n)) + if (ERRNO_IS_TRANSIENT(n)) { + if (ret_mcast_group) + *ret_mcast_group = 0; return 0; + } return (int) n; } @@ -216,6 +219,8 @@ static int socket_recv_message(int fd, struct iovec *iov, uint32_t *ret_mcast_gr return (int) n; } + if (ret_mcast_group) + *ret_mcast_group = 0; return 0; } @@ -242,7 +247,7 @@ int socket_read_message(sd_netlink *nl) { bool multi_part = false, done = false; size_t len, allocated; struct iovec iov = {}; - uint32_t group = 0; + uint32_t group; unsigned i = 0; int r; From b374397ec5cb4eb2036386356420c557f8fd5c2f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 25 Nov 2022 02:41:57 +0900 Subject: [PATCH 02/14] sd-netlink: drop redundant 'else' --- src/libsystemd/sd-netlink/netlink-socket.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libsystemd/sd-netlink/netlink-socket.c b/src/libsystemd/sd-netlink/netlink-socket.c index b5ed533db6..a4feec395e 100644 --- a/src/libsystemd/sd-netlink/netlink-socket.c +++ b/src/libsystemd/sd-netlink/netlink-socket.c @@ -258,8 +258,7 @@ int socket_read_message(sd_netlink *nl) { r = socket_recv_message(nl->fd, &iov, NULL, true); if (r <= 0) return r; - else - len = (size_t) r; + len = (size_t) r; /* make room for the pending message */ if (!greedy_realloc((void**) &nl->rbuffer, len, sizeof(uint8_t))) @@ -272,8 +271,7 @@ int socket_read_message(sd_netlink *nl) { r = socket_recv_message(nl->fd, &iov, &group, false); if (r <= 0) return r; - else - len = (size_t) r; + len = (size_t) r; if (len > allocated) /* message did not fit in read buffer */ From 61d95dc02c26ee4cab3c066839c7bfb280be04c2 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 25 Nov 2022 02:55:32 +0900 Subject: [PATCH 03/14] sd-netlink: check received size in socket_recv_message() No functional change, just refactoring and slightly shorten socket_read_message(). --- src/libsystemd/sd-netlink/netlink-socket.c | 24 +++++++++------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/src/libsystemd/sd-netlink/netlink-socket.c b/src/libsystemd/sd-netlink/netlink-socket.c index a4feec395e..103a23270e 100644 --- a/src/libsystemd/sd-netlink/netlink-socket.c +++ b/src/libsystemd/sd-netlink/netlink-socket.c @@ -180,11 +180,12 @@ int socket_write_message(sd_netlink *nl, sd_netlink_message *m) { return k; } -static int socket_recv_message(int fd, struct iovec *iov, uint32_t *ret_mcast_group, bool peek) { +static int socket_recv_message(int fd, void *buf, size_t buf_size, uint32_t *ret_mcast_group, bool peek) { + struct iovec iov = IOVEC_MAKE(buf, buf_size); union sockaddr_union sender; CMSG_BUFFER_TYPE(CMSG_SPACE(sizeof(struct nl_pktinfo))) control; struct msghdr msg = { - .msg_iov = iov, + .msg_iov = &iov, .msg_iovlen = 1, .msg_name = &sender, .msg_namelen = sizeof(sender), @@ -194,7 +195,7 @@ static int socket_recv_message(int fd, struct iovec *iov, uint32_t *ret_mcast_gr ssize_t n; assert(fd >= 0); - assert(iov); + assert(peek || (buf && buf_size > 0)); n = recvmsg_safe(fd, &msg, MSG_TRUNC | (peek ? MSG_PEEK : 0)); if (n < 0) { @@ -224,6 +225,9 @@ static int socket_recv_message(int fd, struct iovec *iov, uint32_t *ret_mcast_gr return 0; } + if (!peek && (size_t) n > buf_size) /* message did not fit in read buffer */ + return -EIO; + if (ret_mcast_group) { struct nl_pktinfo *pi; @@ -245,8 +249,7 @@ static int socket_recv_message(int fd, struct iovec *iov, uint32_t *ret_mcast_gr int socket_read_message(sd_netlink *nl) { _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *first = NULL; bool multi_part = false, done = false; - size_t len, allocated; - struct iovec iov = {}; + size_t len; uint32_t group; unsigned i = 0; int r; @@ -255,7 +258,7 @@ int socket_read_message(sd_netlink *nl) { assert(nl->rbuffer); /* read nothing, just get the pending message size */ - r = socket_recv_message(nl->fd, &iov, NULL, true); + r = socket_recv_message(nl->fd, NULL, 0, NULL, true); if (r <= 0) return r; len = (size_t) r; @@ -264,19 +267,12 @@ int socket_read_message(sd_netlink *nl) { if (!greedy_realloc((void**) &nl->rbuffer, len, sizeof(uint8_t))) return -ENOMEM; - allocated = MALLOC_SIZEOF_SAFE(nl->rbuffer); - iov = IOVEC_MAKE(nl->rbuffer, allocated); - /* read the pending message */ - r = socket_recv_message(nl->fd, &iov, &group, false); + r = socket_recv_message(nl->fd, nl->rbuffer, MALLOC_SIZEOF_SAFE(nl->rbuffer), &group, false); if (r <= 0) return r; len = (size_t) r; - if (len > allocated) - /* message did not fit in read buffer */ - return -EIO; - if (NLMSG_OK(nl->rbuffer, len) && nl->rbuffer->nlmsg_flags & NLM_F_MULTI) { multi_part = true; From fb298ef10440053fa9a6a72a43d67f04ae14c72f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 25 Nov 2022 02:56:42 +0900 Subject: [PATCH 04/14] sd-netlink: allocate read buffer when necessary --- src/libsystemd/sd-netlink/netlink-socket.c | 1 - src/libsystemd/sd-netlink/sd-netlink.c | 4 ---- 2 files changed, 5 deletions(-) diff --git a/src/libsystemd/sd-netlink/netlink-socket.c b/src/libsystemd/sd-netlink/netlink-socket.c index 103a23270e..55dc51ab50 100644 --- a/src/libsystemd/sd-netlink/netlink-socket.c +++ b/src/libsystemd/sd-netlink/netlink-socket.c @@ -255,7 +255,6 @@ int socket_read_message(sd_netlink *nl) { int r; assert(nl); - assert(nl->rbuffer); /* read nothing, just get the pending message size */ r = socket_recv_message(nl->fd, NULL, 0, NULL, true); diff --git a/src/libsystemd/sd-netlink/sd-netlink.c b/src/libsystemd/sd-netlink/sd-netlink.c index b99abae640..1202fe10a5 100644 --- a/src/libsystemd/sd-netlink/sd-netlink.c +++ b/src/libsystemd/sd-netlink/sd-netlink.c @@ -61,10 +61,6 @@ static int netlink_new(sd_netlink **ret) { .serial = (uint32_t) (now(CLOCK_MONOTONIC) % UINT32_MAX) + 1, }; - /* We guarantee that the read buffer has at least space for a message header */ - if (!greedy_realloc((void**) &nl->rbuffer, sizeof(struct nlmsghdr), sizeof(uint8_t))) - return -ENOMEM; - *ret = TAKE_PTR(nl); return 0; } From b491454dfc5d8081ef429af1d1a01aed0b83bd25 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 25 Nov 2022 03:33:52 +0900 Subject: [PATCH 05/14] sd-netlink: return earlier when received invalid message --- src/libsystemd/sd-netlink/netlink-socket.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/libsystemd/sd-netlink/netlink-socket.c b/src/libsystemd/sd-netlink/netlink-socket.c index 55dc51ab50..5128f8fb4b 100644 --- a/src/libsystemd/sd-netlink/netlink-socket.c +++ b/src/libsystemd/sd-netlink/netlink-socket.c @@ -272,7 +272,12 @@ int socket_read_message(sd_netlink *nl) { return r; len = (size_t) r; - if (NLMSG_OK(nl->rbuffer, len) && nl->rbuffer->nlmsg_flags & NLM_F_MULTI) { + if (!NLMSG_OK(nl->rbuffer, len)) { + log_debug("sd-netlink: received invalid message, discarding %zu bytes of incoming message", len); + return 0; + } + + if (nl->rbuffer->nlmsg_flags & NLM_F_MULTI) { multi_part = true; for (i = 0; i < nl->rqueue_partial_size; i++) @@ -341,7 +346,7 @@ int socket_read_message(sd_netlink *nl) { } if (len > 0) - log_debug("sd-netlink: discarding %zu bytes of incoming message", len); + log_debug("sd-netlink: discarding trailing %zu bytes of incoming message", len); if (!first) return 0; From bc1a8e2b6ea5477fb662f032f0ac553f319b711e Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 25 Nov 2022 03:36:39 +0900 Subject: [PATCH 06/14] sd-netlink: fix possible use-after-free When we receive a multi-part message and fail to parse it, then the prviously received message is freed with the _cleanup_ attribute, but still referenced by sd_netlink.rqueue_partial. That causes use-after-free when we receive another multi-part message. --- src/libsystemd/sd-netlink/netlink-socket.c | 51 ++++++++++------------ 1 file changed, 24 insertions(+), 27 deletions(-) diff --git a/src/libsystemd/sd-netlink/netlink-socket.c b/src/libsystemd/sd-netlink/netlink-socket.c index 5128f8fb4b..fcf971b8f6 100644 --- a/src/libsystemd/sd-netlink/netlink-socket.c +++ b/src/libsystemd/sd-netlink/netlink-socket.c @@ -241,6 +241,24 @@ static int socket_recv_message(int fd, void *buf, size_t buf_size, uint32_t *ret return (int) n; } +static sd_netlink_message *netlink_take_partial_message(sd_netlink *nl, uint32_t seqnum) { + assert(nl); + + for (unsigned i = 0; i < nl->rqueue_partial_size; i++) + if (message_get_serial(nl->rqueue_partial[i]) == seqnum) { + sd_netlink_message *found = nl->rqueue_partial[i]; + + /* remove the message form the partial read queue */ + memmove(nl->rqueue_partial + i, nl->rqueue_partial + i + 1, + sizeof(sd_netlink_message*) * (nl->rqueue_partial_size - i - 1)); + nl->rqueue_partial_size--; + + return found; + } + + return NULL; +} + /* On success, the number of bytes received is returned and *ret points to the received message * which has a valid header and the correct size. * If nothing useful was received 0 is returned. @@ -251,7 +269,6 @@ int socket_read_message(sd_netlink *nl) { bool multi_part = false, done = false; size_t len; uint32_t group; - unsigned i = 0; int r; assert(nl); @@ -279,13 +296,7 @@ int socket_read_message(sd_netlink *nl) { if (nl->rbuffer->nlmsg_flags & NLM_F_MULTI) { multi_part = true; - - for (i = 0; i < nl->rqueue_partial_size; i++) - if (message_get_serial(nl->rqueue_partial[i]) == - nl->rbuffer->nlmsg_seq) { - first = nl->rqueue_partial[i]; - break; - } + first = netlink_take_partial_message(nl, nl->rbuffer->nlmsg_seq); } for (struct nlmsghdr *new_msg = nl->rbuffer; NLMSG_OK(new_msg, len) && !done; new_msg = NLMSG_NEXT(new_msg, len)) { @@ -358,28 +369,14 @@ int socket_read_message(sd_netlink *nl) { return r; nl->rqueue[nl->rqueue_size++] = TAKE_PTR(first); - - if (multi_part && (i < nl->rqueue_partial_size)) { - /* remove the message form the partial read queue */ - memmove(nl->rqueue_partial + i, nl->rqueue_partial + i + 1, - sizeof(sd_netlink_message*) * (nl->rqueue_partial_size - i - 1)); - nl->rqueue_partial_size--; - } - return 1; } else { - /* we only got a partial multi-part message, push it on the - partial read queue */ - if (i < nl->rqueue_partial_size) - nl->rqueue_partial[i] = TAKE_PTR(first); - else { - r = netlink_rqueue_partial_make_room(nl); - if (r < 0) - return r; - - nl->rqueue_partial[nl->rqueue_partial_size++] = TAKE_PTR(first); - } + /* we only got a partial multi-part message, push it on the partial read queue. */ + r = netlink_rqueue_partial_make_room(nl); + if (r < 0) + return r; + nl->rqueue_partial[nl->rqueue_partial_size++] = TAKE_PTR(first); return 0; } } From 766417bd763db913efdd9c48442b1094c23d7042 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 26 Nov 2022 09:57:16 +0900 Subject: [PATCH 07/14] sd-netlink: fix segfault --- src/libsystemd/sd-netlink/sd-netlink.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libsystemd/sd-netlink/sd-netlink.c b/src/libsystemd/sd-netlink/sd-netlink.c index 1202fe10a5..ec1035fdd1 100644 --- a/src/libsystemd/sd-netlink/sd-netlink.c +++ b/src/libsystemd/sd-netlink/sd-netlink.c @@ -593,7 +593,8 @@ int sd_netlink_read( return r; if (type == NLMSG_DONE) { - *ret = NULL; + if (ret) + *ret = NULL; return 0; } From 2ea465ef6246b200f94645d42322c914d5e13082 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 26 Nov 2022 10:06:05 +0900 Subject: [PATCH 08/14] sd-netlink: introduce netlink_queue_received_message() and friend No functional change, just refactoring. --- src/libsystemd/sd-netlink/netlink-internal.h | 2 - src/libsystemd/sd-netlink/netlink-socket.c | 55 +++++++++++++++----- src/libsystemd/sd-netlink/sd-netlink.c | 28 ---------- 3 files changed, 41 insertions(+), 44 deletions(-) diff --git a/src/libsystemd/sd-netlink/netlink-internal.h b/src/libsystemd/sd-netlink/netlink-internal.h index 514f22511c..964f7c0016 100644 --- a/src/libsystemd/sd-netlink/netlink-internal.h +++ b/src/libsystemd/sd-netlink/netlink-internal.h @@ -148,8 +148,6 @@ void message_seal(sd_netlink_message *m); int netlink_open_family(sd_netlink **ret, int family); bool netlink_pid_changed(sd_netlink *nl); -int netlink_rqueue_make_room(sd_netlink *nl); -int netlink_rqueue_partial_make_room(sd_netlink *nl); int socket_bind(sd_netlink *nl); int socket_broadcast_group_ref(sd_netlink *nl, unsigned group); diff --git a/src/libsystemd/sd-netlink/netlink-socket.c b/src/libsystemd/sd-netlink/netlink-socket.c index fcf971b8f6..1480525c73 100644 --- a/src/libsystemd/sd-netlink/netlink-socket.c +++ b/src/libsystemd/sd-netlink/netlink-socket.c @@ -241,6 +241,39 @@ static int socket_recv_message(int fd, void *buf, size_t buf_size, uint32_t *ret return (int) n; } +static int netlink_queue_received_message(sd_netlink *nl, sd_netlink_message *m) { + assert(nl); + assert(m); + + if (nl->rqueue_size >= NETLINK_RQUEUE_MAX) + return log_debug_errno(SYNTHETIC_ERRNO(ENOBUFS), + "sd-netlink: exhausted the read queue size (%d)", + NETLINK_RQUEUE_MAX); + + if (!GREEDY_REALLOC(nl->rqueue, nl->rqueue_size + 1)) + return -ENOMEM; + + nl->rqueue[nl->rqueue_size++] = sd_netlink_message_ref(m); + return 0; +} + +static int netlink_queue_partially_received_message(sd_netlink *nl, sd_netlink_message *m) { + assert(nl); + assert(m); + assert(m->hdr->nlmsg_flags & NLM_F_MULTI); + + if (nl->rqueue_partial_size >= NETLINK_RQUEUE_MAX) + return log_debug_errno(SYNTHETIC_ERRNO(ENOBUFS), + "sd-netlink: exhausted the partial read queue size (%d)", + NETLINK_RQUEUE_MAX); + + if (!GREEDY_REALLOC(nl->rqueue_partial, nl->rqueue_partial_size + 1)) + return -ENOMEM; + + nl->rqueue_partial[nl->rqueue_partial_size++] = sd_netlink_message_ref(m); + return 0; +} + static sd_netlink_message *netlink_take_partial_message(sd_netlink *nl, uint32_t seqnum) { assert(nl); @@ -362,21 +395,15 @@ int socket_read_message(sd_netlink *nl) { if (!first) return 0; - if (!multi_part || done) { + done = done || !multi_part; + if (done) /* we got a complete message, push it on the read queue */ - r = netlink_rqueue_make_room(nl); - if (r < 0) - return r; - - nl->rqueue[nl->rqueue_size++] = TAKE_PTR(first); - return 1; - } else { + r = netlink_queue_received_message(nl, first); + else /* we only got a partial multi-part message, push it on the partial read queue. */ - r = netlink_rqueue_partial_make_room(nl); - if (r < 0) - return r; + r = netlink_queue_partially_received_message(nl, first); + if (r < 0) + return r; - nl->rqueue_partial[nl->rqueue_partial_size++] = TAKE_PTR(first); - return 0; - } + return done; } diff --git a/src/libsystemd/sd-netlink/sd-netlink.c b/src/libsystemd/sd-netlink/sd-netlink.c index ec1035fdd1..7818a179c8 100644 --- a/src/libsystemd/sd-netlink/sd-netlink.c +++ b/src/libsystemd/sd-netlink/sd-netlink.c @@ -175,34 +175,6 @@ int sd_netlink_send( return 1; } -int netlink_rqueue_make_room(sd_netlink *nl) { - assert(nl); - - if (nl->rqueue_size >= NETLINK_RQUEUE_MAX) - return log_debug_errno(SYNTHETIC_ERRNO(ENOBUFS), - "sd-netlink: exhausted the read queue size (%d)", - NETLINK_RQUEUE_MAX); - - if (!GREEDY_REALLOC(nl->rqueue, nl->rqueue_size + 1)) - return -ENOMEM; - - return 0; -} - -int netlink_rqueue_partial_make_room(sd_netlink *nl) { - assert(nl); - - if (nl->rqueue_partial_size >= NETLINK_RQUEUE_MAX) - return log_debug_errno(SYNTHETIC_ERRNO(ENOBUFS), - "sd-netlink: exhausted the partial read queue size (%d)", - NETLINK_RQUEUE_MAX); - - if (!GREEDY_REALLOC(nl->rqueue_partial, nl->rqueue_partial_size + 1)) - return -ENOMEM; - - return 0; -} - static int dispatch_rqueue(sd_netlink *nl, sd_netlink_message **message) { int r; From e417c4ac44f0904827c2451be491482dcfb30120 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 26 Nov 2022 10:10:58 +0900 Subject: [PATCH 09/14] sd-netlink: reimplement received message queue By using OrderedSet and Hashmap, we can drop all memmove() calls. No functional changes, just refactoring. --- src/libsystemd/sd-netlink/netlink-internal.h | 8 ++- src/libsystemd/sd-netlink/netlink-socket.c | 55 +++++++++----------- src/libsystemd/sd-netlink/sd-netlink.c | 51 +++++++----------- 3 files changed, 46 insertions(+), 68 deletions(-) diff --git a/src/libsystemd/sd-netlink/netlink-internal.h b/src/libsystemd/sd-netlink/netlink-internal.h index 964f7c0016..9ad4c03a3c 100644 --- a/src/libsystemd/sd-netlink/netlink-internal.h +++ b/src/libsystemd/sd-netlink/netlink-internal.h @@ -7,6 +7,7 @@ #include "list.h" #include "netlink-types.h" +#include "ordered-set.h" #include "prioq.h" #include "time-util.h" @@ -72,11 +73,8 @@ struct sd_netlink { Hashmap *broadcast_group_refs; bool broadcast_group_dont_leave:1; /* until we can rely on 4.2 */ - sd_netlink_message **rqueue; - unsigned rqueue_size; - - sd_netlink_message **rqueue_partial; - unsigned rqueue_partial_size; + OrderedSet *rqueue; + Hashmap *rqueue_partial_by_serial; struct nlmsghdr *rbuffer; diff --git a/src/libsystemd/sd-netlink/netlink-socket.c b/src/libsystemd/sd-netlink/netlink-socket.c index 1480525c73..95ac7bad1a 100644 --- a/src/libsystemd/sd-netlink/netlink-socket.c +++ b/src/libsystemd/sd-netlink/netlink-socket.c @@ -241,57 +241,50 @@ static int socket_recv_message(int fd, void *buf, size_t buf_size, uint32_t *ret return (int) n; } +DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR( + netlink_message_hash_ops, + void, trivial_hash_func, trivial_compare_func, + sd_netlink_message, sd_netlink_message_unref); + static int netlink_queue_received_message(sd_netlink *nl, sd_netlink_message *m) { + int r; + assert(nl); assert(m); - if (nl->rqueue_size >= NETLINK_RQUEUE_MAX) + if (ordered_set_size(nl->rqueue) >= NETLINK_RQUEUE_MAX) return log_debug_errno(SYNTHETIC_ERRNO(ENOBUFS), - "sd-netlink: exhausted the read queue size (%d)", - NETLINK_RQUEUE_MAX); + "sd-netlink: exhausted the read queue size (%d)", NETLINK_RQUEUE_MAX); - if (!GREEDY_REALLOC(nl->rqueue, nl->rqueue_size + 1)) - return -ENOMEM; + r = ordered_set_ensure_put(&nl->rqueue, &netlink_message_hash_ops, m); + if (r < 0) + return r; - nl->rqueue[nl->rqueue_size++] = sd_netlink_message_ref(m); + sd_netlink_message_ref(m); return 0; } static int netlink_queue_partially_received_message(sd_netlink *nl, sd_netlink_message *m) { + uint32_t serial; + int r; + assert(nl); assert(m); assert(m->hdr->nlmsg_flags & NLM_F_MULTI); - if (nl->rqueue_partial_size >= NETLINK_RQUEUE_MAX) + if (hashmap_size(nl->rqueue_partial_by_serial) >= NETLINK_RQUEUE_MAX) return log_debug_errno(SYNTHETIC_ERRNO(ENOBUFS), - "sd-netlink: exhausted the partial read queue size (%d)", - NETLINK_RQUEUE_MAX); + "sd-netlink: exhausted the partial read queue size (%d)", NETLINK_RQUEUE_MAX); - if (!GREEDY_REALLOC(nl->rqueue_partial, nl->rqueue_partial_size + 1)) - return -ENOMEM; + serial = message_get_serial(m); + r = hashmap_ensure_put(&nl->rqueue_partial_by_serial, &netlink_message_hash_ops, UINT32_TO_PTR(serial), m); + if (r < 0) + return r; - nl->rqueue_partial[nl->rqueue_partial_size++] = sd_netlink_message_ref(m); + sd_netlink_message_ref(m); return 0; } -static sd_netlink_message *netlink_take_partial_message(sd_netlink *nl, uint32_t seqnum) { - assert(nl); - - for (unsigned i = 0; i < nl->rqueue_partial_size; i++) - if (message_get_serial(nl->rqueue_partial[i]) == seqnum) { - sd_netlink_message *found = nl->rqueue_partial[i]; - - /* remove the message form the partial read queue */ - memmove(nl->rqueue_partial + i, nl->rqueue_partial + i + 1, - sizeof(sd_netlink_message*) * (nl->rqueue_partial_size - i - 1)); - nl->rqueue_partial_size--; - - return found; - } - - return NULL; -} - /* On success, the number of bytes received is returned and *ret points to the received message * which has a valid header and the correct size. * If nothing useful was received 0 is returned. @@ -329,7 +322,7 @@ int socket_read_message(sd_netlink *nl) { if (nl->rbuffer->nlmsg_flags & NLM_F_MULTI) { multi_part = true; - first = netlink_take_partial_message(nl, nl->rbuffer->nlmsg_seq); + first = hashmap_remove(nl->rqueue_partial_by_serial, UINT32_TO_PTR(nl->rbuffer->nlmsg_seq)); } for (struct nlmsghdr *new_msg = nl->rbuffer; NLMSG_OK(new_msg, len) && !done; new_msg = NLMSG_NEXT(new_msg, len)) { diff --git a/src/libsystemd/sd-netlink/sd-netlink.c b/src/libsystemd/sd-netlink/sd-netlink.c index 7818a179c8..1f4f8e9015 100644 --- a/src/libsystemd/sd-netlink/sd-netlink.c +++ b/src/libsystemd/sd-netlink/sd-netlink.c @@ -116,18 +116,11 @@ int sd_netlink_increase_rxbuf(sd_netlink *nl, size_t size) { static sd_netlink *netlink_free(sd_netlink *nl) { sd_netlink_slot *s; - unsigned i; assert(nl); - for (i = 0; i < nl->rqueue_size; i++) - sd_netlink_message_unref(nl->rqueue[i]); - free(nl->rqueue); - - for (i = 0; i < nl->rqueue_partial_size; i++) - sd_netlink_message_unref(nl->rqueue_partial[i]); - free(nl->rqueue_partial); - + ordered_set_free(nl->rqueue); + hashmap_free(nl->rqueue_partial_by_serial); free(nl->rbuffer); while ((s = nl->slots)) { @@ -175,29 +168,26 @@ int sd_netlink_send( return 1; } -static int dispatch_rqueue(sd_netlink *nl, sd_netlink_message **message) { +static int dispatch_rqueue(sd_netlink *nl, sd_netlink_message **ret) { + sd_netlink_message *m; int r; assert(nl); - assert(message); + assert(ret); - if (nl->rqueue_size <= 0) { + if (ordered_set_size(nl->rqueue) <= 0) { /* Try to read a new message */ r = socket_read_message(nl); - if (r == -ENOBUFS) { /* FIXME: ignore buffer overruns for now */ + if (r == -ENOBUFS) /* FIXME: ignore buffer overruns for now */ log_debug_errno(r, "sd-netlink: Got ENOBUFS from netlink socket, ignoring."); - return 1; - } - if (r <= 0) + else if (r < 0) return r; } /* Dispatch a queued message */ - *message = nl->rqueue[0]; - nl->rqueue_size--; - memmove(nl->rqueue, nl->rqueue + 1, sizeof(sd_netlink_message*) * nl->rqueue_size); - - return 1; + m = ordered_set_steal_first(nl->rqueue); + *ret = m; + return !!m; } static int process_timeout(sd_netlink *nl) { @@ -437,7 +427,7 @@ int sd_netlink_wait(sd_netlink *nl, uint64_t timeout_usec) { assert_return(nl, -EINVAL); assert_return(!netlink_pid_changed(nl), -ECHILD); - if (nl->rqueue_size > 0) + if (ordered_set_size(nl->rqueue) > 0) return 0; r = netlink_poll(nl, false, timeout_usec); @@ -538,23 +528,21 @@ int sd_netlink_read( timeout = calc_elapse(usec); for (;;) { + sd_netlink_message *m; usec_t left; - for (unsigned i = 0; i < nl->rqueue_size; i++) { + ORDERED_SET_FOREACH(m, nl->rqueue) { _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *incoming = NULL; uint32_t received_serial; uint16_t type; - received_serial = message_get_serial(nl->rqueue[i]); + received_serial = message_get_serial(m); if (received_serial != serial) continue; - incoming = nl->rqueue[i]; - /* found a match, remove from rqueue and return it */ - memmove(nl->rqueue + i, nl->rqueue + i + 1, - sizeof(sd_netlink_message*) * (nl->rqueue_size - i - 1)); - nl->rqueue_size--; + ordered_set_remove(nl->rqueue, m); + incoming = TAKE_PTR(m); r = sd_netlink_message_get_errno(incoming); if (r < 0) @@ -625,7 +613,7 @@ int sd_netlink_get_events(sd_netlink *nl) { assert_return(nl, -EINVAL); assert_return(!netlink_pid_changed(nl), -ECHILD); - return nl->rqueue_size == 0 ? POLLIN : 0; + return ordered_set_size(nl->rqueue) == 0 ? POLLIN : 0; } int sd_netlink_get_timeout(sd_netlink *nl, uint64_t *timeout_usec) { @@ -635,7 +623,7 @@ int sd_netlink_get_timeout(sd_netlink *nl, uint64_t *timeout_usec) { assert_return(timeout_usec, -EINVAL); assert_return(!netlink_pid_changed(nl), -ECHILD); - if (nl->rqueue_size > 0) { + if (ordered_set_size(nl->rqueue) > 0) { *timeout_usec = 0; return 1; } @@ -647,7 +635,6 @@ int sd_netlink_get_timeout(sd_netlink *nl, uint64_t *timeout_usec) { } *timeout_usec = c->timeout; - return 1; } From 7b34bae3b1a8726e241a56600a6edf9b3733a4f4 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 26 Nov 2022 11:17:36 +0900 Subject: [PATCH 10/14] sd-netlink: also manage received messages by serial Then, we can easily find the received message matching with requested serial. --- src/libsystemd/sd-netlink/netlink-internal.h | 1 + src/libsystemd/sd-netlink/netlink-socket.c | 31 ++++++++++++++++++++ src/libsystemd/sd-netlink/sd-netlink.c | 22 ++++++-------- 3 files changed, 41 insertions(+), 13 deletions(-) diff --git a/src/libsystemd/sd-netlink/netlink-internal.h b/src/libsystemd/sd-netlink/netlink-internal.h index 9ad4c03a3c..bca13bce57 100644 --- a/src/libsystemd/sd-netlink/netlink-internal.h +++ b/src/libsystemd/sd-netlink/netlink-internal.h @@ -74,6 +74,7 @@ struct sd_netlink { bool broadcast_group_dont_leave:1; /* until we can rely on 4.2 */ OrderedSet *rqueue; + Hashmap *rqueue_by_serial; Hashmap *rqueue_partial_by_serial; struct nlmsghdr *rbuffer; diff --git a/src/libsystemd/sd-netlink/netlink-socket.c b/src/libsystemd/sd-netlink/netlink-socket.c index 95ac7bad1a..3c7447c73c 100644 --- a/src/libsystemd/sd-netlink/netlink-socket.c +++ b/src/libsystemd/sd-netlink/netlink-socket.c @@ -247,6 +247,7 @@ DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR( sd_netlink_message, sd_netlink_message_unref); static int netlink_queue_received_message(sd_netlink *nl, sd_netlink_message *m) { + uint32_t serial; int r; assert(nl); @@ -260,6 +261,36 @@ static int netlink_queue_received_message(sd_netlink *nl, sd_netlink_message *m) if (r < 0) return r; + sd_netlink_message_ref(m); + + if (sd_netlink_message_is_broadcast(m)) + return 0; + + serial = message_get_serial(m); + if (serial == 0) + return 0; + + if (sd_netlink_message_get_errno(m) < 0) { + _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *old = NULL; + + old = hashmap_remove(nl->rqueue_by_serial, UINT32_TO_PTR(serial)); + if (old) + log_debug("sd-netlink: received error message with serial %"PRIu32", but another message with " + "the same serial is already stored in the read queue, replacing.", serial); + } + + r = hashmap_ensure_put(&nl->rqueue_by_serial, &netlink_message_hash_ops, UINT32_TO_PTR(serial), m); + if (r == -EEXIST) { + if (!sd_netlink_message_is_error(m)) + log_debug("sd-netlink: received message with serial %"PRIu32", but another message with " + "the same serial is already stored in the read queue, ignoring.", serial); + return 0; + } + if (r < 0) { + sd_netlink_message_unref(ordered_set_remove(nl->rqueue, m)); + return r; + } + sd_netlink_message_ref(m); return 0; } diff --git a/src/libsystemd/sd-netlink/sd-netlink.c b/src/libsystemd/sd-netlink/sd-netlink.c index 1f4f8e9015..6eb7f659ae 100644 --- a/src/libsystemd/sd-netlink/sd-netlink.c +++ b/src/libsystemd/sd-netlink/sd-netlink.c @@ -120,6 +120,7 @@ static sd_netlink *netlink_free(sd_netlink *nl) { assert(nl); ordered_set_free(nl->rqueue); + hashmap_free(nl->rqueue_by_serial); hashmap_free(nl->rqueue_partial_by_serial); free(nl->rbuffer); @@ -186,6 +187,7 @@ static int dispatch_rqueue(sd_netlink *nl, sd_netlink_message **ret) { /* Dispatch a queued message */ m = ordered_set_steal_first(nl->rqueue); + sd_netlink_message_unref(hashmap_remove_value(nl->rqueue_by_serial, UINT32_TO_PTR(message_get_serial(m)), m)); *ret = m; return !!m; } @@ -528,27 +530,21 @@ int sd_netlink_read( timeout = calc_elapse(usec); for (;;) { - sd_netlink_message *m; + _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *m = NULL; usec_t left; - ORDERED_SET_FOREACH(m, nl->rqueue) { - _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *incoming = NULL; - uint32_t received_serial; + m = hashmap_remove(nl->rqueue_by_serial, UINT32_TO_PTR(serial)); + if (m) { uint16_t type; - received_serial = message_get_serial(m); - if (received_serial != serial) - continue; - /* found a match, remove from rqueue and return it */ - ordered_set_remove(nl->rqueue, m); - incoming = TAKE_PTR(m); + sd_netlink_message_unref(ordered_set_remove(nl->rqueue, m)); - r = sd_netlink_message_get_errno(incoming); + r = sd_netlink_message_get_errno(m); if (r < 0) return r; - r = sd_netlink_message_get_type(incoming, &type); + r = sd_netlink_message_get_type(m, &type); if (r < 0) return r; @@ -559,7 +555,7 @@ int sd_netlink_read( } if (ret) - *ret = TAKE_PTR(incoming); + *ret = TAKE_PTR(m); return 1; } From 2b176edc7455129d7944ca4c5480afe3aeb086c1 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 26 Nov 2022 10:14:00 +0900 Subject: [PATCH 11/14] sd-netlink: do not use serials currently queued --- src/libsystemd/sd-netlink/netlink-util.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/libsystemd/sd-netlink/netlink-util.c b/src/libsystemd/sd-netlink/netlink-util.c index 12cdc99ff2..c6091542d2 100644 --- a/src/libsystemd/sd-netlink/netlink-util.c +++ b/src/libsystemd/sd-netlink/netlink-util.c @@ -673,6 +673,15 @@ int netlink_open_family(sd_netlink **ret, int family) { return 0; } +static bool serial_used(sd_netlink *nl, uint32_t serial) { + assert(nl); + + return + hashmap_contains(nl->reply_callbacks, UINT32_TO_PTR(serial)) || + hashmap_contains(nl->rqueue_by_serial, UINT32_TO_PTR(serial)) || + hashmap_contains(nl->rqueue_partial_by_serial, UINT32_TO_PTR(serial)); +} + void netlink_seal_message(sd_netlink *nl, sd_netlink_message *m) { uint32_t picked; @@ -689,7 +698,7 @@ void netlink_seal_message(sd_netlink *nl, sd_netlink_message *m) { such messages */ nl->serial = nl->serial == UINT32_MAX ? 1 : nl->serial + 1; - } while (hashmap_contains(nl->reply_callbacks, UINT32_TO_PTR(picked))); + } while (serial_used(nl, picked)); m->hdr->nlmsg_seq = picked; message_seal(m); From 9482429af9696ce16b90b2de97b15336235bcd30 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 26 Nov 2022 09:17:14 +0900 Subject: [PATCH 12/14] sd-netlink: split out parse_message_one() from socket_read_message() No functional change, just refactoring and preparation for later commits. --- src/libsystemd/sd-netlink/netlink-socket.c | 99 +++++++++++++--------- 1 file changed, 60 insertions(+), 39 deletions(-) diff --git a/src/libsystemd/sd-netlink/netlink-socket.c b/src/libsystemd/sd-netlink/netlink-socket.c index 3c7447c73c..1aaa4b712c 100644 --- a/src/libsystemd/sd-netlink/netlink-socket.c +++ b/src/libsystemd/sd-netlink/netlink-socket.c @@ -316,6 +316,60 @@ static int netlink_queue_partially_received_message(sd_netlink *nl, sd_netlink_m return 0; } +static int parse_message_one(sd_netlink *nl, uint32_t group, const struct nlmsghdr *hdr, sd_netlink_message **ret) { + _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *m = NULL; + size_t size; + int r; + + assert(nl); + assert(hdr); + assert(ret); + + /* not broadcast and not for us */ + if (group == 0 && hdr->nlmsg_pid != nl->sockaddr.nl.nl_pid) + goto finalize; + + /* silently drop noop messages */ + if (hdr->nlmsg_type == NLMSG_NOOP) + goto finalize; + + /* check that we support this message type */ + r = netlink_get_policy_set_and_header_size(nl, hdr->nlmsg_type, NULL, &size); + if (r == -EOPNOTSUPP) { + log_debug("sd-netlink: ignored message with unknown type: %i", hdr->nlmsg_type); + goto finalize; + } + if (r < 0) + return r; + + /* check that the size matches the message type */ + if (hdr->nlmsg_len < NLMSG_LENGTH(size)) { + log_debug("sd-netlink: message is shorter than expected, dropping."); + goto finalize; + } + + r = message_new_empty(nl, &m); + if (r < 0) + return r; + + m->multicast_group = group; + m->hdr = memdup(hdr, hdr->nlmsg_len); + if (!m->hdr) + return -ENOMEM; + + /* seal and parse the top-level message */ + r = sd_netlink_message_rewind(m, nl); + if (r < 0) + return r; + + *ret = TAKE_PTR(m); + return 1; + +finalize: + *ret = NULL; + return 0; +} + /* On success, the number of bytes received is returned and *ret points to the received message * which has a valid header and the correct size. * If nothing useful was received 0 is returned. @@ -356,19 +410,16 @@ int socket_read_message(sd_netlink *nl) { first = hashmap_remove(nl->rqueue_partial_by_serial, UINT32_TO_PTR(nl->rbuffer->nlmsg_seq)); } - for (struct nlmsghdr *new_msg = nl->rbuffer; NLMSG_OK(new_msg, len) && !done; new_msg = NLMSG_NEXT(new_msg, len)) { + for (struct nlmsghdr *hdr = nl->rbuffer; NLMSG_OK(hdr, len) && !done; hdr = NLMSG_NEXT(hdr, len)) { _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *m = NULL; - size_t size; - if (group == 0 && new_msg->nlmsg_pid != nl->sockaddr.nl.nl_pid) - /* not broadcast and not for us */ + r = parse_message_one(nl, group, hdr, &m); + if (r < 0) + return r; + if (r == 0) continue; - if (new_msg->nlmsg_type == NLMSG_NOOP) - /* silently drop noop messages */ - continue; - - if (new_msg->nlmsg_type == NLMSG_DONE) { + if (hdr->nlmsg_type == NLMSG_DONE) { /* finished reading multi-part message */ done = true; @@ -377,36 +428,6 @@ int socket_read_message(sd_netlink *nl) { continue; } - /* check that we support this message type */ - r = netlink_get_policy_set_and_header_size(nl, new_msg->nlmsg_type, NULL, &size); - if (r < 0) { - if (r == -EOPNOTSUPP) - log_debug("sd-netlink: ignored message with unknown type: %i", - new_msg->nlmsg_type); - - continue; - } - - /* check that the size matches the message type */ - if (new_msg->nlmsg_len < NLMSG_LENGTH(size)) { - log_debug("sd-netlink: message is shorter than expected, dropping"); - continue; - } - - r = message_new_empty(nl, &m); - if (r < 0) - return r; - - m->multicast_group = group; - m->hdr = memdup(new_msg, new_msg->nlmsg_len); - if (!m->hdr) - return -ENOMEM; - - /* seal and parse the top-level message */ - r = sd_netlink_message_rewind(m, nl); - if (r < 0) - return r; - /* push the message onto the multi-part message stack */ if (first) m->next = first; From a8ac052624495d5f974674187e29c6fb9671f94f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 26 Nov 2022 09:35:53 +0900 Subject: [PATCH 13/14] sd-netlink: do not link non-multipart messages Previously, if a single packet contains multiple non-multipart messages, then the messages were linked and saved as a single entry, especially even if the messages has different serial numbers. Though, not sure if the kernel sends such packet. But at least for safety, let's link only multipart messages. --- src/libsystemd/sd-netlink/netlink-socket.c | 68 +++++++++++----------- 1 file changed, 35 insertions(+), 33 deletions(-) diff --git a/src/libsystemd/sd-netlink/netlink-socket.c b/src/libsystemd/sd-netlink/netlink-socket.c index 1aaa4b712c..2a9f641419 100644 --- a/src/libsystemd/sd-netlink/netlink-socket.c +++ b/src/libsystemd/sd-netlink/netlink-socket.c @@ -376,10 +376,9 @@ finalize: * On failure, a negative error code is returned. */ int socket_read_message(sd_netlink *nl) { - _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *first = NULL; - bool multi_part = false, done = false; - size_t len; + bool done = false; uint32_t group; + size_t len; int r; assert(nl); @@ -405,12 +404,7 @@ int socket_read_message(sd_netlink *nl) { return 0; } - if (nl->rbuffer->nlmsg_flags & NLM_F_MULTI) { - multi_part = true; - first = hashmap_remove(nl->rqueue_partial_by_serial, UINT32_TO_PTR(nl->rbuffer->nlmsg_seq)); - } - - for (struct nlmsghdr *hdr = nl->rbuffer; NLMSG_OK(hdr, len) && !done; hdr = NLMSG_NEXT(hdr, len)) { + for (struct nlmsghdr *hdr = nl->rbuffer; NLMSG_OK(hdr, len); hdr = NLMSG_NEXT(hdr, len)) { _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *m = NULL; r = parse_message_one(nl, group, hdr, &m); @@ -419,36 +413,44 @@ int socket_read_message(sd_netlink *nl) { if (r == 0) continue; - if (hdr->nlmsg_type == NLMSG_DONE) { - /* finished reading multi-part message */ + if (hdr->nlmsg_flags & NLM_F_MULTI) { + if (hdr->nlmsg_type == NLMSG_DONE) { + _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *existing = NULL; + + /* finished reading multi-part message */ + existing = hashmap_remove(nl->rqueue_partial_by_serial, UINT32_TO_PTR(hdr->nlmsg_seq)); + + /* if we receive only NLMSG_DONE, put it into the receive queue. */ + r = netlink_queue_received_message(nl, existing ?: m); + if (r < 0) + return r; + + done = true; + } else { + sd_netlink_message *existing; + + existing = hashmap_remove(nl->rqueue_partial_by_serial, UINT32_TO_PTR(hdr->nlmsg_seq)); + if (existing) + /* push the message onto the multi-part message stack */ + m->next = existing; + + /* put it into the queue for partially received messages. */ + r = netlink_queue_partially_received_message(nl, m); + if (r < 0) + return r; + } + + } else { + r = netlink_queue_received_message(nl, m); + if (r < 0) + return r; + done = true; - - /* if first is not defined, put NLMSG_DONE into the receive queue. */ - if (first) - continue; } - - /* push the message onto the multi-part message stack */ - if (first) - m->next = first; - first = TAKE_PTR(m); } if (len > 0) log_debug("sd-netlink: discarding trailing %zu bytes of incoming message", len); - if (!first) - return 0; - - done = done || !multi_part; - if (done) - /* we got a complete message, push it on the read queue */ - r = netlink_queue_received_message(nl, first); - else - /* we only got a partial multi-part message, push it on the partial read queue. */ - r = netlink_queue_partially_received_message(nl, first); - if (r < 0) - return r; - return done; } From 52ceba53d3ccd3b57dcb795612de7886aaf928ca Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 26 Nov 2022 09:46:40 +0900 Subject: [PATCH 14/14] sd-netlink: append instead of prepend multipart message Previously, e.g., networkd enumerated network interfaces with ifindex in a decreasing order, as sd-netlink inverses the order of the received multipart messages. Let's keep the order of the multipart messages. Hopefully this changes no behavior, as our code do not depend on the order of the received multipart messages. Before: === Nov 26 09:35:10 systemd[1]: Starting Network Configuration... Nov 26 09:35:11 systemd-networkd[36185]: wlp59s0: Saved new link: ifindex=3, iftype=ETHER(1), kind=n/a Nov 26 09:35:12 systemd-networkd[36185]: enp0s31f6: Saved new link: ifindex=2, iftype=ETHER(1), kind=n/a Nov 26 09:35:12 systemd-networkd[36185]: lo: Saved new link: ifindex=1, iftype=LOOPBACK(772), kind=n/a After: === Nov 26 09:45:18 systemd[1]: Starting Network Configuration... Nov 26 09:45:19 systemd-networkd[38372]: lo: Saved new link: ifindex=1, iftype=LOOPBACK(772), kind=n/a Nov 26 09:45:19 systemd-networkd[38372]: enp0s31f6: Saved new link: ifindex=2, iftype=ETHER(1), kind=n/a Nov 26 09:45:19 systemd-networkd[38372]: wlp59s0: Saved new link: ifindex=3, iftype=ETHER(1), kind=n/a --- src/libsystemd/sd-netlink/netlink-socket.c | 23 +++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/libsystemd/sd-netlink/netlink-socket.c b/src/libsystemd/sd-netlink/netlink-socket.c index 2a9f641419..96162963a7 100644 --- a/src/libsystemd/sd-netlink/netlink-socket.c +++ b/src/libsystemd/sd-netlink/netlink-socket.c @@ -429,15 +429,20 @@ int socket_read_message(sd_netlink *nl) { } else { sd_netlink_message *existing; - existing = hashmap_remove(nl->rqueue_partial_by_serial, UINT32_TO_PTR(hdr->nlmsg_seq)); - if (existing) - /* push the message onto the multi-part message stack */ - m->next = existing; - - /* put it into the queue for partially received messages. */ - r = netlink_queue_partially_received_message(nl, m); - if (r < 0) - return r; + existing = hashmap_get(nl->rqueue_partial_by_serial, UINT32_TO_PTR(hdr->nlmsg_seq)); + if (existing) { + /* This is the continuation of the previously read messages. + * Let's append this message at the end. */ + while (existing->next) + existing = existing->next; + existing->next = TAKE_PTR(m); + } else { + /* This is the first message. Put it into the queue for partially + * received messages. */ + r = netlink_queue_partially_received_message(nl, m); + if (r < 0) + return r; + } } } else {