From owner-freebsd-current@FreeBSD.ORG Fri Apr 17 17:24:28 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 0EC1510656BF for ; Fri, 17 Apr 2009 17:24:28 +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 AFEB58FC32 for ; Fri, 17 Apr 2009 17:24:27 +0000 (UTC) (envelope-from maksim.yevmenkin@gmail.com) Received: by gxk20 with SMTP id 20so2655354gxk.19 for ; Fri, 17 Apr 2009 10:24:27 -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=x0BR4Xd/YxioPAXWl/KbeL8569yPk5z6k30c7Oq4hso=; b=dD6n3neijzijdYoOEjLk4xTxAXGCIQpZpf/4fxJiHsB6RGWapiVCrSsTRxOwwDkRI1 zZ2dX0lomQgdJmFLTrpBzH7LZcTa0t1sa5alRq5dlxKMJtnXZBHl6A6Og5qnseOelknN RYn8HVV1KTr0gADKCgtgLTmFu6X8O7TTxWWD8= 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=mJM6K256uLwH+iOVIG3mw2f5YDEZ5BJaYU/fPxC986wMRIcF/SlPZMv0yY8F+5+OdW VmVfR2XkdQnE27oaeoqgJ4L0qItNNIhUbMEEZAPtaHNv0yoxAnpbCKyUM3sHf5/TRPFs fs4lBEg8TF4Ih/TtedHubB853D9ndVd1lWVQ4= MIME-Version: 1.0 Received: by 10.90.55.3 with SMTP id d3mr3421208aga.100.1239989066907; Fri, 17 Apr 2009 10:24:26 -0700 (PDT) In-Reply-To: References: Date: Fri, 17 Apr 2009 10:24:26 -0700 Message-ID: From: Maksim Yevmenkin To: pluknet , Alexander Best Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: 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 17:24:29 -0000 On Fri, Apr 17, 2009 at 8:55 AM, Maksim Yevmenkin wrote: > 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); > } > > /* > > == the patch seems to work for me. i do not see any crashes. please try it and let me know if you still have crashes with SOCKBUF_DEBUG thanks, max