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

[pluknet]

Sorry for top-posting.

This is a fairly old bug.
See my investigation
http://lists.freebsd.org/pipermail/freebsd-net/2008-August/019345.html

2009/4/17 Maksim Yevmenkin <maksim.yevmenkin at gmail.com>:
> On Thu, Apr 16, 2009 at 7:41 PM, Maksim Yevmenkin
> <maksim.yevmenkin at gmail.com> wrote:
>> On Thu, Apr 16, 2009 at 7:22 PM, Maksim Yevmenkin
>> <maksim.yevmenkin at gmail.com> wrote:
>>> On Thu, Apr 16, 2009 at 5:39 PM, Maksim Yevmenkin
>>> <maksim.yevmenkin at gmail.com> wrote:
>>>> On Thu, Apr 16, 2009 at 12:59 PM, Alexander Best
>>>> <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?
>>
>> i think there is also something strange going on in
>> sbappendaddr_locked(), basically,
>>
>> int
>> sbappendaddr_locked(struct sockbuf *sb, const struct sockaddr *asa,
>>    struct mbuf *m0, struct mbuf *control)
>> {
>>        struct mbuf *m, *n, *nlast;
>>        int space = asa->sa_len;
>>
>>        SOCKBUF_LOCK_ASSERT(sb);
>>
>>        if (m0 && (m0->m_flags & M_PKTHDR) == 0)
>>                panic("sbappendaddr_locked");
>>        if (m0)
>>                space += m0->m_pkthdr.len;
>>        space += m_length(control, &n);
>>
>>        if (space > sbspace(sb))
>>                return (0);
>> #if MSIZE <= 256
>>        if (asa->sa_len > MLEN)
>>                return (0);
>> #endif
>>        MGET(m, M_DONTWAIT, MT_SONAME);
>>        if (m == 0)
>>                return (0);
>>        m->m_len = asa->sa_len;
>>        bcopy(asa, mtod(m, caddr_t), asa->sa_len);
>>
>> --> at this point n is not initialized, or at least i do not see where
>> it was initialized. shouldn't be compiler giving a warning here?
>>
>>        if (n)
>>                n->m_next = m0;         /* concatenate data to control */
>>        else
>>                control = m0;
>>
>> am i missing something obvious here?
>
> ignore the last one :) i missed m_length(control, &n). ok, need to get
> away from the screen :)
>
> thanks,
> max
> _______________________________________________
> freebsd-current at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe at freebsd.org"
>



-- 
wbr,
pluknet