On Mon, 27 Apr 2020 16:59:22 +0200
Gerhard Roth <[email protected]> wrote:

> On 4/27/20 4:53 PM, Theo de Raadt wrote:
> > Gerhard Roth <[email protected]> wrote:
> >   
> >> Hi Theo,
> >>
> >> On 4/27/20 4:39 PM, Theo de Raadt wrote:  
> >>> Is this code in umb_decode_ip_configuration() reached again, if
> >>> you do a late ifconfig (don't set inet6 at up time, but set it
> >>> later)  
> >>
> >> no, seting inet6 later doesn't work. On MBIM level I have to tell the
> >> device *before* the CONNECT whether I want IPv6 support or not. And
> >> there is no way to change that selection later on while we are
> >> connected.
> >>
> >> The only way to do this transparently would be an implicit disconnect
> >> followed by a reconnect in if_umb.c. But then I would need some trigger
> >> that is called every time someone does 'ifconfig umb0 inet6 eui64' or
> >> 'ifconfig umb0 -inet6'. And I'm not sure whether the implicit
> >> temporary link loss is appreciated by the user.  
> > 
> > Then this diff seems incorrect.
> > 
> > The alternative of disconnecting, is not nice.
> > 
> > Perhaps umb can always request inet6 at startup, but not actually expose
> > it to the network stack.  Then enabling the software inet6 flag can expose
> > inet6 to the network layer, disabling the inet6 flag can remove exposure
> > to the network layer, but the actual address and support is always attempted
> > by the driver?
> >   
> 
> 
> That would work. Will try this.


So here is an updated diff that behaves in the proposed way.
umb_decode_ip_configuration() just stores the IP addresses in softc
and the real configuration is now done in umb_configure().
The later we also call, when our address-change hook is called.

Gerhard



Index: share/man/man4/umb.4
===================================================================
RCS file: /cvs/src/share/man/man4/umb.4,v
retrieving revision 1.10
diff -u -p -u -p -r1.10 umb.4
--- share/man/man4/umb.4        18 Feb 2020 08:09:37 -0000      1.10
+++ share/man/man4/umb.4        30 Apr 2020 11:47:55 -0000
@@ -40,6 +40,13 @@ will remain in this state until the MBIM
 In case the device is connected to an "always-on" USB port,
 it may be possible to connect to a provider without entering the
 PIN again even if the system was rebooted.
+.Pp
+To use IPv6, configure a link-local address.
+If the device is able to connect to the ISP's network but doesn't
+show an IPv6 address, setting the
+.Sy AUTOCONF6
+flag on the interface before bringing it up may help.
+.Ed
 .Sh HARDWARE
 The following devices should work:
 .Pp
Index: sys/dev/usb/if_umb.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.33
diff -u -p -u -p -r1.33 if_umb.c
--- sys/dev/usb/if_umb.c        27 Apr 2020 11:16:51 -0000      1.33
+++ sys/dev/usb/if_umb.c        30 Apr 2020 11:18:54 -0000
@@ -147,6 +147,9 @@ void                 umb_newstate(struct umb_softc *, 
 void            umb_state_task(void *);
 void            umb_up(struct umb_softc *);
 void            umb_down(struct umb_softc *, int);
+#ifdef INET6
+void            umb_addr6_change(void *);
+#endif
 
 void            umb_get_response_task(void *);
 
@@ -164,11 +167,10 @@ int                umb_decode_packet_service(struct u
 int             umb_decode_signal_state(struct umb_softc *, void *, int);
 int             umb_decode_connect_info(struct umb_softc *, void *, int);
 void            umb_clear_addr(struct umb_softc *);
-int             umb_add_inet_config(struct umb_softc *, struct in_addr, u_int,
-                   struct in_addr);
-int             umb_add_inet6_config(struct umb_softc *, struct in6_addr *,
-                   u_int, struct in6_addr *);
+int             umb_add_inet_config(struct umb_softc *);
+int             umb_add_inet6_config(struct umb_softc *);
 void            umb_send_inet_proposal(struct umb_softc *, int);
+int             umb_configure(struct umb_softc *);
 int             umb_decode_ip_configuration(struct umb_softc *, void *, int);
 void            umb_rx(struct umb_softc *);
 void            umb_rxeof(struct usbd_xfer *, void *, usbd_status);
@@ -477,6 +479,11 @@ umb_attach(struct device *parent, struct
            USB_TASK_TYPE_GENERIC);
        timeout_set(&sc->sc_statechg_timer, umb_statechg_timeout, sc);
 
+#ifdef INET6
+       task_set(&sc->sc_atask, umb_addr6_change, sc);
+       task_set(&sc->sc_ctask, (void (*)(void *))umb_configure, sc);
+#endif
+
        if (usbd_open_pipe_intr(uaa->iface, ctrl_ep, USBD_SHORT_XFER_OK,
            &sc->sc_ctrl_pipe, sc, &sc->sc_intr_msg, sizeof (sc->sc_intr_msg),
            umb_intr, USBD_DEFAULT_INTERVAL)) {
@@ -530,6 +537,10 @@ umb_attach(struct device *parent, struct
 #if NBPFILTER > 0
        bpfattach(&ifp->if_bpf, ifp, DLT_LOOP, sizeof(uint32_t));
 #endif
+#if INET6
+       if_addrhook_add(ifp, &sc->sc_atask);
+#endif
+
        /*
         * Open the device now so that we are able to query device information.
         * XXX maybe close when done?
@@ -553,6 +564,9 @@ umb_detach(struct device *self, int flag
        int      s;
 
        s = splnet();
+#ifdef INET6
+       if_addrhook_del(ifp, &sc->sc_atask);
+#endif
        if (ifp->if_flags & IFF_RUNNING)
                umb_down(sc, 1);
        umb_close(sc);
@@ -698,6 +712,7 @@ umb_ioctl(struct ifnet *ifp, u_long cmd,
        struct umb_softc *sc = ifp->if_softc;
        struct ifreq *ifr = (struct ifreq *)data;
        int      s, error = 0;
+       struct umb_info info;
        struct umb_parameter mp;
 
        if (usbd_is_dying(sc->sc_udev))
@@ -709,8 +724,10 @@ umb_ioctl(struct ifnet *ifp, u_long cmd,
                usb_add_task(sc->sc_udev, &sc->sc_umb_task);
                break;
        case SIOCGUMBINFO:
-               error = copyout(&sc->sc_info, ifr->ifr_data,
-                   sizeof (sc->sc_info));
+               memcpy(&info, &sc->sc_info, sizeof (info));
+               if (in6ifa_ifpforlinklocal(ifp, 0) == NULL)
+                       memset(info.ipv6dns, 0, sizeof (info.ipv6dns));
+               error = copyout(&info, ifr->ifr_data, sizeof (info));
                break;
        case SIOCSUMBPARAM:
                if ((error = suser(p)) != 0)
@@ -964,6 +981,13 @@ umb_newstate(struct umb_softc *sc, enum 
                log(LOG_DEBUG, "%s: state going %s from '%s' to '%s'\n",
                    DEVNAM(sc), newstate > sc->sc_state ? "up" : "down",
                    umb_istate(sc->sc_state), umb_istate(newstate));
+
+       /*
+        * If we are losing the 'up' state, clear all addresses and routes
+        */
+       if (sc->sc_state == UMB_S_UP)
+               umb_clear_addr(sc);
+
        sc->sc_state = newstate;
        usb_add_task(sc->sc_udev, &sc->sc_umb_task);
 }
@@ -1122,6 +1146,17 @@ umb_down(struct umb_softc *sc, int force
                timeout_del(&sc->sc_statechg_timer);
 }
 
+#ifdef INET6
+void
+umb_addr6_change(void *arg)
+{
+       struct umb_softc *sc = arg;
+
+       if (sc->sc_state == UMB_S_UP)
+               task_add(systq, &sc->sc_ctask);
+}
+#endif
+
 void
 umb_get_response_task(void *arg)
 {
@@ -1629,22 +1664,40 @@ umb_clear_addr(struct umb_softc *sc)
        struct ifnet *ifp = GET_IFP(sc);
 
        memset(sc->sc_info.ipv4dns, 0, sizeof (sc->sc_info.ipv4dns));
-       memset(sc->sc_info.ipv6dns, 0, sizeof (sc->sc_info.ipv6dns));
        umb_send_inet_proposal(sc, AF_INET);
-#ifdef INET6
-       umb_send_inet_proposal(sc, AF_INET6);
-#endif
        NET_LOCK();
        in_ifdetach(ifp);
 #ifdef INET6
-       in6_ifdetach(ifp);
+       /*
+        * Cannot call in6_ifdetach() here as it would have irrevertable side
+        * effects like clearing eui64 and autoconf6.
+        */
+       if (!IN6_ARE_ADDR_EQUAL(&sc->sc_addr6, &in6addr_any)) {
+               struct ifaddr *ifa, *next;
+
+               TAILQ_FOREACH_SAFE(ifa, &ifp->if_addrlist, ifa_list, next) {
+                       if (ifa->ifa_addr->sa_family == AF_INET6 &&
+                           IN6_ARE_ADDR_EQUAL(&sc->sc_addr6,
+                           &satosin6(ifa->ifa_addr)->sin6_addr)) {
+                               in6_purgeaddr(ifa);
+                               nd6_purge(ifp);
+                               break;
+                       }
+               }
+       }
 #endif
        NET_UNLOCK();
+
+#ifdef INET6
+       memcpy(&sc->sc_addr6, &in6addr_any, sizeof (sc->sc_addr6));
+       memcpy(&sc->sc_gw6, &in6addr_any, sizeof (sc->sc_gw6));
+       memset(sc->sc_info.ipv6dns, 0, sizeof (sc->sc_info.ipv6dns));
+       umb_send_inet_proposal(sc, AF_INET6);
+#endif
 }
 
 int
-umb_add_inet_config(struct umb_softc *sc, struct in_addr ip, u_int prefixlen,
-    struct in_addr gw)
+umb_add_inet_config(struct umb_softc *sc)
 {
        struct ifnet *ifp = GET_IFP(sc);
        struct in_aliasreq ifra;
@@ -1657,17 +1710,17 @@ umb_add_inet_config(struct umb_softc *sc
        sin = &ifra.ifra_addr;
        sin->sin_family = AF_INET;
        sin->sin_len = sizeof (*sin);
-       sin->sin_addr = ip;
+       sin->sin_addr = sc->sc_addr;
 
        sin = &ifra.ifra_dstaddr;
        sin->sin_family = AF_INET;
        sin->sin_len = sizeof (*sin);
-       sin->sin_addr = gw;
+       sin->sin_addr = sc->sc_gw;
 
        sin = &ifra.ifra_mask;
        sin->sin_family = AF_INET;
        sin->sin_len = sizeof (*sin);
-       in_len2mask(&sin->sin_addr, prefixlen);
+       in_len2mask(&sin->sin_addr, sc->sc_prefixlen);
 
        rv = in_ioctl(SIOCAIFADDR, (caddr_t)&ifra, ifp, 1);
        if (rv != 0) {
@@ -1718,8 +1771,7 @@ umb_add_inet_config(struct umb_softc *sc
 
 #ifdef INET6
 int
-umb_add_inet6_config(struct umb_softc *sc, struct in6_addr *ip, u_int 
prefixlen,
-    struct in6_addr *gw)
+umb_add_inet6_config(struct umb_softc *sc)
 {
        struct ifnet *ifp = GET_IFP(sc);
        struct in6_aliasreq ifra;
@@ -1732,20 +1784,20 @@ umb_add_inet6_config(struct umb_softc *s
        sin6 = &ifra.ifra_addr;
        sin6->sin6_family = AF_INET6;
        sin6->sin6_len = sizeof (*sin6);
-       memcpy(&sin6->sin6_addr, ip, sizeof (sin6->sin6_addr));
+       memcpy(&sin6->sin6_addr, &sc->sc_addr6, sizeof (sin6->sin6_addr));
 
        sin6 = &ifra.ifra_dstaddr;
        sin6->sin6_family = AF_INET6;
        sin6->sin6_len = sizeof (*sin6);
-       memcpy(&sin6->sin6_addr, gw, sizeof (sin6->sin6_addr));
+       memcpy(&sin6->sin6_addr, &sc->sc_gw6, sizeof (sin6->sin6_addr));
 
        /* XXX: in6_update_ifa() accepts only 128 bits for P2P interfaces. */
-       prefixlen = 128;
+       sc->sc_prefixlen6 = 128;
 
        sin6 = &ifra.ifra_prefixmask;
        sin6->sin6_family = AF_INET6;
        sin6->sin6_len = sizeof (*sin6);
-       in6_prefixlen2mask(&sin6->sin6_addr, prefixlen);
+       in6_prefixlen2mask(&sin6->sin6_addr, sc->sc_prefixlen6);
 
        ifra.ifra_lifetime.ia6t_vltime = ND6_INFINITE_LIFETIME;
        ifra.ifra_lifetime.ia6t_pltime = ND6_INFINITE_LIFETIME;
@@ -1838,6 +1890,72 @@ umb_send_inet_proposal(struct umb_softc 
 }
 
 int
+umb_configure(struct umb_softc *sc)
+{
+       struct ifnet *ifp = GET_IFP(sc);
+       struct ifaddr *ifa;
+       int      s;
+       int      configured = 0;
+       int      config_found;
+#ifdef INET6
+       int      want_v6, have_v6;
+#endif
+
+       if ((ifp->if_flags & IFF_UP) == 0)
+               return 0;
+
+       s = splnet();
+       if (sc->sc_addr.s_addr != INADDR_ANY) {
+               config_found = 0;
+               TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
+                       if (ifa->ifa_addr->sa_family == AF_INET &&
+                           satosin(ifa->ifa_addr)->sin_addr.s_addr ==
+                           sc->sc_addr.s_addr) {
+                               config_found = 1;
+                               configured++;
+                               break;
+                       }
+               }
+               if (!config_found) {
+                       if (umb_add_inet_config(sc) == 0)
+                               configured++;
+                       umb_send_inet_proposal(sc, AF_INET);
+               }
+       }
+
+#ifdef INET6
+       if ((sc->sc_flags & UMBFLG_NO_INET6) == 0) {
+               want_v6 = in6ifa_ifpforlinklocal(ifp, 0) != NULL;
+               have_v6 = !IN6_ARE_ADDR_EQUAL(&sc->sc_addr6, &in6addr_any);
+               config_found = 0;
+               if (have_v6) {
+                       TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
+                               if (ifa->ifa_addr->sa_family == AF_INET6 &&
+                                   IN6_ARE_ADDR_EQUAL(&sc->sc_addr6,
+                                   &satosin6(ifa->ifa_addr)->sin6_addr)) {
+                                       config_found = 1;
+                                       configured++;
+                                       break;
+                               }
+                       }
+               }
+               if (want_v6 && have_v6 && !config_found) {
+                       if (umb_add_inet6_config(sc) == 0)
+                               configured++;
+                       umb_send_inet_proposal(sc, AF_INET6);
+               } else if (!want_v6 && config_found) {
+                       NET_LOCK();
+                       in6_ifdetach(ifp);
+                       NET_UNLOCK();
+               }
+       }
+#endif
+
+       splx(s);
+       return configured;
+}
+
+int
 umb_decode_ip_configuration(struct umb_softc *sc, void *data, int len)
 {
        struct mbim_cid_ip_configuration_info *ic = data;
@@ -1849,13 +1967,11 @@ umb_decode_ip_configuration(struct umb_s
        int      off;
        struct mbim_cid_ipv4_element ipv4elem;
        struct in_addr addr, gw;
-       int      state = -1;
-       int      rv;
        int      hasmtu = 0;
 #ifdef INET6
        uint32_t avail_v6;
        struct mbim_cid_ipv6_element ipv6elem;
-       struct in6_addr addr6, gw6;
+       struct in6_addr addr6;
 #endif
 
        if (len < sizeof (*ic))
@@ -1867,12 +1983,13 @@ umb_decode_ip_configuration(struct umb_s
        }
        s = splnet();
 
-       memset(sc->sc_info.ipv4dns, 0, sizeof (sc->sc_info.ipv4dns));
-       memset(sc->sc_info.ipv6dns, 0, sizeof (sc->sc_info.ipv6dns));
-
        /*
         * IPv4 configuation
         */
+       memset(&sc->sc_addr, 0, sizeof (sc->sc_addr));
+       memset(&sc->sc_gw, 0, sizeof (sc->sc_gw));
+       memset(sc->sc_info.ipv4dns, 0, sizeof (sc->sc_info.ipv4dns));
+
        avail_v4 = letoh32(ic->ipv4_available);
        if ((avail_v4 & (MBIM_IPCONF_HAS_ADDRINFO | MBIM_IPCONF_HAS_GWINFO)) ==
            (MBIM_IPCONF_HAS_ADDRINFO | MBIM_IPCONF_HAS_GWINFO)) {
@@ -1887,18 +2004,13 @@ umb_decode_ip_configuration(struct umb_s
 
                /* Only pick the first one */
                memcpy(&ipv4elem, data + off, sizeof (ipv4elem));
-               ipv4elem.prefixlen = letoh32(ipv4elem.prefixlen);
-               addr.s_addr = ipv4elem.addr;
+               sc->sc_prefixlen = letoh32(ipv4elem.prefixlen);
+               sc->sc_addr.s_addr = ipv4elem.addr;
 
                off = letoh32(ic->ipv4_gwoffs);
                if (off + sizeof (gw) > len)
                        goto done;
-               memcpy(&gw, data + off, sizeof(gw));
-
-               rv = umb_add_inet_config(sc, addr, ipv4elem.prefixlen, gw);
-               if (rv == 0) 
-                       state = UMB_S_UP;
-
+               memcpy(&sc->sc_gw, data + off, sizeof(sc->sc_gw));
        }
 
        memset(sc->sc_info.ipv4dns, 0, sizeof (sc->sc_info.ipv4dns));
@@ -1920,7 +2032,6 @@ umb_decode_ip_configuration(struct umb_s
                                    &addr, str, sizeof(str)));
                        }
                }
-               umb_send_inet_proposal(sc, AF_INET);
        }
        if ((avail_v4 & MBIM_IPCONF_HAS_MTUINFO)) {
                val = letoh32(ic->ipv4_mtu);
@@ -1937,6 +2048,10 @@ tryv6:;
        /*
         * IPv6 configuation
         */
+       memcpy(&sc->sc_addr6, &in6addr_any, sizeof (sc->sc_addr6));
+       memcpy(&sc->sc_gw6, &in6addr_any, sizeof (sc->sc_gw6));
+       memset(sc->sc_info.ipv6dns, 0, sizeof (sc->sc_info.ipv6dns));
+
        avail_v6 = letoh32(ic->ipv6_available);
        if (avail_v6 == 0) {
                if (ifp->if_flags & IFF_DEBUG)
@@ -1945,7 +2060,7 @@ tryv6:;
                goto done;
        }
 
-       if ((avail_v4 & (MBIM_IPCONF_HAS_ADDRINFO | MBIM_IPCONF_HAS_GWINFO)) ==
+       if ((avail_v6 & (MBIM_IPCONF_HAS_ADDRINFO | MBIM_IPCONF_HAS_GWINFO)) ==
            (MBIM_IPCONF_HAS_ADDRINFO | MBIM_IPCONF_HAS_GWINFO)) {
                n = letoh32(ic->ipv6_naddr);
                off = letoh32(ic->ipv6_addroffs);
@@ -1958,16 +2073,13 @@ tryv6:;
 
                /* Only pick the first one */
                memcpy(&ipv6elem, data + off, sizeof (ipv6elem));
-               memcpy(&addr6, ipv6elem.addr, sizeof (addr6));
+               memcpy(&sc->sc_addr6, ipv6elem.addr, sizeof (sc->sc_addr6));
+               sc->sc_prefixlen6 = letoh32(ipv6elem.prefixlen);
 
                off = letoh32(ic->ipv6_gwoffs);
-               if (off + sizeof (gw6) > len)
+               if (off + sizeof (sc->sc_gw6) > len)
                        goto done;
-               memcpy(&gw6, data + off, sizeof (gw6));
-
-               rv = umb_add_inet6_config(sc, &addr6, ipv6elem.prefixlen, &gw6);
-               if (rv == 0)
-                       state = UMB_S_UP;
+               memcpy(&sc->sc_gw6, data + off, sizeof (sc->sc_gw6));
        }
 
        if (avail_v6 & MBIM_IPCONF_HAS_DNSINFO) {
@@ -1988,7 +2100,6 @@ tryv6:;
                                    &addr6, str, sizeof(str)));
                        }
                }
-               umb_send_inet_proposal(sc, AF_INET6);
        }
 
        if ((avail_v6 & MBIM_IPCONF_HAS_MTUINFO)) {
@@ -2006,9 +2117,8 @@ done:
        if (hasmtu && (ifp->if_flags & IFF_DEBUG))
                log(LOG_INFO, "%s: MTU %d\n", DEVNAM(sc), ifp->if_hardmtu);
 
-       if (state != -1)
-               umb_newstate(sc, state, 0);
-
+       if (umb_configure(sc))
+               umb_newstate(sc, UMB_S_UP, 0);
        splx(s);
        return 1;
 }
@@ -2583,8 +2693,7 @@ umb_send_connect(struct umb_softc *sc, i
        c->iptype = htole32(MBIM_CONTEXT_IPTYPE_IPV4);
 #ifdef INET6
        /* XXX FIXME: support IPv6-only mode, too */
-       if ((sc->sc_flags & UMBFLG_NO_INET6) == 0 &&
-           in6ifa_ifpforlinklocal(GET_IFP(sc), 0) != NULL)
+       if ((sc->sc_flags & UMBFLG_NO_INET6) == 0)
                c->iptype = htole32(MBIM_CONTEXT_IPTYPE_IPV4V6);
 #endif
        memcpy(c->context, umb_uuid_context_internet, sizeof (c->context));
Index: sys/dev/usb/if_umb.h
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_umb.h,v
retrieving revision 1.6
diff -u -p -u -p -r1.6 if_umb.h
--- sys/dev/usb/if_umb.h        18 Feb 2020 08:09:37 -0000      1.6
+++ sys/dev/usb/if_umb.h        29 Apr 2020 14:26:15 -0000
@@ -355,6 +355,18 @@ struct umb_softc {
        int                      sc_nresp;
        struct timeout           sc_statechg_timer;
 
+       struct in_addr           sc_addr;
+       struct in_addr           sc_gw;
+       int                      sc_prefixlen;
+
+#ifdef INET6
+       struct task              sc_atask;
+       struct task              sc_ctask;
+       struct in6_addr          sc_addr6;
+       struct in6_addr          sc_gw6;
+       int                      sc_prefixlen6;
+#endif
+
        uint8_t                  sc_ctrl_ifaceno;
        struct usbd_pipe        *sc_ctrl_pipe;
        struct usb_cdc_notification sc_intr_msg;

Reply via email to