From owner-svn-src-all@FreeBSD.ORG Thu May 21 16:58:32 2015 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 3F9B311A; Thu, 21 May 2015 16:58:32 +0000 (UTC) Received: from mail-wg0-x231.google.com (mail-wg0-x231.google.com [IPv6:2a00:1450:400c:c00::231]) (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 CA82217FE; Thu, 21 May 2015 16:58:31 +0000 (UTC) Received: by wgfl8 with SMTP id l8so91942814wgf.2; Thu, 21 May 2015 09:58:30 -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:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=YToJd8WRXP9/+wJQ2w6V8+PU57bo50K8PFiB+uywsYY=; b=xFxySaFoSXWxGyhnESrQEuTNHwtUp75Aw8DJoFUf/I/ivNikk6T4EH/ZSte1LJXqnG AlKQm35++fCf+dFJ3cg5kkeWyTl9KCxG75zhKr7aNUHCyCv8iAm3FAQE7eJ8VMn8UwhP EClYw2/6VaUgshiovfIG/DrQ16QJTIuxB9VEw/dpAjLj5HrNBlC3sYxY0wlmUJbpcvQM /llkfE5ZFGbrQ26W8noEa1RzTyERdnB6VTcSF4dBJk3yeamUODHgIoJ2m2mSjj/9i11d dBPlNcTq8gWeBxWH7ufYJXvl/rJOrD+OG/kJHAs4/IaZbmVGeCCWuXg5CjpUfPlPIBnd 6Otw== X-Received: by 10.194.2.47 with SMTP id 15mr7011414wjr.101.1432227510257; Thu, 21 May 2015 09:58:30 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id g14sm33002409wjs.47.2015.05.21.09.58.29 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Thu, 21 May 2015 09:58:29 -0700 (PDT) Date: Thu, 21 May 2015 18:58:27 +0200 From: Mateusz Guzik To: John Baldwin Cc: Alexander Kabaev , 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: <20150521165826.GA17403@dft-labs.eu> References: <201505151350.t4FDocQT054144@svn.freebsd.org> <20150520120046.268dde86@kan> <2114957.Lbz1KNQfZU@ralph.baldwin.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <2114957.Lbz1KNQfZU@ralph.baldwin.cx> User-Agent: Mutt/1.5.21 (2010-09-15) 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: Thu, 21 May 2015 16:58:32 -0000 On Thu, May 21, 2015 at 11:33:02AM -0400, John Baldwin wrote: > On Wednesday, May 20, 2015 12:00:46 PM Alexander Kabaev wrote: > > 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. > > Yes, this is fine to revert for now. I'll have to think about which way > to fix the bug in question. The simplest route is probably to remove > cv_waiters entirely, though hacking up sleepq_timeout to recognize cv's > and decrement the count is another option (but hackier). > The code was buggy even prior to this. _cv_wait does: #ifdef KTRACE if (KTRPOINT(td, KTR_CSW)) ktrcsw(0, 0, cv_wmesg(cvp)); #endif So with ktrace enabled cvp is possibly accessed after it gets freed. I checked "solaris compatiblity layer" from "zfs on linux" project and it looks like they are screwed by this as well. In other words, I would argue modifying native privmitives to accomodate for zfs use may not be the best course of action. Hacking up zfs or cv_ primitives seems better and opens up posibilities for minor optimisations. I'm not up to it though. -- Mateusz Guzik