Re: let dhcpd start on down interfaces
On Mon, Nov 15, 2021 at 12:19:48PM +, 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.c12 Apr 2017 19:17:30 - 1.43
> +++ dispatch.c13 Nov 2021 23:04:36 -
> @@ -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 - 1.43
> > +++ dispatch.c 15 Nov 2021 03:50:46 -
> > @@ -47,6 +47,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> >
> > @@ -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 =
>
Re: let dhcpd start on down interfaces
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.
> 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.
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).
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 - 1.43
+++ dispatch.c 13 Nov 2021 23:04:36 -
@@ -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.c12 Apr 2017 19:17:30 - 1.43
> +++ dispatch.c15 Nov 2021 03:50:46 -
> @@ -47,6 +47,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> @@ -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
