cvs commit: src/sys/kern kern_proc.c

[Poul-Henning Kamp]

In message <20040609.100413.118633043.imp at 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 at 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.