From owner-freebsd-current@FreeBSD.ORG Wed Sep 24 09:12:09 2008 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 62BB01065676; Wed, 24 Sep 2008 09:12:09 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.terabit.net.ua (mail.terabit.net.ua [195.137.202.147]) by mx1.freebsd.org (Postfix) with ESMTP id F166E8FC20; Wed, 24 Sep 2008 09:12:03 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from skuns.zoral.com.ua ([91.193.166.194] helo=mail.zoral.com.ua) by mail.terabit.net.ua with esmtp (Exim 4.63 (FreeBSD)) (envelope-from ) id 1KiQPg-000HpO-2i; Wed, 24 Sep 2008 12:12:00 +0300 Received: from deviant.kiev.zoral.com.ua (root@deviant.kiev.zoral.com.ua [10.1.1.148]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id m8O9BvWS062297 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 24 Sep 2008 12:11:57 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.2/8.14.2) with ESMTP id m8O9BvFt027803; Wed, 24 Sep 2008 12:11:57 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.3/8.14.3/Submit) id m8O9BuEv027802; Wed, 24 Sep 2008 12:11:56 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Wed, 24 Sep 2008 12:11:56 +0300 From: Kostik Belousov To: Maksim Yevmenkin Message-ID: <20080924091156.GA47828@deviant.kiev.zoral.com.ua> References: <48D38DFF.8000803@FreeBSD.org> <20080919203310.GA34131@localhost.my.domain> <48D8196E.7020005@FreeBSD.org> <20080923094134.GM47828@deviant.kiev.zoral.com.ua> <20080923173435.GW47828@deviant.kiev.zoral.com.ua> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="PxDrs/Fpf4pPiewX" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: ClamAV version 0.93.3, clamav-milter version 0.93.3 on skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua X-Virus-Scanned: mail.terabit.net.ua 1KiQPg-000HpO-2i 5ad23e997c7781615b84a7b959e04611 X-Terabit: YES Cc: Alexey Shuvaev , freebsd-current@freebsd.org, Ed Schouten Subject: Re: Interface auto-cloning bug or feature? X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Sep 2008 09:12:09 -0000 --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 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 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--