svn commit: r194643 - head/sys/kern

[Andre Oppermann]

Kip Macy wrote:
> On Mon, Jun 22, 2009 at 12:35 PM, Andre Oppermann<andre at freebsd.org> wrote:
>> Author: andre
>> Date: Mon Jun 22 19:35:39 2009
>> New Revision: 194643
>> URL: http://svn.freebsd.org/changeset/base/194643
>>
>> Log:
>>  Update m_demote:
>>  - remove HT_HEADER test (MT_HEADER == MT_DATA for some time now)
>>  - be more pedantic about m_nextpkt in other than first mbuf
>>  - update m_flags to be retained
>>
>> Modified:
>>  head/sys/kern/uipc_mbuf.c
>>
>> Modified: head/sys/kern/uipc_mbuf.c
>> ==============================================================================
>> --- head/sys/kern/uipc_mbuf.c   Mon Jun 22 19:09:48 2009        (r194642)
>> +++ head/sys/kern/uipc_mbuf.c   Mon Jun 22 19:35:39 2009        (r194643)
>> @@ -320,11 +320,13 @@ m_demote(struct mbuf *m0, int all)
>>                        m->m_flags &= ~M_PKTHDR;
>>                        bzero(&m->m_pkthdr, sizeof(struct pkthdr));
>>                }
>> -               if (m->m_type == MT_HEADER)
>> -                       m->m_type = MT_DATA;
>> -               if (m != m0 && m->m_nextpkt != NULL)
>> +               if (m != m0 && m->m_nextpkt != NULL) {
>> +                       KASSERT(m->m_nextpkt == NULL,
>> +                           ("%s: m_nextpkt not NULL", __func__));
>> +                       m_freem(m->m_nextpkt);
>>                        m->m_nextpkt = NULL;
>> -               m->m_flags = m->m_flags & (M_EXT|M_EOR|M_RDONLY|M_FREELIST);
>> +               }
>> +               m->m_flags = m->m_flags & (M_EXT|M_RDONLY|M_FREELIST|M_NOFREE);
>>        }
>>
> 
> 
> Freeing an mbuf that shouldn't be there is not a safe change. You
> don't know that m_nextpkt isn't pointing at some random value in
> memory and that doing so isn't going to lead to some inexplicable
> crash some time later. This is not a good strategy from a support
> standpoint.

If m_nextpkt is a random value we will crash anyway at the next place
where it is being looked at.  If it is not being looked at we just get by
by chance.

If m_nextpkt is valid we either yank it from someone else who will crash
later on or if we just NULL it out we may have a memory leak.  IMHO the
latter is worse as a slowly building memory leak is much harder to track
down than a crash which provides kernel dumps and backtraces.  As soon as
INVARIANTS are enable the KASSERT will catch the offender red handed.

This is my opinion on it.  Other views are welcome.

-- 
Andre