From 714c939018e24f9ca838f1fe4d4d750de4413b1f Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 21 Jul 2023 09:34:22 +0200 Subject: [PATCH 1/3] kernel-netlink: Increase the default receive buffer size Also simplify how we try to exceed the system-wide maximum. We basically just try to force the value and simply fall back to the regular call. The kernel actually won't let the latter fail if the value is too big, it just caps it at the internal maximum. --- conf/plugins/kernel-netlink.opt | 20 ++++++--------- .../kernel_netlink/kernel_netlink_shared.c | 25 ++++++++++--------- 2 files changed, 20 insertions(+), 25 deletions(-) diff --git a/conf/plugins/kernel-netlink.opt b/conf/plugins/kernel-netlink.opt index 39fdf5b99..9310a3022 100644 --- a/conf/plugins/kernel-netlink.opt +++ b/conf/plugins/kernel-netlink.opt @@ -1,14 +1,6 @@ charon.plugins.kernel-netlink.buflen = Buffer size for received Netlink messages. -charon.plugins.kernel-netlink.force_receive_buffer_size = no - Force maximum Netlink receive buffer on Netlink socket. - - If the maximum Netlink socket receive buffer in bytes set by - _receive_buffer_size_ exceeds the system-wide maximum from - /proc/sys/net/core/rmem_max, this option can be used to override the limit. - Enabling this option requires special privileges (CAP_NET_ADMIN). - charon.plugins.kernel-netlink.fwmark = Firewall mark to set on the routing rule that directs traffic to our routing table. @@ -74,14 +66,16 @@ charon.plugins.kernel-netlink.process_rules = no currently only useful if the kernel based route lookup is used (i.e. if route installation is disabled or an inverted fwmark match is configured). -charon.plugins.kernel-netlink.receive_buffer_size = 0 +charon.plugins.kernel-netlink.receive_buffer_size = 8388608 Maximum Netlink socket receive buffer in bytes. Maximum Netlink socket receive buffer in bytes. This value controls how many - bytes of Netlink messages can be received on a Netlink socket. The default - value is set by /proc/sys/net/core/rmem_default. The specified value cannot - exceed the system-wide maximum from /proc/sys/net/core/rmem_max, unless - _force_receive_buffer_size_ is enabled. + bytes of Netlink messages can be queued to a Netlink socket. If set to 0, + the default from /proc/sys/net/core/rmem_default will apply. Note that the + kernel doubles the configured value to account for overhead. To exceed the + system-wide maximum from /proc/sys/net/core/rmem_max, special privileges + (CAP_NET_ADMIN) are necessary, otherwise, the kernel silently caps the + value. charon.plugins.kernel-netlink.roam_events = yes Whether to trigger roam events when interfaces, addresses or routes change. diff --git a/src/libcharon/plugins/kernel_netlink/kernel_netlink_shared.c b/src/libcharon/plugins/kernel_netlink/kernel_netlink_shared.c index cb0944640..eb20a8515 100644 --- a/src/libcharon/plugins/kernel_netlink/kernel_netlink_shared.c +++ b/src/libcharon/plugins/kernel_netlink/kernel_netlink_shared.c @@ -57,6 +57,13 @@ #define SOL_NETLINK 270 #endif +/** + * Default receive buffer size + */ +#ifndef NETLINK_RCVBUF_DEFAULT +#define NETLINK_RCVBUF_DEFAULT (8 * 1024 * 1024) +#endif + typedef struct private_netlink_socket_t private_netlink_socket_t; typedef struct private_netlink_event_socket_t private_netlink_event_socket_t; @@ -656,7 +663,6 @@ netlink_socket_t *netlink_socket_create(int protocol, enum_name_t *names, struct sockaddr_nl addr = { .nl_family = AF_NETLINK, }; - bool force_buf = FALSE; int on = 1, rcvbuf_size = 0; INIT(this, @@ -707,21 +713,16 @@ netlink_socket_t *netlink_socket_create(int protocol, enum_name_t *names, rcvbuf_size = lib->settings->get_int(lib->settings, "%s.plugins.kernel-netlink.receive_buffer_size", - rcvbuf_size, lib->ns); + NETLINK_RCVBUF_DEFAULT, lib->ns); if (rcvbuf_size) { - int optname; - - force_buf = lib->settings->get_bool(lib->settings, - "%s.plugins.kernel-netlink.force_receive_buffer_size", - force_buf, lib->ns); - optname = force_buf ? SO_RCVBUFFORCE : SO_RCVBUF; - - if (setsockopt(this->socket, SOL_SOCKET, optname, &rcvbuf_size, + if (setsockopt(this->socket, SOL_SOCKET, SO_RCVBUFFORCE, &rcvbuf_size, + sizeof(rcvbuf_size)) == -1 && + setsockopt(this->socket, SOL_SOCKET, SO_RCVBUF, &rcvbuf_size, sizeof(rcvbuf_size)) == -1) { - DBG1(DBG_KNL, "failed to %supdate receive buffer size to %d: %s", - force_buf ? "forcibly " : "", rcvbuf_size, strerror(errno)); + DBG1(DBG_KNL, "failed to set receive buffer size to %d: %s", + rcvbuf_size, strerror(errno)); } } if (this->parallel) From 5971fc36c9b497b8b33f197a57e1dcb229b86a47 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 21 Jul 2023 09:39:09 +0200 Subject: [PATCH 2/3] kernel-netlink: Also set the receive buffer size on event sockets This was weirdly overlooked and could cause issues e.g. on hosts with lots of route changes. --- .../kernel_netlink/kernel_netlink_shared.c | 43 ++++++++++++------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/src/libcharon/plugins/kernel_netlink/kernel_netlink_shared.c b/src/libcharon/plugins/kernel_netlink/kernel_netlink_shared.c index eb20a8515..72c13a697 100644 --- a/src/libcharon/plugins/kernel_netlink/kernel_netlink_shared.c +++ b/src/libcharon/plugins/kernel_netlink/kernel_netlink_shared.c @@ -653,6 +653,29 @@ u_int netlink_get_buflen() return buflen; } +/** + * Set the configured receive buffer size on the given socket. + */ +static void set_rcvbuf_size(int socket) +{ + int rcvbuf_size = 0; + + rcvbuf_size = lib->settings->get_int(lib->settings, + "%s.plugins.kernel-netlink.receive_buffer_size", + NETLINK_RCVBUF_DEFAULT, lib->ns); + if (rcvbuf_size) + { + if (setsockopt(socket, SOL_SOCKET, SO_RCVBUFFORCE, &rcvbuf_size, + sizeof(rcvbuf_size)) == -1 && + setsockopt(socket, SOL_SOCKET, SO_RCVBUF, &rcvbuf_size, + sizeof(rcvbuf_size)) == -1) + { + DBG1(DBG_KNL, "failed to set receive buffer size to %d: %s", + rcvbuf_size, strerror(errno)); + } + } +} + /* * Described in header */ @@ -663,7 +686,7 @@ netlink_socket_t *netlink_socket_create(int protocol, enum_name_t *names, struct sockaddr_nl addr = { .nl_family = AF_NETLINK, }; - int on = 1, rcvbuf_size = 0; + int on = 1; INIT(this, .public = { @@ -711,20 +734,8 @@ netlink_socket_t *netlink_socket_create(int protocol, enum_name_t *names, ignore_result(setsockopt(this->socket, SOL_NETLINK, NETLINK_EXT_ACK, &on, sizeof(on))); - rcvbuf_size = lib->settings->get_int(lib->settings, - "%s.plugins.kernel-netlink.receive_buffer_size", - NETLINK_RCVBUF_DEFAULT, lib->ns); - if (rcvbuf_size) - { - if (setsockopt(this->socket, SOL_SOCKET, SO_RCVBUFFORCE, &rcvbuf_size, - sizeof(rcvbuf_size)) == -1 && - setsockopt(this->socket, SOL_SOCKET, SO_RCVBUF, &rcvbuf_size, - sizeof(rcvbuf_size)) == -1) - { - DBG1(DBG_KNL, "failed to set receive buffer size to %d: %s", - rcvbuf_size, strerror(errno)); - } - } + set_rcvbuf_size(this->socket); + if (this->parallel) { lib->watcher->add(lib->watcher, this->socket, WATCHER_READ, watch, this); @@ -806,6 +817,8 @@ netlink_event_socket_t *netlink_event_socket_create(int protocol, uint32_t group return NULL; } + set_rcvbuf_size(this->socket); + if (bind(this->socket, (struct sockaddr*)&addr, sizeof(addr))) { DBG1(DBG_KNL, "unable to bind netlink event socket: %s (%d)", From ba9228ab00c4babb4272da9d97f1c4179fc1e85f Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 21 Jul 2023 10:01:41 +0200 Subject: [PATCH 3/3] watcher: Change handling of POLLERR and remove WATCHER_EXCEPT We can't actually explicitly listen for errors by passing POLLERR in `events` (the man page for poll() clearly states it's ignored). On the other hand, POLLERR can be returned for any FD and, even worse, it might be the only event indicated. The latter caused an infinite loop as we didn't notify the callback nor clear the error by calling `getsockopt(..., SOL_SOCKET, SO_ERROR, ...)`. And while the latter would be able to reset the state to break the loop, it seems to leave the FD in a defunct state where no further events will be returned by poll(). Notifying the callback works better (the error is then reported by e.g. recvfrom()) and automatically happened already if POLLERR was returned together with e.g. POLLIN. So we now treat POLLERR like the other error indicators we handle (POLLHUP and POLLINVAL) and just notify the callbacks. --- src/libstrongswan/networking/streams/stream.c | 2 - src/libstrongswan/processing/watcher.c | 79 ++++++------------- src/libstrongswan/processing/watcher.h | 1 - 3 files changed, 26 insertions(+), 56 deletions(-) diff --git a/src/libstrongswan/networking/streams/stream.c b/src/libstrongswan/networking/streams/stream.c index 8273af89e..f1eb5a097 100644 --- a/src/libstrongswan/networking/streams/stream.c +++ b/src/libstrongswan/networking/streams/stream.c @@ -188,8 +188,6 @@ static bool watch(private_stream_t *this, int fd, watcher_event_t event) this->write_cb = cb; } break; - case WATCHER_EXCEPT: - break; } return keep; } diff --git a/src/libstrongswan/processing/watcher.c b/src/libstrongswan/processing/watcher.c index e174858ef..1200d6709 100644 --- a/src/libstrongswan/processing/watcher.c +++ b/src/libstrongswan/processing/watcher.c @@ -251,20 +251,17 @@ static void notify_end(notify_data_t *data) if (removed) { - DBG3(DBG_JOB, "removed fd %d[%s%s%s] from watcher after callback", data->fd, + DBG3(DBG_JOB, "removed fd %d[%s%s] from watcher after callback", data->fd, data->event & WATCHER_READ ? "r" : "", - data->event & WATCHER_WRITE ? "w" : "", - data->event & WATCHER_EXCEPT ? "e" : ""); + data->event & WATCHER_WRITE ? "w" : ""); } else if (updated) { - DBG3(DBG_JOB, "updated fd %d[%s%s%s] to %d[%s%s%s] after callback", data->fd, + DBG3(DBG_JOB, "updated fd %d[%s%s] to %d[%s%s] after callback", data->fd, (updated | data->event) & WATCHER_READ ? "r" : "", - (updated | data->event) & WATCHER_WRITE ? "w" : "", - (updated | data->event) & WATCHER_EXCEPT ? "e" : "", data->fd, + (updated | data->event) & WATCHER_WRITE ? "w" : "", data->fd, updated & WATCHER_READ ? "r" : "", - updated & WATCHER_WRITE ? "w" : "", - updated & WATCHER_EXCEPT ? "e" : ""); + updated & WATCHER_WRITE ? "w" : ""); } free(data); } @@ -335,27 +332,6 @@ static inline int find_revents(struct pollfd *pfd, int count, int fd) return 0; } -/** - * Check if entry is waiting for a specific event, and if it got signaled - */ -static inline bool entry_ready(entry_t *entry, watcher_event_t event, - int revents) -{ - if (entry->events & event) - { - switch (event) - { - case WATCHER_READ: - return (revents & (POLLIN | POLLHUP | POLLNVAL)) != 0; - case WATCHER_WRITE: - return (revents & (POLLOUT | POLLHUP | POLLNVAL)) != 0; - case WATCHER_EXCEPT: - return (revents & (POLLERR | POLLHUP | POLLNVAL)) != 0; - } - } - return FALSE; -} - #if DEBUG_LEVEL >= 2 #define reset_log(buf, pos, len) ({ buf[0] = '\0'; pos = buf; len = sizeof(buf); }) #define reset_event_log(buf, pos) ({ pos = buf; }) @@ -431,11 +407,6 @@ static job_requeue_t watch(private_watcher_t *this) log_event(eventpos, 'w'); pfd[count].events |= POLLOUT; } - if (entry->events & WATCHER_EXCEPT) - { - log_event(eventpos, 'e'); - pfd[count].events |= POLLERR; - } end_event_log(eventpos); log_fd(logpos, loglen, entry->fd, eventbuf); count++; @@ -505,23 +476,27 @@ static job_requeue_t watch(private_watcher_t *this) } reset_event_log(eventbuf, eventpos); revents = find_revents(pfd, count, entry->fd); - if (entry_ready(entry, WATCHER_EXCEPT, revents)) + if (revents & POLLERR) { log_event(eventpos, 'e'); - notify(this, entry, WATCHER_EXCEPT); } - else + if (revents & POLLIN) { - if (entry_ready(entry, WATCHER_READ, revents)) - { - log_event(eventpos, 'r'); - notify(this, entry, WATCHER_READ); - } - if (entry_ready(entry, WATCHER_WRITE, revents)) - { - log_event(eventpos, 'w'); - notify(this, entry, WATCHER_WRITE); - } + log_event(eventpos, 'r'); + } + if (revents & POLLOUT) + { + log_event(eventpos, 'w'); + } + if (entry->events & WATCHER_READ && + revents & (POLLIN | POLLERR | POLLHUP | POLLNVAL)) + { + notify(this, entry, WATCHER_READ); + } + if (entry->events & WATCHER_WRITE && + revents & (POLLOUT | POLLERR | POLLHUP | POLLNVAL)) + { + notify(this, entry, WATCHER_WRITE); } end_event_log(eventpos); log_fd(logpos, loglen, entry->fd, eventbuf); @@ -571,10 +546,9 @@ METHOD(watcher_t, add, void, .data = data, ); - DBG3(DBG_JOB, "adding fd %d[%s%s%s] to watcher", fd, + DBG3(DBG_JOB, "adding fd %d[%s%s] to watcher", fd, events & WATCHER_READ ? "r" : "", - events & WATCHER_WRITE ? "w" : "", - events & WATCHER_EXCEPT ? "e" : ""); + events & WATCHER_WRITE ? "w" : ""); this->mutex->lock(this->mutex); add_entry(this, entry); @@ -631,10 +605,9 @@ METHOD(watcher_t, remove_, void, { update_and_unlock(this); - DBG3(DBG_JOB, "removed fd %d[%s%s%s] from watcher", fd, + DBG3(DBG_JOB, "removed fd %d[%s%s] from watcher", fd, found & WATCHER_READ ? "r" : "", - found & WATCHER_WRITE ? "w" : "", - found & WATCHER_EXCEPT ? "e" : ""); + found & WATCHER_WRITE ? "w" : ""); } else { diff --git a/src/libstrongswan/processing/watcher.h b/src/libstrongswan/processing/watcher.h index 8a5fababe..e0db20fdc 100644 --- a/src/libstrongswan/processing/watcher.h +++ b/src/libstrongswan/processing/watcher.h @@ -55,7 +55,6 @@ typedef bool (*watcher_cb_t)(void *data, int fd, watcher_event_t event); enum watcher_event_t { WATCHER_READ = (1<<0), WATCHER_WRITE = (1<<1), - WATCHER_EXCEPT = (1<<2), }; /**