Re: possible bug in the sbappendrecord_locked()? (Was: Re: core dump with bluetooth device)

On Thu, Apr 16, 2009 at 9:48 PM, pluknet <pluknet_at_gmail.com> 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.

>>>>> <alexbestms_at_math.uni-muenster.de> 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)
_at__at_ -577,10 +577,6 _at__at_

        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.
_at__at_ -588,17 +584,17 _at__at_
        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