From owner-freebsd-net@FreeBSD.ORG Thu Dec 29 16:41:02 2011 Return-Path: Delivered-To: net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id AE08310657AB; Thu, 29 Dec 2011 16:41:02 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 7B3F78FC12; Thu, 29 Dec 2011 16:41:02 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [96.47.65.170]) by cyrus.watson.org (Postfix) with ESMTPSA id 1B27446B2E; Thu, 29 Dec 2011 11:41:02 -0500 (EST) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id A14C6B93A; Thu, 29 Dec 2011 11:41:01 -0500 (EST) From: John Baldwin To: Gleb Smirnoff Date: Thu, 29 Dec 2011 11:36:01 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p8; KDE/4.5.5; amd64; ; ) References: <201112221130.01823.jhb@freebsd.org> <20111227041728.GI8035@FreeBSD.org> In-Reply-To: <20111227041728.GI8035@FreeBSD.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="koi8-r" Content-Transfer-Encoding: 7bit Message-Id: <201112291136.01889.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 29 Dec 2011 11:41:01 -0500 (EST) Cc: Robert Watson , net@freebsd.org Subject: Re: Transitioning if_addr_lock to an rwlock X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 Dec 2011 16:41:02 -0000 On Monday, December 26, 2011 11:17:28 pm Gleb Smirnoff wrote: > On Thu, Dec 22, 2011 at 11:30:01AM -0500, John Baldwin wrote: > J> You can find the patch for 8.x at > J> http://www.freebsd.org/~jhb/patches/if_addr_rwlock.patch > > Just my two pennies: for head/ patching if ip_carp.c should > be straightforward: > > 1) Using W in carp_alloc_if() and carp_free_if(). > 2) Using R everywhere else. Yes, this is what I did, but with an extra XXX: @@ -1512,10 +1512,11 @@ carp_alloc_if(struct ifnet *ifp) cif->cif_ifp = ifp; TAILQ_INIT(&cif->cif_vrs); - IF_ADDR_LOCK(ifp); + /* XXX: Race, shouldn't this be checking for concurrent calls? */ + IF_ADDR_WLOCK(ifp); ifp->if_carp = cif; if_ref(ifp); - IF_ADDR_UNLOCK(ifp); + IF_ADDR_WUNLOCK(ifp); return (cif); @@ -1534,10 +1535,10 @@ carp_free_if(struct carp_if *cif) KASSERT(TAILQ_EMPTY(&cif->cif_vrs), ("%s: softc list not empty", __func__)); - IF_ADDR_LOCK(ifp); + IF_ADDR_WLOCK(ifp); ifp->if_carp = NULL; if_rele(ifp); - IF_ADDR_UNLOCK(ifp); + IF_ADDR_WUNLOCK(ifp); CIF_LOCK_DESTROY(cif); Specifically, if two threads both call carp_alloc() at the same time and both see a value of NULL for ifp->if_carp (and I really do not like side effects like assignments in conditional expressions of if() and while()), then both threads can call carp_if_alloc(). Instead, carp_if_alloc() should be doing something like this: IF_ADDR_LOCK(ifp); if (ifp->if_carp != NULL) { CIF_LOCK_DESTROY(cif); free(cif, M_CARP); cif = ifp->if_carp; } else ifp->if_carp = cif; IF_ADDR_UNLOCK(ifp); return (cif); Similarly, you have a race in the SIOCSVH ioctl handling code. You check for a duplicate carp device for a specific (ifnet, vhid), tuple, but carp_alloc() doesn't do a recheck when adding the new softc to the cif_vrs list. Rather, what it should do is move that loop into carp_alloc() while holding the CIF_LOCK() and free the already-created softc and fail carp_alloc() if it finds a duplicate. You also have a race between concurrent carp_alloc() and carp_destroy(). Specifically, carp_alloc() might find a cif and be in the process of building a new carp softc when a carp_destroy() tears down the cif. The right way to fix this is to add a reference count to the cif and have carp_alloc_if() always bump the reference count. carp_destroy() would need to drop the reference count, but under IF_ADDR_LOCK() and only do a carp_free_if() if the count drops to zero. You'd have to grab IF_ADDR_LOCK() in the caller and let carp_free_if() unlock it internally. -- John Baldwin