From owner-svn-src-all@FreeBSD.ORG Thu Dec 19 00:28:32 2013 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id DDC88C18; Thu, 19 Dec 2013 00:28:31 +0000 (UTC) Received: from mail-we0-x22e.google.com (mail-we0-x22e.google.com [IPv6:2a00:1450:400c:c03::22e]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id D3ECC1661; Thu, 19 Dec 2013 00:28:30 +0000 (UTC) Received: by mail-we0-f174.google.com with SMTP id q58so381024wes.5 for ; Wed, 18 Dec 2013 16:28:29 -0800 (PST) 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=W7wX8G0cPGr2JtsbOTuWciK8uBdy8YtcL3FrK+MsFgE=; b=Ei+5OaPvgxPski0rsgZkvGajstEi2k6Ll2kWWjR/meX7Q2bP8Gn65ody2KdvSEij60 jZNJ3whJ202PABvtqPc1wuGF9ZGalpSAulZAv9jIMVFQUeIbYR6bJOXrSbTucoF4p4ss BnFmAfVCKwkNCI4iY2Vsl6e9k0JOcZbVjNzF6+U6j4NDxAaPVAw5a1zqPTVEinvm6kLK bcSEe9uMrOjs9gsl1oWf4rU5eegqiP0jKVlpggzFjNzBIRS8U/li1/d2LT99Lh/sE3Dj ROGBCw6nxffPyJ5X/hyQEFolLGKnRJezfFsKtBRvJtFQ6pz8GE8rxGHnhhOXqscPd76C seFg== X-Received: by 10.180.188.141 with SMTP id ga13mr165787wic.55.1387412909265; Wed, 18 Dec 2013 16:28:29 -0800 (PST) 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 el8sm7805995wic.8.2013.12.18.16.28.27 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Wed, 18 Dec 2013 16:28:28 -0800 (PST) Date: Thu, 19 Dec 2013 01:28:24 +0100 From: Mateusz Guzik To: John Baldwin Subject: Re: svn commit: r259407 - head/sys/kern Message-ID: <20131219002824.GA5664@dft-labs.eu> References: <201312150411.rBF4Bhtg018852@svn.freebsd.org> <201312171141.49251.jhb@freebsd.org> <20131217181745.GB7535@dft-labs.eu> <201312171434.01345.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <201312171434.01345.jhb@freebsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Mateusz Guzik X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.17 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, 19 Dec 2013 00:28:32 -0000 On Tue, Dec 17, 2013 at 02:34:01PM -0500, John Baldwin wrote: > On Tuesday, December 17, 2013 1:17:45 pm Mateusz Guzik wrote: > > On Tue, Dec 17, 2013 at 11:41:49AM -0500, John Baldwin wrote: > > > On Saturday, December 14, 2013 11:11:43 pm Mateusz Guzik wrote: > > > > Author: mjg > > > > Date: Sun Dec 15 04:11:43 2013 > > > > New Revision: 259407 > > > > URL: http://svnweb.freebsd.org/changeset/base/259407 > > > > > > > > Log: > > > > proc exit: don't take PROC_LOCK while freeing rlimits > > > > > > > > Code wishing to check rlimits of some process should check whether it > > > > is exiting first, which current consumers do. > > > > > > Does this measurably reduce contention? > > > > > > > No, this is just a cosmetic change I did while doing some other work > > with rlimits. > > > > It would use some more cosmetic work (e.g. no reason not to > > lim_free(p->plimit); p->p_limit = NULL) and maybe I'll get to that > > later unless this kind of stuff is unwanted. > Sorry for late reply, was pondering moving this code to after the moment the process is removed from allproc. > I find it useful to leave the locking in place so it is clear that p_limit is > always written to with the lock held. If we ever got a static analyzer that > understood locking rules then leaving this locking in would reduce false > positives. When I first did locking for fields in struct proc I did it by > hand based on grepping the source tree for all uses of a field and ensuring > they were locked. I think it might be more confusing later on for another > reader to see unlocked access and then have to think about why that is safe. > I would argue such a tool should support marking given unlocled access as ok. Few lines earlier we already have code modifying proc without locks: if ((vtmp = p->p_textvp) != NULL) { p->p_textvp = NULL; vrele(vtmp); } Despite replacing the pointer with NULL first it still races with anything accessing the field as vref (if any) may be executed after vrele. As such, I would expect the reader to conclude that accessing these fields is not valid at this point. That being said, instead of reverting the change (which would leave other field with similar issue in place) I propose adding the following: --- a/sys/kern/kern_exit.c +++ b/sys/kern/kern_exit.c @@ -220,6 +220,12 @@ exit1(struct thread *td, int rv) p->p_xstat = rv; /* Let event handler change exit status */ PROC_UNLOCK(p); + + /* + * Some fields below are freed without having the proc locked, check + * for P_WEXIT before accessing to make sure it is safe. + */ + Which should make it clear. But again, this is a cosmetic change and I have no strong opinion either way. If you are still unconvinced I'm happy to revert it later. -- Mateusz Guzik