From owner-freebsd-current@FreeBSD.ORG Tue Sep 23 18:00:28 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 E6D6D106568D for ; Tue, 23 Sep 2008 18:00:28 +0000 (UTC) (envelope-from maksim.yevmenkin@gmail.com) Received: from ug-out-1314.google.com (ug-out-1314.google.com [66.249.92.169]) by mx1.freebsd.org (Postfix) with ESMTP id 6BA298FC1A for ; Tue, 23 Sep 2008 18:00:27 +0000 (UTC) (envelope-from maksim.yevmenkin@gmail.com) Received: by ug-out-1314.google.com with SMTP id m2so1606097uge.39 for ; Tue, 23 Sep 2008 11:00:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from:to :subject:cc:in-reply-to:mime-version:content-type :content-transfer-encoding:content-disposition:references; bh=AGjyLrXLTtnQIiKgmqCsaKm2x42onVZfTiLeEGrCQqw=; b=hSAzlKUDG8umuiBehTuCaIOPRehblZcB5EP0v5ONiK2Ati6BUS7epigL6J+bHWkSY8 vfUPmn/jUV5NwXxnxSqe1YhxljoambGbyFD/w34nesfzEDJcYlz9UzZfAD2LuebkBjsW 2EvAp6/F77ULT8Lt69i1HOIhzX6Bu/W/mrVoE= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:to:subject:cc:in-reply-to:mime-version :content-type:content-transfer-encoding:content-disposition :references; b=axpsuAuIw758aSthP+pUaP9qgX8eVCLOy+pSEFnhLiWU3vdBC/LULF9zfHUCCbVMrt 34Jvge4nleW1HjkkXjILeFZJOI5lFKJ/1RuQPy3wXQmvaFuL5+d6iPA+YRj3TNM8fM8r jMSN51n/Gf8gb68LSgFO15nA/vuJSW1TpesQw= Received: by 10.86.92.4 with SMTP id p4mr6409603fgb.45.1222192826698; Tue, 23 Sep 2008 11:00:26 -0700 (PDT) Received: by 10.86.62.1 with HTTP; Tue, 23 Sep 2008 11:00:26 -0700 (PDT) Message-ID: Date: Tue, 23 Sep 2008 11:00:26 -0700 From: "Maksim Yevmenkin" To: "Kostik Belousov" In-Reply-To: <20080923173435.GW47828@deviant.kiev.zoral.com.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <48D2F942.4070801@FreeBSD.org> <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> 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: Tue, 23 Sep 2008 18:00:29 -0000 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. thanks, max