From owner-svn-src-all@FreeBSD.ORG Wed May 20 17:41:09 2015 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 194B0919; Wed, 20 May 2015 17:41:09 +0000 (UTC) Received: from mail-qk0-x235.google.com (mail-qk0-x235.google.com [IPv6:2607:f8b0:400d:c09::235]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id C580A1A7F; Wed, 20 May 2015 17:41:08 +0000 (UTC) Received: by qkgv12 with SMTP id v12so36727275qkg.0; Wed, 20 May 2015 10:41:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-type; bh=7x8PBlTRcedBMX96w/9OwrnPTrMR1fWSc1twh9mg/MI=; b=e+rNqoWM8LAbqHr6ohQ8fLOyAGIoQs+OCwex76eVzvaCiKlDDa8HfLfFPif07+t8GJ QGzp85EAJAL1GMElBrNr6FELH0ywGiLjVW3Xan/dKugMXYREuZETVXhSXdMKAcTrriEH hzwdX4nlQZMgmbJvcPSwE81CBA/JMHclHXLE66fRXzEsrmWCd37rim3C2n5BkMZ3S+BN Btdid50OxgmdfXANX8hu7DSrmiXzlr9otdyR/yRHfuT0teYV2px2C9jXSmE7qREhVyqV KgjyJOCpyuy2EpcEiQYD3uaRKgYn46o6AGGN28UVGjoYd1/3twHvRSi8WngNw9T0YaqM b27Q== X-Received: by 10.140.132.80 with SMTP id 77mr8492809qhe.36.1432143667863; Wed, 20 May 2015 10:41:07 -0700 (PDT) Received: from kan ([2601:6:6780:7e0:226:18ff:fe00:232e]) by mx.google.com with ESMTPSA id f4sm11502825qhe.9.2015.05.20.10.41.07 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 20 May 2015 10:41:07 -0700 (PDT) Date: Wed, 20 May 2015 13:41:01 -0400 From: Alexander Kabaev To: Matthew Ahrens Cc: John Baldwin , "src-committers@freebsd.org" , svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r282971 - in head/sys: kern sys Message-ID: <20150520134101.69e555d7@kan> In-Reply-To: References: <201505151350.t4FDocQT054144@svn.freebsd.org> <20150520120046.268dde86@kan> X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.27; amd64-portbld-freebsd11.0) MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; boundary="Sig_/+23WYkdPx=UKcS=kQ7eg9CX"; protocol="application/pgp-signature" X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 May 2015 17:41:09 -0000 --Sig_/+23WYkdPx=UKcS=kQ7eg9CX Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Wed, 20 May 2015 09:54:45 -0700 Matthew Ahrens wrote: > On Wed, May 20, 2015 at 9:00 AM, Alexander Kabaev > wrote: >=20 > > On Fri, 15 May 2015 13:50:38 +0000 (UTC) > > John Baldwin wrote: > > > > > Author: jhb > > > Date: Fri May 15 13:50:37 2015 > > > New Revision: 282971 > > > URL: https://svnweb.freebsd.org/changeset/base/282971 > > > > > > Log: > > > Previously, cv_waiters was only updated by cv_signal or > > > cv_wait. If a thread awakened due to a time out, then cv_waiters > > > was not decremented. If INT_MAX threads timed out on a cv without > > > an intervening cv_broadcast, then cv_waiters could overflow. To > > > fix this, have each sleeping thread decrement cv_waiters when it > > > resumes. > > > > > > Note that previously cv_waiters was protected by the sleepq > > > chain lock. However, that lock is not held when threads resume > > > from sleep. In addition, the interlock is also not always > > > reacquired after resuming (cv_wait_unlock), nor is it always held > > > by callers of cv_signal() or cv_broadcast(). Instead, use atomic > > > ops to update cv_waiters. Since the sleepq chain lock is still > > > held on every increment, it should still be safe to compare > > > cv_waiters against zero while holding the lock in the wakeup > > > routines as the only way the race should be lost would result in > > > extra calls to sleepq_signal() or sleepq_broadcast(). > > > Differential Revision: https://reviews.freebsd.org/D2427 > > > Reviewed by: benno > > > Reported by: benno (wrap of cv_waiters in the field) > > > MFC after: 2 weeks > > > > > > Modified: > > > head/sys/kern/kern_condvar.c > > > head/sys/sys/condvar.h > > > > > > > This breaks ZFS range locking code, which expects to be able to > > wakeup everyone on the condition variable and then free the > > structure that contains it. Having woken up threads modify > > cv_waiters results in a race that leads to already freed memory to > > be accessed. > > > > It is debatable just how correct ZFS code in its expectations, but I > > think this commit should probably be reverted until either ZFS is > > changed not to expect cv modifiable by waking threads or until > > alternative solution is found to the cv_waiters overflow issue > > fixed by this commit. > > > > > It isn't clear to me how the zfs_range_unlock() code could know when > all the waiters have woken up and updated the CV, and thus it's safe > to destroy/free the CV. Would the woken threads ask, "was I the last > thread to be woken by this CV" and if so free the struct containing > the CV? Obviously such a check would need to ensure that the other > threads have completed their updates to the CV. >=20 > --matt Assuming other threads _need_ to update cv after they have been woken up. Clearly Solaris implementation managed to do without and our code changed that breaking range locks implementation we took directly from OpenSolaris (or illumos). What was previously possible now isn't. As I wrote before, while merits of this expectations are debatable and it is not hard to see where Solaris way is advantageous, that is really besides the point. Are you arguing that we should leave kernel in known broken state until 'proper' fix makes its way through possible upstream detour? Also, we have large code base taken from Solaris and chances are this is not the only place that might be affected. I think we are better off with this commit temporarily reverted until necessary repairs and auditing are complete for it to be safely re-enabled. --=20 Alexander Kabaev --Sig_/+23WYkdPx=UKcS=kQ7eg9CX Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEARECAAYFAlVcxy4ACgkQQ6z1jMm+XZZqLgCfWR19mzDKRYbPwqdPbMCKQmQu ln4An1GXwwgBfxvL61QgIojAmQvuHuIa =4fN3 -----END PGP SIGNATURE----- --Sig_/+23WYkdPx=UKcS=kQ7eg9CX--