From owner-freebsd-bugs@FreeBSD.ORG Wed Feb 21 22:45:46 2007 Return-Path: X-Original-To: freebsd-bugs@FreeBSD.org Delivered-To: freebsd-bugs@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 3990F16A401 for ; Wed, 21 Feb 2007 22:45:46 +0000 (UTC) (envelope-from dmitry@atlantis.dp.ua) Received: from postman.atlantis.dp.ua (postman.atlantis.dp.ua [193.108.47.1]) by mx1.freebsd.org (Postfix) with ESMTP id A510513C491 for ; Wed, 21 Feb 2007 22:45:39 +0000 (UTC) (envelope-from dmitry@atlantis.dp.ua) Received: from smtp.atlantis.dp.ua (smtp.atlantis.dp.ua [193.108.46.231]) by postman.atlantis.dp.ua (8.13.1/8.13.1) with ESMTP id l1LMUGLn078592; Thu, 22 Feb 2007 00:30:16 +0200 (EET) (envelope-from dmitry@atlantis.dp.ua) Date: Thu, 22 Feb 2007 00:30:16 +0200 (EET) From: Dmitry Pryanishnikov To: Bruce Evans In-Reply-To: <20070219101530.Q9237@besplex.bde.org> Message-ID: <20070221235556.K66009@atlantis.atlantis.dp.ua> References: <200702181417.l1IEHsCJ001879@homelynx.homenet> <20070219101530.Q9237@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: freebsd-bugs@FreeBSD.org, FreeBSD-gnats-submit@FreeBSD.org Subject: Re: kern/109277: kernel ppp(4) botches clist reservation in RELENG_6 X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 21 Feb 2007 22:45:46 -0000 Hello! On Mon, 19 Feb 2007, Bruce Evans wrote: > On Sun, 18 Feb 2007, Dmitry Pryanishnikov wrote: >> Last such a crash was "panic: clist reservation botch" (see >> cblock_alloc() >> function in /sys/kern/tty_subr.c), this was RELENG_6 as of 1-Feb-2007, >>> Fix: >> I'm ready to provide further debugging information on this issue. >> Unfortunately, I'm not familiar enough with the locking concepts >> in modern FreeBSD kernels (and in tty subsystem particularly) >> in order to make the fix myself. > > Tty locking is especially simple and not very good -- everything must > be Giant-locked to work. However, the default for network drivers is > now not to use Giant locking. ppp doesn't seem to be aware of this. I was under the wrong impression that the addition of IFF_NEEDSGIANT to ppp's ifp->if_flags (made 30-Mar-2006 by rwatson@) cured exactly this problem. But apparently it's not enough: simple addition of the GIANT_REQUIRED at start of the cblock_alloc() reveals that Giant isn't held at this point. > The only simple fix seems to be to pessimize all network drivers by > configuring Giant locking for them all -- see netisr.c. I'm not sure > if this is enough -- it is probably necessary to Giant-lock all calls > into ppp (especially ioctls), but things in netisr.c only logically > affect isrs. I've tried to solve the problem from the other end. Upon some analysis of the ppp(4) code I concluded that: a) all calls of the tty functions are concentrated in the single ppp(4) source file ppp_tty.c, and b) _almost_ all these calls are protected by the now dummy pair of function calls "s = spltty(); / splx(s);". So I went ahead and converted those pairs to the conditional Giant acquisition: --- ppp_tty.c.orig Tue Dec 12 23:56:22 2006 +++ ppp_tty.c Wed Feb 21 21:39:50 2007 @@ -86,6 +86,8 @@ #include #include #include +#include +#include #ifdef PPP_FILTER #include @@ -93,6 +95,12 @@ #include #include +#undef spltty +#undef splx +#define spltty() mtx_owned(&Giant) ? 0 : (mtx_lock(&Giant), 1) +#define splx(relflg) if (relflg) mtx_unlock(&Giant) + + static int pppopen(struct cdev *dev, struct tty *tp); static int pppclose(struct tty *tp, int flag); static int pppread(struct tty *tp, struct uio *uio, int flag); @@ -556,7 +564,9 @@ /* XXX as above. */ if (CCOUNT(&tp->t_outq) == 0) { ++sc->sc_stats.ppp_obytes; + s = spltty(); (void) putc(PPP_FLAG, &tp->t_outq); + splx(s); } /* Calculate the FCS for the first mbuf's worth. */ @@ -579,7 +589,9 @@ n = cp - start; if (n) { /* NetBSD (0.9 or later), 4.3-Reno or similar. */ + s = spltty(); ndone = n - b_to_q(start, n, &tp->t_outq); + splx(s); len -= ndone; start += ndone; sc->sc_stats.ppp_obytes += ndone; This actually insures that Giant is held within tty layer (GIANT_REQUIRED check in cblock_alloc() confirms this), but this causes lock order reversals like these: lock order reversal: (Giant after non-sleepable) 1st 0xc39904c8 inp (rawinp) @ /usr/RELENG_6/src/sys/netinet/raw_ip.c:289 2nd 0xc06488a0 Giant (Giant) @ /usr/RELENG_6/src/sys/net/ppp_tty.c:567 KDB: stack backtrace: kdb_backtrace(0,ffffffff,c06556a0,c0657ba8,c0625fe4,...) at 0xc04b2f59 = kdb_backtrace+0x29 witness_checkorder(c06488a0,9,c05fff6e,237) at 0xc04bdb00 = witness_checkorder+0x578 _mtx_lock_flags(c06488a0,0,c05fff6e,237) at 0xc0493180 = _mtx_lock_flags+0x78 pppasyncstart(c35ab800,c3692d0c,0,c05ffb7e,3e3) at 0xc050c6e4 = pppasyncstart+0x90 pppoutput(c3692c00,c3804700,c39764d0,c3987bdc,0,...) at 0xc0509d7a = pppoutput+0x326 ip_output(c3804700,0,dae08b24,20,0,...) at 0xc052a764 = ip_output+0xa64 rip_output(c3804700,c3a8b000,12e6cc1,40,c3804700,...) at 0xc052cb3b = rip_output+0x29f rip_send(c3a8b000,0,c3804700,c3a45470,0,...) at 0xc052d62f = rip_send+0x93 sosend(c3a8b000,c3a45470,dae08c3c,c3804700,0,0,c6895780) at 0xc04d4ff7 = sosend+0x5eb kern_sendit(c6895780,3,dae08cbc,0,0,0) at 0xc04da28c = kern_sendit+0x104 sendit(c6895780,3,dae08cbc,0,804e0b4,...) at 0xc04da15f = sendit+0x163 sendto(c6895780,dae08d04) at 0xc04da3bd = sendto+0x4d syscall(3b,3b,3b,804e074,40,...) at 0xc05ca317 = syscall+0x22f Xint0x80_syscall() at 0xc05b89af = Xint0x80_syscall+0x1f --- syscall (133, FreeBSD ELF32, sendto), eip = 0x481401eb, esp = 0xbfbee90c, ebp = 0xbfbee958 --- lock order reversal: (Giant after non-sleepable) 1st 0xc398457c inp (udpinp) @ /usr/RELENG_6/src/sys/netinet/udp_usrreq.c:804 2nd 0xc06488a0 Giant (Giant) @ /usr/RELENG_6/src/sys/net/ppp_tty.c:567 KDB: stack backtrace: kdb_backtrace(0,ffffffff,c0657838,c0657ba8,c0625fe4,...) at 0xc04b2f59 = kdb_backtrace+0x29 witness_checkorder(c06488a0,9,c05fff6e,237) at 0xc04bdb00 = witness_checkorder+0x578 _mtx_lock_flags(c06488a0,0,c05fff6e,237) at 0xc0493180 = _mtx_lock_flags+0x78 pppasyncstart(c35ab800,c3692d0c,0,c05ffb7e,3e3) at 0xc050c6e4 = pppasyncstart+0x90 pppoutput(c3692c00,c3803900,c39764d0,c3987bdc,0,...) at 0xc0509d7a = pppoutput+0x326 ip_output(c3803900,0,dae02b00,0,0,...) at 0xc052a764 = ip_output+0xa64 udp_output(c39844ec,c3803900,c384a330,0,c6895480,...) at 0xc053a5b2 = udp_output+0x4b6 udp_send(c3a8b000,0,c3803900,c384a330,0,...) at 0xc053ab0a = udp_send+0x1a sosend(c3a8b000,c384a330,dae02c3c,c3803900,0,0,c6895480) at 0xc04d4ff7 = sosend+0x5eb kern_sendit(c6895480,4,dae02cbc,0,0,0) at 0xc04da28c = kern_sendit+0x104 sendit(c6895480,4,dae02cbc,0,bfbfe890,...) at 0xc04da15f = sendit+0x163 sendto(c6895480,dae02d04) at 0xc04da3bd = sendto+0x4d syscall(3b,3b,3b,bfbfed4c,bfbfe860,...) at 0xc05ca317 = syscall+0x22f Xint0x80_syscall() at 0xc05b89af = Xint0x80_syscall+0x1f --- syscall (133, FreeBSD ELF32, sendto), eip = 0x481341eb, esp = 0xbfbfe80c, ebp = 0xbfbfe838 --- So, is it possible to avoid these LORs w/o massive ppp/tty/network code rewriting? > Bruce Sincerely, Dmitry -- Atlantis ISP, System Administrator e-mail: dmitry@atlantis.dp.ua nic-hdl: LYNX-RIPE