timeout/untimeout race conditions/crash [patch]

--F8dlzb82+Fcn6AgP
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

We think we tracked down a defect in timeout/untimeout in
FreeBSD.

We have reduced the problem to the following scenario:

2+ cpu system, one cpu is running softclock at the same time
another thread is running on another cpu which makes use of
timeout/untimeout.

CPU 0 is running "softclock"
CPU 1 is running "driver" with Giant held.

softclock: mtx_lock_spin(&callout_lock)
softclock: CACHES the callout structure's fields.
softclock: sees that it's a CALLOUT_LOCAL_ALLOC
softclock: executes this code:
  if (c->c_flags & CALLOUT_LOCAL_ALLOC) {
  	c->c_func = NULL;
  	c->c_flags = CALLOUT_LOCAL_ALLOC;
  	SLIST_INSERT_HEAD(&callfree, c,
  		c_links.sle);
  	curr_callout = NULL;
  } else {
  
  NOTE: that c->c_func has been set to NULL and curr_callout
        is also NULL.
softclock: mtx_unlock_spin(&callout_lock)
driver: calls untimeout(), the following sequence happens:
        mtx_lock_spin(&callout_lock);
        if (handle.callout->c_func == ftn && handle.callout->c_arg == arg)
                callout_stop(handle.callout);
        mtx_unlock_spin(&callout_lock);

  NOTE: untimeout() sees that handle.callout->c_func is not set
        to the function so it does NOT call callout_stop(9)!
driver: free's backing structure for c->c_arg.
softclock: executes callout.
softclock: likely crashes at this point due to access after free.

I have a patch I'm trying out here, but I need feedback on it.

The way the patch works is to treat CALLOUT_LOCAL_ALLOC (timeout/untimeout)
callouts the same as ~CALLOUT_LOCAL_ALLOC allocs, and moves the
freelist manipulation to the end of the callout dispatch.

Some light testing seems to have the system work.

We are doing some testing in-house to also make sure this works.

Please provide feedback.

See attached delta.

-- 
- Alfred Perlstein

--F8dlzb82+Fcn6AgP
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="kern_timeout.diff"

Index: kern_timeout.c
===================================================================
RCS file: /cvs/ncvs/src/sys/kern/kern_timeout.c,v
retrieving revision 1.97.2.2
diff -u -r1.97.2.2 kern_timeout.c
--- kern_timeout.c	26 Sep 2005 19:49:12 -0000	1.97.2.2
+++ kern_timeout.c	15 Mar 2008 02:28:48 -0000
@@ -241,17 +241,8 @@
 				c_arg = c->c_arg;
 				c_mtx = c->c_mtx;
 				c_flags = c->c_flags;
-				if (c->c_flags & CALLOUT_LOCAL_ALLOC) {
-					c->c_func = NULL;
-					c->c_flags = CALLOUT_LOCAL_ALLOC;
-					SLIST_INSERT_HEAD(&callfree, c,
-							  c_links.sle);
-					curr_callout = NULL;
-				} else {
-					c->c_flags =
-					    (c->c_flags & ~CALLOUT_PENDING);
-					curr_callout = c;
-				}
+				c->c_flags &= ~CALLOUT_PENDING;
+				curr_callout = c;
 				curr_cancelled = 0;
 				mtx_unlock_spin(&callout_lock);
 				if (c_mtx != NULL) {
@@ -310,6 +301,12 @@
 					mtx_unlock(c_mtx);
 				mtx_lock_spin(&callout_lock);
 done_locked:
+				if (c->c_flags & CALLOUT_LOCAL_ALLOC) {
+					c->c_func = NULL;
+					c->c_flags = CALLOUT_LOCAL_ALLOC;
+					SLIST_INSERT_HEAD(&callfree, c,
+							  c_links.sle);
+				}
 				curr_callout = NULL;
 				if (wakeup_needed) {
 					/*

--F8dlzb82+Fcn6AgP
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
[e-mail] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-smp
To unsubscribe, send any mail to "[e-mail]"
--F8dlzb82+Fcn6AgP--