From 4edc2c9b6b5b921873eb82e58719ed4d9e0d69bf Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 15 Feb 2016 22:50:01 +0100 Subject: networkd: FIONREAD is not reliable on some sockets Fixes: #2457 --- src/basic/socket-util.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) (limited to 'src/basic/socket-util.c') diff --git a/src/basic/socket-util.c b/src/basic/socket-util.c index 49e5f5b125..58512686e3 100644 --- a/src/basic/socket-util.c +++ b/src/basic/socket-util.c @@ -936,3 +936,37 @@ int receive_one_fd(int transport_fd, int flags) { return *(int*) CMSG_DATA(found); } + +ssize_t next_datagram_size_fd(int fd) { + ssize_t l; + int k; + + /* This is a bit like FIONREAD/SIOCINQ, however a bit more powerful. The difference being: recv(MSG_PEEK) will + * actually cause the next datagram in the queue to be validated regarding checksums, which FIONREAD dosn't + * do. This difference is actually of major importance as we need to be sure that the size returned here + * actually matches what we will read with recvmsg() next, as otherwise we might end up allocating a buffer of + * the wrong size. */ + + l = recv(fd, NULL, 0, MSG_PEEK|MSG_TRUNC); + if (l < 0) { + if (errno == EOPNOTSUPP) + goto fallback; + + return -errno; + } + if (l == 0) + goto fallback; + + return l; + +fallback: + k = 0; + + /* Some sockets (AF_PACKET) do not support null-sized recv() with MSG_TRUNC set, let's fall back to FIONREAD + * for them. Checksums don't matter for raw sockets anyway, hence this should be fine. */ + + if (ioctl(fd, FIONREAD, &k) < 0) + return -errno; + + return (ssize_t) k; +} -- cgit v1.2.3-54-g00ecf From 96d490114900686b4d17f9b751fab6e39cfcc560 Mon Sep 17 00:00:00 2001 From: Torstein Husebø Date: Mon, 8 Feb 2016 13:27:22 +0100 Subject: treewide: fix typos and then/that use --- CODING_STYLE | 2 +- NEWS | 2 +- man/systemd.netdev.xml | 2 +- src/basic/socket-util.c | 2 +- src/libsystemd/sd-daemon/sd-daemon.c | 2 +- src/resolve/resolved-dns-answer.c | 2 +- src/resolve/resolved-dns-rr.h | 2 +- src/shared/condition.c | 2 +- src/shared/gcrypt-util.c | 2 +- 9 files changed, 9 insertions(+), 9 deletions(-) (limited to 'src/basic/socket-util.c') diff --git a/CODING_STYLE b/CODING_STYLE index 46e366898e..e5ba396368 100644 --- a/CODING_STYLE +++ b/CODING_STYLE @@ -163,7 +163,7 @@ programming error with assert_return() and return a sensible return code. In all other calls, it is recommended to check for programming errors with a more brutal assert(). We are more forgiving to public - users then for ourselves! Note that assert() and assert_return() + users than for ourselves! Note that assert() and assert_return() really only should be used for detecting programming errors, not for runtime errors. assert() and assert_return() by usage of _likely_() inform the compiler that he should not expect these checks to fail, diff --git a/NEWS b/NEWS index 0cce79443b..8b30bee6b7 100644 --- a/NEWS +++ b/NEWS @@ -11,7 +11,7 @@ CHANGES WITH 230 in spe: interested in collecting feedback about the DNSSEC validator and its limitations in the wild. Note however, that DNSSEC support is probably nothing downstreams should turn on in stable distros just - yet, as it might create incompabilities with a few DNS servers and + yet, as it might create incompatibilities with a few DNS servers and networks. We tried hard to make sure we downgrade to non-DNSSEC mode automatically whenever we detect such incompatible setups, but there might be systems we do not cover yet. Hence: please help us testing diff --git a/man/systemd.netdev.xml b/man/systemd.netdev.xml index b697d0c9a6..c5fb2fa7fb 100644 --- a/man/systemd.netdev.xml +++ b/man/systemd.netdev.xml @@ -361,7 +361,7 @@ The [MACVTAP] section applies for netdevs of kind macvtap and accepts the - same key as [MACVLAN]. + same key as [MACVLAN]. diff --git a/src/basic/socket-util.c b/src/basic/socket-util.c index 58512686e3..0f38f9a0f3 100644 --- a/src/basic/socket-util.c +++ b/src/basic/socket-util.c @@ -942,7 +942,7 @@ ssize_t next_datagram_size_fd(int fd) { int k; /* This is a bit like FIONREAD/SIOCINQ, however a bit more powerful. The difference being: recv(MSG_PEEK) will - * actually cause the next datagram in the queue to be validated regarding checksums, which FIONREAD dosn't + * actually cause the next datagram in the queue to be validated regarding checksums, which FIONREAD doesn't * do. This difference is actually of major importance as we need to be sure that the size returned here * actually matches what we will read with recvmsg() next, as otherwise we might end up allocating a buffer of * the wrong size. */ diff --git a/src/libsystemd/sd-daemon/sd-daemon.c b/src/libsystemd/sd-daemon/sd-daemon.c index 4e50b61979..bd1c7f15ff 100644 --- a/src/libsystemd/sd-daemon/sd-daemon.c +++ b/src/libsystemd/sd-daemon/sd-daemon.c @@ -465,7 +465,7 @@ _public_ int sd_pid_notify_with_fds(pid_t pid, int unset_environment, const char have_pid = pid != 0 && pid != getpid(); if (n_fds > 0 || have_pid) { - /* CMSG_SPACE(0) may return value different then zero, which results in miscalculated controllen. */ + /* CMSG_SPACE(0) may return value different than zero, which results in miscalculated controllen. */ msghdr.msg_controllen = (n_fds > 0 ? CMSG_SPACE(sizeof(int) * n_fds) : 0) + (have_pid ? CMSG_SPACE(sizeof(struct ucred)) : 0); diff --git a/src/resolve/resolved-dns-answer.c b/src/resolve/resolved-dns-answer.c index 5d7b4b4b5c..0dadf8b1dd 100644 --- a/src/resolve/resolved-dns-answer.c +++ b/src/resolve/resolved-dns-answer.c @@ -757,7 +757,7 @@ int dns_answer_reserve_or_clone(DnsAnswer **a, unsigned n_free) { assert(a); /* Tries to extend the DnsAnswer object. And if that's not - * possibly, since we are not the sole owner, then allocate a + * possible, since we are not the sole owner, then allocate a * new, appropriately sized one. Either way, after this call * the object will only have a single reference, and has room * for at least the specified number of RRs. */ diff --git a/src/resolve/resolved-dns-rr.h b/src/resolve/resolved-dns-rr.h index 646e34598d..020a2abd77 100644 --- a/src/resolve/resolved-dns-rr.h +++ b/src/resolve/resolved-dns-rr.h @@ -82,7 +82,7 @@ enum { struct DnsResourceKey { unsigned n_ref; /* (unsigned -1) for const keys, see below */ uint16_t class, type; - char *_name; /* don't access directy, use dns_resource_key_name()! */ + char *_name; /* don't access directly, use dns_resource_key_name()! */ }; /* Creates a temporary resource key. This is only useful to quickly diff --git a/src/shared/condition.c b/src/shared/condition.c index f93785865e..3a45ed265c 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -295,7 +295,7 @@ static int condition_test_needs_update(Condition *c) { return false; /* Any other failure means we should allow the condition to be true, - * so that we rather invoke too many update tools then too + * so that we rather invoke too many update tools than too * few. */ if (!path_is_absolute(c->parameter)) diff --git a/src/shared/gcrypt-util.c b/src/shared/gcrypt-util.c index b887243849..4ff94520c3 100644 --- a/src/shared/gcrypt-util.c +++ b/src/shared/gcrypt-util.c @@ -32,7 +32,7 @@ void initialize_libgcrypt(bool secmem) { p = gcry_check_version("1.4.5"); assert(p); - /* Turn off "secmem". Clients which whish to make use of this + /* Turn off "secmem". Clients which wish to make use of this * feature should initialize the library manually */ if (!secmem) gcry_control(GCRYCTL_DISABLE_SECMEM); -- cgit v1.2.3-54-g00ecf From 60d9771c593e0702a892a4372443e63b38cdbcba Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 6 May 2016 13:29:26 +0200 Subject: core: rework how we flush incoming traffic when a socket unit goes down Previously, we'd simply close and reopen the socket file descriptors. This is problematic however, as we won't transition through the SOCKET_CHOWN state then, and thus the file ownership won't be correct for the sockets. Rework the flushing logic, and actually read any queued data from the sockets for flushing, and accept any queued messages and disconnect them. --- src/basic/io-util.c | 5 +++++ src/basic/socket-util.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/basic/socket-util.h | 2 ++ src/core/socket.c | 39 +++++++++++++++++++-------------------- 4 files changed, 66 insertions(+), 20 deletions(-) (limited to 'src/basic/socket-util.c') diff --git a/src/basic/io-util.c b/src/basic/io-util.c index 0037a37f2a..cc6dfa8c1b 100644 --- a/src/basic/io-util.c +++ b/src/basic/io-util.c @@ -33,6 +33,11 @@ int flush_fd(int fd) { .events = POLLIN, }; + /* Read from the specified file descriptor, until POLLIN is not set anymore, throwing away everything + * read. Note that some file descriptors (notable IP sockets) will trigger POLLIN even when no data can be read + * (due to IP packet checksum mismatches), hence this function is only safe to be non-blocking if the fd used + * was set to non-blocking too. */ + for (;;) { char buf[LINE_MAX]; ssize_t l; diff --git a/src/basic/socket-util.c b/src/basic/socket-util.c index 0f38f9a0f3..c634f1d564 100644 --- a/src/basic/socket-util.c +++ b/src/basic/socket-util.c @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -970,3 +971,42 @@ fallback: return (ssize_t) k; } + +int flush_accept(int fd) { + + struct pollfd pollfd = { + .fd = fd, + .events = POLLIN, + }; + int r; + + + /* Similar to flush_fd() but flushes all incoming connection by accepting them and immediately closing them. */ + + for (;;) { + int cfd; + + r = poll(&pollfd, 1, 0); + if (r < 0) { + if (errno == EINTR) + continue; + + return -errno; + + } else if (r == 0) + return 0; + + cfd = accept4(fd, NULL, NULL, SOCK_NONBLOCK|SOCK_CLOEXEC); + if (cfd < 0) { + if (errno == EINTR) + continue; + + if (errno == EAGAIN) + return 0; + + return -errno; + } + + close(cfd); + } +} diff --git a/src/basic/socket-util.h b/src/basic/socket-util.h index d17a2f35f8..9f6a301368 100644 --- a/src/basic/socket-util.h +++ b/src/basic/socket-util.h @@ -135,5 +135,7 @@ int receive_one_fd(int transport_fd, int flags); ssize_t next_datagram_size_fd(int fd); +int flush_accept(int fd); + #define CMSG_FOREACH(cmsg, mh) \ for ((cmsg) = CMSG_FIRSTHDR(mh); (cmsg); (cmsg) = CMSG_NXTHDR((mh), (cmsg))) diff --git a/src/core/socket.c b/src/core/socket.c index 50912c33dd..091567ed4f 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -28,7 +28,6 @@ #include #include -#include "sd-event.h" #include "alloc-util.h" #include "bus-error.h" #include "bus-util.h" @@ -38,6 +37,7 @@ #include "exit-status.h" #include "fd-util.h" #include "formats-util.h" +#include "io-util.h" #include "label.h" #include "log.h" #include "missing.h" @@ -1960,6 +1960,21 @@ fail: socket_enter_dead(s, SOCKET_FAILURE_RESOURCES); } +static void flush_ports(Socket *s) { + SocketPort *p; + + /* Flush all incoming traffic, regardless if actual bytes or new connections, so that this socket isn't busy + * anymore */ + + LIST_FOREACH(port, p, s->ports) { + if (p->fd < 0) + continue; + + (void) flush_accept(p->fd); + (void) flush_fd(p->fd); + } +} + static void socket_enter_running(Socket *s, int cfd) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; int r; @@ -1969,31 +1984,15 @@ static void socket_enter_running(Socket *s, int cfd) { assert(s); - /* We don't take connections anymore if we are supposed to - * shut down anyway */ + /* We don't take connections anymore if we are supposed to shut down anyway */ if (unit_stop_pending(UNIT(s))) { log_unit_debug(UNIT(s), "Suppressing connection request since unit stop is scheduled."); if (cfd >= 0) cfd = safe_close(cfd); - else { - /* Flush all sockets by closing and reopening them */ - socket_close_fds(s); - - r = socket_open_fds(s); - if (r < 0) { - log_unit_warning_errno(UNIT(s), r, "Failed to listen on sockets: %m"); - socket_enter_stop_pre(s, SOCKET_FAILURE_RESOURCES); - return; - } - - r = socket_watch_fds(s); - if (r < 0) { - log_unit_warning_errno(UNIT(s), r, "Failed to watch sockets: %m"); - socket_enter_stop_pre(s, SOCKET_FAILURE_RESOURCES); - } - } + else + flush_ports(s); return; } -- cgit v1.2.3-54-g00ecf From ef76dff225a00008fe0edd1f528c9096f1a91179 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 6 May 2016 20:58:32 +0200 Subject: util-lib: add new ifname_valid() call that validates interface names Make use of this in nspawn at a couple of places. A later commit should port more code over to this, including networkd. --- src/basic/socket-util.c | 38 ++++++++++++++++++++++++++++++++++++++ src/basic/socket-util.h | 2 ++ src/nspawn/nspawn-network.c | 6 ++++-- src/nspawn/nspawn.c | 24 ++++++++++++++++++++++++ src/test/test-socket-util.c | 25 +++++++++++++++++++++++++ 5 files changed, 93 insertions(+), 2 deletions(-) (limited to 'src/basic/socket-util.c') diff --git a/src/basic/socket-util.c b/src/basic/socket-util.c index c634f1d564..c8769a54f4 100644 --- a/src/basic/socket-util.c +++ b/src/basic/socket-util.c @@ -43,7 +43,9 @@ #include "socket-util.h" #include "string-table.h" #include "string-util.h" +#include "strv.h" #include "user-util.h" +#include "utf8.h" #include "util.h" int socket_address_parse(SocketAddress *a, const char *s) { @@ -795,6 +797,42 @@ static const char* const ip_tos_table[] = { DEFINE_STRING_TABLE_LOOKUP_WITH_FALLBACK(ip_tos, int, 0xff); +bool ifname_valid(const char *p) { + bool numeric = true; + + /* Checks whether a network interface name is valid. This is inspired by dev_valid_name() in the kernel sources + * but slightly stricter, as we only allow non-control, non-space ASCII characters in the interface name. We + * also don't permit names that only container numbers, to avoid confusion with numeric interface indexes. */ + + if (isempty(p)) + return false; + + if (strlen(p) >= IFNAMSIZ) + return false; + + if (STR_IN_SET(p, ".", "..")) + return false; + + while (*p) { + if ((unsigned char) *p >= 127U) + return false; + + if ((unsigned char) *p <= 32U) + return false; + + if (*p == ':' || *p == '/') + return false; + + numeric = numeric && (*p >= '0' && *p <= '9'); + p++; + } + + if (numeric) + return false; + + return true; +} + int getpeercred(int fd, struct ucred *ucred) { socklen_t n = sizeof(struct ucred); struct ucred u; diff --git a/src/basic/socket-util.h b/src/basic/socket-util.h index 160f7c484b..e9230e4a9f 100644 --- a/src/basic/socket-util.h +++ b/src/basic/socket-util.h @@ -123,6 +123,8 @@ int fd_inc_rcvbuf(int fd, size_t n); int ip_tos_to_string_alloc(int i, char **s); int ip_tos_from_string(const char *s); +bool ifname_valid(const char *p); + int getpeercred(int fd, struct ucred *ucred); int getpeersec(int fd, char **ret); diff --git a/src/nspawn/nspawn-network.c b/src/nspawn/nspawn-network.c index f2b7e4dd79..9938fce503 100644 --- a/src/nspawn/nspawn-network.c +++ b/src/nspawn/nspawn-network.c @@ -29,6 +29,8 @@ #include "netlink-util.h" #include "nspawn-network.h" #include "siphash24.h" +#include "socket-util.h" +#include "stat-util.h" #include "string-util.h" #include "udev-util.h" #include "util.h" @@ -515,13 +517,13 @@ int veth_extra_parse(char ***l, const char *p) { r = extract_first_word(&p, &a, ":", EXTRACT_DONT_COALESCE_SEPARATORS); if (r < 0) return r; - if (r == 0 || isempty(a)) + if (r == 0 || !ifname_valid(a)) return -EINVAL; r = extract_first_word(&p, &b, ":", EXTRACT_DONT_COALESCE_SEPARATORS); if (r < 0) return r; - if (r == 0 || isempty(b)) { + if (r == 0 || !ifname_valid(b)) { free(b); b = strdup(a); if (!b) diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 3fc6cc955c..643f459851 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -467,6 +467,12 @@ static int parse_argv(int argc, char *argv[]) { break; case ARG_NETWORK_BRIDGE: + + if (!ifname_valid(optarg)) { + log_error("Bridge interface name not valid: %s", optarg); + return -EINVAL; + } + r = free_and_strdup(&arg_network_bridge, optarg); if (r < 0) return log_oom(); @@ -489,6 +495,12 @@ static int parse_argv(int argc, char *argv[]) { break; case ARG_NETWORK_INTERFACE: + + if (!ifname_valid(optarg)) { + log_error("Network interface name not valid: %s", optarg); + return -EINVAL; + } + if (strv_extend(&arg_network_interfaces, optarg) < 0) return log_oom(); @@ -497,6 +509,12 @@ static int parse_argv(int argc, char *argv[]) { break; case ARG_NETWORK_MACVLAN: + + if (!ifname_valid(optarg)) { + log_error("MACVLAN network interface name not valid: %s", optarg); + return -EINVAL; + } + if (strv_extend(&arg_network_macvlan, optarg) < 0) return log_oom(); @@ -505,6 +523,12 @@ static int parse_argv(int argc, char *argv[]) { break; case ARG_NETWORK_IPVLAN: + + if (!ifname_valid(optarg)) { + log_error("IPVLAN network interface name not valid: %s", optarg); + return -EINVAL; + } + if (strv_extend(&arg_network_ipvlan, optarg) < 0) return log_oom(); diff --git a/src/test/test-socket-util.c b/src/test/test-socket-util.c index 9e01f3afd4..b480fdaa9c 100644 --- a/src/test/test-socket-util.c +++ b/src/test/test-socket-util.c @@ -27,6 +27,29 @@ #include "string-util.h" #include "util.h" +static void test_ifname_valid(void) { + assert(ifname_valid("foo")); + assert(ifname_valid("eth0")); + + assert(!ifname_valid("0")); + assert(!ifname_valid("99")); + assert(ifname_valid("a99")); + assert(ifname_valid("99a")); + + assert(!ifname_valid(NULL)); + assert(!ifname_valid("")); + assert(!ifname_valid(" ")); + assert(!ifname_valid(" foo")); + assert(!ifname_valid("bar\n")); + assert(!ifname_valid(".")); + assert(!ifname_valid("..")); + assert(ifname_valid("foo.bar")); + assert(!ifname_valid("x:y")); + + assert(ifname_valid("xxxxxxxxxxxxxxx")); + assert(!ifname_valid("xxxxxxxxxxxxxxxx")); +} + static void test_socket_address_parse(void) { SocketAddress a; @@ -362,6 +385,8 @@ int main(int argc, char *argv[]) { log_set_max_level(LOG_DEBUG); + test_ifname_valid(); + test_socket_address_parse(); test_socket_address_parse_netlink(); test_socket_address_equal(); -- cgit v1.2.3-54-g00ecf