Date: Tue, 23 Jun 2009 00:27:59 +0200 From: Andre Oppermann <andre@freebsd.org> To: Kip Macy <kmacy@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Sam Leffler <sam@freebsd.org> Subject: Re: svn commit: r194643 - head/sys/kern Message-ID: <4A40056F.4000207@freebsd.org> In-Reply-To: <3c1674c90906221510s9b62e25w3a5d41c21bd512e1@mail.gmail.com> References: <200906221935.n5MJZdLJ050266@svn.freebsd.org> <3c1674c90906221510s9b62e25w3a5d41c21bd512e1@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Kip Macy wrote: > On Mon, Jun 22, 2009 at 12:35 PM, Andre Oppermann<andre@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4A40056F.4000207>