From: Claudio Jeker Subject: Re: dhcpd(8): use the rdomain it executes with To: David Gwynne Cc: tech@openbsd.org Date: Tue, 10 Jun 2025 06:51:48 +0200 On Sat, Jun 07, 2025 at 02:36:09PM +1000, David Gwynne wrote: > dhcpd is weird about how it interacts with rdomains. it uses the rdomain > from the first interface given to it on the command line. > > i would argue it's more natural to use the rdomain it's executed > with, which in turn integrates it better with stuff like rcctl. it > also makes it work as expected with the "-u" flag. > > it shuffles some of the address/subnet matching to avoid reporting > overlapping address space if you have multiple interfaces with the same > networks but in separate rdomains. > > i'm not even sure we need to mention rdomains in the manpage with this > change either. > > tests? ok? In general I agree but see below. > at some point i should introduce dhcpd to sys/queue.h. > > Index: dhcpd.8 > =================================================================== > RCS file: /cvs/src/usr.sbin/dhcpd/dhcpd.8,v > diff -u -p -r1.36 dhcpd.8 > --- dhcpd.8 27 Jun 2024 16:39:31 -0000 1.36 > +++ dhcpd.8 7 Jun 2025 04:29:40 -0000 > @@ -132,15 +132,8 @@ on systems where > is unable to identify non-broadcast interfaces. > .Pp > .Nm > -normally runs in routing domain 0. > -In order to run in another > -.Xr rdomain 4 , > -.Nm > -needs to be started with a list of interfaces > -which share the same routing domain, > -which allows > -.Nm > -to set its own routing domain accordingly. > +will skip interfaces configured in a different routing domain to > +the one it was executed in. > .Pp > DHCP traffic always bypasses IPsec. > Otherwise there could be situations when a server has an IPsec SA for the > Index: dhcpd.c > =================================================================== > RCS file: /cvs/src/usr.sbin/dhcpd/dhcpd.c,v > diff -u -p -r1.61 dhcpd.c > --- dhcpd.c 4 Jun 2025 21:16:25 -0000 1.61 > +++ dhcpd.c 7 Jun 2025 04:29:40 -0000 > @@ -71,6 +71,7 @@ struct group root_group; > u_int16_t server_port; > u_int16_t client_port; > > +int rdomain; > struct passwd *pw; > int log_priority; > int pfpipe[2]; > @@ -87,7 +88,7 @@ char *leased_tab = NULL; > int > main(int argc, char *argv[]) > { > - int ch, cftest = 0, rdomain = -1, udpsockmode = 0; > + int ch, cftest = 0, udpsockmode = 0; > int debug = 0, verbose = 0; > char *sync_iface = NULL; > char *sync_baddr = NULL; > @@ -98,6 +99,8 @@ main(int argc, char *argv[]) > log_init(1, LOG_DAEMON); /* log to stderr until daemonized */ > log_setverbose(1); > > + rdomain = getrtable(); > + > opterr = 0; > while ((ch = getopt(argc, argv, "A:C:L:c:dfl:nu::vY:y:")) != -1) > switch (ch) { > @@ -197,11 +200,7 @@ main(int argc, char *argv[]) > > db_startup(); > if (!udpsockmode || argc > 0) > - discover_interfaces(&rdomain); > - > - if (rdomain != -1) > - if (setrtable(rdomain) == -1) > - fatal("setrtable"); > + discover_interfaces(); > > if (syncsend || syncrecv) { > if (sync_init(sync_iface, sync_baddr, sync_port) == -1) > Index: dhcpd.h > =================================================================== > RCS file: /cvs/src/usr.sbin/dhcpd/dhcpd.h,v > diff -u -p -r1.72 dhcpd.h > --- dhcpd.h 31 May 2025 07:47:45 -0000 1.72 > +++ dhcpd.h 7 Jun 2025 04:29:40 -0000 > @@ -443,7 +443,7 @@ ssize_t receive_packet(struct interface_ > extern struct interface_info *interfaces; > extern struct protocol *protocols; > extern struct dhcpd_timeout *timeouts; > -void discover_interfaces(int *); > +void discover_interfaces(void); > void dispatch(void); > int locate_network(struct packet *); > void got_one(struct protocol *); > Index: dispatch.c > =================================================================== > RCS file: /cvs/src/usr.sbin/dhcpd/dispatch.c,v > diff -u -p -r1.49 dispatch.c > --- dispatch.c 4 Jun 2025 21:16:25 -0000 1.49 > +++ dispatch.c 7 Jun 2025 04:29:40 -0000 > @@ -67,6 +67,8 @@ > #include "log.h" > #include "sync.h" > > +extern int rdomain; > + > struct interface_info *interfaces; > struct protocol *protocols; > struct dhcpd_timeout *timeouts; > @@ -81,14 +83,14 @@ int get_rdomain(char *); > subnet it's on, and add it to the list of interfaces. */ > > void > -discover_interfaces(int *rdomain) > +discover_interfaces(void) > { > struct interface_info *tmp; > struct interface_info *last, *next; > struct subnet *subnet; > struct shared_network *share; > struct sockaddr_in foo; > - int ir = 0, ird; > + int ir; > struct ifreq *tif; > struct ifaddrs *ifap, *ifa; > > @@ -99,11 +101,7 @@ discover_interfaces(int *rdomain) > * If we already have a list of interfaces, the interfaces were > * requested. > */ > - if (interfaces != NULL) > - ir = 1; > - else > - /* must specify an interface when rdomains are used */ > - *rdomain = 0; > + ir = (interfaces != NULL); > > /* Cycle through the list of interfaces looking for IP addresses. */ > for (ifa = ifap; ifa != NULL; ifa = ifa->ifa_next) { > @@ -125,15 +123,6 @@ discover_interfaces(int *rdomain) > if (tmp == NULL && ir) > continue; > > - ird = get_rdomain(ifa->ifa_name); > - if (*rdomain == -1) > - *rdomain = ird; > - else if (*rdomain != ird && ir) > - fatalx("Interface %s is not in rdomain %d", > - tmp->name, *rdomain); > - else if (*rdomain != ird && !ir) > - continue; > - > /* If there isn't already an interface by this name, > allocate one. */ > if (tmp == NULL) { > @@ -150,16 +139,19 @@ discover_interfaces(int *rdomain) > /* If we have the capability, extract link information > and record it in a linked list. */ > if (ifa->ifa_addr->sa_family == AF_LINK) { > - struct sockaddr_dl *sdl = > - ((struct sockaddr_dl *)(ifa->ifa_addr)); > + struct if_data *ifi = ifa->ifa_data; > + struct sockaddr_dl *sdl; > + > + if (rdomain != ifi->ifi_rdomain) > + continue; > + The check and skip of interfaces happens now after tmp has been allocated. But it does not skip over all of it. Since the AF_INET block below will be still be entered afterwards... IIRC the order of the ifap list from getifaddrs puts AF_LINK last. So you can't skip the AF_INET else simply because tmp->index == 0. Not against this but there is then a problem below. > + sdl = (struct sockaddr_dl *)ifa->ifa_addr; > tmp->index = sdl->sdl_index; > tmp->hw_address.hlen = sdl->sdl_alen; > tmp->hw_address.htype = HTYPE_ETHER; /* XXX */ > memcpy(tmp->hw_address.haddr, > LLADDR(sdl), sdl->sdl_alen); > } else if (ifa->ifa_addr->sa_family == AF_INET) { > - struct iaddr addr; > - > /* Get a pointer to the address... */ > memcpy(&foo, ifa->ifa_addr, sizeof(foo)); > > @@ -182,55 +174,27 @@ discover_interfaces(int *rdomain) > tmp->ifp = tif; > tmp->primary_address = foo.sin_addr; > } > - > - /* Grab the address... */ > - addr.len = 4; > - memcpy(addr.iabuf, &foo.sin_addr.s_addr, addr.len); > - > - /* If there's a registered subnet for this address, > - connect it together... */ > - if ((subnet = find_subnet(addr))) { > - /* If this interface has multiple aliases > - on the same subnet, ignore all but the > - first we encounter. */ > - if (!subnet->interface) { > - subnet->interface = tmp; > - subnet->interface_address = addr; > - } else if (subnet->interface != tmp) { > - log_warnx("Multiple %s %s: %s %s", > - "interfaces match the", > - "same subnet", > - subnet->interface->name, > - tmp->name); > - } > - share = subnet->shared_network; > - if (tmp->shared_network && > - tmp->shared_network != share) { > - log_warnx("Interface %s matches %s", > - tmp->name, > - "multiple shared networks"); > - } else { > - tmp->shared_network = share; > - } > - > - if (!share->interface) { > - share->interface = tmp; > - } else if (share->interface != tmp) { > - log_warnx("Multiple %s %s: %s %s", > - "interfaces match the", > - "same shared network", > - share->interface->name, > - tmp->name); > - } > - } > } > } > > /* Discard interfaces we can't listen on. */ > last = NULL; > for (tmp = interfaces; tmp; tmp = next) { > + struct iaddr addr; > + > next = tmp->next; > > + if (tmp->index == 0) { > + log_warnx("Can't listen on %s - wrong rdomain", > + tmp->name); > + /* Remove tmp from the list of interfaces. */ > + if (!last) > + interfaces = interfaces->next; > + else > + last->next = tmp->next; Here you skip over these interfaces with a warning but you also leak tmp. So is the idea that when you use rdomains that dhcpd needs the list of interfaces to listen on? Else the warning could be very annoying. Also be careful with tmp->ifp since that is also allocated in the ifa->ifa_addr->sa_family == AF_INET case. I realize that the current code also leaks memory. What horrible mess. > + continue; > + } > + > if (!tmp->ifp) { > log_warnx("Can't listen on %s - it has no IP address.", > tmp->name); > @@ -244,6 +208,47 @@ discover_interfaces(int *rdomain) > > memcpy(&foo, &tmp->ifp->ifr_addr, sizeof tmp->ifp->ifr_addr); > > + /* Grab the address... */ > + addr.len = 4; > + memcpy(addr.iabuf, &foo.sin_addr.s_addr, addr.len); > + > + /* If there's a registered subnet for this address, > + connect it together... */ > + if ((subnet = find_subnet(addr))) { > + /* If this interface has multiple aliases > + on the same subnet, ignore all but the > + first we encounter. */ > + if (!subnet->interface) { > + subnet->interface = tmp; > + subnet->interface_address = addr; > + } else if (subnet->interface != tmp) { > + log_warnx("Multiple %s %s: %s %s", > + "interfaces match the", > + "same subnet", > + subnet->interface->name, > + tmp->name); > + } > + share = subnet->shared_network; > + if (tmp->shared_network && > + tmp->shared_network != share) { > + log_warnx("Interface %s matches %s", > + tmp->name, > + "multiple shared networks"); > + } else { > + tmp->shared_network = share; > + } > + > + if (!share->interface) { > + share->interface = tmp; > + } else if (share->interface != tmp) { > + log_warnx("Multiple %s %s: %s %s", > + "interfaces match the", > + "same shared network", > + share->interface->name, > + tmp->name); > + } > + } > + > if (!tmp->shared_network) { > log_warnx("Can't listen on %s - dhcpd.conf has no " > "subnet declaration for %s.", tmp->name, > @@ -616,22 +621,4 @@ remove_protocol(struct protocol *proto) > free(p); > } > } > -} > - > -int > -get_rdomain(char *name) > -{ > - int rv = 0, s; > - struct ifreq ifr; > - > - if ((s = socket(AF_INET, SOCK_DGRAM, 0)) == -1) > - fatal("get_rdomain socket"); > - > - memset(&ifr, 0, sizeof(ifr)); > - strlcpy(ifr.ifr_name, name, sizeof(ifr.ifr_name)); > - if (ioctl(s, SIOCGIFRDOMAIN, (caddr_t)&ifr) != -1) > - rv = ifr.ifr_rdomainid; > - > - close(s); > - return rv; > } > -- :wq Claudio