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 510D25A026F for ; Thu, 3 Aug 2023 07:26:27 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1691040382; bh=u4Jy3AP9FA+bpNKSZSnBf+1DKln6lnTA7tXq6CaknBk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=BExbo2QbHpxO16MDTWN972fcqhNVVJKGIe8ycMt8I4AuQ67IDjO4MsrWWUX78UJyn zVgB1veMatGC68IGFknLBH4C7U0npgmqcC5v1UfZNsC0hKoqnL7eMJ06IuM/N+TLpU DB16K8YKijWfuCgMeHtnFIbFWZYuUN6DdR51F5kU= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4RGclL1fpZz4wbj; Thu, 3 Aug 2023 15:26:22 +1000 (AEST) Date: Thu, 3 Aug 2023 14:29:28 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 01/17] netlink: Split up functionality if nl_link() Message-ID: References: <20230724060936.952659-1-david@gibson.dropbear.id.au> <20230724060936.952659-2-david@gibson.dropbear.id.au> <20230803004729.03ca0e36@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="UAHu7xCZtMxPlXip" Content-Disposition: inline In-Reply-To: Message-ID-Hash: U5IL5K45T2W5LYFSSTFHLLPFGROFSFXA X-Message-ID-Hash: U5IL5K45T2W5LYFSSTFHLLPFGROFSFXA 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: 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: --UAHu7xCZtMxPlXip Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Aug 03, 2023 at 12:09:16PM +1000, David Gibson wrote: > On Thu, Aug 03, 2023 at 12:47:29AM +0200, Stefano Brivio wrote: [snip] > > > -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 =3D !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 =3D { > > > - .nlh.nlmsg_type =3D change ? RTM_NEWLINK : RTM_GETLINK, > > > - .nlh.nlmsg_len =3D NLMSG_LENGTH(sizeof(struct ifinfomsg)), > > > - .nlh.nlmsg_flags =3D NLM_F_REQUEST | (change ? NLM_F_ACK : 0), > > > + .nlh.nlmsg_type =3D RTM_GETLINK, > > > + .nlh.nlmsg_len =3D sizeof(req), > >=20 > > 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: > >=20 > > - NLMSG_LENGTH() aligns to 4 bytes, not to whatever > > architecture-dependent alignment we might have: the message might > > actually be smaller >=20 > Oof... so. On the one hand, I see the issue; if these are different, > I'm not sure what the effect will be. On the other hand, if we use > NLMSG_LENGTH and it *is* longer than the structure size, we'll be > saying that this message is longer than the datagram containing it. > I'm not sure what the effect of that will be either. Duh, sorry, I realized I had this backwards. NLSMSG_LENGTH() is the non-aligned length, sizeof() may include alignment. I'll rework based on that understanding. > Not really sure what to do about this. >=20 > > - 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). >=20 > I'm pretty sure it's ok, regardless of C11 state. It's not really a > question of sequence points: those are about the ordering of run time > operations. Even though the structure is being defined inline, > determining it's size and layout will still happen at compile time, > whereas the initialization is obviously a runtime event. >=20 > > Then, in 9/17, NLMSG_LENGTH() could be conveniently used by nl_req(). > >=20 > > > + .nlh.nlmsg_flags =3D NLM_F_REQUEST | NLM_F_ACK, > > > .nlh.nlmsg_seq =3D nl_seq++, > > > .ifm.ifi_family =3D AF_UNSPEC, > > > .ifm.ifi_index =3D ifi, > > > - .ifm.ifi_flags =3D up ? IFF_UP : 0, > > > - .ifm.ifi_change =3D 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 =3D sizeof(req); > > > - memcpy(req.set.mac, mac, ETH_ALEN); > > > - req.rta.rta_type =3D IFLA_ADDRESS; > > > - req.rta.rta_len =3D RTA_LENGTH(ETH_ALEN); > > > - if (nl_req(ns, buf, &req, req.nlh.nlmsg_len) < 0) > > > - return; > > > - > > > - up =3D 0; > > > - } > > > - > > > - if (mtu) { > > > - req.nlh.nlmsg_len =3D offsetof(struct req_t, set.mtu) > > > - + sizeof(req.set.mtu); > > > - req.set.mtu.mtu =3D mtu; > > > - req.rta.rta_type =3D IFLA_MTU; > > > - req.rta.rta_len =3D RTA_LENGTH(sizeof(unsigned int)); > > > - if (nl_req(ns, buf, &req, req.nlh.nlmsg_len) < 0) > > > - return; > > > - > > > - up =3D 0; > > > - } > > > - > > > - if (up && nl_req(ns, buf, &req, req.nlh.nlmsg_len) < 0) > > > - return; > > > - > > > - if (change) > > > - return; > > > =20 > > > - if ((n =3D nl_req(ns, buf, &req, req.nlh.nlmsg_len)) < 0) > > > + n =3D nl_req(ns, buf, &req, sizeof(req)); > > > + if (n < 0) > > > return; > > > +=09 > > > + for (nh =3D (struct nlmsghdr *)buf; > > > + NLMSG_OK(nh, n) && nh->nlmsg_type !=3D NLMSG_DONE; > > > + nh =3D NLMSG_NEXT(nh, n)) { > > > + struct ifinfomsg *ifm =3D (struct ifinfomsg *)NLMSG_DATA(nh); > > > + struct rtattr *rta; > > > + size_t na; > > > =20 > > > - nh =3D (struct nlmsghdr *)buf; > > > - for ( ; NLMSG_OK(nh, n); nh =3D NLMSG_NEXT(nh, n)) { > > > if (nh->nlmsg_type !=3D RTM_NEWLINK) > > > - goto next; > > > - > > > - ifm =3D (struct ifinfomsg *)NLMSG_DATA(nh); > > > + continue; > > > =20 > > > - for (rta =3D IFLA_RTA(ifm), na =3D RTM_PAYLOAD(nh); RTA_OK(rta, na= ); > > > + for (rta =3D IFLA_RTA(ifm), na =3D RTM_PAYLOAD(nh); > > > + RTA_OK(rta, na); > > > rta =3D RTA_NEXT(rta, na)) { > > > if (rta->rta_type !=3D 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 =3D=3D 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 =3D { > > > + .nlh.nlmsg_type =3D RTM_NEWLINK, > > > + .nlh.nlmsg_len =3D sizeof(req), > >=20 > > Same here. > >=20 > > > + .nlh.nlmsg_flags =3D NLM_F_REQUEST | NLM_F_ACK, > > > + .nlh.nlmsg_seq =3D nl_seq++, > > > + .ifm.ifi_family =3D AF_UNSPEC, > > > + .ifm.ifi_index =3D ifi, > > > + .rta.rta_type =3D IFLA_ADDRESS, > > > + .rta.rta_len =3D 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 =3D { > > > + .nlh.nlmsg_type =3D RTM_NEWLINK, > > > + .nlh.nlmsg_len =3D sizeof(req), > >=20 > > And here. > >=20 > > > + .nlh.nlmsg_flags =3D NLM_F_REQUEST | NLM_F_ACK, > > > + .nlh.nlmsg_seq =3D nl_seq++, > > > + .ifm.ifi_family =3D AF_UNSPEC, > > > + .ifm.ifi_index =3D ifi, > > > + .ifm.ifi_flags =3D IFF_UP, > > > + .ifm.ifi_change =3D IFF_UP, > > > + .rta.rta_type =3D IFLA_MTU, > > > + .rta.rta_len =3D RTA_LENGTH(sizeof(unsigned int)), > > > + .mtu =3D mtu, > > > + }; > > > + char buf[NLBUFSIZ]; > > > + > > > + if (!mtu) > > > + /* Shorten request to drop MTU attribute */ > > > + req.nlh.nlmsg_len =3D offsetof(struct req_t, rta); > >=20 > > Pre-existing issue I see now: we should probably use NLMSG_LENGTH() > > here, in any case. >=20 > Well.. if NLMSG_LENGTH() really is different here, we're (by > definition) including some of req.rta in the message, which isn't our > intention. So.. if we trust the rta member to be aligned properly for > the case where we *do* include it, can't we also trust it for the case > where we don't? >=20 > > > + > > > + 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, unsi= gned 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); > > > =20 > > > #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, g= id_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 */ > >=20 > > I know it's called mac_guest, my bad, but what about "MAC address in > > the target namespace"? >=20 > Good idea, changed. >=20 --=20 David Gibson | 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 --UAHu7xCZtMxPlXip Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmTLLSEACgkQzQJF27ox 2GdaMw//Qo4fGHRJeRiEFiyMRp5rl5ndPReyPY3yQOgBJSkC7KcHhGsqYHkpoaVl x43sMCtfFbHZesGAWKR5eDRb/N+CqQOin83jtht0h9abTI9p+AewKWoCZ2J8ruGM EnuFP2mpjamf7of2Zd2wVZT7WZOElPeVIe2cpEmQ9s7uc9vkfWWxM8UpGAydS/a1 2g+UCAh9q/tG3hui4DxUTBpN5VXJy1h12x4iu2H9kLiQHSzB/KcscybFLdfUL0VJ +1UZWniq3rzi57GVuzthB0HIq0SX5exQHGC/lFQsU8EpyTZuESNeqQcd0nvwODoB E105Pv7fotUa7vpv/y9u50NuJSI9/C/eOXAOBKjEuBDxyuxAchuc8b5B+ZeWB2gf qyS5gOSgx0gUDV0Yfxu7NngaV7DcYn+XSXbnBBbXHUj+fSWq7zVY2bY79yYotf7+ M4NMCjztP0i+w1z827cNqfGw1PJW52naJ1mwTPpkeNs9iRirBXmFDzrqtJ1xzxNJ A3HW+hb9BN/r8SL6K5Lo6TDLqllJwoc9x6oTi/YqCz9bRHzNx0JnKAjNXRkq4eJj yIxgjVoStJbGFcr3YBAonZ9deNVxB8JGjoerrAIbhSQJbufVUL7hLXn6cGaEolPQ 9Y8dGJT2agTunWeWeAHkTPMQ5EMEYmv2zxLTytZsrCa71xGdCKE= =fKjs -----END PGP SIGNATURE----- --UAHu7xCZtMxPlXip--