On Tue, Feb 15, 2022 at 02:35:33PM +1000, David Gwynne wrote:
> if an open tun (or tap) device is destroyed via ifconfig destroy,
> there is a window while the open device is being revoked on the vfs
> side that a third thread can come and open it again. this in turn
> triggers a kassert in the ifconfig destroy path where it expects the
> device to be closed.
>
> i think this diff fixes it by having the open code refuse to work with a
> device that's in the process of being destroyed.
>
> this was found by syzkaller. the detail is at
> https://syzkaller.appspot.com/bug?id=1d1ea7860c36e5066edea1145cf2d56715d5042b.
>
> i was able to reproduce the problem with this code. it was pretty hard
> to hit, but ive had no luck since adding the TUN_DEAD handling to
> tun_dev_open.
The diff looks good to me.
OK visa@
> Index: if_tun.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_tun.c,v
> retrieving revision 1.233
> diff -u -p -r1.233 if_tun.c
> --- if_tun.c 15 Feb 2022 04:19:52 -0000 1.233
> +++ if_tun.c 15 Feb 2022 04:21:53 -0000
> @@ -218,6 +218,8 @@ tun_create(struct if_clone *ifc, int uni
> KERNEL_ASSERT_LOCKED();
>
> sc = malloc(sizeof(*sc), M_DEVBUF, M_WAITOK|M_ZERO);
> + refcnt_init(&sc->sc_refs);
> +
> ifp = &sc->sc_if;
> snprintf(ifp->if_xname, sizeof(ifp->if_xname),
> "%s%d", ifc->ifc_name, unit);
> @@ -267,7 +269,6 @@ tun_create(struct if_clone *ifc, int uni
> }
>
> sigio_init(&sc->sc_sigio);
> - refcnt_init(&sc->sc_refs);
>
> /* tell tun_dev_open we're initialised */
>
> @@ -381,7 +382,7 @@ tun_dev_open(dev_t dev, const struct if_
> rdomain = rtable_l2(p->p_p->ps_rtableid);
>
> /* let's find or make an interface to work with */
> - while ((ifp = if_unit(name)) == NULL) {
> + while ((sc = tun_name_lookup(name)) == NULL) {
> error = if_clone_create(name, rdomain);
> switch (error) {
> case 0: /* it's probably ours */
> @@ -394,34 +395,45 @@ tun_dev_open(dev_t dev, const struct if_
> }
> }
>
> - sc = ifp->if_softc;
> + refcnt_take(&sc->sc_refs);
> +
> /* wait for it to be fully constructed before we use it */
> - while (!ISSET(sc->sc_flags, TUN_INITED)) {
> + for (;;) {
> + if (ISSET(sc->sc_flags, TUN_DEAD)) {
> + error = ENXIO;
> + goto done;
> + }
> +
> + if (ISSET(sc->sc_flags, TUN_INITED))
> + break;
> +
> error = tsleep_nsec(sc, PCATCH, "tuninit", INFSLP);
> if (error != 0) {
> /* XXX if_clone_destroy if stayup? */
> - if_put(ifp);
> - return (error);
> + goto done;
> }
> }
>
> if (sc->sc_dev != 0) {
> /* aww, we lost */
> - if_put(ifp);
> - return (EBUSY);
> + error = EBUSY;
> + goto done;
> }
> /* it's ours now */
> sc->sc_dev = dev;
> CLR(sc->sc_flags, stayup);
>
> /* automatically mark the interface running on open */
> + ifp = &sc->sc_if;
> NET_LOCK();
> SET(ifp->if_flags, IFF_UP | IFF_RUNNING);
> NET_UNLOCK();
> tun_link_state(ifp, LINK_STATE_FULL_DUPLEX);
> - if_put(ifp);
> + error = 0;
>
> - return (0);
> +done:
> + tun_put(sc);
> + return (error);
> }
>
> /*
>
