From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 954B65A026F for ; Thu, 3 Aug 2023 09:20:05 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1691047199; bh=JpHwCrqE+Z26msBiqM7RGe6R9a+UyYZDzkuE7YBR0hE=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=KAanox6MUls26U13BrrsIQSibN0tXJuBlqjESfXR6LIht3BcMU+ZjM1COM/CrNYvB 4BnFJfjuSX+M52OtWCRV71buP7RaJak366BVZcOxsEazGjrunFU2LMfQnck3tovcrd w+Bk+siE0saiFaC0T3FlnFtihxf17I8H0dQVJd5M= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4RGgGR5GWDz4wb5; Thu, 3 Aug 2023 17:19:59 +1000 (AEST) From: David Gibson To: passt-dev@passt.top, Stefano Brivio Subject: [PATCH v2 01/17] netlink: Split up functionality of nl_link() Date: Thu, 3 Aug 2023 17:19:40 +1000 Message-ID: <20230803071956.3198452-2-david@gibson.dropbear.id.au> X-Mailer: git-send-email 2.41.0 In-Reply-To: <20230803071956.3198452-1-david@gibson.dropbear.id.au> References: <20230803071956.3198452-1-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Message-ID-Hash: OENYDX2AKZH5RF4OWPX364VVGYDCKITX X-Message-ID-Hash: OENYDX2AKZH5RF4OWPX364VVGYDCKITX X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: David Gibson X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: nl_link() performs a number of functions: it can bring links up, set MAC address and MTU and also retrieve the existing MAC. This makes for a small number of lines of code, but high conceptual complexity: it's quite hard to follow what's going on both in nl_link() itself and it's also not very obvious which function its callers are intending to use. Clarify this, by splitting nl_link() into nl_link_up(), nl_link_set_mac(), and nl_link_get_mac(). The first brings up a link, optionally setting the MTU, the others get or set the MAC address. This fixes an arguable bug in pasta_ns_conf(): it looks as though that was intended to retrieve the guest MAC whether or not c->pasta_conf_ns is set. However, it only actually does so in the !c->pasta_conf_ns case: the fact that we set up==1 means we would only ever set, never get, the MAC in the nl_link() call in the other path. We get away with this because the MAC will quickly be discovered once we receive packets on the tap interface. Still, it's neater to always get the MAC address here. Signed-off-by: David Gibson --- conf.c | 4 +- netlink.c | 141 +++++++++++++++++++++++++++++++----------------------- netlink.h | 4 +- pasta.c | 12 +++-- 4 files changed, 95 insertions(+), 66 deletions(-) diff --git a/conf.c b/conf.c index 78eaf2d..2ff9e2a 100644 --- a/conf.c +++ b/conf.c @@ -670,7 +670,7 @@ static unsigned int conf_ip4(unsigned int ifi, memcpy(&ip4->addr_seen, &ip4->addr, sizeof(ip4->addr_seen)); if (MAC_IS_ZERO(mac)) - nl_link(0, ifi, mac, 0, 0); + nl_link_get_mac(0, ifi, mac); if (IN4_IS_ADDR_UNSPECIFIED(&ip4->addr) || MAC_IS_ZERO(mac)) @@ -711,7 +711,7 @@ static unsigned int conf_ip6(unsigned int ifi, memcpy(&ip6->addr_ll_seen, &ip6->addr_ll, sizeof(ip6->addr_ll)); if (MAC_IS_ZERO(mac)) - nl_link(0, ifi, mac, 0, 0); + nl_link_get_mac(0, ifi, mac); if (IN6_IS_ADDR_UNSPECIFIED(&ip6->addr) || IN6_IS_ADDR_UNSPECIFIED(&ip6->addr_ll) || diff --git a/netlink.c b/netlink.c index e15e23f..a41dc63 100644 --- a/netlink.c +++ b/netlink.c @@ -486,83 +486,44 @@ next: } /** - * nl_link() - Get/set link attributes + * nl_link_get_mac() - Get link MAC address * @ns: Use netlink socket in namespace * @ifi: Interface index - * @mac: MAC address to fill, if passed as zero, to set otherwise - * @up: If set, bring up the link - * @mtu: If non-zero, set interface MTU + * @mac: Fill with current MAC address */ -void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu) +void nl_link_get_mac(int ns, unsigned int ifi, void *mac) { - int change = !MAC_IS_ZERO(mac) || up || mtu; struct req_t { struct nlmsghdr nlh; struct ifinfomsg ifm; - struct rtattr rta; - union { - unsigned char mac[ETH_ALEN]; - struct { - unsigned int mtu; - } mtu; - } set; } req = { - .nlh.nlmsg_type = change ? RTM_NEWLINK : RTM_GETLINK, - .nlh.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)), - .nlh.nlmsg_flags = NLM_F_REQUEST | (change ? NLM_F_ACK : 0), + .nlh.nlmsg_type = RTM_GETLINK, + .nlh.nlmsg_len = sizeof(req), + .nlh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK, .nlh.nlmsg_seq = nl_seq++, .ifm.ifi_family = AF_UNSPEC, .ifm.ifi_index = ifi, - .ifm.ifi_flags = up ? IFF_UP : 0, - .ifm.ifi_change = up ? IFF_UP : 0, }; - struct ifinfomsg *ifm; struct nlmsghdr *nh; - struct rtattr *rta; char buf[NLBUFSIZ]; ssize_t n; - size_t na; - - if (!MAC_IS_ZERO(mac)) { - req.nlh.nlmsg_len = sizeof(req); - memcpy(req.set.mac, mac, ETH_ALEN); - req.rta.rta_type = IFLA_ADDRESS; - req.rta.rta_len = RTA_LENGTH(ETH_ALEN); - if (nl_req(ns, buf, &req, req.nlh.nlmsg_len) < 0) - return; - up = 0; - } - - if (mtu) { - req.nlh.nlmsg_len = offsetof(struct req_t, set.mtu) - + sizeof(req.set.mtu); - req.set.mtu.mtu = mtu; - req.rta.rta_type = IFLA_MTU; - req.rta.rta_len = RTA_LENGTH(sizeof(unsigned int)); - if (nl_req(ns, buf, &req, req.nlh.nlmsg_len) < 0) - return; - - up = 0; - } - - if (up && nl_req(ns, buf, &req, req.nlh.nlmsg_len) < 0) - return; - - if (change) + n = nl_req(ns, buf, &req, sizeof(req)); + if (n < 0) return; - if ((n = nl_req(ns, buf, &req, req.nlh.nlmsg_len)) < 0) - return; + for (nh = (struct nlmsghdr *)buf; + NLMSG_OK(nh, n) && nh->nlmsg_type != NLMSG_DONE; + nh = NLMSG_NEXT(nh, n)) { + struct ifinfomsg *ifm = (struct ifinfomsg *)NLMSG_DATA(nh); + struct rtattr *rta; + size_t na; - nh = (struct nlmsghdr *)buf; - for ( ; NLMSG_OK(nh, n); nh = NLMSG_NEXT(nh, n)) { if (nh->nlmsg_type != RTM_NEWLINK) - goto next; - - ifm = (struct ifinfomsg *)NLMSG_DATA(nh); + continue; - for (rta = IFLA_RTA(ifm), na = RTM_PAYLOAD(nh); RTA_OK(rta, na); + for (rta = IFLA_RTA(ifm), na = RTM_PAYLOAD(nh); + RTA_OK(rta, na); rta = RTA_NEXT(rta, na)) { if (rta->rta_type != IFLA_ADDRESS) continue; @@ -570,8 +531,70 @@ void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu) memcpy(mac, RTA_DATA(rta), ETH_ALEN); break; } -next: - if (nh->nlmsg_type == NLMSG_DONE) - break; } } + +/** + * nl_link_set_mac() - Set link MAC address + * @ns: Use netlink socket in namespace + * @ifi: Interface index + * @mac: MAC address to set + */ +void nl_link_set_mac(int ns, unsigned int ifi, void *mac) +{ + struct req_t { + struct nlmsghdr nlh; + struct ifinfomsg ifm; + struct rtattr rta; + unsigned char mac[ETH_ALEN]; + } req = { + .nlh.nlmsg_type = RTM_NEWLINK, + .nlh.nlmsg_len = sizeof(req), + .nlh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK, + .nlh.nlmsg_seq = nl_seq++, + .ifm.ifi_family = AF_UNSPEC, + .ifm.ifi_index = ifi, + .rta.rta_type = IFLA_ADDRESS, + .rta.rta_len = RTA_LENGTH(ETH_ALEN), + }; + char buf[NLBUFSIZ]; + + memcpy(req.mac, mac, ETH_ALEN); + + nl_req(ns, buf, &req, sizeof(req)); +} + +/** + * nl_link_up() - Bring link up + * @ns: Use netlink socket in namespace + * @ifi: Interface index + * @mtu: If non-zero, set interface MTU + */ +void nl_link_up(int ns, unsigned int ifi, int mtu) +{ + struct req_t { + struct nlmsghdr nlh; + struct ifinfomsg ifm; + struct rtattr rta; + unsigned int mtu; + } req = { + .nlh.nlmsg_type = RTM_NEWLINK, + .nlh.nlmsg_len = sizeof(req), + .nlh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK, + .nlh.nlmsg_seq = nl_seq++, + .ifm.ifi_family = AF_UNSPEC, + .ifm.ifi_index = ifi, + .ifm.ifi_flags = IFF_UP, + .ifm.ifi_change = IFF_UP, + .rta.rta_type = IFLA_MTU, + .rta.rta_len = RTA_LENGTH(sizeof(unsigned int)), + .mtu = mtu, + }; + char buf[NLBUFSIZ]; + + if (!mtu) + /* Shorten request to drop MTU attribute */ + req.nlh.nlmsg_len = offsetof(struct req_t, rta); + + nl_req(ns, buf, &req, req.nlh.nlmsg_len); +} diff --git a/netlink.h b/netlink.h index cd0e666..980ac44 100644 --- a/netlink.h +++ b/netlink.h @@ -18,6 +18,8 @@ void nl_route(enum nl_op op, unsigned int ifi, unsigned int ifi_ns, sa_family_t af, void *gw); void nl_addr(enum nl_op op, unsigned int ifi, unsigned int ifi_ns, sa_family_t af, void *addr, int *prefix_len, void *addr_l); -void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu); +void nl_link_get_mac(int ns, unsigned int ifi, void *mac); +void nl_link_set_mac(int ns, unsigned int ifi, void *mac); +void nl_link_up(int ns, unsigned int ifi, int mtu); #endif /* NETLINK_H */ diff --git a/pasta.c b/pasta.c index 8c85546..cb509dd 100644 --- a/pasta.c +++ b/pasta.c @@ -272,13 +272,19 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid, */ void pasta_ns_conf(struct ctx *c) { - nl_link(1, 1 /* lo */, MAC_ZERO, 1, 0); + nl_link_up(1, 1 /* lo */, 0); + + /* Get or set MAC in target namespace */ + if (MAC_IS_ZERO(c->mac_guest)) + nl_link_get_mac(1, c->pasta_ifi, c->mac_guest); + else + nl_link_set_mac(1, c->pasta_ifi, c->mac_guest); if (c->pasta_conf_ns) { enum nl_op op_routes = c->no_copy_routes ? NL_SET : NL_DUP; enum nl_op op_addrs = c->no_copy_addrs ? NL_SET : NL_DUP; - nl_link(1, c->pasta_ifi, c->mac_guest, 1, c->mtu); + nl_link_up(1, c->pasta_ifi, c->mtu); if (c->ifi4) { nl_addr(op_addrs, c->ifi4, c->pasta_ifi, AF_INET, @@ -294,8 +300,6 @@ void pasta_ns_conf(struct ctx *c) nl_route(op_routes, c->ifi6, c->pasta_ifi, AF_INET6, &c->ip6.gw); } - } else { - nl_link(1, c->pasta_ifi, c->mac_guest, 0, 0); } proto_update_l2_buf(c->mac_guest, NULL, NULL); -- 2.41.0