On Thu, Feb 24, 2022 at 11:13:48AM +0100, Claudio Jeker wrote:
> On Thu, Feb 24, 2022 at 07:39:54PM +1000, David Gwynne wrote:
> >
> > here's the diff.
> >
> > Index: if_tun.c
> > ===================================================================
> > RCS file: /cvs/src/sys/net/if_tun.c,v
> > retrieving revision 1.234
> > diff -u -p -r1.234 if_tun.c
> > --- if_tun.c 16 Feb 2022 02:22:39 -0000 1.234
> > +++ if_tun.c 24 Feb 2022 08:08:38 -0000
> > @@ -374,10 +374,19 @@ tun_dev_open(dev_t dev, const struct if_
> > struct ifnet *ifp;
> > int error;
> > u_short stayup = 0;
> > + struct vnode *vp;
> >
>
> Why is there this empty line? It was there before but still wondering.
feng shui? laziness? i'll fix it later.
> > char name[IFNAMSIZ];
> > unsigned int rdomain;
> >
> > + /*
> > + * Find the vnode associated with this open before we sleep
> > + * and let something else revoke it. Our caller has a reference
> > + * to it so we don't need to account for it.
> > + */
> > + if (!vfinddev(dev, VCHR, &vp))
> > + panic("%s vfinddev failed", __func__);
> > +
> > snprintf(name, sizeof(name), "%s%u", ifc->ifc_name, minor(dev));
> > rdomain = rtable_l2(p->p_p->ps_rtableid);
> >
> > @@ -412,6 +421,12 @@ tun_dev_open(dev_t dev, const struct if_
> > /* XXX if_clone_destroy if stayup? */
> > goto done;
> > }
> > + }
> > +
> > + /* Has tun_clone_destroy torn the rug out under us? */
> > + if (vp->v_type == VBAD) {
> > + error = ENXIO;
> > + goto done;
> > }
> >
> > if (sc->sc_dev != 0) {
> >
>
> OK claudio@
>
> After sleeping over this I think this is the cleanest and simplest way
> around this problem. A bit ugly that tun needs to peek into the vnode.
>
> Another option is to split the clone destroy from the softc / device node.
> Remove the VOP_REVOKE and actually allow tun to be destroyed and recreated
> while open and in that case the device remains open and only the network
> bits are destroyed and later recreated. So in your example from above:
>
> > > - ifconfig tun0 create
> > > - open /dev/tun0 -> fd 3
> > > - ifconfig tun0 destroy
> > > - ifconfig tun0 create
> > > - write to fd 3
>
> The write would be perfectly fine since the destroy did not distroy this
> connection (only close(2) would do that). Actually a call to open
> /dev/tun0 after the 2nd create would fail because the device is still
> open. Doing this seems a lot more complex.
we talked about this a lot around the time of src/sys/net/if_tun.c
r1.210. there seemed to be more weight on the side of the argument for
VOP_REVOKE than against, and i still think that's the case now. tun
going away and coming back in between open and write could go either
way, but what about these:
- open() /dev/tun0, tun0 is destroyed, write() to tun0
should the write error? if we run with "only close can destroy the
connection" does this mean the write will create tun0 again? in which
rdomain should it be?
- open() /dev/tun0, tun0 is destroyed, read() from tun0
same as above?
- begin blocking read of tun0, tun0 is destroyed, let's go shopping!
should the read wake up and return an error, or does it just block?
- poll on tun0, tun0 is destroyed
same as above?
if we leave the /dev side of things operational if the interface goes
away, then this would be inconsistent with something workign with bpf on
the same interfaces. wouldnt this be inconsistent with hotplug devices
and their /dev things?
maybe destroy should be blocked if the /dev entry is open?
i dunno. it feels more natural that detaching or destroying an interface
should push userland off it.
