BSD: struct if_data->ifi_link_state is the single source of truth

Vastly improve and simplify link detection on BSD.
dhcpcd either examines the whole system via getifaddrs(3) or
reacts to events via route(4).
In both cases we have struct if_data which has ifi_link_state.

Armed with this knowledge, we no longer need SIOCGIFDATA or
SIOCGIFMEDIA.

To solve the issue of newly attached interfaces having
LINK_STATE_UNKNOWN or some interfaces not even changing it,
we only change the local knowledge of interface flags when
reports them by getifaddrs(3) or route(4) when we change them.
For example, if we set IFF_UP and it succeeds we don't set this
internally until reported by the kernel as above.

This keeps flags and link state in sync with each other.
The hope is that the kernel can set the real link state before
it reports IFF_UP.

As such, we no longer require the poll option or need to enter a
tight loop for old interfaces.

diff --git a/src/dhcpcd.c b/src/dhcpcd.c
index e193b6eea4117573e3b5e00b31c65b163f797aa0..1fd0ae1889244ff124dcdc8674ed884aa099a718 100644 (file)
--- a/src/dhcpcd.c
+++ b/src/dhcpcd.c
@@ -432,8 +432,6 @@ stop_interface(struct interface *ifp)
        /* De-activate the interface */
        ifp->active = IF_INACTIVE;
        ifp->options->options &= ~DHCPCD_STOPPING;
-       /* Set the link state to unknown as we're no longer tracking it. */
-       ifp->carrier = LINK_UNKNOWN;
 
        if (!(ctx->options & (DHCPCD_MASTER | DHCPCD_TEST)))
                eloop_exit(ctx->eloop, EXIT_FAILURE);
@@ -704,42 +702,32 @@ dhcpcd_reportssid(struct interface *ifp)
 }
 
 void
-dhcpcd_handlecarrier(struct dhcpcd_ctx *ctx, int carrier, unsigned int flags,
-    const char *ifname)
+dhcpcd_handlecarrier(struct interface *ifp, int carrier, unsigned int flags)
 {
-       struct interface *ifp;
-
-       ifp = if_find(ctx->ifaces, ifname);
-       if (ifp == NULL ||
-           ifp->options == NULL || !(ifp->options->options & DHCPCD_LINK))
-               return;
+       bool nolink = ifp->options == NULL ||
+           !(ifp->options->options & DHCPCD_LINK);
 
+       ifp->flags = flags;
        if (carrier == LINK_UNKNOWN) {
-               if (ifp->wireless) {
+               if (ifp->wireless)
                        carrier = LINK_DOWN;
-                       ifp->flags = flags;
-               } else
-                       carrier = if_carrier(ifp);
-       } else
-               ifp->flags = flags;
-       if (carrier == LINK_UNKNOWN)
-               carrier = IF_UPANDRUNNING(ifp) ? LINK_UP : LINK_DOWN;
+               else
+                       carrier = IF_UPANDRUNNING(ifp) ? LINK_UP : LINK_DOWN;
+       }
 
        if (carrier == LINK_DOWN || (ifp->flags & IFF_UP) == 0) {
                if (ifp->carrier != LINK_DOWN) {
-                       int oldcarrier = ifp->carrier;
-
 #ifdef NOCARRIER_PRESERVE_IP
                        if (ifp->flags & IFF_UP &&
-                           !(ifp->options->options & DHCPCD_ANONYMOUS))
+                           (ifp->options == NULL ||
+                           !(ifp->options->options & DHCPCD_ANONYMOUS)))
                                ifp->carrier = LINK_DOWN_IFFUP;
                        else
 #endif
                                ifp->carrier = LINK_DOWN;
-                       if (!ifp->active)
+                       if (!ifp->active || nolink)
                                return;
-                       if (oldcarrier == LINK_UP)
-                               loginfox("%s: carrier lost", ifp->name);
+                       loginfox("%s: carrier lost", ifp->name);
                        script_runreason(ifp, "NOCARRIER");
 #ifdef NOCARRIER_PRESERVE_IP
                        if (ifp->flags & IFF_UP &&
@@ -801,7 +789,7 @@ dhcpcd_handlecarrier(struct dhcpcd_ctx *ctx, int carrier, unsigned int flags,
 #endif
                                }
                        }
-                       if (!ifp->active)
+                       if (!ifp->active || nolink)
                                return;
                        dhcpcd_initstate(ifp, 0);
                        script_runreason(ifp, "CARRIER");
@@ -878,20 +866,11 @@ dhcpcd_startinterface(void *arg)
        struct interface *ifp = arg;
        struct if_options *ifo = ifp->options;
 
-       if (ifo->options & DHCPCD_LINK) {
-               switch (ifp->carrier) {
-               case LINK_UP:
-                       break;
-               case LINK_DOWN:
-                       loginfox("%s: waiting for carrier", ifp->name);
-                       return;
-               case LINK_UNKNOWN:
-                       /* No media state available.
-                        * Loop until both IFF_UP and IFF_RUNNING are set */
-                       if (ifo->poll == 0)
-                               if_pollinit(ifp);
-                       return;
-               }
+       if (ifo->options & DHCPCD_LINK && (ifp->carrier == LINK_DOWN ||
+           (ifp->carrier == LINK_UNKNOWN && !IF_UPANDRUNNING(ifp))))
+       {
+               loginfox("%s: waiting for carrier", ifp->name);
+               return;
        }
 
        if (ifo->options & (DHCPCD_DUID | DHCPCD_IPV6) &&
@@ -1000,9 +979,6 @@ dhcpcd_prestartinterface(void *arg)
                        logerr(__func__);
        }
 
-       if (ifp->options->poll != 0)
-               if_pollinit(ifp);
-
        dhcpcd_startinterface(ifp);
 }
 
@@ -1138,14 +1114,14 @@ dhcpcd_handlelink(void *arg)
 static void
 dhcpcd_checkcarrier(void *arg)
 {
-       struct interface *ifp = arg;
-       int carrier;
+       struct interface *ifp0 = arg, *ifp;
 
-       /* Check carrier here rather than setting LINK_UNKNOWN.
-        * This is because we force LINK_UNKNOWN as down for wireless which
-        * we do not want when dealing with a route socket overflow. */
-       carrier = if_carrier(ifp);
-       dhcpcd_handlecarrier(ifp->ctx, carrier, ifp->flags, ifp->name);
+       ifp = if_find(ifp0->ctx->ifaces, ifp0->name);
+       if (ifp == NULL || ifp->carrier == ifp0->carrier)
+               return;
+
+       dhcpcd_handlecarrier(ifp, ifp0->carrier, ifp0->flags);
+       if_free(ifp0);
 }
 
 #ifndef SMALL
@@ -1231,10 +1207,10 @@ dhcpcd_linkoverflow(struct dhcpcd_ctx *ctx)
                ifp1 = if_find(ctx->ifaces, ifp->name);
                if (ifp1 != NULL) {
                        /* If the interface already exists,
-                        * check carrier state. */
+                        * check carrier state.
+                        * dhcpcd_checkcarrier will free ifp. */
                        eloop_timeout_add_sec(ctx->eloop, 0,
-                           dhcpcd_checkcarrier, ifp1);
-                       if_free(ifp);
+                           dhcpcd_checkcarrier, ifp);
                        continue;
                }
                TAILQ_INSERT_TAIL(ctx->ifaces, ifp, next);

diff --git a/src/dhcpcd.conf.5.in b/src/dhcpcd.conf.5.in
index a5e9958141208cb4c0758c23e7c911a919751500..1169ae7a20d26a72d64a6ac090760e8c508ed736 100644 (file)
--- a/src/dhcpcd.conf.5.in
+++ b/src/dhcpcd.conf.5.in
@@ -24,7 +24,7 @@
 .\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
 .\" SUCH DAMAGE.
 .\"
-.Dd September 15, 2020
+.Dd September 28, 2020
 .Dt DHCPCD.CONF 5
 .Os
 .Sh NAME
@@ -541,12 +541,6 @@ Don't receive link messages about carrier status.
 You should only set this for buggy interface drivers.
 .It Ic noup
 Don't bring the interface up when in master mode.
-If
-.Nm
-cannot determine the carrier state,
-.Nm
-will enter a tight polling loop until the interface is marked up and running
-or a valid carrier state is reported.
 .It Ic option Ar option
 Requests the
 .Ar option
@@ -604,12 +598,6 @@ If
 detects an address added to a point to point interface (PPP, TUN, etc) then
 it will set the listed DHCP options to the destination address of the
 interface.
-.It Ic poll Op Ar time
-Polls the interface every
-.Ar time
-milliseconds (default of 100) to check flags and carrier status.
-This option should only be used if the driver does not report link state
-changes but can report the link state.
 .It Ic profile Ar name
 Subsequent options are only parsed for this profile
 .Ar name .

diff --git a/src/dhcpcd.h b/src/dhcpcd.h
index 03449e5846053d3255087762420ad14c2aa75d70..d2c90c1ce75e552a5506313b022732cbef49985f 100644 (file)
--- a/src/dhcpcd.h
+++ b/src/dhcpcd.h
@@ -271,7 +271,7 @@ void dhcpcd_daemonise(struct dhcpcd_ctx *);
 
 void dhcpcd_linkoverflow(struct dhcpcd_ctx *);
 int dhcpcd_handleargs(struct dhcpcd_ctx *, struct fd_list *, int, char **);
-void dhcpcd_handlecarrier(struct dhcpcd_ctx *, int, unsigned int, const char *);
+void dhcpcd_handlecarrier(struct interface *, int, unsigned int);
 int dhcpcd_handleinterface(void *, int, const char *);
 void dhcpcd_handlehwaddr(struct interface *, uint16_t, const void *, uint8_t);
 void dhcpcd_dropinterface(struct interface *, const char *);

diff --git a/src/if-bsd.c b/src/if-bsd.c
index bdcd29011827f76d2c2a20e511d3a4fe440a55ca..3eb78045577761dc28985087643776da0d653e83 100644 (file)
--- a/src/if-bsd.c
+++ b/src/if-bsd.c
@@ -377,84 +377,23 @@ static int if_indirect_ioctl(struct dhcpcd_ctx *ctx,
        return ioctl(ctx->pf_inet_fd, cmd, &ifr);
 }
 
-static int
-if_carrier0(struct interface *ifp)
-{
-       struct ifmediareq ifmr = { .ifm_status = 0 };
-
-       /* Not really needed, but the other OS update flags here also */
-       if (if_getflags(ifp) == -1)
-               return LINK_UNKNOWN;
-
-       strlcpy(ifmr.ifm_name, ifp->name, sizeof(ifmr.ifm_name));
-       if (ioctl(ifp->ctx->pf_inet_fd, SIOCGIFMEDIA, &ifmr) == -1)
-               return LINK_UNKNOWN;
-
-       if (!(ifmr.ifm_status & IFM_AVALID))
-               return LINK_UNKNOWN;
-
-       return (ifmr.ifm_status & IFM_ACTIVE) ? LINK_UP : LINK_DOWN;
-}
-
 int
-if_carrier(struct interface *ifp)
+if_carrier(__unused struct interface *ifp, const void *ifadata)
 {
-       int carrier = if_carrier0(ifp);
+       const struct if_data *ifi = ifadata;
 
-#ifdef SIOCGIFDATA
-       if (carrier != LINK_UNKNOWN)
-               return carrier;
-
-#ifdef __NetBSD__
-       struct ifdatareq ifdr = { .ifdr_data.ifi_link_state = 0 };
-       struct if_data *ifdata = &ifdr.ifdr_data;
-
-       strlcpy(ifdr.ifdr_name, ifp->name, sizeof(ifdr.ifdr_name));
-       if (ioctl(ifp->ctx->pf_inet_fd, SIOCGIFDATA, &ifdr) == -1)
-               return LINK_UNKNOWN;
-#else
-       struct if_data ifd = { .ifi_link_state = 0 };
-       struct if_data *ifdata = &ifd;
-
-       if (if_indirect_ioctl(ifp->ctx, ifp->name, SIOCGIFDATA,
-           &ifd, sizeof(ifd)) == -1)
-               return LINK_UNKNOWN;
-#endif
-
-       switch (ifdata->ifi_link_state) {
-       case LINK_STATE_DOWN:
-               return LINK_DOWN;
-#ifdef LINK_STATE_HALF_DUPLEX
-       case LINK_STATE_HALF_DUPLEX:
-       case LINK_STATE_FULL_DUPLEX:
-#endif
-       case LINK_STATE_UP:
-               return LINK_UP;
-       }
-       return LINK_UNKNOWN;
-#else
-#warning no SIOCGIFDATA - not all interfaces support SIOCGIFMEDIA
-       return carrier;
-#endif
-}
-
-int
-if_carrier_ifadata(struct interface *ifp, void *ifadata)
-{
-       int carrier = if_carrier0(ifp);
-       struct if_data *ifdata;
-
-       if (carrier != LINK_UNKNOWN || ifadata == NULL)
-               return carrier;
+       /*
+        * Every BSD returns this and it is the sole source of truth.
+        * Not all BSD's support SIOCGIFDATA and not all interfaces
+        * support SIOCGIFMEDIA.
+        */
+       assert(ifadata != NULL);
 
-       ifdata = ifadata;
-       switch (ifdata->ifi_link_state) {
-       case LINK_STATE_DOWN:
-               return LINK_DOWN;
-       case LINK_STATE_UP:
+       if (ifi->ifi_link_state >= LINK_STATE_UP)
                return LINK_UP;
-       }
-       return LINK_UNKNOWN;
+       if (ifi->ifi_link_state == LINK_STATE_UNKNOWN)
+               return LINK_UNKNOWN;
+       return LINK_DOWN;
 }
 
 static void
@@ -1254,24 +1193,8 @@ if_ifinfo(struct dhcpcd_ctx *ctx, const struct if_msghdr *ifm)
        if ((ifp = if_findindex(ctx->ifaces, ifm->ifm_index)) == NULL)
                return 0;
 
-       switch (ifm->ifm_data.ifi_link_state) {
-       case LINK_STATE_UNKNOWN:
-               link_state = LINK_UNKNOWN;
-               break;
-#ifdef LINK_STATE_FULL_DUPLEX
-       case LINK_STATE_HALF_DUPLEX:    /* FALLTHROUGH */
-       case LINK_STATE_FULL_DUPLEX:    /* FALLTHROUGH */
-#endif
-       case LINK_STATE_UP:
-               link_state = LINK_UP;
-               break;
-       default:
-               link_state = LINK_DOWN;
-               break;
-       }
-
-       dhcpcd_handlecarrier(ctx, link_state,
-           (unsigned int)ifm->ifm_flags, ifp->name);
+       link_state = if_carrier(ifp, &ifm->ifm_data);
+       dhcpcd_handlecarrier(ifp, link_state, (unsigned int)ifm->ifm_flags);
        return 0;
 }
 

diff --git a/src/if-linux.c b/src/if-linux.c
index 901c68e82b0526b8aba8e1e95830ccbe3925c9d4..0cda9e780e25f95f017eabb8b59abb95e31ec525 100644 (file)
--- a/src/if-linux.c
+++ b/src/if-linux.c
@@ -509,21 +509,12 @@ if_setmac(struct interface *ifp, void *mac, uint8_t maclen)
 }
 
 int
-if_carrier(struct interface *ifp)
+if_carrier(struct interface *ifp, __unused const void *ifadata)
 {
 
-       if (if_getflags(ifp) == -1)
-               return LINK_UNKNOWN;
        return ifp->flags & IFF_RUNNING ? LINK_UP : LINK_DOWN;
 }
 
-int
-if_carrier_ifadata(struct interface *ifp, __unused void *ifadata)
-{
-
-       return if_carrier(ifp);
-}
-
 int
 if_getnetlink(struct dhcpcd_ctx *ctx, struct iovec *iov, int fd, int flags,
     int (*cb)(struct dhcpcd_ctx *, void *, struct nlmsghdr *), void *cbarg)
@@ -1018,9 +1009,9 @@ link_netlink(struct dhcpcd_ctx *ctx, void *arg, struct nlmsghdr *nlm)
                dhcpcd_handlehwaddr(ifp, ifi->ifi_type, hwa, hwl);
        }
 
-       dhcpcd_handlecarrier(ctx,
+       dhcpcd_handlecarrier(ifp,
            ifi->ifi_flags & IFF_RUNNING ? LINK_UP : LINK_DOWN,
-           ifi->ifi_flags, ifn);
+           ifi->ifi_flags);
        return 0;
 }
 

diff --git a/src/if-options.c b/src/if-options.c
index 0d6b8e3a39d43b5bd2b70e918e0ce0bdc69922f1..f21ff4b8502390f2d8c5880337753f1cc64f64a9 100644 (file)
--- a/src/if-options.c
+++ b/src/if-options.c
@@ -165,7 +165,6 @@ const struct option cf_options[] = {
        {"inactive",        no_argument,       NULL, O_INACTIVE},
        {"mudurl",          required_argument, NULL, O_MUDURL},
        {"link_rcvbuf",     required_argument, NULL, O_LINK_RCVBUF},
-       {"poll",            optional_argument, NULL, O_POLL},
        {NULL,              0,                 NULL, '\0'}
 };
 
@@ -2235,18 +2234,6 @@ invalid_token:
                }
 #endif
                break;
-       case O_POLL:
-               if (arg == NULL) {
-                       ifo->poll = IF_POLL_UP;
-                       break;
-               }
-               ifo->poll = (unsigned long)
-                   strtou(arg, NULL, 0, 0, ULONG_MAX, &e);
-               if (e) {
-                       logerrx("failed to convert poll %s", arg);
-                       return -1;
-               }
-               break;
        default:
                return 0;
        }

diff --git a/src/if-options.h b/src/if-options.h
index efb6929093d05f7d5cc7ec154980e7b90936b2f1..2c974f578521c19a6baccf08f2ceefaa681fd3be 100644 (file)
--- a/src/if-options.h
+++ b/src/if-options.h
@@ -180,7 +180,6 @@
 #define O_INACTIVE             O_BASE + 47
 #define O_MUDURL               O_BASE + 48
 #define O_MSUSERCLASS          O_BASE + 49
-#define        O_POLL                  O_BASE + 50
 
 extern const struct option cf_options[];
 
@@ -216,7 +215,6 @@ struct if_options {
        time_t mtime;
        uint8_t iaid[4];
        int metric;
-       unsigned long poll;
        uint8_t requestmask[256 / NBBY];
        uint8_t requiremask[256 / NBBY];
        uint8_t nomask[256 / NBBY];

diff --git a/src/if-sun.c b/src/if-sun.c
index f9dc7bf1d763a9fb50fa38f96a3715307a32ebe9..51c0cfd5ee2f79e36f72aefd0aa080434f8741be 100644 (file)
--- a/src/if-sun.c
+++ b/src/if-sun.c
@@ -203,7 +203,7 @@ if_setmac(struct interface *ifp, void *mac, uint8_t maclen)
 }
 
 int
-if_carrier(struct interface *ifp)
+if_carrier(struct interface *ifp, __unused const void *ifadata)
 {
        kstat_ctl_t             *kcp;
        kstat_t                 *ksp;
@@ -245,13 +245,6 @@ err:
        return LINK_UNKNOWN;
 }
 
-int
-if_carrier_ifadata(struct interface *ifp, __unused void *ifadata)
-{
-
-       return if_carrier(ifp);
-}
-
 int
 if_mtu_os(const struct interface *ifp)
 {
@@ -1065,7 +1058,7 @@ if_ifinfo(struct dhcpcd_ctx *ctx, const struct if_msghdr *ifm)
                state = LINK_UP;
                flags |= IFF_UP;
        }
-       dhcpcd_handlecarrier(ctx, state, flags, ifp->name);
+       dhcpcd_handlecarrier(ifp, state, flags);
        return 0;
 }
 

diff --git a/src/if.c b/src/if.c
index 802fe461fe53276e7fa15eb16d7ecccf31915091..24d1f26665f293587e6e20d4a617e3ea36eef2b0 100644 (file)
--- a/src/if.c
+++ b/src/if.c
@@ -185,7 +185,11 @@ if_setflag(struct interface *ifp, short setflag, short unsetflag)
            if_ioctl(ifp->ctx, SIOCSIFFLAGS, &ifr, sizeof(ifr)) == -1)
                return -1;
 
-       ifp->flags = (unsigned int)ifr.ifr_flags;
+       /*
+        * Do NOT set ifp->flags here.
+        * We need to listen for flag updates from the kernel as they
+        * need to sync with carrier.
+        */
        return 0;
 }
 
@@ -687,37 +691,13 @@ if_discover(struct dhcpcd_ctx *ctx, struct ifaddrs **ifaddrs,
 #endif
 
                ifp->active = active;
-               ifp->carrier = if_carrier_ifadata(ifp, ifa->ifa_data);
+               ifp->carrier = if_carrier(ifp, ifa->ifa_data);
                TAILQ_INSERT_TAIL(ifs, ifp, next);
        }
 
        return ifs;
 }
 
-static void
-if_poll(void *arg)
-{
-       struct interface *ifp = arg;
-       unsigned int flags = ifp->flags;
-       int carrier;
-
-       carrier = if_carrier(ifp); /* if_carrier will update ifp->flags */
-       if (ifp->carrier != carrier || ifp->flags != flags)
-               dhcpcd_handlecarrier(ifp->ctx, carrier, ifp->flags, ifp->name);
-
-       if (ifp->options->poll != 0 || ifp->carrier != LINK_UP)
-               if_pollinit(ifp);
-}
-
-int
-if_pollinit(struct interface *ifp)
-{
-       unsigned long msec;
-
-       msec = ifp->options->poll != 0 ? ifp->options->poll : IF_POLL_UP;
-       return eloop_timeout_add_msec(ifp->ctx->eloop, msec, if_poll, ifp);
-}
-
 /*
  * eth0.100:2 OR eth0i100:2 (seems to be NetBSD xvif(4) only)
  *

diff --git a/src/if.h b/src/if.h
index 7123c3a13004f7458e24aa06fee667b273fd4496..fc046820bca4b621900b25d9c0b2194986857037 100644 (file)
--- a/src/if.h
+++ b/src/if.h
@@ -159,9 +159,7 @@ void if_free(struct interface *);
 int if_domtu(const struct interface *, short int);
 #define if_getmtu(ifp) if_domtu((ifp), 0)
 #define if_setmtu(ifp, mtu) if_domtu((ifp), (mtu))
-int if_carrier(struct interface *);
-int if_carrier_ifadata(struct interface *, void *);
-int if_pollinit(struct interface *ifp);
+int if_carrier(struct interface *, const void *);
 
 #ifdef ALIAS_ADDR
 int if_makealias(char *, size_t, const char *, int);