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

On Fri, Apr 17, 2009 at 8:55 AM, Maksim Yevmenkin
<maksim.yevmenkin_at_gmail.com> wrote:
> 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);
>  }
>
>  /*
>
> ==

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