Date: Fri, 14 Mar 2008 19:41:14 -0700 From: Alfred Perlstein <alfred@freebsd.org> To: stable@freebsd.org Cc: smp@freebsd.org Subject: timeout/untimeout race conditions/crash [patch] Message-ID: <20080315024114.GD67856@elvis.mu.org>
next in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080315024114.GD67856>