syzkaller managed to trigger one of the assertions added by this
diff.
https://syzkaller.appspot.com/bug?id=2f4de8101553f64fcf847f8ed15cd1862b355122
On Thu, Nov 07, 2019 at 09:22:17PM +1000, David Gwynne wrote:
> this applies the use of tasks and a task_list to interface address
> hooks. it's like the detach and linkstate hooks, except it seems other
> things run the hooks more than things register hooks, and i can't tell
> if the places that run the hooks have the NET_LOCK or not. not by casual
> reading anyway.
>
> to cope with if_addrhooks_run maybe not being called with NET_LOCK being
> held, i made it safe to call the hook runner multiple times
> concurrently.
>
> one of the users of address hooks is pf, and the pfi_kif struct. it's
> part of the ABI, pfctl and snmpd use it, so i kept it using a void * and
> had it allocate the task separately. it should be as robust as it was
> before.
>
> everything else was pretty straightforward.
>
> tests? ok?
>
> Index: kern/kern_task.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_task.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 kern_task.c
> --- kern/kern_task.c 23 Jun 2019 12:56:10 -0000 1.26
> +++ kern/kern_task.c 7 Nov 2019 11:21:00 -0000
> @@ -258,6 +258,8 @@ taskq_barrier_task(void *p)
> void
> task_set(struct task *t, void (*fn)(void *), void *arg)
> {
> + KASSERT(fn != NULL);
> +
> t->t_func = fn;
> t->t_arg = arg;
> t->t_flags = 0;
> Index: net/if.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.591
> diff -u -p -r1.591 if.c
> --- net/if.c 7 Nov 2019 08:03:18 -0000 1.591
> +++ net/if.c 7 Nov 2019 11:21:00 -0000
> @@ -630,9 +630,7 @@ if_attach_common(struct ifnet *ifp)
> ifp->if_iqs = ifp->if_rcv.ifiq_ifiqs;
> ifp->if_niqs = 1;
>
> - ifp->if_addrhooks = malloc(sizeof(*ifp->if_addrhooks),
> - M_TEMP, M_WAITOK);
> - TAILQ_INIT(ifp->if_addrhooks);
> + TAILQ_INIT(&ifp->if_addrhooks);
> TAILQ_INIT(&ifp->if_linkstatehooks);
> TAILQ_INIT(&ifp->if_detachhooks);
>
> @@ -1046,19 +1044,18 @@ if_netisr(void *unused)
> void
> if_hooks_run(struct task_list *hooks)
> {
> - struct task *t, *nt, cursor;
> + struct task *t, *nt;
> + struct task cursor = { .t_func = NULL };
> void (*func)(void *);
> void *arg;
>
> - /*
> - * holding the NET_LOCK guarantees that concurrent if_hooks_run
> - * calls can't happen, and they therefore can't try and call
> - * each others cursors as actual hooks.
> - */
> - NET_ASSERT_LOCKED();
> -
> mtx_enter(&if_hooks_mtx);
> for (t = TAILQ_FIRST(hooks); t != NULL; t = nt) {
> + while (t->t_func == NULL) { /* skip cursors */
> + t = TAILQ_NEXT(t, t_entry);
> + if (t == NULL)
> + break;
> + }
> func = t->t_func;
> arg = t->t_arg;
>
> @@ -1177,7 +1174,7 @@ if_detach(struct ifnet *ifp)
> }
> }
>
> - free(ifp->if_addrhooks, M_TEMP, sizeof(*ifp->if_addrhooks));
> + KASSERT(TAILQ_EMPTY(&ifp->if_addrhooks));
> KASSERT(TAILQ_EMPTY(&ifp->if_linkstatehooks));
> KASSERT(TAILQ_EMPTY(&ifp->if_detachhooks));
>
> @@ -3100,7 +3097,7 @@ ifnewlladdr(struct ifnet *ifp)
> ifa = &in6ifa_ifpforlinklocal(ifp, 0)->ia_ifa;
> if (ifa) {
> in6_purgeaddr(ifa);
> - dohooks(ifp->if_addrhooks, 0);
> + if_hooks_run(&ifp->if_addrhooks);
> in6_ifattach(ifp);
> }
> }
> @@ -3112,6 +3109,28 @@ ifnewlladdr(struct ifnet *ifp)
> (*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, (caddr_t)&ifrq);
> }
> splx(s);
> +}
> +
> +void
> +if_addrhook_add(struct ifnet *ifp, struct task *t)
> +{
> + mtx_enter(&if_hooks_mtx);
> + TAILQ_INSERT_TAIL(&ifp->if_addrhooks, t, t_entry);
> + mtx_leave(&if_hooks_mtx);
> +}
> +
> +void
> +if_addrhook_del(struct ifnet *ifp, struct task *t)
> +{
> + mtx_enter(&if_hooks_mtx);
> + TAILQ_REMOVE(&ifp->if_addrhooks, t, t_entry);
> + mtx_leave(&if_hooks_mtx);
> +}
> +
> +void
> +if_addrhooks_run(struct ifnet *ifp)
> +{
> + if_hooks_run(&ifp->if_addrhooks);
> }
>
> int net_ticks;
> Index: net/if_pppx.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.68
> diff -u -p -r1.68 if_pppx.c
> --- net/if_pppx.c 24 Jun 2019 13:43:19 -0000 1.68
> +++ net/if_pppx.c 7 Nov 2019 11:21:00 -0000
> @@ -919,7 +919,7 @@ pppx_add_session(struct pppx_dev *pxd, s
> printf("pppx: unable to set addresses for %s, error=%d\n",
> ifp->if_xname, error);
> } else {
> - dohooks(ifp->if_addrhooks, 0);
> + if_addrhooks_run(ifp);
> }
> rw_enter_write(&pppx_ifs_lk);
> pxi->pxi_ready = 1;
> Index: net/if_spppsubr.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_spppsubr.c,v
> retrieving revision 1.179
> diff -u -p -r1.179 if_spppsubr.c
> --- net/if_spppsubr.c 24 Jun 2019 21:36:53 -0000 1.179
> +++ net/if_spppsubr.c 7 Nov 2019 11:21:00 -0000
> @@ -4230,7 +4230,7 @@ sppp_set_ip_addrs(void *arg1)
> }
> }
> if (!(error = in_ifinit(ifp, ifatoia(ifa), &new_sin, 0)))
> - dohooks(ifp->if_addrhooks, 0);
> + if_addrhooks_run(ifp);
> if (debug && error) {
> log(LOG_DEBUG, SPP_FMT "sppp_set_ip_addrs: in_ifinit "
> " failed, error=%d\n", SPP_ARGS(ifp), error);
> @@ -4290,7 +4290,7 @@ sppp_clear_ip_addrs(void *arg1)
> /* replace peer addr in place */
> dest->sin_addr.s_addr = sp->ipcp.saved_hisaddr;
> if (!(error = in_ifinit(ifp, ifatoia(ifa), &new_sin, 0)))
> - dohooks(ifp->if_addrhooks, 0);
> + if_addrhooks_run(ifp);
> if (debug && error) {
> log(LOG_DEBUG, SPP_FMT "sppp_clear_ip_addrs: in_ifinit "
> " failed, error=%d\n", SPP_ARGS(ifp), error);
> Index: net/if_var.h
> ===================================================================
> RCS file: /cvs/src/sys/net/if_var.h,v
> retrieving revision 1.102
> diff -u -p -r1.102 if_var.h
> --- net/if_var.h 7 Nov 2019 07:36:32 -0000 1.102
> +++ net/if_var.h 7 Nov 2019 11:21:00 -0000
> @@ -124,7 +124,7 @@ struct ifnet { /* and the
> entries */
> TAILQ_HEAD(, ifaddr) if_addrlist; /* [N] list of addresses per if */
> TAILQ_HEAD(, ifmaddr) if_maddrlist; /* [N] list of multicast records */
> TAILQ_HEAD(, ifg_list) if_groups; /* [N] list of groups per if */
> - struct hook_desc_head *if_addrhooks; /* [I] address change callbacks */
> + struct task_list if_addrhooks; /* [I] address change callbacks */
> struct task_list if_linkstatehooks; /* [I] link change callbacks*/
> struct task_list if_detachhooks; /* [I] detach callbacks */
> /* [I] check or clean routes (+ or -)'d */
> @@ -374,10 +374,13 @@ void if_ih_insert(struct ifnet *, int (*
> void if_ih_remove(struct ifnet *, int (*)(struct ifnet *, struct mbuf *,
> void *), void *);
>
> -void if_detachhook_add(struct ifnet *, struct task *);
> -void if_detachhook_del(struct ifnet *, struct task *);
> +void if_addrhook_add(struct ifnet *, struct task *);
> +void if_addrhook_del(struct ifnet *, struct task *);
> +void if_addrhooks_run(struct ifnet *);
> void if_linkstatehook_add(struct ifnet *, struct task *);
> void if_linkstatehook_del(struct ifnet *, struct task *);
> +void if_detachhook_add(struct ifnet *, struct task *);
> +void if_detachhook_del(struct ifnet *, struct task *);
>
> void if_rxr_livelocked(struct if_rxring *);
> void if_rxr_init(struct if_rxring *, u_int, u_int);
> Index: net/if_vxlan.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_vxlan.c,v
> retrieving revision 1.75
> diff -u -p -r1.75 if_vxlan.c
> --- net/if_vxlan.c 7 Nov 2019 07:36:32 -0000 1.75
> +++ net/if_vxlan.c 7 Nov 2019 11:21:00 -0000
> @@ -62,7 +62,7 @@ struct vxlan_softc {
> struct ifmedia sc_media;
>
> struct ip_moptions sc_imo;
> - void *sc_ahcookie;
> + struct task sc_atask;
> struct task sc_ltask;
> struct task sc_dtask;
>
> @@ -138,6 +138,7 @@ vxlan_clone_create(struct if_clone *ifc,
> sc->sc_vnetid = VXLAN_VNI_UNSET;
> sc->sc_txhprio = IFQ_TOS2PRIO(IPTOS_PREC_ROUTINE); /* 0 */
> sc->sc_df = htons(0);
> + task_set(&sc->sc_atask, vxlan_addr_change, sc);
> task_set(&sc->sc_ltask, vxlan_link_change, sc);
> task_set(&sc->sc_dtask, vxlan_if_change, sc);
> task_set(&sc->sc_sendtask, vxlan_send_dispatch, sc);
> @@ -216,10 +217,7 @@ vxlan_multicast_cleanup(struct ifnet *if
>
> mifp = if_get(imo->imo_ifidx);
> if (mifp != NULL) {
> - if (sc->sc_ahcookie != NULL) {
> - hook_disestablish(mifp->if_addrhooks, sc->sc_ahcookie);
> - sc->sc_ahcookie = NULL;
> - }
> + if_addrhook_del(mifp, &sc->sc_atask);
> if_linkstatehook_del(mifp, &sc->sc_ltask);
> if_detachhook_del(mifp, &sc->sc_dtask);
>
> @@ -291,12 +289,9 @@ vxlan_multicast_join(struct ifnet *ifp,
> * Use interface hooks to track any changes on the interface
> * that is used to send out the tunnel traffic as multicast.
> */
> + if_addrhook_add(mifp, &sc->sc_atask);
> if_linkstatehook_add(mifp, &sc->sc_ltask);
> if_detachhook_add(mifp, &sc->sc_dtask);
> - if ((sc->sc_ahcookie = hook_establish(mifp->if_addrhooks,
> - 0, vxlan_addr_change, sc)) == NULL)
> - panic("%s: cannot allocate interface hook",
> - mifp->if_xname);
>
> return (0);
> }
> Index: net/pf_if.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf_if.c,v
> retrieving revision 1.97
> diff -u -p -r1.97 pf_if.c
> --- net/pf_if.c 9 Jul 2019 11:30:19 -0000 1.97
> +++ net/pf_if.c 7 Nov 2019 11:21:00 -0000
> @@ -235,6 +235,7 @@ void
> pfi_attach_ifnet(struct ifnet *ifp)
> {
> struct pfi_kif *kif;
> + struct task *t;
>
> pfi_initialize();
> pfi_update++;
> @@ -244,10 +245,10 @@ pfi_attach_ifnet(struct ifnet *ifp)
> kif->pfik_ifp = ifp;
> ifp->if_pf_kif = (caddr_t)kif;
>
> - if ((kif->pfik_ah_cookie = hook_establish(ifp->if_addrhooks, 1,
> - pfi_kifaddr_update, kif)) == NULL)
> - panic("pfi_attach_ifnet: cannot allocate '%s' address hook",
> - ifp->if_xname);
> + t = malloc(sizeof(*t), PFI_MTYPE, M_WAITOK);
> + task_set(t, pfi_kifaddr_update, kif);
> + if_addrhook_add(ifp, t);
> + kif->pfik_ah_cookie = t;
>
> pfi_kif_update(kif);
> }
> @@ -256,12 +257,14 @@ void
> pfi_detach_ifnet(struct ifnet *ifp)
> {
> struct pfi_kif *kif;
> + struct task *t;
>
> if ((kif = (struct pfi_kif *)ifp->if_pf_kif) == NULL)
> return;
>
> pfi_update++;
> - hook_disestablish(ifp->if_addrhooks, kif->pfik_ah_cookie);
> + t = kif->pfik_ah_cookie;
> + if_addrhook_del(ifp, t);
> pfi_kif_update(kif);
>
> kif->pfik_ifp = NULL;
> Index: netinet/in.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/in.c,v
> retrieving revision 1.164
> diff -u -p -r1.164 in.c
> --- netinet/in.c 23 Oct 2019 19:58:32 -0000 1.164
> +++ netinet/in.c 7 Nov 2019 11:21:00 -0000
> @@ -371,7 +371,7 @@ in_ioctl_sifaddr(u_long cmd, caddr_t dat
> in_ifscrub(ifp, ia);
> error = in_ifinit(ifp, ia, satosin(&ifr->ifr_addr), newifaddr);
> if (!error)
> - dohooks(ifp->if_addrhooks, 0);
> + if_addrhooks_run(ifp);
>
> NET_UNLOCK();
> return error;
> @@ -484,7 +484,7 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
> if (error)
> break;
> }
> - dohooks(ifp->if_addrhooks, 0);
> + if_addrhooks_run(ifp);
> break;
> }
> case SIOCDIFADDR:
> @@ -504,7 +504,7 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
> * the scrub but before the other steps?
> */
> in_purgeaddr(&ia->ia_ifa);
> - dohooks(ifp->if_addrhooks, 0);
> + if_addrhooks_run(ifp);
> break;
>
> default:
> @@ -923,7 +923,7 @@ in_ifdetach(struct ifnet *ifp)
> if (ifa->ifa_addr->sa_family != AF_INET)
> continue;
> in_purgeaddr(ifa);
> - dohooks(ifp->if_addrhooks, 0);
> + if_addrhooks_run(ifp);
> }
>
> if (ifp->if_xflags & IFXF_AUTOCONF4)
> Index: netinet/ip_carp.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_carp.c,v
> retrieving revision 1.340
> diff -u -p -r1.340 ip_carp.c
> --- netinet/ip_carp.c 7 Nov 2019 07:36:32 -0000 1.340
> +++ netinet/ip_carp.c 7 Nov 2019 11:21:00 -0000
> @@ -131,7 +131,7 @@ struct carp_softc {
> struct arpcom sc_ac;
> #define sc_if sc_ac.ac_if
> #define sc_carpdev sc_ac.ac_if.if_carpdev
> - void *ah_cookie;
> + struct task sc_atask;
> struct task sc_ltask;
> struct task sc_dtask;
> struct ip_moptions sc_imo;
> @@ -808,6 +808,7 @@ carp_clone_create(struct if_clone *ifc,
> return (ENOMEM);
> }
>
> + task_set(&sc->sc_atask, carp_addr_updated, sc);
> task_set(&sc->sc_ltask, carp_carpdev_state, sc);
> task_set(&sc->sc_dtask, carpdetach, sc);
>
> @@ -841,8 +842,7 @@ carp_clone_create(struct if_clone *ifc,
> ifp->if_link_state = LINK_STATE_INVALID;
>
> /* Hook carp_addr_updated to cope with address and route changes. */
> - sc->ah_cookie = hook_establish(sc->sc_if.if_addrhooks, 0,
> - carp_addr_updated, sc);
> + if_addrhook_add(&sc->sc_if, &sc->sc_atask);
>
> return (0);
> }
> @@ -893,10 +893,10 @@ carp_clone_destroy(struct ifnet *ifp)
> {
> struct carp_softc *sc = ifp->if_softc;
>
> + if_addrhook_del(&sc->sc_if, &sc->sc_atask);
> +
> NET_LOCK();
> carpdetach(sc);
> - if (sc->ah_cookie != NULL)
> - hook_disestablish(sc->sc_if.if_addrhooks, sc->ah_cookie);
> NET_UNLOCK();
>
> ether_ifdetach(ifp);
> Index: netinet6/in6.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/in6.c,v
> retrieving revision 1.231
> diff -u -p -r1.231 in6.c
> --- netinet6/in6.c 22 Oct 2019 21:40:12 -0000 1.231
> +++ netinet6/in6.c 7 Nov 2019 11:21:00 -0000
> @@ -307,7 +307,7 @@ in6_ioctl_change_ifaddr(u_long cmd, cadd
> break;
> }
> in6_purgeaddr(&ia6->ia_ifa);
> - dohooks(ifp->if_addrhooks, 0);
> + if_addrhooks_run(ifp);
> break;
>
> case SIOCAIFADDR_IN6:
> @@ -366,13 +366,13 @@ in6_ioctl_change_ifaddr(u_long cmd, cadd
> nd6_dad_start(&ia6->ia_ifa);
>
> if (!newifaddr) {
> - dohooks(ifp->if_addrhooks, 0);
> + if_addrhooks_run(ifp);
> break;
> }
>
> plen = in6_mask2len(&ia6->ia_prefixmask.sin6_addr, NULL);
> if ((ifp->if_flags & IFF_LOOPBACK) || plen == 128) {
> - dohooks(ifp->if_addrhooks, 0);
> + if_addrhooks_run(ifp);
> break; /* No need to install a connected route. */
> }
>
> @@ -383,7 +383,7 @@ in6_ioctl_change_ifaddr(u_long cmd, cadd
> in6_purgeaddr(&ia6->ia_ifa);
> break;
> }
> - dohooks(ifp->if_addrhooks, 0);
> + if_addrhooks_run(ifp);
> break;
> }
>
> Index: netinet6/in6_ifattach.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/in6_ifattach.c,v
> retrieving revision 1.114
> diff -u -p -r1.114 in6_ifattach.c
> --- netinet6/in6_ifattach.c 21 Aug 2019 15:32:18 -0000 1.114
> +++ netinet6/in6_ifattach.c 7 Nov 2019 11:21:00 -0000
> @@ -289,7 +289,7 @@ in6_ifattach_linklocal(struct ifnet *ifp
> nd6_dad_start(&ia6->ia_ifa);
>
> if (ifp->if_flags & IFF_LOOPBACK) {
> - dohooks(ifp->if_addrhooks, 0);
> + if_addrhooks_run(ifp);
> return (0); /* No need to install a connected route. */
> }
>
> @@ -303,7 +303,7 @@ in6_ifattach_linklocal(struct ifnet *ifp
> in6_purgeaddr(&ia6->ia_ifa);
> return (error);
> }
> - dohooks(ifp->if_addrhooks, 0);
> + if_addrhooks_run(ifp);
>
> return (0);
> }
> @@ -419,7 +419,7 @@ in6_ifdetach(struct ifnet *ifp)
> if (ifa->ifa_addr->sa_family != AF_INET6)
> continue;
> in6_purgeaddr(ifa);
> - dohooks(ifp->if_addrhooks, 0);
> + if_addrhooks_run(ifp);
> }
>
> /*
