Date: Wed, 24 Sep 2008 12:11:56 +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: <20080924091156.GA47828@deviant.kiev.zoral.com.ua> In-Reply-To: <bb4a86c70809231100k76890dfdr220a80e998b497e6@mail.gmail.com> References: <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> <20080923173435.GW47828@deviant.kiev.zoral.com.ua> <bb4a86c70809231100k76890dfdr220a80e998b497e6@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--PxDrs/Fpf4pPiewX Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 23, 2008 at 11:00:26AM -0700, Maksim Yevmenkin wrote: > On 9/23/08, Kostik Belousov <kostikbel@gmail.com> wrote: > > On Tue, Sep 23, 2008 at 10:19:13AM -0700, Maksim Yevmenkin wrote: > > > On 9/23/08, Kostik Belousov <kostikbel@gmail.com> 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 appre= ciated. > > > > > 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 reaso= ns, I > > > > think it is slightly better to do a dev_ref() immediately after s= etting > > > > 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 be= ing > > > > 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 cra= sh > > to a memory leak. >=20 > 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. --PxDrs/Fpf4pPiewX Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (FreeBSD) iEYEARECAAYFAkjaBFwACgkQC3+MBN1Mb4iitwCfTYHd9rSr6NTe5/EWM+Jx+rRT ztYAnR/vlJGTjqUKQ1JNzrwk/i1ma37m =Gkif -----END PGP SIGNATURE----- --PxDrs/Fpf4pPiewX--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080924091156.GA47828>