Date: Mon, 25 Nov 2002 13:41:13 -0500 (EST) From: Robert Watson <rwatson@FreeBSD.ORG> To: Sam Leffler <sam@errno.com> Cc: Andrew Gallatin <gallatin@cs.duke.edu>, Luigi Rizzo <rizzo@icir.org>, current@FreeBSD.ORG Subject: Re: mbuf header bloat ? Message-ID: <Pine.NEB.3.96L.1021125133654.42342C-100000@fledge.watson.org> In-Reply-To: <232901c294b0$feda0090$52557f42@errno.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 25 Nov 2002, Sam Leffler wrote:
> As I explained to you; the handling of mtags mimics what was there for
> the aux mbufs. I did this intentionally to avoid changes that might
> introduce subtle problems. My intent was to cleanup this stuff after
> 5.0 releases by replacing the pkthdr copy macros with separate DUP+MOVE
> macros ala openbsd. I did this in my original implemention but
> discarded it when I did the -current integration.
And I agree this is the right direction to take this in once we are out of
the freeze.
> I don't believe it's necessary to overload the basic mtag structure but
> instead introduce a specific cookie that enables a more general
> mechanism that would be suitable for your needs.
That sounds like a reasonable approach.
> > (3) Not all code generating packets properly initializes m_tag field. The
> > one example I've come across so far is the use of m_getcl() to grab
> > mbufs with an attached cluster -- it never initializes the SLIST
> > properly, as far as I can tell. Right now that's used in device
> > drivers, and also in the BPF packet generation code. If the header is
> > zero'd, this may be OK due to an implicit proper initialization, but
> > this is concerning. We need to do more work to normalize packet
> > handling.
>
> I don't see this problem; m_getcl appears to do the right thing.
>
> > (4) Code still exists that improperly hand-copies mbuf packet header data.
> > if_loop.c contains some particular bogus code that also triggers a
> > panic in the MAC code for the same reason. If m_tag data ever passes
> > through if_loop and hits the re-alignment case introduced by KAME, the
> > system will panic when the tag data is free'd. This code all needs to
> > be normalized, and proper semantics must be enforced.
> >
>
> I don't see this problem; looutput looks to do the right thing. FWIW I've
> passed mbufs w/ mtags through the loopback interface.
This refers specifically to the following code snippet:
if (m && m->m_next != NULL && m->m_pkthdr.len < MCLBYTES) {
struct mbuf *n;
MGETHDR(n, M_DONTWAIT, MT_HEADER);
if (!n)
goto contiguousfail;
MCLGET(n, M_DONTWAIT);
if (! (n->m_flags & M_EXT)) {
m_freem(n);
goto contiguousfail;
}
m_copydata(m, 0, m->m_pkthdr.len, mtod(n, caddr_t));
n->m_pkthdr = m->m_pkthdr;
n->m_len = m->m_pkthdr.len;
n->m_pkthdr.aux = m->m_pkthdr.aux;
m->m_pkthdr.aux = (struct mbuf *)NULL;
m_freem(m);
m = n;
}
In this scenario, the mbuf header for (n) is initialized to an empty m_tag
chain. The direct assignment of (n)'s pkthdr data from (m) copies the
pointers from (m). (m) is then freed, which causes the mbuf allocator to
go through and delete the m_tag chain on (m), freeing the individual
entries in the chain, which are now still referenced by (n). You only
bump into this case if you trigger the conditional clause above. In the
MAC code, this case results in a duplicate free() when (n) is later
released -- unless I'm mis-reading things (quite possible), the same
failure mode should exist for the m_tag code. In my local tree, I have
this case disabled, and I've been trying to figure out what the right
solution is -- probably to move to using M_COPY_PKTHDR() and then doing
the fixup.
Robert N M Watson FreeBSD Core Team, TrustedBSD Projects
robert@fledge.watson.org Network Associates Laboratories
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.NEB.3.96L.1021125133654.42342C-100000>