i386/73224: Lock order reversal in ntoskrnl_timercall()

[Frank Mayhar]

>Number:         73224
>Category:       i386
>Synopsis:       Lock order reversal in ntoskrnl_timercall()
>Confidential:   no
>Severity:       critical
>Priority:       high
>Responsible:    freebsd-i386
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Thu Oct 28 02:30:24 GMT 2004
>Closed-Date:
>Last-Modified:
>Originator:     Frank Mayhar
>Release:        FreeBSD 5.3-STABLE i386
>Organization:
Exit Consulting
>Environment:
System: FreeBSD lap 5.3-STABLE FreeBSD 5.3-STABLE #6: Wed Oct 27 18:08:57 PDT 2004 frank at lap:/home/obj/usr/src/sys/AUTON i386

	This happened on boot of a DIAGNOSTIC kernel.

lock order reversal
 1st 0xc06c90c0 dont_sleep_in_callout (dont_sleep_in_callout) @ /usr/src/sys/kern/kern_timeout.c:257
 2nd 0xc06c75a0 Giant (Giant) @ /usr/src/sys/modules/ndis/../../compat/ndis/subr_ntoskrnl.c:1647
KDB: stack backtrace:
kdb_backtrace(0,ffffffff,c06cf7c0,c06d06e8,c069a2dc) at kdb_backtrace+0x29
witness_checkorder(c06c75a0,9,c0d0a5f2,66f) at witness_checkorder+0x544
_mtx_lock_flags(c06c75a0,0,c0d0a5f2,66f,c1fd3360) at _mtx_lock_flags+0x5b
ntoskrnl_timercall(c1fd3360,c06c90c0,0,c0665fbd,101) at ntoskrnl_timercall+0x98
softclock(0) at softclock+0x1af
ithread_loop(c1d8fc80,d55e8d48,c1d8fc80,c04e6160,0) at ithread_loop+0x124
fork_exit(c04e6160,c1d8fc80,d55e8d48) at fork_exit+0xa4
fork_trampoline() at fork_trampoline+0x8
--- trap 0x1, eip = 0, esp = 0xd55e8d7c, ebp = 0 ---
KDB: enter: witness_checkorder

#1  0xc0517083 in witness_checkorder (lock=0xc06c75a0, flags=0x9, 
    file=0xc0d0a5f2 "/usr/src/sys/modules/ndis/../../compat/ndis/subr_ntoskrnl.c", line=0x66f)
    at /usr/src/sys/kern/subr_witness.c:952
#2  0xc04f0233 in _mtx_lock_flags (m=0xc06c75a0, opts=0x0, 
    file=0xc0d0a5f2 "/usr/src/sys/modules/ndis/../../compat/ndis/subr_ntoskrnl.c", line=0x66f)
    at /usr/src/sys/kern/kern_mutex.c:271
#3  0xc0d07410 in ntoskrnl_timercall (arg=0xc1fd3360)
    at /usr/src/sys/modules/ndis/../../compat/ndis/subr_ntoskrnl.c:1647
#4  0xc0503307 in softclock (dummy=0x0) at /usr/src/sys/kern/kern_timeout.c:259
#5  0xc04e6284 in ithread_loop (arg=0xc1d8fc80) at /usr/src/sys/kern/kern_intr.c:547
#6  0xc04e5694 in fork_exit (callout=0xc04e6160 <ithread_loop>, arg=0xc1d8fc80, frame=0xd55e8d48)
    at /usr/src/sys/kern/kern_fork.c:811
#7  0xc0614e3c in fork_trampoline () at /usr/src/sys/i386/i386/exception.s:209


The ntoskrnl_timercall() drops Giant on entry and tries to pick it up again
on exit.  I suspect that it shouldn't do anything with Giant at all, but I
don't know the code well enough to say that for certain.  Assuming that my
suspicion is correct, though, the patch would be:

Index: subr_ntoskrnl.c
===================================================================
RCS file: /cvs/repos/src/sys/compat/ndis/subr_ntoskrnl.c,v
retrieving revision 1.43.2.1
diff -u -r1.43.2.1 subr_ntoskrnl.c
--- subr_ntoskrnl.c     13 Oct 2004 19:23:33 -0000      1.43.2.1
+++ subr_ntoskrnl.c     28 Oct 2004 02:14:39 -0000
@@ -1616,8 +1616,6 @@
        ktimer                  *timer;
        struct timeval          tv;
 
-       mtx_unlock(&Giant);
-
        timer = arg;
 
        timer->k_header.dh_inserted = FALSE;
@@ -1644,8 +1642,6 @@
 
        ntoskrnl_wakeup(&timer->k_header);
 
-       mtx_lock(&Giant);
-
        return;
 }

>Description:
>How-To-Repeat:
>Fix:
>Release-Note:
>Audit-Trail:
>Unformatted: