kern/116034: Giant not owned at /usr/src/sys/netinet/tcp_sack.c:271=tcp_clean_sackreport(tp)

[Dan Lukes]

>Number:         116034
>Category:       kern
>Synopsis:       Giant not owned at /usr/src/sys/netinet/tcp_sack.c:271=tcp_clean_sackreport(tp)
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Sun Sep 02 22:30:02 GMT 2007
>Closed-Date:
>Last-Modified:
>Originator:     Dan Lukes
>Release:        FreeBSD 6.2-STABLE i386
>Organization:
Obludarium
>Environment:
System: FreeBSD 6.2-STABLE (sources from Sep 2 21:30:25 CEST 2007)
src/sys/netinet/tcp_sack.c,v 1.26.2.3 2007/06/12 19:21:54 jhb
src/sys/netinet/tcp_subr.c,v 1.228.2.14 2006/12/30 17:58:46 jhb
src/sys/kern/kern_mbuf.c,v 1.9.2.9 2007/02/11 03:31:18 mohans
src/sys/netinet/tcp_input.c,v 1.281.2.13 2007/06/12 18:53:32 jhb

Kernel compiled with IPSEC (=> MPSAFE network stack forced disabled; IPSEC require Giant)
Kernel compiled with INVARIANTS and INVARIANT_SUPPORT so missing lock trigger panic

NOTE: This doesn't apply for CURRENT, but may be significant for 6.3-RELEASE unless
Giant dependency completely MFCed-out from network stack code

>Description:

tcp_clean_sackreport() called from tcp_drain() without Giant but calee require it when
not on MPSAFE network stack 


The relevant code fragments (the INP_LOCK_ASSERT call is the point of panic):

 -----------------------
tcp_clean_sackreport(tp)
        struct tcpcb *tp;
{
        int i;

        INP_LOCK_ASSERT(tp->t_inpcb);
        tp->rcv_numsacks = 0;
        for (i = 0; i < MAX_SACK_BLKS; i++)
                tp->sackblks[i].start = tp->sackblks[i].end=0;
}

 -----------------------

#define INP_LOCK_ASSERT(inp)    do {                                    \
        mtx_assert(&(inp)->inp_mtx, MA_OWNED);                          \
        NET_ASSERT_GIANT();                                             \
} while (0)

 -----------------------

#define NET_ASSERT_GIANT() do {                                         \
        if (!debug_mpsafenet)                                           \
                mtx_assert(&Giant, MA_OWNED);                           \
} while (0)

 -----------------------


1. debug->mpsafenet is forced to off because IPSEC compiled in kernel. 
2. The INP_LOCK_ASSERT(tp->t_inpcb) test for Giant when mpsafenet off. 
3. tcp_clean_sackreport called from tcp_drain called from mb_reclaim without Giant

4. ABEND on kernel compiled with INVARIANTS/INVARIANT_SUPPORT; possible race condition 
	when compiled without INVARIANT



 ===================================================

Kernel code backtrace:

#2  0xc04b9b7d in panic (fmt=0xc064cd1e "mutex %s not owned at %s:%d")
    at /usr/src/sys/kern/kern_shutdown.c:565
#3  0xc04b1e4f in _mtx_assert (m=0xc06a8880, what=0,
    file=0xc065a3bb "/usr/src/sys/netinet/tcp_sack.c", line=271)
    at /usr/src/sys/kern/kern_mutex.c:768
#4  0xc05480bb in tcp_clean_sackreport (tp=0xc49aa910)
    at /usr/src/sys/netinet/tcp_sack.c:271
#5  0xc0549927 in tcp_drain () at /usr/src/sys/netinet/tcp_subr.c:891
#6  0xc04b0a69 in mb_reclaim (junk=0x0) at /usr/src/sys/kern/kern_mbuf.c:557
#7  0xc05d6cd4 in vm_pageout_scan (pass=0) at /usr/src/sys/vm/vm_pageout.c:724
#8  0xc05d7eaf in vm_pageout () at /usr/src/sys/vm/vm_pageout.c:1554

>How-To-Repeat:
	Compile kernel with INVARIANTS/INVARIANT_SUPPORT + (IPSEC or set mpsafenet to 0)

	Do something that trigger mb_reclaim
>Fix:

	Workaround is simple:
net.inet.tcp.do_tcpdrain=0

	Fix:

add NET_LOCK_GIANT() / NET_UNLOCK_GIANT() into tcp_drain()
unless someone smarter than me claim that Giant is NOT required here
>Release-Note:
>Audit-Trail:
>Unformatted: