From owner-freebsd-current@FreeBSD.ORG Sun Sep 21 15:49:56 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 684C4106564A for ; Sun, 21 Sep 2008 15:49:56 +0000 (UTC) (envelope-from maksim.yevmenkin@gmail.com) Received: from fg-out-1718.google.com (fg-out-1718.google.com [72.14.220.156]) by mx1.freebsd.org (Postfix) with ESMTP id DFEF28FC0A for ; Sun, 21 Sep 2008 15:49:55 +0000 (UTC) (envelope-from maksim.yevmenkin@gmail.com) Received: by fg-out-1718.google.com with SMTP id l26so1069812fgb.35 for ; Sun, 21 Sep 2008 08:49:54 -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=cjLe9NLnrpw14P+o2ukNu3pPch0ZDs94NtoqfuCPJRg=; b=vMmvyt5f01zHLNoZJv5hH1aJhJ4uFOQS7VSIG5DEm0aTHtJ7mo9XDLYO5dN5Il4rCC mabHpI1C3c9+vxdXJRcZE5oWQdAAQ4g9wpdMVZvxlpkjBp+y11laG8WZEebT/J0JTYSS mQrDX119C2Inegy1G96QrFsPAftUJfiT7N4x0= 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=YSIzSp7M7tCc+kf5/fGQA3/qXNKN+kcWG0ydnXcNdYuWpU09fzjLDQeru6hS5NqrG/ TEHIIj4xwOlbtKWusqPz4ISA4U8+1a5emvhMliQ21ixt+Fchz4oy7Sey5kA9QsZHWoSj whe1IrMiDEfO2UacpxLN8xR7kRQPkCf5haQB4= Received: by 10.86.97.7 with SMTP id u7mr3942614fgb.29.1222012194647; Sun, 21 Sep 2008 08:49:54 -0700 (PDT) Received: by 10.86.62.1 with HTTP; Sun, 21 Sep 2008 08:49:54 -0700 (PDT) Message-ID: Date: Sun, 21 Sep 2008 08:49:54 -0700 From: "Maksim Yevmenkin" To: "Kostik Belousov" In-Reply-To: <20080920132703.GG47828@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> <20080919084201.GD44330@wep4035.physik.uni-wuerzburg.de> <48D38DFF.8000803@FreeBSD.org> <20080919203310.GA34131@localhost.my.domain> <20080920132703.GG47828@deviant.kiev.zoral.com.ua> Cc: Alexey Shuvaev , freebsd-current@freebsd.org 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: Sun, 21 Sep 2008 15:49:56 -0000 [...] >> --- if_tap.c.orig 2008-09-08 17:20:57.000000000 -0700 >> +++ if_tap.c 2008-09-19 15:35:02.000000000 -0700 >> @@ -94,6 +94,7 @@ >> static int tapifioctl(struct ifnet *, u_long, caddr_t); >> static void tapifinit(void *); >> >> +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,28 @@ >> DEV_MODULE(if_tap, tapmodevent, NULL); >> >> 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|extra)) == extra) { >> + *dev = 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; >> @@ -353,8 +376,18 @@ >> >> /* We're interested in only tap/vmnet devices. */ >> if (strcmp(name, TAP) == 0) { >> + if (tap_clone_lookup(dev, 0)) { >> + dev_ref(*dev); >> + return; > What would prevent two concurrent threads from selecting the same device > there ? First thread could look up the device, unloc tapmtx and be > preempted. Then second thread is put on CPU, do the same selection. > Now you have a problem. i told it was *very* quick :) and, yes, it seems like the race you describing would be possible. clone handler gets invoked with sx lock held, however its shared lock. also devfs mount point exclusive sx lock is dropped before invoking clone handler. anyway, the fix probably would be to move dev_rel() into the clone_lookup() routine and protect it by tap softc mutex. also clone_lookup() should check the number of references to the device to make sure 2 threads do not grab the same device. another approach would be to add another flag like TAP_OPEN to indicate that device is about to be open ans set it in the clone_lookup() routine. i'll put something together on monday (may be soon if i will have a free minute). please feel free to beat me to it :) thanks, max