From: Florian Obser Subject: Re: dhcpleased(8): Reduce if_indextoname(3) usage. To: tech@openbsd.org Date: Wed, 17 Sep 2025 22:54:32 +0200 On 2024-07-13 15:59 +02, Florian Obser wrote: > 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? I found this old diff I was slacking on. OK? diff --git dhcpleased.h dhcpleased.h index d3d79b61c68..13c9dba6253 100644 --- dhcpleased.h +++ dhcpleased.h @@ -269,6 +269,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 013953b6423..4eaba8a5443 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; @@ -555,8 +556,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: %s without IMSG_RECONF_CONF", @@ -567,12 +566,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) @@ -658,6 +656,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); @@ -757,15 +758,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)); @@ -1224,8 +1222,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: @@ -1392,7 +1390,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; @@ -1506,9 +1503,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) { @@ -1669,9 +1665,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, @@ -1680,10 +1674,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 @@ -1783,9 +1777,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)); @@ -1806,12 +1798,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 6174792fc34..71de68a3da3 100644 --- frontend.c +++ frontend.c @@ -428,7 +428,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: %s without IMSG_RECONF_CONF", @@ -439,9 +438,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 : "", if_index); frontend_imsg_compose_engine( IMSG_REQUEST_REBOOT, 0, 0, &if_index, sizeof(if_index)); @@ -597,7 +593,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; @@ -605,21 +600,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); @@ -629,6 +614,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)) == @@ -639,7 +627,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; } @@ -708,6 +697,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); @@ -1060,7 +1050,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; @@ -1069,13 +1058,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.