From 340f05ba42b4e4ad89250f4ab947d6b944b3eca1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 22 Jan 2022 16:50:37 +0100 Subject: [PATCH 1/4] platform: use proper g_free() function for netlink receive buffer In the past, nl_recv() was libnl3 code and thus used malloc()/realloc() to allocate the buffer. That meant, we should free the buffer with libc's free() function instead of glib's g_free(). That is what nm_auto_free is for. Nowadays, nl_recv() is forked and glib-ified, and uses the glib wrappers to allocate the buffer. Thus the buffer should also be freed with g_free() (and gs_free). In practice there is no difference, because glib's allocation directly uses libc's malloc/free. This is purely a matter of style. --- src/libnm-platform/nm-linux-platform.c | 27 +++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index ed67afe665..c596d3ef8a 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -9021,21 +9021,22 @@ event_handler(int fd, GIOCondition io_condition, gpointer user_data) static int event_handler_recvmsgs(NMPlatform *platform, gboolean handle_events) { - NMLinuxPlatformPrivate *priv = NM_LINUX_PLATFORM_GET_PRIVATE(platform); - struct nl_sock *sk = priv->nlh; - int n; - int err = 0; - gboolean multipart = 0; - gboolean interrupted = FALSE; - struct nlmsghdr *hdr; - WaitForNlResponseResult seq_result; - struct sockaddr_nl nla = {0}; - struct ucred creds; - gboolean creds_has; - nm_auto_free unsigned char *buf = NULL; + NMLinuxPlatformPrivate *priv = NM_LINUX_PLATFORM_GET_PRIVATE(platform); + struct nl_sock *sk = priv->nlh; + int n; + int err = 0; + gboolean multipart = 0; + gboolean interrupted = FALSE; + struct nlmsghdr *hdr; + WaitForNlResponseResult seq_result; + struct sockaddr_nl nla = {0}; + struct ucred creds; + gboolean creds_has; + gs_free unsigned char *buf = NULL; continue_reading: - nm_clear_pointer(&buf, free); + nm_clear_g_free(&buf); + n = nl_recv(sk, &nla, &buf, &creds, &creds_has); if (n <= 0) { From e8cfa229290ff5c80ceea05514f5632dd4a1ec4e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 22 Jan 2022 16:43:49 +0100 Subject: [PATCH 2/4] platform/netlink: accept pre-allocated receive buffer for nl_recv() Add parameter to accept a pre-allocated buffer for nl_recv(). In practice, rtnetlink messages are not larger than 32k, so we can always pre-allocate it and avoid the need to heap allocate it. --- src/libnm-platform/nm-linux-platform.c | 2 +- src/libnm-platform/nm-netlink.c | 45 +++++++++++++++++++++++--- src/libnm-platform/nm-netlink.h | 2 ++ 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index c596d3ef8a..8a4beb7604 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -9037,7 +9037,7 @@ event_handler_recvmsgs(NMPlatform *platform, gboolean handle_events) continue_reading: nm_clear_g_free(&buf); - n = nl_recv(sk, &nla, &buf, &creds, &creds_has); + n = nl_recv(sk, NULL, 0, &nla, &buf, &creds, &creds_has); if (n <= 0) { if (n == -NME_NL_MSG_TRUNC) { diff --git a/src/libnm-platform/nm-netlink.c b/src/libnm-platform/nm-netlink.c index 949514536f..98f75f604a 100644 --- a/src/libnm-platform/nm-netlink.c +++ b/src/libnm-platform/nm-netlink.c @@ -1176,7 +1176,7 @@ nl_recvmsgs(struct nl_sock *sk, const struct nl_cb *cb) gboolean creds_has; continue_reading: - n = nl_recv(sk, &nla, &buf, &creds, &creds_has); + n = nl_recv(sk, NULL, 0, &nla, &buf, &creds, &creds_has); if (n <= 0) return n; @@ -1401,8 +1401,37 @@ nl_send_auto(struct nl_sock *sk, struct nl_msg *msg) return nl_send(sk, msg); } +/** + * nl_recv(): + * @sk: the netlink socket + * @buf0: NULL or a receive buffer of length @buf0_len + * @buf0_len: the length of the optional receive buffer. + * @nla: (out): the source address on success. + * @buf: (out): pointer to the result buffer on success. This is + * either @buf0 or an allocated buffer that gets returned. + * @out_creds: (out) (allow-none): optional out buffer for the credentials + * on success. + * @out_creds_has: (out) (allow-none): result indicating whether + * @out_creds was filled. + * + * If @buf0_len is zero, the function will g_malloc() a new receive buffer of size + * nl_socket_get_msg_buf_size(). If @buf0_len is larger than zero, then @buf0 + * is used as receive buffer. That is also the buffer returned by @buf. + * + * If NL_MSG_PEEK is not enabled and the receive buffer is too small, then + * the message was lost and -NME_NL_MSG_TRUNC gets returned. + * If NL_MSG_PEEK is enabled, then we first peek. If the buffer is too small, + * we g_malloc() a new buffer. In any case, we proceed to receive the buffer. + * NL_MSG_PEEK is great because it means no messages are lost. But it's bad, + * because we always need two syscalls on every receive. + * + * Returns: a negative error code or the length of the received message in + * @buf. + */ int nl_recv(struct nl_sock *sk, + unsigned char *buf0, + size_t buf0_len, struct sockaddr_nl *nla, unsigned char **buf, struct ucred *out_creds, @@ -1443,8 +1472,13 @@ nl_recv(struct nl_sock *sk, || (!(sk->s_flags & NL_MSG_PEEK_EXPLICIT) && sk->s_bufsize == 0)) flags |= MSG_PEEK | MSG_TRUNC; - iov.iov_len = sk->s_bufsize ?: (((size_t) nm_utils_getpagesize()) * 4u); - iov.iov_base = g_malloc(iov.iov_len); + if (buf0_len > 0) { + iov.iov_len = buf0_len; + iov.iov_base = buf0; + } else { + iov.iov_len = sk->s_bufsize ?: (((size_t) nm_utils_getpagesize()) * 4u); + iov.iov_base = g_malloc(iov.iov_len); + } if (out_creds && (sk->s_flags & NL_SOCK_PASSCRED)) { msg.msg_controllen = sizeof(msg_contol_buf); @@ -1482,7 +1516,7 @@ retry: /* Provided buffer is not long enough, enlarge it * to size of n (which should be total length of the message) * and try again. */ - iov.iov_base = g_realloc(iov.iov_base, n); + iov.iov_base = g_realloc(iov.iov_base != buf0 ? iov.iov_base : NULL, n); iov.iov_len = n; flags = 0; goto retry; @@ -1517,7 +1551,8 @@ retry: abort: if (retval <= 0) { - g_free(iov.iov_base); + if (iov.iov_base != buf0) + g_free(iov.iov_base); return retval; } diff --git a/src/libnm-platform/nm-netlink.h b/src/libnm-platform/nm-netlink.h index ac3159bf5f..bb1e41eed5 100644 --- a/src/libnm-platform/nm-netlink.h +++ b/src/libnm-platform/nm-netlink.h @@ -518,6 +518,8 @@ int nl_socket_add_memberships(struct nl_sock *sk, int group, ...); int nl_connect(struct nl_sock *sk, int protocol); int nl_recv(struct nl_sock *sk, + unsigned char *buf0, + size_t buf0_len, struct sockaddr_nl *nla, unsigned char **buf, struct ucred *out_creds, From dd3dffb1b09182b0980cce4b950c28c04448acc1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 22 Jan 2022 16:43:49 +0100 Subject: [PATCH 3/4] platform: use pre-allocated receive buffer for rtnetlink socket Instead of allocating a receive buffer for each nl_recv() call, re-use a pre-allocated buffer. The buffer is part of NMPlatform and kept around. As we would not have more than one NMPlatform instance per netns, we waste a limited amount of memory. The buffer gets initialized with 32k, which should be large enough for any rtnetlink message that we might receive. As before, if the buffer would be too small, then the first large message would be lost (as we don't peek). But then we would allocate a larger buffer, resync the platform cache and recover. --- src/libnm-platform/nm-linux-platform.c | 56 +++++++++++++++++--------- 1 file changed, 38 insertions(+), 18 deletions(-) diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index 8a4beb7604..44e662670a 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -451,6 +451,17 @@ typedef struct { int is_handling; } delayed_action; + /* This is the receive buffer for netlink messages. This buffer should be large + * enough for any rtnetlink message. When too small, nl_recv() would notice the + * truncation and lose the message. In that case, we reallocate a larger buffer. + * + * We keep the receive buffer around for the entire lifetime of the platform instance. + * Usually we only have one platform instance per netns, so we don't waste too much. */ + struct { + unsigned char *buf; + gsize len; + } netlink_recv_buf; + } NMLinuxPlatformPrivate; struct _NMLinuxPlatform { @@ -9032,29 +9043,33 @@ event_handler_recvmsgs(NMPlatform *platform, gboolean handle_events) struct sockaddr_nl nla = {0}; struct ucred creds; gboolean creds_has; - gs_free unsigned char *buf = NULL; + unsigned char *buf; continue_reading: - nm_clear_g_free(&buf); + buf = NULL; - n = nl_recv(sk, NULL, 0, &nla, &buf, &creds, &creds_has); + n = nl_recv(sk, + priv->netlink_recv_buf.buf, + priv->netlink_recv_buf.len, + &nla, + &buf, + &creds, + &creds_has); + + nm_assert((n <= 0 && !buf) + || (n > 0 && n <= priv->netlink_recv_buf.len && buf == priv->netlink_recv_buf.buf)); if (n <= 0) { if (n == -NME_NL_MSG_TRUNC) { - int buf_size; - /* the message receive buffer was too small. We lost one message, which * is unfortunate. Try to double the buffer size for the next time. */ - buf_size = nl_socket_get_msg_buf_size(sk); - if (buf_size < 512 * 1024) { - buf_size *= 2; - _LOGT("netlink: recvmsg: increase message buffer size for recvmsg() to %d bytes", - buf_size); - if (nl_socket_set_msg_buf_size(sk, buf_size) < 0) - nm_assert_not_reached(); - if (!handle_events) - goto continue_reading; - } + priv->netlink_recv_buf.len *= 2; + priv->netlink_recv_buf.buf = + g_realloc(priv->netlink_recv_buf.buf, priv->netlink_recv_buf.len); + _LOGT("netlink: recvmsg: increase message buffer size for recvmsg() to %zu bytes", + priv->netlink_recv_buf.len); + if (!handle_events) + goto continue_reading; } return n; @@ -9487,6 +9502,9 @@ nm_linux_platform_init(NMLinuxPlatform *self) { NMLinuxPlatformPrivate *priv = NM_LINUX_PLATFORM_GET_PRIVATE(self); + priv->netlink_recv_buf.len = 32 * 1024; + priv->netlink_recv_buf.buf = g_malloc(priv->netlink_recv_buf.len); + c_list_init(&priv->sysctl_clear_cache_lst); c_list_init(&priv->sysctl_list); @@ -9555,10 +9573,10 @@ constructed(GObject *_object) _LOGD("could not enable extended acks on netlink socket"); /* explicitly set the msg buffer size and disable MSG_PEEK. - * If we later encounter NME_NL_MSG_TRUNC, we will adjust the buffer size. */ + * We use our own receive buffer priv->netlink_recv_buf. + * If we encounter NME_NL_MSG_TRUNC, we will increase the buffer + * and resync (as we would have lost the message without NL_MSG_PEEK). */ nl_socket_disable_msg_peek(priv->nlh); - nle = nl_socket_set_msg_buf_size(priv->nlh, 32 * 1024); - g_assert(!nle); nle = nl_socket_add_memberships(priv->nlh, RTNLGRP_IPV4_IFADDR, @@ -9726,6 +9744,8 @@ finalize(GObject *object) priv->udev_client = nm_udev_client_destroy(priv->udev_client); G_OBJECT_CLASS(nm_linux_platform_parent_class)->finalize(object); + + g_free(priv->netlink_recv_buf.buf); } static void From d73594682f7b9eef526bf19a3dba58f98686bcdd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 Jan 2022 14:19:00 +0100 Subject: [PATCH 4/4] platform: no need to initialize nla sockaddr parameter to nl_recv() We always call nl_recv() in a loop. If it would be necessary to clear the variable, then it would need to happen inside the loop. But it's not necessary. --- src/libnm-platform/nm-linux-platform.c | 2 +- src/libnm-platform/nm-netlink.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index 44e662670a..83f2207d37 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -9040,7 +9040,7 @@ event_handler_recvmsgs(NMPlatform *platform, gboolean handle_events) gboolean interrupted = FALSE; struct nlmsghdr *hdr; WaitForNlResponseResult seq_result; - struct sockaddr_nl nla = {0}; + struct sockaddr_nl nla; struct ucred creds; gboolean creds_has; unsigned char *buf; diff --git a/src/libnm-platform/nm-netlink.c b/src/libnm-platform/nm-netlink.c index 98f75f604a..697ae5919d 100644 --- a/src/libnm-platform/nm-netlink.c +++ b/src/libnm-platform/nm-netlink.c @@ -1171,7 +1171,7 @@ nl_recvmsgs(struct nl_sock *sk, const struct nl_cb *cb) int n, nmerr = 0, multipart = 0, interrupted = 0, nrecv = 0; gs_free unsigned char *buf = NULL; struct nlmsghdr *hdr; - struct sockaddr_nl nla = {0}; + struct sockaddr_nl nla; struct ucred creds; gboolean creds_has;