From: Roy Marples Date: Mon, 28 Sep 2020 16:09:38 +0000 (+0100) Subject: BSD: struct if_data->ifi_link_state is the single source of truth X-Git-Tag: v9.3.0~10 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=96702451aea98a8d682ebac5a46e14802e05f624;p=thirdparty%2Fdhcpcd.git 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 e193b6ee..1fd0ae18 100644 --- 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 a5e99581..1169ae7a 100644 --- 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 03449e58..d2c90c1c 100644 --- 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 bdcd2901..3eb78045 100644 --- 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 901c68e8..0cda9e78 100644 --- 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 0d6b8e3a..f21ff4b8 100644 --- 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 efb69290..2c974f57 100644 --- 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 f9dc7bf1..51c0cfd5 100644 --- 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 802fe461..24d1f266 100644 --- 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 7123c3a1..fc046820 100644 --- 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);