Index | Thread | Search

From:
Florian Obser <florian@openbsd.org>
Subject:
dhcpleased(8): Reduce if_indextoname(3) usage.
To:
tech <tech@openbsd.org>
Date:
Sat, 13 Jul 2024 15:59:38 +0200

Download raw body.

Thread
There is no need to constantly ask the kernel for the interface name,
it is not going to change.

We pass the interface name from the frontend to the engine because
it is a fixed length string that is only used to find the
configuration for the interface and debugging output.

We do *not* pass the name to the main process because that would allow
the frontend to create arbitrary files inside of /var/db/dhcpleased/,
send arbitrary data to the SIOCAIFADDR et. al. ioctls, and mess with
the bpf socket.

OK?

diff --git dhcpleased.h dhcpleased.h
index 41749fc5031..6e39b1a7f00 100644
--- dhcpleased.h
+++ dhcpleased.h
@@ -270,6 +270,7 @@ struct dhcpleased_conf {
 
 struct imsg_ifinfo {
 	uint32_t		if_index;
+	char			if_name[IF_NAMESIZE];
 	int			rdomain;
 	int			running;
 	int			link_state;
diff --git engine.c engine.c
index 289dffdf20a..5f60f12b117 100644
--- engine.c
+++ engine.c
@@ -93,6 +93,7 @@ struct dhcpleased_iface {
 	struct event			 timer;
 	struct timeval			 timo;
 	uint32_t			 if_index;
+	char				 if_name[IF_NAMESIZE];
 	int				 rdomain;
 	int				 running;
 	struct ether_addr		 hw_address;
@@ -533,8 +534,6 @@ engine_dispatch_main(int fd, short event, void *bula)
 			struct dhcpleased_iface	*iface;
 			int			*ifaces;
 			int			 i, if_index;
-			char			*if_name;
-			char			 ifnamebuf[IF_NAMESIZE];
 
 			if (nconf == NULL)
 				fatalx("%s: IMSG_RECONF_END without "
@@ -544,12 +543,11 @@ engine_dispatch_main(int fd, short event, void *bula)
 			nconf = NULL;
 			for (i = 0; ifaces[i] != 0; i++) {
 				if_index = ifaces[i];
-				if_name = if_indextoname(if_index, ifnamebuf);
 				iface = get_dhcpleased_iface_by_id(if_index);
-				if (if_name == NULL || iface == NULL)
+				if (iface == NULL)
 					continue;
 				iface_conf = find_iface_conf(
-				    &engine_conf->iface_list, if_name);
+				    &engine_conf->iface_list, iface->if_name);
 				if (iface_conf == NULL)
 					continue;
 				if (iface_conf->ignore & IGN_DNS)
@@ -644,6 +642,9 @@ engine_update_iface(struct imsg_ifinfo *imsg_ifinfo)
 		iface->running = imsg_ifinfo->running;
 		iface->link_state = imsg_ifinfo->link_state;
 		iface->requested_ip.s_addr = INADDR_ANY;
+		memcpy(iface->if_name, imsg_ifinfo->if_name,
+		    sizeof(iface->if_name));
+		iface->if_name[sizeof(iface->if_name) - 1] = '\0';
 		memcpy(&iface->hw_address, &imsg_ifinfo->hw_address,
 		    sizeof(struct ether_addr));
 		LIST_INSERT_HEAD(&dhcpleased_interfaces, iface, entries);
@@ -743,15 +744,12 @@ parse_dhcp(struct dhcpleased_iface *iface, struct imsg_dhcp *dhcp)
 	char			 hbuf[INET_ADDRSTRLEN];
 	char			 domainname[4 * 255 + 1];
 	char			 hostname[4 * 255 + 1];
-	char			 ifnamebuf[IF_NAMESIZE], *if_name;
 
 	if (bcast_mac.ether_addr_octet[0] == 0)
 		memset(bcast_mac.ether_addr_octet, 0xff, ETHER_ADDR_LEN);
 
-	if_name = if_indextoname(iface->if_index, ifnamebuf);
-
 #ifndef SMALL
-	iface_conf = find_iface_conf(&engine_conf->iface_list, if_name);
+	iface_conf = find_iface_conf(&engine_conf->iface_list, iface->if_name);
 #endif /* SMALL*/
 
 	memset(hbuf_src, 0, sizeof(hbuf_src));
@@ -1214,8 +1212,8 @@ parse_dhcp(struct dhcpleased_iface *iface, struct imsg_dhcp *dhcp)
 		    from);
 
 	log_debug("%s on %s from %s/%s to %s/%s",
-	    dhcp_message_type2str(dhcp_message_type), if_name == NULL ? "?" :
-	    if_name, from, hbuf_src, to, hbuf_dst);
+	    dhcp_message_type2str(dhcp_message_type), iface->if_name, from,
+	    hbuf_src, to, hbuf_dst);
 
 	switch (dhcp_message_type) {
 	case DHCPOFFER:
@@ -1382,7 +1380,6 @@ state_transition(struct dhcpleased_iface *iface, enum if_state new_state)
 {
 	enum if_state	 old_state = iface->state;
 	struct timespec	 now, res;
-	char		 ifnamebuf[IF_NAMESIZE], *if_name;
 
 	iface->state = new_state;
 
@@ -1496,9 +1493,8 @@ state_transition(struct dhcpleased_iface *iface, enum if_state new_state)
 		}
 	}
 
-	if_name = if_indextoname(iface->if_index, ifnamebuf);
-	log_debug("%s[%s] %s -> %s, timo: %lld", __func__, if_name == NULL ?
-	    "?" : if_name, if_state_name[old_state], if_state_name[new_state],
+	log_debug("%s[%s] %s -> %s, timo: %lld", __func__, iface->if_name,
+	    if_state_name[old_state], if_state_name[new_state],
 	    iface->timo.tv_sec);
 
 	if (iface->timo.tv_sec == -1) {
@@ -1659,9 +1655,7 @@ void
 log_lease(struct dhcpleased_iface *iface, int deconfigure)
 {
 	char	 hbuf_lease[INET_ADDRSTRLEN], hbuf_server[INET_ADDRSTRLEN];
-	char	 ifnamebuf[IF_NAMESIZE], *if_name;
 
-	if_name = if_indextoname(iface->if_index, ifnamebuf);
 	inet_ntop(AF_INET, &iface->requested_ip, hbuf_lease,
 	    sizeof(hbuf_lease));
 	inet_ntop(AF_INET, &iface->server_identifier, hbuf_server,
@@ -1670,10 +1664,10 @@ log_lease(struct dhcpleased_iface *iface, int deconfigure)
 
 	if (deconfigure)
 		log_info("deleting %s from %s (lease from %s)", hbuf_lease,
-		    if_name == NULL ? "?" : if_name, hbuf_server);
+		    iface->if_name, hbuf_server);
 	else
 		log_info("adding %s to %s (lease from %s)", hbuf_lease,
-		    if_name == NULL ? "?" : if_name, hbuf_server);
+		    iface->if_name, hbuf_server);
 }
 
 void
@@ -1772,9 +1766,7 @@ log_rdns(struct dhcpleased_iface *iface, int withdraw)
 {
 	int	 i;
 	char	 hbuf_rdns[INET_ADDRSTRLEN], hbuf_server[INET_ADDRSTRLEN];
-	char	 ifnamebuf[IF_NAMESIZE], *if_name, *rdns_buf = NULL, *tmp_buf;
-
-	if_name = if_indextoname(iface->if_index, ifnamebuf);
+	char	*rdns_buf = NULL, *tmp_buf;
 
 	inet_ntop(AF_INET, &iface->server_identifier, hbuf_server,
 	    sizeof(hbuf_server));
@@ -1795,12 +1787,10 @@ log_rdns(struct dhcpleased_iface *iface, int withdraw)
 	if (rdns_buf != NULL) {
 		if (withdraw) {
 			log_info("deleting nameservers%s (lease from %s on %s)",
-			    rdns_buf, hbuf_server, if_name == NULL ? "?" :
-			    if_name);
+			    rdns_buf, hbuf_server, iface->if_name);
 		} else {
 			log_info("adding nameservers%s (lease from %s on %s)",
-			    rdns_buf, hbuf_server, if_name == NULL ? "?" :
-			    if_name);
+			    rdns_buf, hbuf_server, iface->if_name);
 		}
 		free(rdns_buf);
 	}
diff --git frontend.c frontend.c
index fdc4ae8ca16..a3717108f21 100644
--- frontend.c
+++ frontend.c
@@ -408,7 +408,6 @@ frontend_dispatch_main(int fd, short event, void *bula)
 		case IMSG_RECONF_END: {
 			int	 i;
 			int	*ifaces;
-			char	 ifnamebuf[IF_NAMESIZE], *if_name;
 
 			if (nconf == NULL)
 				fatalx("%s: IMSG_RECONF_END without "
@@ -419,9 +418,6 @@ frontend_dispatch_main(int fd, short event, void *bula)
 			nconf = NULL;
 			for (i = 0; ifaces[i] != 0; i++) {
 				if_index = ifaces[i];
-				if_name = if_indextoname(if_index, ifnamebuf);
-				log_debug("changed iface: %s[%d]", if_name !=
-				    NULL ? if_name : "<unknown>", if_index);
 				frontend_imsg_compose_engine(
 				    IMSG_REQUEST_REBOOT, 0, 0, &if_index,
 				    sizeof(if_index));
@@ -578,7 +574,6 @@ update_iface(struct if_msghdr *ifm, struct sockaddr_dl *sdl)
 	struct imsg_ifinfo	 ifinfo;
 	uint32_t		 if_index;
 	int			 flags, xflags;
-	char			 ifnamebuf[IF_NAMESIZE], *if_name;
 
 	if_index = ifm->ifm_index;
 
@@ -586,21 +581,11 @@ update_iface(struct if_msghdr *ifm, struct sockaddr_dl *sdl)
 	xflags = ifm->ifm_xflags;
 
 	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;
-	}
 
 	if (!(xflags & IFXF_AUTOCONF4)) {
 		if (iface != NULL) {
-			log_info("Removed autoconf flag from %s", if_name);
+			log_info("Removed autoconf flag from %s",
+			    iface->ifinfo.if_name);
 			frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0, 0,
 			    &if_index, sizeof(if_index));
 			remove_iface(if_index);
@@ -610,6 +595,9 @@ update_iface(struct if_msghdr *ifm, struct sockaddr_dl *sdl)
 
 	memset(&ifinfo, 0, sizeof(ifinfo));
 	ifinfo.if_index = if_index;
+	if (if_indextoname(if_index, ifinfo.if_name) == NULL)
+		return; /* interface disappeared */
+
 	ifinfo.link_state = ifm->ifm_data.ifi_link_state;
 	ifinfo.rdomain = ifm->ifm_tableid;
 	ifinfo.running = (flags & (IFF_UP | IFF_RUNNING)) ==
@@ -620,7 +608,8 @@ update_iface(struct if_msghdr *ifm, struct sockaddr_dl *sdl)
 		memcpy(ifinfo.hw_address.ether_addr_octet, LLADDR(sdl),
 		    ETHER_ADDR_LEN);
 	else if (iface == NULL) {
-		log_warnx("Could not find AF_LINK address for %s.", if_name);
+		log_warnx("Could not find AF_LINK address for %s.",
+		    ifinfo.if_name);
 		return;
 	}
 
@@ -689,6 +678,7 @@ init_ifaces(void)
 
 		memset(&ifinfo, 0, sizeof(ifinfo));
 		ifinfo.if_index = if_index;
+		memcpy(&ifinfo.if_name, if_name, sizeof(ifinfo.if_name));
 		ifinfo.link_state = -1;
 		ifinfo.running = (flags & (IFF_UP | IFF_RUNNING)) ==
 		    (IFF_UP | IFF_RUNNING);
@@ -1041,7 +1031,6 @@ void
 send_packet(uint8_t message_type, struct iface *iface)
 {
 	ssize_t			 pkt_len;
-	char			 ifnamebuf[IF_NAMESIZE], *if_name;
 
 	if (!event_initialized(&iface->bpfev.ev)) {
 		iface->send_discover = 1;
@@ -1050,13 +1039,10 @@ send_packet(uint8_t message_type, struct iface *iface)
 
 	iface->send_discover = 0;
 
-	if ((if_name = if_indextoname(iface->ifinfo.if_index, ifnamebuf)) == NULL)
-		return; /* iface went away, nothing to do */
-
 	log_debug("%s on %s", message_type == DHCPDISCOVER ? "DHCPDISCOVER" :
-	    "DHCPREQUEST", if_name);
+	    "DHCPREQUEST", iface->ifinfo.if_name);
 
-	pkt_len = build_packet(message_type, if_name, iface->xid,
+	pkt_len = build_packet(message_type, iface->ifinfo.if_name, iface->xid,
 	    &iface->ifinfo.hw_address, &iface->ciaddr, &iface->requested_ip,
 	    &iface->server_identifier);
 	if (iface->dhcp_server.s_addr != INADDR_ANY) {

-- 
In my defence, I have been left unsupervised.