Interface auto-cloning bug or feature? - FreeBSD

This is a discussion on Interface auto-cloning bug or feature? - FreeBSD ; Hi, I've noticed that stat/open call on /dev/tun always creates new interface, despite the fact that existing spare interfaces may be available. I believe that it's a bug, since the whole purpose of auto-cloning is to create new instance only ...

+ Reply to Thread
Results 1 to 8 of 8

Thread: Interface auto-cloning bug or feature?

  1. Interface auto-cloning bug or feature?

    Hi,

    I've noticed that stat/open call on /dev/tun always creates new
    interface, despite the fact that existing spare interfaces may be
    available. I believe that it's a bug, since the whole purpose of
    auto-cloning is to create new instance only when no existing one could
    be allocated. At least that's my reading of the manual page for the tun(4).


    If the sysctl(8) variable net.link.tun.devfs_cloning is non-zero,
    the tun
    interface permits opens on the special control device /dev/tun. When
    this device is opened, tun will return a handle for the lowest
    unused tun
    device (use devname(3) to determine which).


    -Maxim

    [root@sp1 /usr/home/sobomax]# ifconfig -a
    tun0: flags=8010 mtu 1500
    [root@sp1 /usr/home/sobomax]# stat /dev/tun
    67174144 96 crw------- 1 uucp dialer 96 0 "May 2 22:21:28 2008" "May 2
    22:21:28 2008" "May 2 22:21:28 2008" "Dec 31 23:59:59 1969" 4096 0 0
    /dev/tun
    [root@sp1 /usr/home/sobomax]# ifconfig -a
    tun0: flags=8010 mtu 1500
    tun1: flags=8010 mtu 1500
    [root@sp1 /usr/home/sobomax]# stat /dev/tun
    67174144 99 crw------- 1 uucp dialer 99 0 "May 2 22:21:28 2008" "May 2
    22:21:28 2008" "May 2 22:21:28 2008" "Dec 31 23:59:59 1969" 4096 0 0
    /dev/tun
    [root@sp1 /usr/home/sobomax]# ifconfig -a
    tun0: flags=8010 mtu 1500
    tun1: flags=8010 mtu 1500
    tun2: flags=8010 mtu 1500
    [root@sp1 /usr/home/sobomax]# stat /dev/tun
    67174144 105 crw------- 1 uucp dialer 105 0 "May 2 22:21:28 2008" "May
    2 22:21:28 2008" "May 2 22:21:28 2008" "Dec 31 23:59:59 1969" 4096 0
    0 /dev/tun
    [root@sp1 /usr/home/sobomax]# ifconfig -a
    tun0: flags=8010 mtu 1500
    tun1: flags=8010 mtu 1500
    tun2: flags=8010 mtu 1500
    tun3: flags=8010 mtu 1500

    _______________________________________________
    freebsd-current@freebsd.org mailing list
    http://lists.freebsd.org/mailman/lis...reebsd-current
    To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"


  2. Re: Interface auto-cloning bug or feature?

    On Thu, Sep 18, 2008 at 05:58:42PM -0700, Maxim Sobolev wrote:
    > Hi,
    >
    > I've noticed that stat/open call on /dev/tun always creates new
    > interface, despite the fact that existing spare interfaces may be
    > available. I believe that it's a bug, since the whole purpose of
    > auto-cloning is to create new instance only when no existing one could
    > be allocated. At least that's my reading of the manual page for the tun(4).
    >
    >
    > If the sysctl(8) variable net.link.tun.devfs_cloning is non-zero,
    > the tun
    > interface permits opens on the special control device /dev/tun. When
    > this device is opened, tun will return a handle for the lowest
    > unused tun
    > device (use devname(3) to determine which).
    >

    >
    > -Maxim
    >
    > [root@sp1 /usr/home/sobomax]# ifconfig -a
    > tun0: flags=8010 mtu 1500
    > [root@sp1 /usr/home/sobomax]# stat /dev/tun
    > 67174144 96 crw------- 1 uucp dialer 96 0 "May 2 22:21:28 2008" "May 2
    > 22:21:28 2008" "May 2 22:21:28 2008" "Dec 31 23:59:59 1969" 4096 0 0
    > /dev/tun
    > [root@sp1 /usr/home/sobomax]# ifconfig -a
    > tun0: flags=8010 mtu 1500
    > tun1: flags=8010 mtu 1500
    > [root@sp1 /usr/home/sobomax]# stat /dev/tun
    > 67174144 99 crw------- 1 uucp dialer 99 0 "May 2 22:21:28 2008" "May 2
    > 22:21:28 2008" "May 2 22:21:28 2008" "Dec 31 23:59:59 1969" 4096 0 0
    > /dev/tun
    > [root@sp1 /usr/home/sobomax]# ifconfig -a
    > tun0: flags=8010 mtu 1500
    > tun1: flags=8010 mtu 1500
    > tun2: flags=8010 mtu 1500
    > [root@sp1 /usr/home/sobomax]# stat /dev/tun
    > 67174144 105 crw------- 1 uucp dialer 105 0 "May 2 22:21:28 2008" "May
    > 2 22:21:28 2008" "May 2 22:21:28 2008" "Dec 31 23:59:59 1969" 4096 0
    > 0 /dev/tun
    > [root@sp1 /usr/home/sobomax]# ifconfig -a
    > tun0: flags=8010 mtu 1500
    > tun1: flags=8010 mtu 1500
    > tun2: flags=8010 mtu 1500
    > tun3: flags=8010 mtu 1500
    >

    Me too.
    I have seen that using ppp(8) and security/vpnc.

    Alexey.
    _______________________________________________
    freebsd-current@freebsd.org mailing list
    http://lists.freebsd.org/mailman/lis...reebsd-current
    To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"


  3. Re: Interface auto-cloning bug or feature?

    Hello Maxim,

    * Maxim Sobolev wrote:
    > I've noticed that stat/open call on /dev/tun always creates new
    > interface, despite the fact that existing spare interfaces may be
    > available. I believe that it's a bug, since the whole purpose of
    > auto-cloning is to create new instance only when no existing one could
    > be allocated. At least that's my reading of the manual page for the tun(4).


    I'd say the best way to fix this would be to convert tun and tap to
    devfs_[gs]et_cdevpriv() and turn tun and tap into single device nodes.
    That's what we've already been doing to bpf, snp, audit_pipe, etc.
    Unfortunately I'm no expert when it comes to our tun and tap
    implementations.

    We should eventually eliminate the dev_stdclone()/clone_create()
    interface. See the following URL for a list of drivers that still need
    to be converted:

    http://fxr.watson.org/fxr/ident?i=D_NEEDMINOR

    --
    Ed Schouten
    WWW: http://80386.nl/

    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.4.9 (FreeBSD)

    iEYEARECAAYFAkjTfBwACgkQ52SDGA2eCwXQmACdFVPyQeiHBP cHy/kY18v+B/0r
    +w4An2f/M+xFhiR0tujV2bW6rwHp4CIE
    =xdMv
    -----END PGP SIGNATURE-----


  4. Re: Interface auto-cloning bug or feature?

    * Ed Schouten wrote:
    > We should eventually eliminate the dev_stdclone()/clone_create()

    ^^^^^^^^^^^^
    > interface.


    Correct myself: there's nothing wrong with dev_stdclone - it can be used
    independently from the clone lists.

    --
    Ed Schouten
    WWW: http://80386.nl/

    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.4.9 (FreeBSD)

    iEYEARECAAYFAkjTfJAACgkQ52SDGA2eCwUddQCdGkFbPMspJY SOCxQzjJwSL2Nv
    WW0AnjOWJagJTzTSKEFedkDyOOF6TjMk
    =F1C8
    -----END PGP SIGNATURE-----


  5. Re: Interface auto-cloning bug or feature?

    Alexey Shuvaev wrote:
    >> [root@sp1 /usr/home/sobomax]# ifconfig -a
    >> tun0: flags=8010 mtu 1500
    >> tun1: flags=8010 mtu 1500
    >> tun2: flags=8010 mtu 1500
    >> tun3: flags=8010 mtu 1500
    >>

    > Me too.
    > I have seen that using ppp(8) and security/vpnc.


    That what has caused me to look into this issue. You can find patch for
    security/vpnc to prevent unbounded interface cloning here:

    http://sobomax.sippysoft.com/~sobomax/vpnc.diff

    -Maxim
    _______________________________________________
    freebsd-current@freebsd.org mailing list
    http://lists.freebsd.org/mailman/lis...reebsd-current
    To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"


  6. Re: Interface auto-cloning bug or feature?

    On Fri, Sep 19, 2008 at 03:43:21PM -0700, Maksim Yevmenkin wrote:
    > [....]
    >
    > >> That what has caused me to look into this issue. You can find patch for
    > >> security/vpnc to prevent unbounded interface cloning here:
    > >>
    > >> http://sobomax.sippysoft.com/~sobomax/vpnc.diff
    > >>

    > > Ok, the patch prevents interface cloning, but I think it doesn't solve
    > > the actual problem.
    > > Let's wait for Maksim

    >
    > ok, how about attached patch. i put it together *very* quickly and
    > only gave it a light testing. its for tap(4), because i could compile
    > it as a module and tun(4) is compiled into kernel by default, but the
    > idea should identical for tun(4). should be even simpler for tun(4)
    > because it does not have to deal with 2 kind of devices (i.e. tap and
    > vmnet). give it a try, and see if it works. please try both cloning
    > paths, i.e.
    >
    > 1) cat /dev/tap (/dev/vmnet) with and/or without unit number
    >
    > and
    >
    > 2) ifconfig tapX (vmnetX) create/destroy
    >
    > in the mean time i will prepare something similar for tun(4).
    >
    > thanks,
    > max


    > --- if_tap.c.orig 2008-09-08 17:20:57.000000000 -0700
    > +++ if_tap.c 2008-09-19 15:35:02.000000000 -0700
    > @@ -94,6 +94,7 @@
    > static int tapifioctl(struct ifnet *, u_long, caddr_t);
    > static void tapifinit(void *);
    >
    > +static int tap_clone_lookup(struct cdev **, u_short);
    > static int tap_clone_create(struct if_clone *, int, caddr_t);
    > static void tap_clone_destroy(struct ifnet *);
    > static int vmnet_clone_create(struct if_clone *, int, caddr_t);
    > @@ -176,6 +177,28 @@
    > DEV_MODULE(if_tap, tapmodevent, NULL);
    >
    > static int
    > +tap_clone_lookup(struct cdev **dev, u_short extra)
    > +{
    > + struct tap_softc *tp;
    > +
    > + mtx_lock(&tapmtx);
    > + SLIST_FOREACH(tp, &taphead, tap_next) {
    > + mtx_lock(&tp->tap_mtx);
    > + if ((tp->tap_flags & (TAP_OPEN|extra)) == extra) {
    > + *dev = tp->tap_dev;
    > + mtx_unlock(&tp->tap_mtx);
    > + mtx_unlock(&tapmtx);
    > +
    > + return (1);
    > + }
    > + mtx_unlock(&tp->tap_mtx);
    > + }
    > + mtx_unlock(&tapmtx);
    > +
    > + return (0);
    > +}
    > +
    > +static int
    > tap_clone_create(struct if_clone *ifc, int unit, caddr_t params)
    > {
    > struct cdev *dev;
    > @@ -353,8 +376,18 @@
    >
    > /* We're interested in only tap/vmnet devices. */
    > if (strcmp(name, TAP) == 0) {
    > + if (tap_clone_lookup(dev, 0)) {
    > + dev_ref(*dev);
    > + return;

    What would prevent two concurrent threads from selecting the same device
    there ? First thread could look up the device, unloc tapmtx and be
    preempted. Then second thread is put on CPU, do the same selection.
    Now you have a problem.

    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.4.9 (FreeBSD)

    iEYEARECAAYFAkjU+iYACgkQC3+MBN1Mb4g78wCfddJKStzNj9 xTz79XbtUJ8do0
    zqkAoJY/J9EELGG2kCa1Lz++C7/U7Gwg
    =t1a/
    -----END PGP SIGNATURE-----


  7. Re: Interface auto-cloning bug or feature?

    Patch works for me just fine. Thanks!

    -Maxim

    Maksim Yevmenkin wrote:
    > On Fri, Sep 19, 2008 at 3:43 PM, Maksim Yevmenkin
    > wrote:
    >> [....]
    >>
    >>>> That what has caused me to look into this issue. You can find patch for
    >>>> security/vpnc to prevent unbounded interface cloning here:
    >>>>
    >>>> http://sobomax.sippysoft.com/~sobomax/vpnc.diff
    >>>>
    >>> Ok, the patch prevents interface cloning, but I think it doesn't solve
    >>> the actual problem.
    >>> Let's wait for Maksim

    >> ok, how about attached patch. i put it together *very* quickly and
    >> only gave it a light testing. its for tap(4), because i could compile
    >> it as a module and tun(4) is compiled into kernel by default, but the
    >> idea should identical for tun(4). should be even simpler for tun(4)
    >> because it does not have to deal with 2 kind of devices (i.e. tap and
    >> vmnet). give it a try, and see if it works. please try both cloning
    >> paths, i.e.
    >>
    >> 1) cat /dev/tap (/dev/vmnet) with and/or without unit number
    >>
    >> and
    >>
    >> 2) ifconfig tapX (vmnetX) create/destroy
    >>
    >> in the mean time i will prepare something similar for tun(4).

    >
    > attached is similar patch for tun(4). i only made sure it compiles
    > rebuilding kernel now...
    >
    > thanks,
    > max
    >


    _______________________________________________
    freebsd-current@freebsd.org mailing list
    http://lists.freebsd.org/mailman/lis...reebsd-current
    To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"


  8. Re: Interface auto-cloning bug or feature?

    On Tue, Sep 23, 2008 at 11:00:26AM -0700, Maksim Yevmenkin wrote:
    > On 9/23/08, Kostik Belousov wrote:
    > > On Tue, Sep 23, 2008 at 10:19:13AM -0700, Maksim Yevmenkin wrote:
    > > > On 9/23/08, Kostik Belousov wrote:
    > > >
    > > > [...]
    > > >
    > > > > > attached is a slightly better patch for tap(4). the idea is to use
    > > > > > extra ALLOCATED flag that prevents the race Kostik pointed out.could
    > > > > > you please give it a try? any review comments are greatly appreciated.
    > > > > > if this is acceptable, i will prepare something similar for tun(4)
    > > > >
    > > > > The tap should use make_dev_credf(MAKEDEV_REF) instead of
    > > > > make_dev/dev_ref sequence in the clone handler. For similar reasons, I
    > > > > think it is slightly better to do a dev_ref() immediately after setting
    > > > > the TAP_ALLOCATED flag without dropping tapmtx.
    > > >
    > > > could you please explain why it is better?
    > > >
    > > > > I cannot figure out how tap_clone_create/tap_clone_destroy are being
    > > > > called. Can it be garbage-collected ?
    > > >
    > > > ah, this is interface clone feature, i.e. one can do 'ifconfig tap0
    > > > create/destroy' to create an interface and device node. take a look at
    > > > IFC_SIMPLE_DECLARE() macro.

    > >
    > > Thanks for the explanation.
    > > >
    > > > > The whole module unload sequence looks unsafe.
    > > >
    > > > yes, it is unsafe. it even has comment about it i guess, i could
    > > > fix it too while i'm at it

    > >
    > > One of the reason why the module unload is unsafe is the complete lack
    > > of synchronization between cloner and device destruction. Leaving
    > > tapmtx and tp->tap_mtx protected region in the clone handler, you
    > > allow for module unload routine to destroy device, and then dev_ref()
    > > would operate on the freed memory.
    > >
    > > Not that doing that without dropping the mutex(es) fix the bug, but
    > > at least it is a right move, it seems. At least this would trade a crash
    > > to a memory leak.

    >
    > well, unload race is easy to fix, no? just add a global flag protected
    > by taphead (tapmtx) mutex. in unload path (after checking all the
    > devices for OPEN and ALLOCATED) we will set this flag counter. each
    > clone and open routines will check for the flag and refuse to
    > open/clone if its set.


    Then you would get a transient failures when attempt to unload module
    fails because some devices are busy.

    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.4.9 (FreeBSD)

    iEYEARECAAYFAkjaBFwACgkQC3+MBN1Mb4iitwCfTYHd9rSr6N Te5/EWM+Jx+rRT
    ztYAnR/vlJGTjqUKQ1JNzrwk/i1ma37m
    =Gkif
    -----END PGP SIGNATURE-----


+ Reply to Thread