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]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.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.-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 smallerNot really sure what to do about this.-- 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- 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).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.Then, in 9/17, NLMSG_LENGTH() could be conveniently used by nl_req().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?+ .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.Good idea, changed.+ + 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"?