From owner-freebsd-current@FreeBSD.ORG Fri Apr 17 15:55:08 2009 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 807EA106566C for ; Fri, 17 Apr 2009 15:55:08 +0000 (UTC) (envelope-from maksim.yevmenkin@gmail.com) Received: from mail-gx0-f220.google.com (mail-gx0-f220.google.com [209.85.217.220]) by mx1.freebsd.org (Postfix) with ESMTP id 3648B8FC1D for ; Fri, 17 Apr 2009 15:55:07 +0000 (UTC) (envelope-from maksim.yevmenkin@gmail.com) Received: by gxk20 with SMTP id 20so2565187gxk.19 for ; Fri, 17 Apr 2009 08:55:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=bMXqutEYzC6jDXLkfVpzQlR/0RWNhPyZNoXiBsWYSOs=; b=o7IIbGQJh4lFIAYB+WMaflhapJkL+0PX+8nex9Twop1dvDMOszWPuT4wPru2Q/UcZW 1MdbJJBAKiPMNESsYMfgKYlGgsb2HWUUHH0DMIQWqA9illPoUUOsOwdNeC1UFlJJ7pqm 3geaEK61cidw/3lSlyugl+0bh/mP7f7jcAfpQ= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=Kz4+ouw3PKo66henBLARoc/X6FAxcM5klFntaSLZCl2iPr2QZ/TYqk3yW7sqAvjE6S LA21FISt4d7IhkgXV4xeA1YA7Hz0n9KB/UeYeAXPPDmChn9yvWpvjKgOM9CgLlpXVty2 PaBeAVoPFmIPZHt/YISRp3sneYIHD7O1OfJs0= MIME-Version: 1.0 Received: by 10.90.31.8 with SMTP id e8mr3328935age.65.1239983707017; Fri, 17 Apr 2009 08:55:07 -0700 (PDT) In-Reply-To: References: Date: Fri, 17 Apr 2009 08:55:06 -0700 Message-ID: From: Maksim Yevmenkin To: pluknet Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Alexander Best , freebsd-current@freebsd.org Subject: Re: possible bug in the sbappendrecord_locked()? (Was: Re: core dump with bluetooth device) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 17 Apr 2009 15:55:09 -0000 On Thu, Apr 16, 2009 at 9:48 PM, pluknet wrote: > Sorry for top-posting. > > This is a fairly old bug. > See my investigation > http://lists.freebsd.org/pipermail/freebsd-net/2008-August/019345.html ahh, i see. thanks for the pointer. please read below. >>>>> wrote: >>>>>> hi there, >>>>>> >>>>>> i'm running r191076M. when i try to send files from my mobile phone to my >>>>>> computer via bt the core dumps. here's a backtrace: >>>>>> >>>>>> Unread portion of the kernel message buffer: >>>>>> sblastmbufchk: sb_mb 0xc8d54d00 sb_mbtail 0 last 0xc8d54d00 >>>>>> packet tree: >>>>>> 0xc8d54d00 >>>>>> panic: sblastmbufchk from /usr/src/sys/kern/uipc_sockbuf.c:797 >>>>>> cpuid = 0 >>>>> >>>>> are you, by change, have "options SOCKBUF_DEBUG" in your kernel? >>>> >>>> ok, there is something strange going on in the >>>> sbappendrecord_locked(). consider the following initial conditions: >>>> >>>> 1) sockbuf sb is empty, i.e. sb_mb == sb_mbtail == sb_lastrecord == NULL >>>> >>>> 2) sbappendrecord_locked() is given mbuf m0 with has exactly one mbuf, >>>> i.e. m0->m_next == NULL >>>> >>>> so >>>> >>>> void >>>> sbappendrecord_locked(struct sockbuf *sb, struct mbuf *m0) >>>> { >>>> struct mbuf *m; >>>> >>>> SOCKBUF_LOCK_ASSERT(sb); >>>> >>>> if (m0 == 0) >>>> return; >>>> m = sb->sb_mb; >>>> >>>> if (m) >>>> while (m->m_nextpkt) >>>> m = m->m_nextpkt; >>>> >>>> --> m is still NULL at this point >>>> >>>> /* >>>> * Put the first mbuf on the queue. Note this permits zero length >>>> * records. >>>> */ >>>> sballoc(sb, m0); >>>> SBLASTRECORDCHK(sb); >>>> >>>> --> passed successfully, because sb_mb == sb_lastrecord == NULL (i.e. >>>> sockbuf is empty) >>>> >>>> SBLINKRECORD(sb, m0); >>>> >>>> --> at this point sb_mb == sb_lastrecord == m0, _but_ sb_mtail == NULL >>>> >>>> if (m) >>>> m->m_nextpkt = m0; >>>> else >>>> sb->sb_mb = m0; >>>> >>>> --> not sure about those lines above, didn't SBLINKRECORD(sb, m0) take >>>> care of it already? >>>> --> in any case, still, sb_mb == sb_lastrecord == m0 _and_ still >>>> sb_mtail == NULL >>>> >>>> m = m0->m_next; >>>> m0->m_next = 0; >>>> >>>> --> m is still NULL here >>>> >>>> if (m && (m0->m_flags & M_EOR)) { >>>> m0->m_flags &= ~M_EOR; >>>> m->m_flags |= M_EOR; >>>> } >>>> >>>> --> sbcompress() is called with m == NULL, which is triggers the panic >>>> (read below) >>>> >>>> sbcompress(sb, m, m0); >>>> } >>>> >>>> =========== >>>> >>>> void >>>> sbcompress(struct sockbuf *sb, struct mbuf *m, struct mbuf *n) >>>> { >>>> int eor = 0; >>>> struct mbuf *o; >>>> >>>> SOCKBUF_LOCK_ASSERT(sb); >>>> >>>> while (m) { >>>> >>>> --> lots of code that never gets executed because m == NULL >>>> >>>> } >>>> if (eor) { >>>> KASSERT(n != NULL, ("sbcompress: eor && n == NULL")); >>>> n->m_flags |= eor; >>>> } >>>> >>>> --> this where panic happens, because sb_mbtail is still NULL, but >>>> sockbuf now contains exactly one record >>>> >>>> SBLASTMBUFCHK(sb); >>>> } >>>> >>>> so, it looks like, sbcompress() should only be called when m != NULL. >>>> also, when m == NULL, m0 should be marked as EOR. >>> >>> actually, no, EOR should be set (or not set already). >>> >>>> comments anyone? ok, this is completely untested, so be warned :) would something like the following work? am i missing something? > svn diff Index: uipc_sockbuf.c =================================================================== --- uipc_sockbuf.c (revision 191012) +++ uipc_sockbuf.c (working copy) @@ -577,10 +577,6 @@ if (m0 == 0) return; - m = sb->sb_mb; - if (m) - while (m->m_nextpkt) - m = m->m_nextpkt; /* * Put the first mbuf on the queue. Note this permits zero length * records. @@ -588,17 +584,17 @@ sballoc(sb, m0); SBLASTRECORDCHK(sb); SBLINKRECORD(sb, m0); - if (m) - m->m_nextpkt = m0; - else - sb->sb_mb = m0; + sb->sb_mbtail = m0; m = m0->m_next; m0->m_next = 0; - if (m && (m0->m_flags & M_EOR)) { - m0->m_flags &= ~M_EOR; - m->m_flags |= M_EOR; + if (m != NULL) { + if (m0->m_flags & M_EOR) { + m0->m_flags &= ~M_EOR; + m->m_flags |= M_EOR; + } + + sbcompress(sb, m, m0); } - sbcompress(sb, m, m0); } /* == thanks, max