- dev - passt

[PATCH v2 0/6] Fixes for early logging/prints and related cleanups
by Stefano Brivio 20 Jun '24

20 Jun '24

The most apparent issue fixed by this series is the one from 3/6: with a log file configured, we wouldn't print to standard error anymore, during initialisation, which means that users such as libvirt lost the ability to report meaningful error messages that occurred during initialisation, in that case. v2: - turn flag bitmap into simple, separate boolean flags - move errno description after message in _perror() functions - make some of the old perror() messages more descriptive Stefano Brivio (6): conf, passt: Don't try to log to stderr after we close it conf, log: Instead of abusing log levels, add log_conf_parsed flag log, passt: Always print to stderr before initialisation is complete log: Add _perror() logging function variants treewide: Replace perror() calls with calls to logging functions treewide: Replace strerror() calls arch.c | 10 ++++---- conf.c | 45 ++++++++++++++++++---------------- fwd.c | 2 +- isolation.c | 46 +++++++++++++++-------------------- log.c | 49 +++++++++++++++++++++++++------------ log.h | 25 ++++++++++++++++--- netlink.c | 4 +-- passt.1 | 3 ++- passt.c | 70 +++++++++++++++++++++++------------------------------ pasta.c | 41 +++++++++++++++---------------- pcap.c | 8 +++--- tap.c | 14 +++++------ tcp.c | 24 ++++++------------ util.c | 12 ++++----- 14 files changed, 179 insertions(+), 174 deletions(-) -- 2.43.0

2 14

[PATCH 0/6] Fixes for early logging/prints and related cleanups
by Stefano Brivio 20 Jun '24

20 Jun '24

The most apparent issue fixed by this series is the one from 3/6: with a log file configured, we wouldn't print to standard error anymore, during initialisation, which means that users such as libvirt lost the ability to report meaningful error messages that occurred during initialisation, in that case. Stefano Brivio (6): conf, passt: Don't try to log to stderr after we close it conf, log: Introduce internal log flags, instead of abusing log levels log, passt: Always print to stderr before initialisation is complete log: Add _perror() logging function variants treewide: Replace perror() calls with calls to logging functions treewide: Replace strerror() calls arch.c | 10 ++++---- conf.c | 45 ++++++++++++++++++---------------- fwd.c | 2 +- isolation.c | 46 +++++++++++++++-------------------- log.c | 57 +++++++++++++++++++++++++++++++------------ log.h | 26 +++++++++++++++++--- netlink.c | 4 +-- passt.1 | 3 ++- passt.c | 70 +++++++++++++++++++++++------------------------------ pasta.c | 41 +++++++++++++++---------------- pcap.c | 8 +++--- tap.c | 14 +++++------ tcp.c | 24 ++++++------------ util.c | 12 ++++----- 14 files changed, 189 insertions(+), 173 deletions(-) -- 2.43.0

2 26

[PATCH] conf: Accept duplicate and conflicting options, the last one wins
by Stefano Brivio 20 Jun '24

20 Jun '24

In multiple occasions, especially when passt(1) and pasta(1) are used in integrations such as the one with Podman, the ability to override earlier options on the command line with later one would have been convenient. Recently, to debug a number of issues happening with Podman, I would have liked to ask users to share a debug log by passing --debug as additional option, but pasta refuses --quiet (always passed by Podman) and --debug at the same time. Finally, somebody took care of this on Podman side at: https://github.com/containers/common/pull/2052 but still, we'll probably have similar cases, or older versions of Podman around, etc. I think there's some value in telling users about duplicated or conflicting options, because that might reveal issues in integrations or accidental misconfigurations, but by now I'm fairly convinced that the downsides outweigh this. Drop checks about duplicate options and mutually exclusive ones. In some cases, we need to also undo a couple of initialisations caused by earlier options, but this looks like a simplification, overall. Suggested-by: Paul Holzinger <pholzing(a)redhat.com> Suggested-by: David Gibson <david(a)gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- conf.c | 140 +++++++++++++++----------------------------------------- passt.1 | 2 + 2 files changed, 40 insertions(+), 102 deletions(-) diff --git a/conf.c b/conf.c index 2a6f05c..75b46c0 100644 --- a/conf.c +++ b/conf.c @@ -517,9 +517,6 @@ static void conf_netns_opt(char *netns, const char *arg) static void conf_pasta_ns(int *netns_only, char *userns, char *netns, int optind, int argc, char *argv[]) { - if (*netns_only && *userns) - die("Both --userns and --netns-only given"); - if (*netns && optind != argc) die("Both --netns and PID or command given"); @@ -1206,7 +1203,6 @@ void conf(struct ctx *c, int argc, char **argv) {"udp-ns", required_argument, NULL, 'U' }, {"userns", required_argument, NULL, 2 }, {"netns", required_argument, NULL, 3 }, - {"netns-only", no_argument, &netns_only, 1 }, {"ns-mac-addr", required_argument, NULL, 4 }, {"dhcp-dns", no_argument, NULL, 5 }, {"no-dhcp-dns", no_argument, NULL, 6 }, @@ -1223,6 +1219,7 @@ void conf(struct ctx *c, int argc, char **argv) {"config-net", no_argument, NULL, 17 }, {"no-copy-routes", no_argument, NULL, 18 }, {"no-copy-addrs", no_argument, NULL, 19 }, + {"netns-only", no_argument, NULL, 20 }, { 0 }, }; char userns[PATH_MAX] = { 0 }, netns[PATH_MAX] = { 0 }; @@ -1267,6 +1264,8 @@ void conf(struct ctx *c, int argc, char **argv) if (ret <= 0 || ret >= (int)sizeof(userns)) die("Invalid userns: %s", optarg); + netns_only = 0; + break; case 3: if (c->mode != MODE_PASTA) @@ -1327,24 +1326,13 @@ void conf(struct ctx *c, int argc, char **argv) c->no_netns_quit = 1; break; case 11: - if (c->trace) - die("Multiple --trace options given"); - - if (c->quiet) - die("Either --trace or --quiet"); - c->trace = c->debug = 1; + c->quiet = 0; break; case 12: - if (runas) - die("Multiple --runas options given"); - runas = optarg; break; case 13: - if (logsize) - die("Multiple --log-size options given"); - errno = 0; logsize = strtol(optarg, NULL, 0); @@ -1358,9 +1346,6 @@ void conf(struct ctx *c, int argc, char **argv) fprintf(stdout, VERSION_BLOB); exit(EXIT_SUCCESS); case 15: - if (*c->ip4.ifname_out) - die("Redundant outbound interface: %s", optarg); - ret = snprintf(c->ip4.ifname_out, sizeof(c->ip4.ifname_out), "%s", optarg); if (ret <= 0 || ret >= (int)sizeof(c->ip4.ifname_out)) @@ -1368,9 +1353,6 @@ void conf(struct ctx *c, int argc, char **argv) break; case 16: - if (*c->ip6.ifname_out) - die("Redundant outbound interface: %s", optarg); - ret = snprintf(c->ip6.ifname_out, sizeof(c->ip6.ifname_out), "%s", optarg); if (ret <= 0 || ret >= (int)sizeof(c->ip6.ifname_out)) @@ -1397,62 +1379,41 @@ void conf(struct ctx *c, int argc, char **argv) warn("--no-copy-addrs will be dropped soon"); c->no_copy_addrs = copy_addrs_opt = true; break; - case 'd': - if (c->debug) - die("Multiple --debug options given"); - - if (c->quiet) - die("Either --debug or --quiet"); + case 20: + if (c->mode != MODE_PASTA) + die("--netns-only is for pasta mode only"); + netns_only = 1; + *userns = 0; + break; + case 'd': c->debug = 1; + c->quiet = 0; break; case 'e': - if (logfile) - die("Can't log to both file and stderr"); - - if (c->force_stderr) - die("Multiple --stderr options given"); - c->force_stderr = 1; + logfile = NULL; break; case 'l': - if (c->force_stderr) - die("Can't log to both stderr and file"); - - if (logfile) - die("Multiple --log-file options given"); - logfile = optarg; + c->force_stderr = 0; break; case 'q': - if (c->quiet) - die("Multiple --quiet options given"); - - if (c->debug) - die("Either --debug or --quiet"); - c->quiet = 1; + c->debug = c->trace = 0; break; case 'f': - if (c->foreground) - die("Multiple --foreground options given"); - c->foreground = 1; break; case 's': - if (*c->sock_path) - die("Multiple --socket options given"); - ret = snprintf(c->sock_path, UNIX_SOCK_MAX - 1, "%s", optarg); if (ret <= 0 || ret >= (int)sizeof(c->sock_path)) die("Invalid socket path: %s", optarg); + c->fd_tap = -1; break; case 'F': - if (c->fd_tap >= 0) - die("Multiple --fd options given"); - errno = 0; c->fd_tap = strtol(optarg, NULL, 0); @@ -1460,12 +1421,9 @@ void conf(struct ctx *c, int argc, char **argv) die("Invalid --fd: %s", optarg); c->one_off = true; - + *c->sock_path = 0; break; case 'I': - if (*c->pasta_ifn) - die("Multiple --ns-ifname options given"); - ret = snprintf(c->pasta_ifn, IFNAMSIZ, "%s", optarg); if (ret <= 0 || ret >= IFNAMSIZ) @@ -1473,18 +1431,12 @@ void conf(struct ctx *c, int argc, char **argv) break; case 'p': - if (*c->pcap) - die("Multiple --pcap options given"); - ret = snprintf(c->pcap, sizeof(c->pcap), "%s", optarg); if (ret <= 0 || ret >= (int)sizeof(c->pcap)) die("Invalid pcap path: %s", optarg); break; case 'P': - if (*c->pidfile) - die("Multiple --pid options given"); - ret = snprintf(c->pidfile, sizeof(c->pidfile), "%s", optarg); if (ret <= 0 || ret >= (int)sizeof(c->pidfile)) @@ -1492,9 +1444,6 @@ void conf(struct ctx *c, int argc, char **argv) break; case 'm': - if (c->mtu) - die("Multiple --mtu options given"); - errno = 0; c->mtu = strtol(optarg, NULL, 0); @@ -1544,14 +1493,12 @@ void conf(struct ctx *c, int argc, char **argv) if (c->mode == MODE_PASTA) c->no_copy_routes = 1; - if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.gw) && - inet_pton(AF_INET6, optarg, &c->ip6.gw) && + if (inet_pton(AF_INET6, optarg, &c->ip6.gw) && !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.gw) && !IN6_IS_ADDR_LOOPBACK(&c->ip6.gw)) break; - if (IN4_IS_ADDR_UNSPECIFIED(&c->ip4.gw) && - inet_pton(AF_INET, optarg, &c->ip4.gw) && + if (inet_pton(AF_INET, optarg, &c->ip4.gw) && !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.gw) && !IN4_IS_ADDR_BROADCAST(&c->ip4.gw) && !IN4_IS_ADDR_LOOPBACK(&c->ip4.gw)) @@ -1560,15 +1507,11 @@ void conf(struct ctx *c, int argc, char **argv) die("Invalid gateway address: %s", optarg); break; case 'i': - if (ifi4 || ifi6) - die("Redundant interface: %s", optarg); - if (!(ifi4 = ifi6 = if_nametoindex(optarg))) die_perror("Invalid interface name %s", optarg); break; case 'o': - if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr_out) && - inet_pton(AF_INET6, optarg, &c->ip6.addr_out) && + if (inet_pton(AF_INET6, optarg, &c->ip6.addr_out) && !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr_out) && !IN6_IS_ADDR_LOOPBACK(&c->ip6.addr_out) && !IN6_IS_ADDR_V4MAPPED(&c->ip6.addr_out) && @@ -1576,8 +1519,7 @@ void conf(struct ctx *c, int argc, char **argv) !IN6_IS_ADDR_MULTICAST(&c->ip6.addr_out)) break; - if (IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr_out) && - inet_pton(AF_INET, optarg, &c->ip4.addr_out) && + if (inet_pton(AF_INET, optarg, &c->ip4.addr_out) && !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr_out) && !IN4_IS_ADDR_BROADCAST(&c->ip4.addr_out) && !IN4_IS_ADDR_MULTICAST(&c->ip4.addr_out)) @@ -1588,18 +1530,23 @@ void conf(struct ctx *c, int argc, char **argv) break; case 'D': if (!strcmp(optarg, "none")) { - if (c->no_dns) - die("Redundant DNS options"); + c->no_dns = 1; - if (dns4 - c->ip4.dns || dns6 - c->ip6.dns) - die("Conflicting DNS options"); + dns4 = &c->ip4.dns[0]; + memset(c->ip4.dns, 0, sizeof(c->ip4.dns)); + c->ip4.dns[0] = (struct in_addr){ 0 }; + c->ip4.dns_match = (struct in_addr){ 0 }; + c->ip4.dns_host = (struct in_addr){ 0 }; + + dns6 = &c->ip6.dns[0]; + memset(c->ip6.dns, 0, sizeof(c->ip6.dns)); + c->ip6.dns_match = (struct in6_addr){ 0 }; + c->ip6.dns_host = (struct in6_addr){ 0 }; - c->no_dns = 1; break; } - if (c->no_dns) - die("Conflicting DNS options"); + c->no_dns = 0; if (dns4 - &c->ip4.dns[0] < ARRAY_SIZE(c->ip4.dns) && inet_pton(AF_INET, optarg, &dns4_tmp)) { @@ -1617,18 +1564,14 @@ void conf(struct ctx *c, int argc, char **argv) break; case 'S': if (!strcmp(optarg, "none")) { - if (c->no_dns_search) - die("Redundant DNS search options"); + c->no_dns_search = 1; - if (dnss != c->dns_search) - die("Conflicting DNS search options"); + memset(c->dns_search, 0, sizeof(c->dns_search)); - c->no_dns_search = 1; break; } - if (c->no_dns_search) - die("Conflicting DNS search options"); + c->no_dns_search = 0; if (dnss - c->dns_search < ARRAY_SIZE(c->dns_search)) { ret = snprintf(dnss->n, sizeof(*c->dns_search), @@ -1644,17 +1587,16 @@ void conf(struct ctx *c, int argc, char **argv) break; case '4': v4_only = true; + v6_only = false; break; case '6': v6_only = true; + v4_only = false; break; case '1': if (c->mode == MODE_PASTA) die("--one-off is for passt mode only"); - if (c->one_off) - die("Redundant --one-off option"); - c->one_off = true; break; case 't': @@ -1673,12 +1615,6 @@ void conf(struct ctx *c, int argc, char **argv) } } while (name != -1); - if (v4_only && v6_only) - die("Options ipv4-only and ipv6-only are mutually exclusive"); - - if (*c->sock_path && c->fd_tap >= 0) - die("Options --socket and --fd are mutually exclusive"); - if (c->mode == MODE_PASTA && !c->pasta_conf_ns) { if (copy_routes_opt) die("--no-copy-routes needs --config-net"); diff --git a/passt.1 b/passt.1 index 31e528e..6dfa670 100644 --- a/passt.1 +++ b/passt.1 @@ -73,6 +73,8 @@ for performance reasons. .SH OPTIONS +\fBIf conflicting or multiple options are given, the last one takes effect.\fR + .TP .BR \-d ", " \-\-debug Be verbose, don't log to the system logger. -- 2.43.0

3 5

[PATCH] passt.1, qrap.1: align license description with SPDX identifier
by Danish Prakash 19 Jun '24

19 Jun '24

The SPDX identifier states GPL-2.0-or-later but the copyright section mentions GPL-3.0 or later causing a mismatch. Signed-off-by: Danish Prakash <contact(a)danishpraka.sh> --- passt.1 | 2 +- qrap.1 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/passt.1 b/passt.1 index 3a23a43cc2e2..7d414f38e341 100644 --- a/passt.1 +++ b/passt.1 @@ -978,7 +978,7 @@ Copyright (c) 2020-2022 Red Hat GmbH. \fBpasst\fR and \fBpasta\fR are free software: you can redistribute them and/or modify them under the terms of the GNU Affero General Public License as -published by the Free Software Foundation, either version 3 of the License, or +published by the Free Software Foundation, either version 2 of the License, or (at your option) any later version. .SH SEE ALSO diff --git a/qrap.1 b/qrap.1 index c9e0598d64fb..8704c31cb8ce 100644 --- a/qrap.1 +++ b/qrap.1 @@ -67,7 +67,7 @@ Copyright (c) 2020-2021 Red Hat GmbH. \fBqrap\fR is free software: you can redistribute is and/or modify it under the terms of the GNU Affero General Public License as published by the Free Software -Foundation, either version 3 of the License, or (at your option) any later +Foundation, either version 2 of the License, or (at your option) any later version. .SH SEE ALSO -- 2.45.2

2 4

[PATCH] siphash: Remove stale prototypes
by David Gibson 19 Jun '24

19 Jun '24

In fc8f0f8c ("siphash: Use incremental rather than all-at-once siphash functions") we removed the older interface to the SipHash implementation, which took fixed sized blocks of data. However, we forgot to remove the prototypes for those functions, so do that now. Fixes: fc8f0f8c ("siphash: Use incremental rather than all-at-once"...) Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- siphash.h | 6 ------ 1 file changed, 6 deletions(-) diff --git a/siphash.h b/siphash.h index f966cdb0..a2ca2a99 100644 --- a/siphash.h +++ b/siphash.h @@ -115,10 +115,4 @@ static inline uint64_t siphash_final(struct siphash_state *state, return state->v[0] ^ state->v[1] ^ state->v[2] ^ state->v[3]; } -uint64_t siphash_8b(const uint8_t *in, const uint64_t *k); -uint64_t siphash_12b(const uint8_t *in, const uint64_t *k); -uint64_t siphash_20b(const uint8_t *in, const uint64_t *k); -uint64_t siphash_32b(const uint8_t *in, const uint64_t *k); -uint64_t siphash_36b(const uint8_t *in, const uint64_t *k); - #endif /* SIPHASH_H */ -- 2.45.2

2 1

[PATCH] netlink: With no default route, pick the first interface with a route
by Stefano Brivio 19 Jun '24

19 Jun '24

While commit f919dc7a4b1c ("conf, netlink: Don't require a default route to start") sounded reasonable in the assumption that, if we don't find default routes for a given address family, we can still proceed by selecting an interface with any route *iff it's the only one for that protocol family*, Jelle reported a further issue in a similar setup. There, multiple interfaces are present, and while remote container connectivity doesn't matter for the container, local connectivity is desired. There are no default routes, but those multiple interfaces all have non-default routes, so we should just pick one and start. Pick the first interface reported by the kernel with any route, if there are no default routes. There should be no harm in doing so. Reported-by: Jelle van der Waa <jvanderwaa(a)redhat.com> Reported-by: Martin Pitt <mpitt(a)redhat.com> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2277954 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- netlink.c | 16 ++++++++-------- passt.1 | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/netlink.c b/netlink.c index 0be4ea3..3aed7e5 100644 --- a/netlink.c +++ b/netlink.c @@ -269,8 +269,7 @@ unsigned int nl_get_ext_if(int s, sa_family_t af) size_t na; /* Look for an interface with a default route first, failing that, look - * for any interface with a route, and pick it only if it's the only - * interface with a route. + * for any interface with a route, and pick the first one, if any. */ seq = nl_send(s, &req, RTM_GETROUTE, NLM_F_DUMP, sizeof(req)); nl_foreach_oftype(nh, status, s, buf, seq, RTM_NEWROUTE) { @@ -324,18 +323,19 @@ unsigned int nl_get_ext_if(int s, sa_family_t af) warn("netlink: RTM_GETROUTE failed: %s", strerror(-status)); if (defifi) { - if (ndef > 1) + if (ndef > 1) { info("Multiple default %s routes, picked first", af_name(af)); + } return defifi; } if (anyifi) { - if (nany == 1) - return anyifi; - - info("Multiple interfaces with %s routes, use -i to select one", - af_name(af)); + if (nany > 1) { + info("Multiple interfaces with %s routes, picked first", + af_name(af)); + } + return anyifi; } if (!nany) diff --git a/passt.1 b/passt.1 index 6dfa670..6ee1e2e 100644 --- a/passt.1 +++ b/passt.1 @@ -152,8 +152,8 @@ This option can be specified zero (for defaults) to two times (once for IPv4, once for IPv6). By default, assigned IPv4 and IPv6 addresses are taken from the host interfaces with the first default route, if any, for the corresponding IP version. If no -default routes are available and there is just one interface with any route, -that interface will be chosen instead. +default routes are available and there is any interface with any route for a +given IP version, the first of these interfaces will be chosen instead. .TP .BR \-n ", " \-\-netmask " " \fImask -- 2.43.0

3 2

[PATCH] netlink: Ignore EHOSTUNREACH failures when duplicating routes
by Stefano Brivio 19 Jun '24

19 Jun '24

To implicitly resolve possible dependencies between routes as we duplicate them into the target namespace, we go through a set of n routes n times, and ignore EEXIST responses to netlink messages (we already inserted the route) and ENETUNREACH (we didn't insert the route yet, but we need to insert another one first). Until now, we didn't ignore EHOSTUNREACH responses. However, NetworkManager users with multiple non-subnet routes for the same interface report that pasta exits with "no route to host" while duplicating routes. This happens because NetworkManager sets the 'noprefixroute' attribute on addresses, meaning that the kernel won't create subnet routes automatically depending on the prefix length of the address. We copy this attribute as we copy the address into the target namespace, and as a result, the kernel doesn't create subnet routes in the target namespace either. This means that the gateway for routes that are inserted later can be unreachable at some points during the sequence of route duplication. That is, we don't just have dependencies between regular routes, but we can also have dependencies between regular routes and subnet routes, as subnet routes are not automatically inserted in advance. Link: https://github.com/containers/podman/issues/22824 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- netlink.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/netlink.c b/netlink.c index 4dbddb2..0be4ea3 100644 --- a/netlink.c +++ b/netlink.c @@ -655,7 +655,8 @@ int nl_route_dup(int s_src, unsigned int ifi_src, rc = nl_do(s_dst, nh, RTM_NEWROUTE, (flags & ~NLM_F_DUMP_FILTERED) | NLM_F_CREATE, nh->nlmsg_len); - if (rc < 0 && rc != -ENETUNREACH && rc != -EEXIST) + if (rc < 0 && rc != -EEXIST && + rc != -ENETUNREACH && rc != -EHOSTUNREACH) return rc; } } -- 2.43.0

2 1

[PATCH v2] tcp: Don't rely on bind() to fail to decide that connection target is valid
by Stefano Brivio 19 Jun '24

19 Jun '24

Commit e1a2e2780c91 ("tcp: Check if connection is local or low RTT was seen before using large MSS") added a call to bind() before we issue a connect() to the target for an outbound connection. If bind() fails, but neither with EADDRNOTAVAIL, nor with EACCESS, we can conclude that the target address is a local (host) address, and we can use an unlimited MSS. While at it, according to the reasoning of that commit, if bind() succeeds, we would know right away that nobody is listening at that (local) address and port, and we don't even need to call connect(): we can just fail early and reset the connection attempt. But if non-local binds are enabled via net.ipv4.ip_nonlocal_bind or net.ipv6.ip_nonlocal_bind sysctl, binding to a non-local address will actually succeed, so we can't rely on it to fail in general. The visible issue with the existing behaviour is that we would reset any outbound connection to non-local addresses, if non-local binds are enabled. Keep the significant optimisation for local addresses along with the bind() call, but if it succeeds, don't draw any conclusion: close the socket, grab another one, and proceed normally. This will incur a small latency penalty if non-local binds are enabled (we'll likely fetch an existing socket from the pool but additionally call close()), or if the target is local but not bound: we'll need to call connect() and get a failure before relaying that failure back. Link: https://github.com/containers/podman/issues/23003 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- v2: Avoid (bogus) reports of uninitialised 'sa' in bind() using an assertion on 'af' (it must be AF_INET or AF_INET6) tcp.c | 48 +++++++++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/tcp.c b/tcp.c index 231f63b..698e7ec 100644 --- a/tcp.c +++ b/tcp.c @@ -1623,6 +1623,9 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, flow_initiate(flow, PIF_TAP); + flow_target(flow, PIF_HOST); + conn = FLOW_SET_TYPE(flow, FLOW_TCP, tcp); + if (af == AF_INET) { if (IN4_IS_ADDR_UNSPECIFIED(saddr) || IN4_IS_ADDR_BROADCAST(saddr) || @@ -1639,6 +1642,9 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, dstport); goto cancel; } + + sa = (struct sockaddr *)&addr4; + sl = sizeof(addr4); } else if (af == AF_INET6) { if (IN6_IS_ADDR_UNSPECIFIED(saddr) || IN6_IS_ADDR_MULTICAST(saddr) || srcport == 0 || @@ -1653,6 +1659,11 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, dstport); goto cancel; } + + sa = (struct sockaddr *)&addr6; + sl = sizeof(addr6); + } else { + ASSERT(0); } if ((s = tcp_conn_sock(c, af)) < 0) @@ -1665,6 +1676,26 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, addr6.sin6_addr = in6addr_loopback; } + /* Use bind() to check if the target address is local (EADDRINUSE or + * similar) and already bound, and set the LOCAL flag in that case. + * + * If bind() succeeds, in general, we could infer that nobody (else) is + * listening on that address and port and reset the connection attempt + * early, but we can't rely on that if non-local binds are enabled, + * because bind() would succeed for any non-local address we can reach. + * + * So, if bind() succeeds, close the socket, get a new one, and proceed. + */ + if (bind(s, sa, sl)) { + if (errno != EADDRNOTAVAIL && errno != EACCES) + conn_flag(c, conn, LOCAL); + } else { + /* Not a local, bound destination, inconclusive test */ + close(s); + if ((s = tcp_conn_sock(c, af)) < 0) + goto cancel; + } + if (af == AF_INET6 && IN6_IS_ADDR_LINKLOCAL(&addr6.sin6_addr)) { struct sockaddr_in6 addr6_ll = { .sin6_family = AF_INET6, @@ -1675,8 +1706,6 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, goto cancel; } - flow_target(flow, PIF_HOST); - conn = FLOW_SET_TYPE(flow, FLOW_TCP, tcp); conn->sock = s; conn->timer = -1; conn_event(c, conn, TAP_SYN_RCVD); @@ -1698,14 +1727,6 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, inany_from_af(&conn->faddr, af, daddr); - if (af == AF_INET) { - sa = (struct sockaddr *)&addr4; - sl = sizeof(addr4); - } else { - sa = (struct sockaddr *)&addr6; - sl = sizeof(addr6); - } - conn->fport = dstport; conn->eport = srcport; @@ -1718,13 +1739,6 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, tcp_hash_insert(c, conn); - if (!bind(s, sa, sl)) { - tcp_rst(c, conn); /* Nobody is listening then */ - goto cancel; - } - if (errno != EADDRNOTAVAIL && errno != EACCES) - conn_flag(c, conn, LOCAL); - if ((af == AF_INET && !IN4_IS_ADDR_LOOPBACK(&addr4.sin_addr)) || (af == AF_INET6 && !IN6_IS_ADDR_LOOPBACK(&addr6.sin6_addr) && !IN6_IS_ADDR_LINKLOCAL(&addr6.sin6_addr))) -- 2.43.0

2 1

[PATCH] netlink: Strip nexthop identifiers when duplicating routes
by Stefano Brivio 19 Jun '24

19 Jun '24

If routing daemons set up host routes, for example FRR via OSPF as in the reported issue, they might add nexthop identifiers (not objects) that are generally not valid in the target namespace. Strip them off as well, otherwise we'll get EINVAL from the kernel. Link: https://github.com/containers/podman/issues/22960 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- netlink.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/netlink.c b/netlink.c index 4dbddb2..58822e9 100644 --- a/netlink.c +++ b/netlink.c @@ -608,6 +608,15 @@ int nl_route_dup(int s_src, unsigned int ifi_src, * route invalid in the namespace. Strip off * RTA_PREFSRC attributes to avoid that. */ rta->rta_type = RTA_UNSPEC; + } else if (rta->rta_type == RTA_NH_ID) { + /* Host routes set up via routing protocols + * (e.g. OSPF) might contain a nexthop ID (and + * not nexthop objects, which are taken care of + * in the RTA_MULTIPATH case above) that's not + * valid in the target namespace. Strip those as + * well. + */ + rta->rta_type = RTA_UNSPEC; } } -- 2.43.0

2 1

[PATCH] tcp: Don't rely on bind() to fail to decide that connection target is valid
by Stefano Brivio 18 Jun '24

18 Jun '24

Commit e1a2e2780c91 ("tcp: Check if connection is local or low RTT was seen before using large MSS") added a call to bind() before we issue a connect() to the target for an outbound connection. If bind() fails, but neither with EADDRNOTAVAIL, nor with EACCESS, we can conclude that the target address is a local (host) address, and we can use an unlimited MSS. While at it, according to the reasoning of that commit, if bind() succeeds, we would know right away that nobody is listening at that (local) address and port, and we don't even need to call connect(): we can just fail early and reset the connection attempt. But if non-local binds are enabled via net.ipv4.ip_nonlocal_bind or net.ipv6.ip_nonlocal_bind sysctl, binding to a non-local address will actually succeed, so we can't rely on it to fail in general. The visible issue with the existing behaviour is that we would reset any outbound connection to non-local addresses, if non-local binds are enabled. Keep the significant optimisation for local addresses along with the bind() call, but if it succeeds, don't draw any conclusion: close the socket, grab another one, and proceed normally. This will incur a small latency penalty if non-local binds are enabled (we'll likely fetch an existing socket from the pool but additionally call close()), or if the target is local but not bound: we'll need to call connect() and get a failure before relaying that failure back. Link: https://github.com/containers/podman/issues/23003 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- tcp.c | 48 ++++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/tcp.c b/tcp.c index 231f63b..fd1c550 100644 --- a/tcp.c +++ b/tcp.c @@ -1615,14 +1615,17 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, const struct sockaddr *sa; struct tcp_tap_conn *conn; union flow *flow; + socklen_t sl = 0; int s = -1, mss; - socklen_t sl; if (!(flow = flow_alloc())) return; flow_initiate(flow, PIF_TAP); + flow_target(flow, PIF_HOST); + conn = FLOW_SET_TYPE(flow, FLOW_TCP, tcp); + if (af == AF_INET) { if (IN4_IS_ADDR_UNSPECIFIED(saddr) || IN4_IS_ADDR_BROADCAST(saddr) || @@ -1639,6 +1642,9 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, dstport); goto cancel; } + + sa = (struct sockaddr *)&addr4; + sl = sizeof(addr4); } else if (af == AF_INET6) { if (IN6_IS_ADDR_UNSPECIFIED(saddr) || IN6_IS_ADDR_MULTICAST(saddr) || srcport == 0 || @@ -1653,6 +1659,9 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, dstport); goto cancel; } + + sa = (struct sockaddr *)&addr6; + sl = sizeof(addr6); } if ((s = tcp_conn_sock(c, af)) < 0) @@ -1665,6 +1674,26 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, addr6.sin6_addr = in6addr_loopback; } + /* Use bind() to check if the target address is local (EADDRINUSE or + * similar) and already bound, and set the LOCAL flag in that case. + * + * If bind() succeeds, in general, we could infer that nobody (else) is + * listening on that address and port and reset the connection attempt + * early, but we can't rely on that if non-local binds are enabled, + * because bind() would succeed for any non-local address we can reach. + * + * So, if bind() succeeds, close the socket, get a new one, and proceed. + */ + if (bind(s, sa, sl)) { + if (errno != EADDRNOTAVAIL && errno != EACCES) + conn_flag(c, conn, LOCAL); + } else { + /* Not a local, bound destination, inconclusive test */ + close(s); + if ((s = tcp_conn_sock(c, af)) < 0) + goto cancel; + } + if (af == AF_INET6 && IN6_IS_ADDR_LINKLOCAL(&addr6.sin6_addr)) { struct sockaddr_in6 addr6_ll = { .sin6_family = AF_INET6, @@ -1675,8 +1704,6 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, goto cancel; } - flow_target(flow, PIF_HOST); - conn = FLOW_SET_TYPE(flow, FLOW_TCP, tcp); conn->sock = s; conn->timer = -1; conn_event(c, conn, TAP_SYN_RCVD); @@ -1698,14 +1725,6 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, inany_from_af(&conn->faddr, af, daddr); - if (af == AF_INET) { - sa = (struct sockaddr *)&addr4; - sl = sizeof(addr4); - } else { - sa = (struct sockaddr *)&addr6; - sl = sizeof(addr6); - } - conn->fport = dstport; conn->eport = srcport; @@ -1718,13 +1737,6 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, tcp_hash_insert(c, conn); - if (!bind(s, sa, sl)) { - tcp_rst(c, conn); /* Nobody is listening then */ - goto cancel; - } - if (errno != EADDRNOTAVAIL && errno != EACCES) - conn_flag(c, conn, LOCAL); - if ((af == AF_INET && !IN4_IS_ADDR_LOOPBACK(&addr4.sin_addr)) || (af == AF_INET6 && !IN6_IS_ADDR_LOOPBACK(&addr6.sin6_addr) && !IN6_IS_ADDR_LINKLOCAL(&addr6.sin6_addr))) -- 2.43.0

1 0