On Mon, Nov 15, 2021 at 12:19:48PM +0000, Klemens Nanni wrote:
> On Mon, Nov 15, 2021 at 02:08:33PM +1000, David Gwynne wrote:
> > The subject line only tells half the story. The other half is that
> > instead of hoping it only identifies Ethernet interfaces it now actually
> > checks and restricts itself to Ethernet interfaces. I couldn't help
> > myself.
> >
> > If it helps my case for this code, I've been using this semantic for a
> > few years in a home grown dhcp-relay we run on our firewalls at
> > work. We used to run the dhcp-relay on a bunch of vlan(4) interfaces,
> > but we currently run them on carp(4). There's no significant difference
> > between using an Ethernet or carp interface from a dhcpd or relay point
> > of view, so the code accepts either.
>
> This sounds reasonable, but I have no means to test these setups.
>
> > The kernel lets you bind to addresses on down interfaces, so I don't
> > know why dhcpd has to be precious about it. We often start the
> > firewalls up with whole groups of these interfaces forced down.
> > We want DHCP packets to start moving when then interface comes up
> > though, therefore the daemon should start and be running even if
> > the interface is down.
>
> I wondered about this as well but was hesitant to touch dhcpd(8) because
> I rarely use it.
>
> FWIW, ISC DHCP has the same code and comment wrt. interface check,
> altough I didn't do any runtime tests.
our dhcpd came from isc dhcpd, and the guts havent been touched
much.
> > Should I warn if the interface is down on startup?
>
> Wouldn't hurt, although you don't see it at boot time anyway, so not
> much help, imho.
>
> > There's some other questionable code in here, but I'm going to take
> > some deep breaths and try to ignore them for now.
>
> Here is the minimal diff I first used to fix my vport/DOWN issue,
> before going the implicit UP route.
this is a good start. im ok with you putting your change in, just get
someone else to ok it too.
> Regardless of vport's behaviour, this seemed sensible; I mean, I can
> `nc -l' on down interfaces, other daemons don't care about UP and I'd
> rather have dhcpd running and pull interfaces UP to fix DHCP instead of
> restarting failed dhcpd (which could still serve on other UP interfaces).
exactly.
>
> Index: dispatch.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/dhcpd/dispatch.c,v
> retrieving revision 1.43
> diff -u -p -r1.43 dispatch.c
> --- dispatch.c 12 Apr 2017 19:17:30 -0000 1.43
> +++ dispatch.c 13 Nov 2021 23:04:36 -0000
> @@ -112,13 +112,12 @@ discover_interfaces(int *rdomain)
> for (ifa = ifap; ifa != NULL; ifa = ifa->ifa_next) {
> /*
> * See if this is the sort of interface we want to
> - * deal with. Skip loopback, point-to-point and down
> + * deal with. Skip loopback and point-to-point
> * interfaces, except don't skip down interfaces if we're
> * trying to get a list of configurable interfaces.
> */
> if ((ifa->ifa_flags & IFF_LOOPBACK) ||
> (ifa->ifa_flags & IFF_POINTOPOINT) ||
> - (!(ifa->ifa_flags & IFF_UP)) ||
> (!(ifa->ifa_flags & IFF_BROADCAST)))
> continue;
>
>
> > OK?
> >
> > Index: dispatch.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/dhcpd/dispatch.c,v
> > retrieving revision 1.43
> > diff -u -p -r1.43 dispatch.c
> > --- dispatch.c 12 Apr 2017 19:17:30 -0000 1.43
> > +++ dispatch.c 15 Nov 2021 03:50:46 -0000
> > @@ -47,6 +47,7 @@
> > #include <net/if.h>
> > #include <net/if_dl.h>
> > #include <net/if_media.h>
> > +#include <net/if_types.h>
> >
> > #include <netinet/in.h>
> >
> > @@ -112,14 +113,10 @@ discover_interfaces(int *rdomain)
> > for (ifa = ifap; ifa != NULL; ifa = ifa->ifa_next) {
> > /*
> > * See if this is the sort of interface we want to
> > - * deal with. Skip loopback, point-to-point and down
> > - * interfaces, except don't skip down interfaces if we're
> > - * trying to get a list of configurable interfaces.
> > + * deal with, which is basically Ethernet.
> > */
> > if ((ifa->ifa_flags & IFF_LOOPBACK) ||
> > - (ifa->ifa_flags & IFF_POINTOPOINT) ||
> > - (!(ifa->ifa_flags & IFF_UP)) ||
> > - (!(ifa->ifa_flags & IFF_BROADCAST)))
> > + (ifa->ifa_flags & IFF_POINTOPOINT))
> > continue;
> >
> > /* See if we've seen an interface that matches this one. */
> > @@ -156,11 +153,27 @@ 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;
> > + struct sockaddr_dl *sdl;
> > +
> > + ifi = (struct if_data *)ifa->ifa_data;
> > + if (ifi->ifi_type != IFT_ETHER &&
> > + ifi->ifi_type != IFT_CARP) {
> > + /* this interface will be cleaned up later */
> > + continue;
> > + }
> > +
> > + sdl = ((struct sockaddr_dl *)(ifa->ifa_addr));
> > + if (sdl->sdl_alen > sizeof(tmp->hw_address.haddr)) {
> > + log_warnx("interface %s long hardware address",
> > + tmp->name);
> > + /* this interface will be cleaned up later */
> > + continue;
> > + }
> > +
> > tmp->index = sdl->sdl_index;
> > tmp->hw_address.hlen = sdl->sdl_alen;
> > - tmp->hw_address.htype = HTYPE_ETHER; /* XXX */
> > + tmp->hw_address.htype = HTYPE_ETHER;
> > memcpy(tmp->hw_address.haddr,
> > LLADDR(sdl), sdl->sdl_alen);
> > } else if (ifa->ifa_addr->sa_family == AF_INET) {
> > @@ -236,6 +249,17 @@ discover_interfaces(int *rdomain)
> > last = NULL;
> > for (tmp = interfaces; tmp; tmp = next) {
> > next = tmp->next;
> > +
> > + if (tmp->index == 0) {
> > + log_warnx("Can't listen on %s - wrong interface type",
> > + tmp->name);
> > + /* Remove tmp from the list of interfaces. */
> > + if (!last)
> > + interfaces = interfaces->next;
> > + else
> > + last->next = tmp->next;
> > + continue;
> > + }
> >
> > if (!tmp->ifp) {
> > log_warnx("Can't listen on %s - it has no IP address.",
> >
