On Thu, Feb 13, 2025 at 08:13:27PM +0300, Vitaliy Makkoveev wrote: > > On 13 Feb 2025, at 19:33, Claudio Jeker <cje...@diehard.n-r-g.com> wrote: > > > > On Wed, Feb 12, 2025 at 06:47:36PM +0300, Vitaliy Makkoveev wrote: > >> Hi, > >> > >> This diff serializes tun(4)/tap(4) interfaces creation and destruction > >> from ifconfig(8) and device nodes path. Not only simplifies, but fixes > >> syzkaller [1] report. > >> > >> Can I ask openvpn or vmd(8) users to test it and prove that there is no > >> behavior changes? > > > > Just to be clear the diff changes the way tun(4) and tap(4) interfaces are > > created and destroyed. So it matters on how the interface is created and > > if it does behave properly. > > The original creation and destruction paths of tun(4) are full of > sleep points, so those paths have dances to protect half-created > or half-destroyed interface. This diff uses the dedicated rwlock > for cloners in the device nodes handlers to make races impossible. > > > I think the things to test are: > > - If you open /dev/tun1 is tun1 created and if you close /dev/tun1 is > > it destroyed again. > > It works. > > > - If tun1 alreay exists on open of /dev/tun1 then the last close of > > /dev/tun1 should not destroy tun1. > > This is not the current behaviour. The TUN_STAYUP exists to prevent > the concurrent destruction from tun_dev_close() while we are in the > middle of tun_dev_open(). This could happen from the VOP_REWOKE() > path of tun_clone_destroy(). In this case the interface will be > destroyed, but not by tun_dev_close().
It is very much the current behaviour. I use this every day. I have preconfigured tap0 and tap1 interfaces that are always around. So this is very much current behaviour. > > - If you have /dev/tun1 open and destroy tun1 (the interface) are you > > notified that /dev/tun1 is no longer available. > > > > Only tun_dev_read() should be notified, and yes, it works. > > > I think there are other cases to consider, those are just the obvious ones > > that I remember. I glanced at the diff, it changes a hell of a lot and the > > bit that scares me is that TUN_STAYUP becomes unused and looking at > > tun_dev_close() I think this is indeed not right. In other words I think > > the 2nd bit from the list above is violated. > > As I previously said, the TUN_STAYUP bit required only to prevent > concurrent destruction in the middle of tun_dev_open(). Now > concurrent destruction is impossible. I don't think your right at least I always understood that TUN_STAYUP was added exactly to prevent the destruction of an interface that was created through interface cloning. -- :wq Claudio
