Split off init_ifaces from update_iface. init_ifaces discovers the

state of the machine on startup using ioctl(2) and getifaddrs(3).
We can then update this state with information provided by route
messages. We still need getifaddrs(3) to check if the layer 2 address
has changed.

This simplifies error handling (what should we do if ioctl(2) fails?),
reduces kernel round trips (no need to ask the kernel again for
information RTM_IFINFO provided already) and prevents a theoretical
race between RTM_IFINFO and getaddrinfo(3).

In a fast link state UP -> DOWN -> UP transition RTM_IFINFO informs us
that the link went down but we were not using this information but
rather looked at getifaddrs(3) information which might see the link as
already up again. We would then do nothing while we should try to get
a new lease.

By storing all interface information in the frontend process we can
skip imsgs to the engine process if we get an RTM_IFINFO without
relevant changes for us.

1 files changed, 152 insertions, 76 deletions

diff --git a/sbin/dhcpleased/frontend.c b/sbin/dhcpleased/frontend.c
index fbbeb63fbb5..57e60187b92 100644
--- a/sbin/dhcpleased/frontend.c
+++ b/sbin/dhcpleased/frontend.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: frontend.c,v 1.5 2021/03/16 17:39:15 florian Exp $	*/
+/*	$OpenBSD: frontend.c,v 1.6 2021/03/17 15:24:04 florian Exp $	*/
 
 /*
  * Copyright (c) 2017, 2021 Florian Obser <florian@openbsd.org>
@@ -66,9 +66,7 @@ struct bpf_ev {
 struct iface           {
 	LIST_ENTRY(iface)	 entries;
 	struct bpf_ev		 bpfev;
-	struct ether_addr	 hw_address;
-	uint32_t		 if_index;
-	int			 rdomain;
+	struct imsg_ifinfo	 ifinfo;
 	int			 send_discover;
 	uint32_t		 xid;
 	struct in_addr		 requested_ip;
@@ -79,8 +77,9 @@ struct iface           {
 
 __dead void	 frontend_shutdown(void);
 void		 frontend_sig_handler(int, short, void *);
-void		 update_iface(uint32_t, char*);
+void		 update_iface(struct if_msghdr *);
 void		 frontend_startup(void);
+void		 init_ifaces(void);
 void		 route_receive(int, short, void *);
 void		 handle_route_message(struct rt_msghdr *, struct sockaddr **);
 void		 get_rtaddrs(int, struct sockaddr *, struct sockaddr **);
@@ -463,26 +462,49 @@ get_xflags(char *if_name)
 }
 
 void
-update_iface(uint32_t if_index, char* if_name)
+update_iface(struct if_msghdr *ifm)
 {
 	struct iface		*iface;
-	struct imsg_ifinfo	 imsg_ifinfo;
+	struct imsg_ifinfo	 ifinfo;
 	struct ifaddrs		*ifap, *ifa;
-	struct sockaddr_dl	*sdl;
-	int			 flags, xflags;
+	uint32_t		 if_index;
+	int			 flags, xflags, found = 0;
+	char			 ifnamebuf[IF_NAMESIZE], *if_name;
 
-	if ((flags = get_flags(if_name)) == -1 || (xflags =
-	    get_xflags(if_name)) == -1)
-		return;
+	if_index = ifm->ifm_index;
+
+	flags = ifm->ifm_flags;
+	xflags = ifm->ifm_xflags;
 
-	if (!(xflags & IFXF_AUTOCONF4))
+	iface = get_iface_by_id(if_index);
+	if_name = if_indextoname(if_index, ifnamebuf);
+
+	if (if_name == NULL) {
+		if (iface != NULL) {
+			log_debug("interface with idx %d removed", if_index);
+			frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0, 0,
+			    &if_index, sizeof(if_index));
+			remove_iface(if_index);
+		}
 		return;
+	}
 
-	memset(&imsg_ifinfo, 0, sizeof(imsg_ifinfo));
+	if (!(xflags & IFXF_AUTOCONF4)) {
+		if (iface != NULL) {
+			log_info("Removed autoconf flag from %s", if_name);
+			frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0, 0,
+			    &if_index, sizeof(if_index));
+			remove_iface(if_index);
+		}
+		return;
+	}
 
-	imsg_ifinfo.if_index = if_index;
-	imsg_ifinfo.running = (flags & (IFF_UP | IFF_RUNNING)) == (IFF_UP |
-	    IFF_RUNNING);
+	memset(&ifinfo, 0, sizeof(ifinfo));
+	ifinfo.if_index = if_index;
+	ifinfo.link_state = ifm->ifm_data.ifi_link_state;
+	ifinfo.rdomain = ifm->ifm_tableid;
+	ifinfo.running = (flags & (IFF_UP | IFF_RUNNING)) ==
+	    (IFF_UP | IFF_RUNNING);
 
 	if (getifaddrs(&ifap) != 0)
 		fatal("getifaddrs");
@@ -495,17 +517,15 @@ update_iface(uint32_t if_index, char* if_name)
 
 		switch(ifa->ifa_addr->sa_family) {
 		case AF_LINK: {
-			struct if_data	*if_data;
+			struct sockaddr_dl	*sdl;
 
-			if_data = (struct if_data *)ifa->ifa_data;
-			imsg_ifinfo.link_state = if_data->ifi_link_state;
-			imsg_ifinfo.rdomain = if_data->ifi_rdomain;
 			sdl = (struct sockaddr_dl *)ifa->ifa_addr;
 			if (sdl->sdl_type != IFT_ETHER ||
 			    sdl->sdl_alen != ETHER_ADDR_LEN)
 				continue;
-			memcpy(imsg_ifinfo.hw_address.ether_addr_octet,
-			    LLADDR(sdl), ETHER_ADDR_LEN);
+			memcpy(ifinfo.hw_address.ether_addr_octet, LLADDR(sdl),
+			    ETHER_ADDR_LEN);
+			found = 1;
 			goto out;
 		}
 		default:
@@ -515,52 +535,127 @@ update_iface(uint32_t if_index, char* if_name)
  out:
 	freeifaddrs(ifap);
 
-	iface = get_iface_by_id(if_index);
-
-	if (iface != NULL) {
-		if (iface->rdomain != imsg_ifinfo.rdomain) {
-			iface->rdomain = imsg_ifinfo.rdomain;
-			if (iface->udpsock != -1) {
-				close(iface->udpsock);
-				iface->udpsock = -1;
-			}
+	if (!found) {
+		log_warnx("Could not find AF_LINK address for %s.", if_name);
+		if (iface != NULL) {
+			frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0, 0,
+			    &if_index, sizeof(if_index));
+			remove_iface(if_index);
 		}
-	} else {
+		return;
+	}
+
+	if (iface == NULL) {
 		if ((iface = calloc(1, sizeof(*iface))) == NULL)
 			fatal("calloc");
-		iface->if_index = if_index;
-		iface->rdomain = imsg_ifinfo.rdomain;
 		iface->udpsock = -1;
 		LIST_INSERT_HEAD(&interfaces, iface, entries);
 		frontend_imsg_compose_main(IMSG_OPEN_BPFSOCK, 0,
 		    &if_index, sizeof(if_index));
+	} else {
+		if (iface->ifinfo.rdomain != ifinfo.rdomain &&
+		    iface->udpsock != -1) {
+			close(iface->udpsock);
+			iface->udpsock = -1;
+		}
 	}
 
-	memcpy(&iface->hw_address, &imsg_ifinfo.hw_address,
-	    sizeof(iface->hw_address));
-
-	frontend_imsg_compose_main(IMSG_UPDATE_IF, 0, &imsg_ifinfo,
-	    sizeof(imsg_ifinfo));
+	if (memcmp(&iface->ifinfo, &ifinfo, sizeof(iface->ifinfo)) != 0) {
+		memcpy(&iface->ifinfo, &ifinfo, sizeof(iface->ifinfo));
+		frontend_imsg_compose_main(IMSG_UPDATE_IF, 0, &iface->ifinfo,
+		    sizeof(iface->ifinfo));
+	}
 }
 
 void
 frontend_startup(void)
 {
-	struct if_nameindex	*ifnidxp, *ifnidx;
-
 	if (!event_initialized(&ev_route))
 		fatalx("%s: did not receive a route socket from the main "
 		    "process", __func__);
 
+	init_ifaces();
 	event_add(&ev_route, NULL);
+}
+
+void
+init_ifaces(void)
+{
+	struct iface		*iface;
+	struct imsg_ifinfo	 ifinfo;
+	struct if_nameindex	*ifnidxp, *ifnidx;
+	struct ifaddrs		*ifap, *ifa;
+	uint32_t		 if_index;
+	int			 flags, xflags;
+	char			*if_name;
 
 	if ((ifnidxp = if_nameindex()) == NULL)
 		fatalx("if_nameindex");
 
-	for(ifnidx = ifnidxp; ifnidx->if_index !=0 && ifnidx->if_name != NULL;
-	    ifnidx++)
-		update_iface(ifnidx->if_index, ifnidx->if_name);
+	if (getifaddrs(&ifap) != 0)
+		fatal("getifaddrs");
+
+	for(ifnidx = ifnidxp; ifnidx->if_index != 0 && ifnidx->if_name != NULL;
+	    ifnidx++) {
+		if_index = ifnidx->if_index;
+		if_name = ifnidx->if_name;
+		if ((flags = get_flags(if_name)) == -1)
+			continue;
+		if((xflags = get_xflags(if_name)) == -1)
+			continue;
+		if (!(xflags & IFXF_AUTOCONF4))
+			continue;
+
+		memset(&ifinfo, 0, sizeof(ifinfo));
+		ifinfo.if_index = if_index;
+		ifinfo.link_state = -1;
+		ifinfo.running = (flags & (IFF_UP | IFF_RUNNING)) ==
+		    (IFF_UP | IFF_RUNNING);
+
+		for (ifa = ifap; ifa != NULL; ifa = ifa->ifa_next) {
+			if (strcmp(if_name, ifa->ifa_name) != 0)
+				continue;
+			if (ifa->ifa_addr == NULL)
+				continue;
+
+			switch(ifa->ifa_addr->sa_family) {
+			case AF_LINK: {
+				struct if_data		*if_data;
+				struct sockaddr_dl	*sdl;
+
+				sdl = (struct sockaddr_dl *)ifa->ifa_addr;
+				if (sdl->sdl_type != IFT_ETHER ||
+				    sdl->sdl_alen != ETHER_ADDR_LEN)
+					continue;
+				memcpy(ifinfo.hw_address.ether_addr_octet,
+				    LLADDR(sdl), ETHER_ADDR_LEN);
+
+				if_data = (struct if_data *)ifa->ifa_data;
+				ifinfo.link_state = if_data->ifi_link_state;
+				ifinfo.rdomain = if_data->ifi_rdomain;
+				goto out;
+			}
+			default:
+				break;
+			}
+		}
+ out:
+		if (ifinfo.link_state == -1)
+			/* no AF_LINK found */
+			continue;
+
+		if ((iface = calloc(1, sizeof(*iface))) == NULL)
+			fatal("calloc");
+		iface->udpsock = -1;
+		memcpy(&iface->ifinfo, &ifinfo, sizeof(iface->ifinfo));
+		LIST_INSERT_HEAD(&interfaces, iface, entries);
+		frontend_imsg_compose_main(IMSG_OPEN_BPFSOCK, 0,
+		    &if_index, sizeof(if_index));
+		frontend_imsg_compose_main(IMSG_UPDATE_IF, 0, &iface->ifinfo,
+		    sizeof(iface->ifinfo));
+	}
 
+	freeifaddrs(ifap);
 	if_freenameindex(ifnidxp);
 }
 
@@ -607,31 +702,11 @@ void
 handle_route_message(struct rt_msghdr *rtm, struct sockaddr **rti_info)
 {
 	struct if_msghdr		*ifm;
-	int				 xflags, if_index;
-	char				 ifnamebuf[IF_NAMESIZE], *if_name;
 
 	switch (rtm->rtm_type) {
 	case RTM_IFINFO:
 		ifm = (struct if_msghdr *)rtm;
-		if_name = if_indextoname(ifm->ifm_index, ifnamebuf);
-		if (if_name == NULL) {
-			log_debug("RTM_IFINFO: lost if %d", ifm->ifm_index);
-			if_index = ifm->ifm_index;
-			frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0, 0,
-			    &if_index, sizeof(if_index));
-			remove_iface(if_index);
-		} else {
-			xflags = get_xflags(if_name);
-			if (xflags == -1 || !(xflags & IFXF_AUTOCONF4)) {
-				log_debug("RTM_IFINFO: %s(%d) no(longer) "
-				   "autoconf4", if_name, ifm->ifm_index);
-				if_index = ifm->ifm_index;
-				frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0,
-				    0, &if_index, sizeof(if_index));
-			} else {
-				update_iface(ifm->ifm_index, if_name);
-			}
-		}
+		update_iface(ifm);
 		break;
 	case RTM_PROPOSAL:
 		if (rtm->rtm_priority == RTP_PROPOSAL_SOLICIT) {
@@ -644,7 +719,6 @@ handle_route_message(struct rt_msghdr *rtm, struct sockaddr **rti_info)
 		log_debug("unexpected RTM: %d", rtm->rtm_type);
 		break;
 	}
-
 }
 
 #define ROUNDUP(a) \
@@ -685,7 +759,7 @@ bpf_receive(int fd, short events, void *arg)
 		fatal("%s len == 0", __func__);
 
 	memset(&imsg_dhcp, 0, sizeof(imsg_dhcp));
-	imsg_dhcp.if_index = iface->if_index;
+	imsg_dhcp.if_index = iface->ifinfo.if_index;
 
 	rem = len;
 	p = iface->bpfev.buf;
@@ -803,11 +877,11 @@ send_discover(struct iface *iface)
 	}
 	iface->send_discover = 0;
 
-	if_name = if_indextoname(iface->if_index, ifnamebuf);
+	if_name = if_indextoname(iface->ifinfo.if_index, ifnamebuf);
 	log_debug("DHCPDISCOVER on %s", if_name == NULL ? "?" : if_name);
 
-	pkt_len = build_packet(DHCPDISCOVER, iface->xid, &iface->hw_address,
-	    &iface->requested_ip, NULL);
+	pkt_len = build_packet(DHCPDISCOVER, iface->xid,
+	    &iface->ifinfo.hw_address, &iface->requested_ip, NULL);
 	bpf_send_packet(iface, dhcp_packet, pkt_len);
 }
 
@@ -817,11 +891,12 @@ send_request(struct iface *iface)
 	ssize_t	 pkt_len;
 	char	 ifnamebuf[IF_NAMESIZE], *if_name;
 
-	if_name = if_indextoname(iface->if_index, ifnamebuf);
+	if_name = if_indextoname(iface->ifinfo.if_index, ifnamebuf);
 	log_debug("DHCPREQUEST on %s", if_name == NULL ? "?" : if_name);
 
-	pkt_len = build_packet(DHCPREQUEST, iface->xid, &iface->hw_address,
-	    &iface->requested_ip, &iface->server_identifier);
+	pkt_len = build_packet(DHCPREQUEST, iface->xid,
+	    &iface->ifinfo.hw_address, &iface->requested_ip,
+	    &iface->server_identifier);
 	if (iface->dhcp_server.s_addr != INADDR_ANY)
 		udp_send_packet(iface, dhcp_packet, pkt_len);
 	else
@@ -854,7 +929,8 @@ bpf_send_packet(struct iface *iface, uint8_t *packet, ssize_t len)
 	int			 iovcnt = 0, i;
 
 	memset(eh.ether_dhost, 0xff, sizeof(eh.ether_dhost));
-	memcpy(eh.ether_shost, &iface->hw_address, sizeof(eh.ether_dhost));
+	memcpy(eh.ether_shost, &iface->ifinfo.hw_address,
+	    sizeof(eh.ether_dhost));
 	eh.ether_type = htons(ETHERTYPE_IP);
 	iov[0].iov_base = &eh;
 	iov[0].iov_len = sizeof(eh);
@@ -912,7 +988,7 @@ get_iface_by_id(uint32_t if_index)
 	struct iface	*iface;
 
 	LIST_FOREACH (iface, &interfaces, entries) {
-		if (iface->if_index == if_index)
+		if (iface->ifinfo.if_index == if_index)
 			return (iface);
 	}