From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id 8BC265A026D for ; Thu, 3 Aug 2023 00:47:37 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1691016456; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=zDnYXQYcEmpSWnRGET4keaM5KMP4OLWtyhTKq4sKGAw=; b=ig/tA1H3Q/qnCYjfvF853zqcIRWFBnkIl99mFhYzjnuDT0TTcc0W1L245Opqr0MhGJ+TbS VJuy33PQX0vWUvx8b7y7zLQllQCB39AZjZL7zqrI4IYWonMMeNAmXRx7LaSKMkWButDwaD LYTpWBzTLZO0sa+oiZR9Vz8dMANncX8= Received: from mimecast-mx02.redhat.com (66.187.233.73 [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-479-aGjJoauVOLq1L0ClLBPhRA-1; Wed, 02 Aug 2023 18:47:33 -0400 X-MC-Unique: aGjJoauVOLq1L0ClLBPhRA-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 17DCE1C0782E; Wed, 2 Aug 2023 22:47:33 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.24]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 754944087C74; Wed, 2 Aug 2023 22:47:32 +0000 (UTC) Date: Thu, 3 Aug 2023 00:47:29 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 01/17] netlink: Split up functionality if nl_link() Message-ID: <20230803004729.03ca0e36@elisabeth> In-Reply-To: <20230724060936.952659-2-david@gibson.dropbear.id.au> References: <20230724060936.952659-1-david@gibson.dropbear.id.au> <20230724060936.952659-2-david@gibson.dropbear.id.au> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: TJFZ3NSNHLGFC66ZES7C2DTEKEAEMCQY X-Message-ID-Hash: TJFZ3NSNHLGFC66ZES7C2DTEKEAEMCQY X-MailFrom: sbrivio@redhat.com 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: passt-dev@passt.top 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: In the subject: s/if/of/. On Mon, 24 Jul 2023 16:09:20 +1000 David Gibson wrote: > 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. Actually I don't find nl_link() *that* bad, but for consistency with the next patches this definitely makes sense. > 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. Actually, the intention wasn't to always retrieve the namespaced MAC address: I thought I'd do that only if we don't configure the interface, because we want NDP and DHCP to be "ready". But that's not really relevant... I guess yes, it's more consistent if we fetch it in any case (as long as we don't configure it). > > Signed-off-by: David Gibson > --- > conf.c | 4 +- > netlink.c | 143 +++++++++++++++++++++++++++++++----------------------- > netlink.h | 4 +- > pasta.c | 12 +++-- > 4 files changed, 96 insertions(+), 67 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..4b1f75e 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), I don't think there's a practical issue with this, but there were two reasons why I used NLMSG_LENGTH(sizeof(struct ifinfomsg)) instead: - NLMSG_LENGTH() aligns to 4 bytes, not to whatever architecture-dependent alignment we might have: the message might actually be smaller - I see that this works with gcc and clang, but, strictly speaking, is the size of the struct known "before" (sequence-point-wise) we're done initialising it? I have a very vague memory of this not working with gcc 2.9 or suchlike -- which is not a problem, as long as our new friend C11 actually supports this (but I'm not entirely sure). Then, in 9/17, NLMSG_LENGTH() could be conveniently used by nl_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) > - return; > > - if ((n = nl_req(ns, buf, &req, req.nlh.nlmsg_len)) < 0) > + n = nl_req(ns, buf, &req, sizeof(req)); > + if (n < 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), Same here. > + .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), And here. > + .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); Pre-existing issue I see now: we should probably use NLMSG_LENGTH() here, in any case. > + > + 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..3b5537d 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 guest MAC */ I know it's called mac_guest, my bad, but what about "MAC address in the target namespace"? -- Stefano