On Sun, Apr 12, 2026 at 08:53:13PM -0400, Jon Maloy wrote:
After the previous changes in this series it becomes possible to simplify the pasta_ns_conf() function.
We extract address and route configuration into helper functions pasta_conf_addrs() and pasta_conf_routes(), reducing nesting and improving readability.
To allow pasta_conf_addrs() to handle both address families uniformly, we change nl_addr_set() to take a union inany_addr pointer instead of void pointer, moving the address family handling into the function itself.
We also fix a bug where the IPv6 code path incorrectly wrote to req.set.a4.rta_l.rta_type instead of req.set.a6.rta_l.rta_type.
Signed-off-by: Jon Maloy
Reviewed-by: David Gibson
--- v7: -Removed redundant argument 'af' in nl_addr_set() -Removed redundant label and 'goto's in pasta_ns_conf() -Since I excluded addition of all LINKLOCAL addresses from host address array in a previous commit I can now omit this test in pasta_conf_addrs() as suggested by David. -Here is the example of agnostic usage of inany_prefix_len() I referred to in a previous commit. --- netlink.c | 17 ++-- netlink.h | 4 +- pasta.c | 232 ++++++++++++++++++++++++++---------------------------- 3 files changed, 123 insertions(+), 130 deletions(-)
diff --git a/netlink.c b/netlink.c index 3f5a812..3d25212 100644 --- a/netlink.c +++ b/netlink.c @@ -863,15 +863,15 @@ int nl_addr_get_ll(int s, unsigned int ifi, struct in6_addr *addr) * nl_addr_set() - Set IP addresses for given interface and address family * @s: Netlink socket * @ifi: Interface index - * @af: Address family * @addr: Global address to set * @prefix_len: Mask or prefix length to set * * Return: 0 on success, negative error code on failure */ -int nl_addr_set(int s, unsigned int ifi, sa_family_t af, - const void *addr, int prefix_len) +int nl_addr_set(int s, unsigned int ifi, const union inany_addr *addr, + int prefix_len) { + sa_family_t af = inany_af(addr); struct req_t { struct nlmsghdr nlh; struct ifaddrmsg ifa; @@ -905,21 +905,22 @@ int nl_addr_set(int s, unsigned int ifi, sa_family_t af,
len = offsetof(struct req_t, set.a6) + sizeof(req.set.a6);
- memcpy(&req.set.a6.l, addr, sizeof(req.set.a6.l)); + memcpy(&req.set.a6.l, &addr->a6, sizeof(req.set.a6.l)); req.set.a6.rta_l.rta_len = rta_len; - req.set.a4.rta_l.rta_type = IFA_LOCAL; - memcpy(&req.set.a6.a, addr, sizeof(req.set.a6.a)); + req.set.a6.rta_l.rta_type = IFA_LOCAL; + memcpy(&req.set.a6.a, &addr->a6, sizeof(req.set.a6.a)); req.set.a6.rta_a.rta_len = rta_len; req.set.a6.rta_a.rta_type = IFA_ADDRESS; } else { + const struct in_addr *v4 = inany_v4(addr);
I suspect the static checkers won't be able to deduce that this cannot be NULL, because of the earlier inany_af() call - in a sense you're checking the address family twice. I think it would be more elegant (and avoid possible checker false positives) to have the if based on the return value from inany_v4(), then set req.ifa.ifa_family to constant values in each branch
size_t rta_len = RTA_LENGTH(sizeof(req.set.a4.l));
len = offsetof(struct req_t, set.a4) + sizeof(req.set.a4);
- memcpy(&req.set.a4.l, addr, sizeof(req.set.a4.l)); + memcpy(&req.set.a4.l, v4, sizeof(req.set.a4.l)); req.set.a4.rta_l.rta_len = rta_len; req.set.a4.rta_l.rta_type = IFA_LOCAL; - memcpy(&req.set.a4.a, addr, sizeof(req.set.a4.a)); + memcpy(&req.set.a4.a, v4, sizeof(req.set.a4.a)); req.set.a4.rta_a.rta_len = rta_len; req.set.a4.rta_a.rta_type = IFA_ADDRESS; } diff --git a/netlink.h b/netlink.h index 3af6d58..ec859b7 100644 --- a/netlink.h +++ b/netlink.h @@ -22,8 +22,8 @@ int nl_route_dup(int s_src, unsigned int ifi_src, int nl_addr_get_all(struct ctx *c, int s, unsigned int ifi, sa_family_t af); bool nl_neigh_mac_get(int s, const union inany_addr *addr, int ifi, unsigned char *mac); -int nl_addr_set(int s, unsigned int ifi, sa_family_t af, - const void *addr, int prefix_len); +int nl_addr_set(int s, unsigned int ifi, const union inany_addr *addr, + int prefix_len); int nl_addr_get_ll(int s, unsigned int ifi, struct in6_addr *addr); int nl_addr_set_ll_nodad(int s, unsigned int ifi); int nl_addr_dup(int s_src, unsigned int ifi_src, diff --git a/pasta.c b/pasta.c index b3936f5..f949dc2 100644 --- a/pasta.c +++ b/pasta.c @@ -46,6 +46,7 @@
#include "util.h" #include "passt.h" +#include "conf.h" #include "isolation.h" #include "netlink.h" #include "log.h" @@ -303,13 +304,69 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid, die_perror("Failed to join network namespace"); }
+/** + * pasta_conf_addrs() - Configure addresses for one address family in namespace + * @c: Execution context + * @af: Address family (AF_INET or AF_INET6) + * @ifi: Host interface index for this address family + * @no_copy: If true, set addresses from c->addrs; if false, copy from host + * + * Return: 0 on success, negative error code on failure + */ +static int pasta_conf_addrs(struct ctx *c, sa_family_t af, + int ifi, bool no_copy) +{ + const struct guest_addr *a; + + if (!ifi) + return 0; + + if (!no_copy) + return nl_addr_dup(nl_sock, ifi, nl_sock_ns, c->pasta_ifi, af); + + for_each_addr(a, c->addrs, c->addr_count, af) { + int rc; + + rc = nl_addr_set(nl_sock_ns, c->pasta_ifi, &a->addr, + inany_prefix_len(&a->addr, a->prefix_len)); + if (rc < 0) + return rc; + } + return 0; +} + +/** + * pasta_conf_routes() - Configure routes for one address family in namespace + * @c: Execution context + * @af: Address family (AF_INET or AF_INET6) + * @ifi: Host interface index for this address family + * @no_copy: If true, set default route; if false, copy routes from host + * + * Return: 0 on success, negative error code on failure + */ +static int pasta_conf_routes(struct ctx *c, sa_family_t af, int ifi, + bool no_copy) +{ + const void *gw = (af == AF_INET) ? + (const void *)&c->ip4.guest_gw : (const void *)&c->ip6.guest_gw; + + if (!ifi) + return 0; + + if (no_copy)
It'd be nicer to have the !no_copy branch first for consistency with pasta_conf_addrs().
+ return nl_route_set_def(nl_sock_ns, c->pasta_ifi, af, gw); + + return nl_route_dup(nl_sock, ifi, nl_sock_ns, c->pasta_ifi, af); +} + /** * pasta_ns_conf() - Set up loopback and tap interfaces in namespace as needed * @c: Execution context */ void pasta_ns_conf(struct ctx *c) { - int rc = 0; + unsigned int flags = IFF_UP; + int rc;
rc = nl_link_set_flags(nl_sock_ns, 1 /* lo */, IFF_UP, IFF_UP); if (rc < 0) @@ -328,127 +385,62 @@ void pasta_ns_conf(struct ctx *c) die("Couldn't set MAC address in namespace: %s", strerror_(-rc));
- if (c->pasta_conf_ns) { - unsigned int flags = IFF_UP; - const struct guest_addr *a; - int plen; - - if (c->mtu) - nl_link_set_mtu(nl_sock_ns, c->pasta_ifi, c->mtu); - - if (c->ifi6) /* Avoid duplicate address detection on link up */ - flags |= IFF_NOARP; - - nl_link_set_flags(nl_sock_ns, c->pasta_ifi, flags, flags); - - if (c->ifi4) { - if (c->ip4.no_copy_addrs) { - for_each_addr(a, c->addrs, c->addr_count, AF_INET) { - plen = inany_prefix_len(&a->addr, - a->prefix_len); - rc = nl_addr_set(nl_sock_ns, - c->pasta_ifi, AF_INET, - inany_v4(&a->addr), - plen); - if (rc < 0) - break; - } - } else { - rc = nl_addr_dup(nl_sock, c->ifi4, - nl_sock_ns, c->pasta_ifi, - AF_INET); - } - - if (c->ifi4 == -1 && rc == -ENOTSUP) { - warn("IPv4 not supported, disabling"); - c->ifi4 = 0; - goto ipv4_done; - } - - if (rc < 0) { - die("Couldn't set IPv4 address(es) in namespace: %s", - strerror_(-rc)); - } - - if (c->ip4.no_copy_routes) { - rc = nl_route_set_def(nl_sock_ns, c->pasta_ifi, - AF_INET, - &c->ip4.guest_gw); - } else { - rc = nl_route_dup(nl_sock, c->ifi4, nl_sock_ns, - c->pasta_ifi, AF_INET); - } - - if (rc < 0) { - die("Couldn't set IPv4 route(s) in guest: %s", - strerror_(-rc)); - } - } -ipv4_done: - - if (c->ifi6) { - rc = nl_addr_get_ll(nl_sock_ns, c->pasta_ifi, - &c->ip6.addr_ll_seen); - if (rc < 0) { - warn("Can't get LL address from namespace: %s", - strerror_(-rc)); - } - - rc = nl_addr_set_ll_nodad(nl_sock_ns, c->pasta_ifi); - if (rc < 0) { - warn("Can't set nodad for LL in namespace: %s", - strerror_(-rc)); - } - - /* We dodged DAD: re-enable neighbour solicitations */ - nl_link_set_flags(nl_sock_ns, c->pasta_ifi, - 0, IFF_NOARP); - - if (c->ip6.no_copy_addrs) { - for_each_addr(a, c->addrs, c->addr_count, AF_INET6) { - rc = nl_addr_set(nl_sock_ns, - c->pasta_ifi, - AF_INET6, &a->addr.a6, - a->prefix_len); - if (rc < 0) - break; - } - } else { - rc = nl_addr_dup(nl_sock, c->ifi6, - nl_sock_ns, c->pasta_ifi, - AF_INET6); - } - - if (rc < 0) { - die("Couldn't set IPv6 address(es) in namespace: %s", - strerror_(-rc)); - } - - if (c->ip6.no_copy_routes) { - rc = nl_route_set_def(nl_sock_ns, c->pasta_ifi, - AF_INET6, - &c->ip6.guest_gw); - } else { - rc = nl_route_dup(nl_sock, c->ifi6, - nl_sock_ns, c->pasta_ifi, - AF_INET6); - } - - if (c->ifi6 == -1 && rc == -ENOTSUP) { - warn("IPv6 not supported, disabling"); - c->ifi6 = 0; - goto ipv6_done; - } - - if (rc < 0) { - die("Couldn't set IPv6 route(s) in guest: %s", - strerror_(-rc)); - } - } + proto_update_l2_buf(c->guest_mac); + + if (!c->pasta_conf_ns) + return; + + if (c->mtu) + nl_link_set_mtu(nl_sock_ns, c->pasta_ifi, c->mtu); + + if (c->ifi6) /* Avoid duplicate address detection on link up */ + flags |= IFF_NOARP;
Pre-existing, but this looks weird. Why are we setting a property for IPv6 next to the IPv4 configuration code? And does IFF_NOARP actually affect IPv6 DAD? I thought that was the IFA_F_NODAD flag which we use elsewhere.
+ + nl_link_set_flags(nl_sock_ns, c->pasta_ifi, flags, flags); + + /* IPv4 configuration */ + rc = pasta_conf_addrs(c, AF_INET, c->ifi4, c->ip4.no_copy_addrs); + if (c->ifi4 == -1 && rc == -ENOTSUP) { + warn("IPv4 not supported, disabling"); + c->ifi4 = 0; + } else if (rc < 0) { + die("Couldn't set IPv4 address(es): %s", strerror_(-rc)); + } else if (c->ifi4) { + rc = pasta_conf_routes(c, AF_INET, c->ifi4, + c->ip4.no_copy_routes); + if (rc < 0) + die("Couldn't set IPv4 route(s): %s", strerror_(-rc)); } -ipv6_done:
- proto_update_l2_buf(c->guest_mac); + if (!c->ifi6) + return; + + rc = nl_addr_get_ll(nl_sock_ns, c->pasta_ifi, + &c->ip6.addr_ll_seen); + if (rc < 0) + warn("Can't get LL address from namespace: %s", + strerror_(-rc)); + + rc = nl_addr_set_ll_nodad(nl_sock_ns, c->pasta_ifi); + if (rc < 0) + warn("Can't set nodad for LL in namespace: %s", + strerror_(-rc)); + + /* We dodged DAD: re-enable neighbour solicitations */ + nl_link_set_flags(nl_sock_ns, c->pasta_ifi, 0, IFF_NOARP); + + rc = pasta_conf_addrs(c, AF_INET6, c->ifi6, c->ip6.no_copy_addrs); + if (c->ifi6 == -1 && rc == -ENOTSUP) { + warn("IPv6 not supported, disabling"); + c->ifi6 = 0; + } else if (rc < 0) { + die("Couldn't set IPv6 address(es): %s", strerror_(-rc)); + } else { + rc = pasta_conf_routes(c, AF_INET6, c->ifi6, + c->ip6.no_copy_routes); + if (rc < 0) + die("Couldn't set IPv6 route(s): %s", strerror_(-rc)); + } }
/** -- 2.52.0
-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson