Download raw body.
dhcpleased(8): Reduce if_indextoname(3) usage.
On 2024-07-13 15:59 +02, Florian Obser <florian@openbsd.org> 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 : "<unknown>", 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.
dhcpleased(8): Reduce if_indextoname(3) usage.