Date: Tue, 23 Sep 2008 12:41:34 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Maksim Yevmenkin <maksim.yevmenkin@gmail.com> Cc: Alexey Shuvaev <shuvaev@physik.uni-wuerzburg.de>, freebsd-current@freebsd.org, Ed Schouten <ed@80386.nl> Subject: Re: Interface auto-cloning bug or feature? Message-ID: <20080923094134.GM47828@deviant.kiev.zoral.com.ua> In-Reply-To: <bb4a86c70809221849v640e66awa52a2b5d944ca0dc@mail.gmail.com> References: <48D2F942.4070801@FreeBSD.org> <20080919084201.GD44330@wep4035.physik.uni-wuerzburg.de> <48D38DFF.8000803@FreeBSD.org> <20080919203310.GA34131@localhost.my.domain> <bb4a86c70809191543y7f3d38ex73c48186dfd163c5@mail.gmail.com> <bb4a86c70809191551y774c233g5e664c431be62a50@mail.gmail.com> <48D8196E.7020005@FreeBSD.org> <bb4a86c70809221849v640e66awa52a2b5d944ca0dc@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--V3eawNQxI9TAjvgi
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Mon, Sep 22, 2008 at 06:49:14PM -0700, Maksim Yevmenkin wrote:
> [...]
>=20
> >>> 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...
>=20
> 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.
I cannot figure out how tap_clone_create/tap_clone_destroy are being
called. Can it be garbage-collected ?
The whole module unload sequence looks unsafe.
>=20
> thanks,
> max
> --- if_tap.c.orig 2008-09-08 17:20:57.000000000 -0700
> +++ if_tap.c 2008-09-22 18:36:16.000000000 -0700
> @@ -94,6 +94,7 @@
> static int tapifioctl(struct ifnet *, u_long, caddr_t);
> static void tapifinit(void *);
> =20
> +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,30 @@
> DEV_MODULE(if_tap, tapmodevent, NULL);
> =20
> 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|TAP_ALLOCATED|extra)) =3D=3D extra) {
> + tp->tap_flags |=3D TAP_ALLOCATED;
> + *dev =3D 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;
> @@ -288,7 +313,7 @@
> mtx_lock(&tapmtx);
> SLIST_FOREACH(tp, &taphead, tap_next) {
> mtx_lock(&tp->tap_mtx);
> - if (tp->tap_flags & TAP_OPEN) {
> + if (tp->tap_flags & (TAP_OPEN|TAP_ALLOCATED)) {
> mtx_unlock(&tp->tap_mtx);
> mtx_unlock(&tapmtx);
> return (EBUSY);
> @@ -353,8 +378,18 @@
> =20
> /* We're interested in only tap/vmnet devices. */
> if (strcmp(name, TAP) =3D=3D 0) {
> + if (tap_clone_lookup(dev, 0)) {
> + dev_ref(*dev);
> + return;
> + }
> +
> unit =3D -1;
> } else if (strcmp(name, VMNET) =3D=3D 0) {
> + if (tap_clone_lookup(dev, TAP_VMNET)) {
> + dev_ref(*dev);
> + return;
> + }
> +
> unit =3D -1;
> extra =3D VMNET_DEV_MASK;
> } else if (dev_stdclone(name, NULL, TAP, &unit) !=3D 1) {
> @@ -559,12 +594,11 @@
> KNOTE_UNLOCKED(&tp->tap_rsel.si_note, 0);
> =20
> mtx_lock(&tp->tap_mtx);
> - tp->tap_flags &=3D ~TAP_OPEN;
> + tp->tap_flags &=3D ~(TAP_OPEN|TAP_ALLOCATED);
> tp->tap_pid =3D 0;
> mtx_unlock(&tp->tap_mtx);
> =20
> - TAPDEBUG("%s is closed. minor =3D %#x\n",=20
> - ifp->if_xname, minor(dev));
> + TAPDEBUG("%s is closed. minor =3D %#x\n", ifp->if_xname, minor(dev));
> =20
> return (0);
> } /* tapclose */
> --- if_tapvar.h.orig 2005-06-10 12:04:52.000000000 -0700
> +++ if_tapvar.h 2008-09-22 17:34:00.000000000 -0700
> @@ -54,6 +54,7 @@
> #define TAP_ASYNC (1 << 3)
> #define TAP_READY (TAP_OPEN|TAP_INITED)
> #define TAP_VMNET (1 << 4)
> +#define TAP_ALLOCATED (1 << 5)
> =20
> u_int8_t ether_addr[ETHER_ADDR_LEN]; /* ether addr of the remote side =
*/
> =20
--V3eawNQxI9TAjvgi
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (FreeBSD)
iEYEARECAAYFAkjYuc4ACgkQC3+MBN1Mb4gTGwCghsr+GIis+QqqCdFgcpwE7gwc
nrkAoMMNdOjzLazZZfJTaVtwJ47qTVbr
=qrMV
-----END PGP SIGNATURE-----
--V3eawNQxI9TAjvgi--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080923094134.GM47828>