Date: Tue, 23 Sep 2008 20:34:35 +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: <20080923173435.GW47828@deviant.kiev.zoral.com.ua> In-Reply-To: <bb4a86c70809231019v4c0ee495r99f37382d7aa55d3@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> <20080923094134.GM47828@deviant.kiev.zoral.com.ua> <bb4a86c70809231019v4c0ee495r99f37382d7aa55d3@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--Zd8I2GZVcdxtyaG/ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 23, 2008 at 10:19:13AM -0700, Maksim Yevmenkin wrote: > On 9/23/08, Kostik Belousov <kostikbel@gmail.com> wrote: >=20 > [...] >=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 appreciate= d. > > > 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. >=20 > could you please explain why it is better? >=20 > > I cannot figure out how tap_clone_create/tap_clone_destroy are being > > called. Can it be garbage-collected ? >=20 > 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. >=20 > > The whole module unload sequence looks unsafe. >=20 > 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. --Zd8I2GZVcdxtyaG/ Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (FreeBSD) iEYEARECAAYFAkjZKKsACgkQC3+MBN1Mb4gikACcD4dbxfiairgFVR4bN1UcQW98 cNsAoO5vdmvsSU0omGGvfpCpELiw4ses =vSMA -----END PGP SIGNATURE----- --Zd8I2GZVcdxtyaG/--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080923173435.GW47828>