From owner-freebsd-arch@FreeBSD.ORG Fri Feb 9 14:04:17 2007 Return-Path: X-Original-To: freebsd-arch@freebsd.org Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 8587916A541 for ; Fri, 9 Feb 2007 14:04:15 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from server.baldwin.cx (66-23-211-162.clients.speedfactory.net [66.23.211.162]) by mx1.freebsd.org (Postfix) with ESMTP id DE69C13C4A6 for ; Fri, 9 Feb 2007 14:04:14 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from zion.baldwin.cx (zion.baldwin.cx [192.168.0.7]) (authenticated bits=0) by server.baldwin.cx (8.13.6/8.13.6) with ESMTP id l19E3ple096573; Fri, 9 Feb 2007 09:03:54 -0500 (EST) (envelope-from jhb@freebsd.org) From: John Baldwin To: freebsd-arch@freebsd.org Date: Fri, 9 Feb 2007 08:37:04 -0500 User-Agent: KMail/1.9.4 References: <45CC0EB9.7030400@delphij.net> In-Reply-To: <45CC0EB9.7030400@delphij.net> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200702090837.04495.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [192.168.0.1]); Fri, 09 Feb 2007 09:03:55 -0500 (EST) X-Virus-Scanned: ClamAV 0.88.3/2544/Fri Feb 9 03:44:48 2007 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=4.2 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.1.3 X-Spam-Checker-Version: SpamAssassin 3.1.3 (2006-06-01) on server.baldwin.cx Cc: LI Xin Subject: Re: Patch for review: resolve a race condition in [sg]etpriority() X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Feb 2007 14:04:17 -0000 On Friday 09 February 2007 01:03, LI Xin wrote: > Hi, > > Here is a patch which resolves a race condition in [sg]etpriority(), but > I think there might be some better idea to resolve the problem, so I > would appreciate if someone would shed me some light :-) > > Description of the problem: > > In PRIO_USER of both [sg]etpriority(), p_ucred is expected to be a valid > reference to a valid user credential. However, for PRS_NEW processes, > there is chances where it is not properly initialized, causing a fatal > trap 12 [1]. > > On the other hand, just determining whether a process is in PRS_NEW > state and skip those who are newly born is not enough for semantical > correctness. A process could either have p_{start,end}copy section > copied or not, which needs to be handled with care. > > The attached solution: > > What I did in the attached patch is basically: > > - Before allproc_lock is sx_xunlock'ed in fork1(), lock both the parent > and the child. > - After releasing allproc_lock, do process p_{start,end}copy, > p_{start,end}zero and crhold() immediately, and release parent and > child's p_mtx as soon as possible. > - In getpriority(), simply skip PRS_NEW processes because they does not > affect PRIO_USER path's result. This part is not really necessary for > correctness, though. > > The pros for this is that it does not require an extensive API change, > and in theory it does not require that the allproc lock to be held for a > extended period; the cons is that this adds some overhead because it > adds two pairs of mutex lock/unlock. > > In a private discussion, there was also some other ideas. One is to > just move the unlock to where the process is completely initialized, > another is to introduce an event handler and msleep() p_state when > necessary, and do a wakeup once we bumped the state, but I think these > solution have their own limitations just like mine. My only reason for favoring the wakeup for complete initialization is that while this patch may solve the getprio/setprio race, it doesn't solve all PRS_NEW-related races, which the sleep/wakeup proposal did. -- John Baldwin