From owner-cvs-all@FreeBSD.ORG Wed Jun 9 16:20:09 2004 Return-Path: Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 845DF16A4CE; Wed, 9 Jun 2004 16:20:09 +0000 (GMT) Received: from critter.freebsd.dk (critter.freebsd.dk [212.242.86.163]) by mx1.FreeBSD.org (Postfix) with ESMTP id E70BD43D1F; Wed, 9 Jun 2004 16:20:08 +0000 (GMT) (envelope-from phk@phk.freebsd.dk) Received: from critter.freebsd.dk (localhost [127.0.0.1]) by critter.freebsd.dk (8.12.11/8.12.11) with ESMTP id i59GK0cw055930; Wed, 9 Jun 2004 18:20:00 +0200 (CEST) (envelope-from phk@phk.freebsd.dk) To: "M. Warner Losh" From: "Poul-Henning Kamp" In-Reply-To: Your message of "Wed, 09 Jun 2004 10:04:13 MDT." <20040609.100413.118633043.imp@bsdimp.com> Date: Wed, 09 Jun 2004 18:20:00 +0200 Message-ID: <55929.1086798000@critter.freebsd.dk> cc: cvs-src@FreeBSD.org cc: src-committers@FreeBSD.org cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/kern kern_proc.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Jun 2004 16:20:09 -0000 In message <20040609.100413.118633043.imp@bsdimp.com>, "M. Warner Losh" writes: >Can you provide a couple of lines about why BAD is BAD and why GOOD >fixes that flaw? That should help others from making this mistake in >the future. LOCK(foo->lock) foo->refcount--; UNLOCK(foo->lock) if (foo->refcount == 0) destroy(foo); The problem is that there is the risk that another thread will modify the refcount between our modification and our test: Assume foo->refcount = 2; thread1 (low priority) thread2 (high priority) ---------------------- ----------------------- ... ... LOCK(foo->lock) ... foo->refcount--; ... # refcount now == 1 LOCK(foo->lock) At this point, thread2 sleeps, spins or whatever until it can get the lock it wants. UNLOCK(foo->lock) Now thread2 is runnable and since it has a higher priority it will be run: foo->refcount--; # refcount now == 0 UNLOCK(foo->lock); if(foo->refount == 0) destroy(foo); ... At some point thread1 gets to continue: if (foo->refcount == 0) destroy(foo); But at this time foo may be gone or recycled and a panic is our best hope and random memory corruption is our worst fear. The way to fix this is to make sure that the test for zero-ness is done on the result of our own decrement operation: LOCK(foo->lock) i = --foo->refcount; UNLOCK(foo->lock) if (i == 0) destroy(foo); Assume foo->refcount = 2; thread1 (low priority) thread2 (high priority) ---------------------- ----------------------- ... ... LOCK(foo->lock) ... i = --foo->refcount; LOCK(foo->lock) # i == 1, refcount == 1 UNLOCK(foo->lock) i = --foo->refcount; # i == 0, refcount == 0 UNLOCK(foo->lock) if (i == 0) # true destroy(foo) ... if (i == 0) # false destroy(foo) I'm not very good at explaining this am I ? -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 phk@FreeBSD.ORG | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence.