On Thu, Feb 24, 2022 at 08:56:59PM +1000, David Gwynne wrote:
> 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?
Normally, yes you would generate an error like EPIPE in such a case.
You may even need to send SIGPIPE but that is one of those nasty details.
> - open() /dev/tun0, tun0 is destroyed, read() from tun0
>
> same as above?
Idealy you would signal EOF with a return of 0 (again similar to how pipes
behave).
> - 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?
Again return 0.
> - poll on tun0, tun0 is destroyed
>
> same as above?
poll should signal POLLHUP | POLLIN (iirc that's what pipes do).
> 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, on the other hand it is how pipes, socketpairs and unix sockets
behave. As I said going down this road is possible but much more complex
because all these edgecases need to be defined. Also there may be bugs
triggered in userland apps with a tun(4) behaving like this.
> 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.
I think one can argue about benefits from either mode. The big problem is
that because of how if_clone locking works you can not block in the
destroy function or people will become very unhappy. You may be even able
to cause deadlocks.
As I said above I think your diff is the best solution to fix this race and
while implementing this without VOP_REVOKE is possible it is too complex.
I'm perfectly fine with the current behaviour and think it actually fits
better with user's mental image on how tun(4) should behave.
--
:wq Claudio
