From owner-freebsd-current@FreeBSD.ORG Sat Jun 25 13:31:05 2005 Return-Path: X-Original-To: current@freebsd.org Delivered-To: freebsd-current@FreeBSD.ORG Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 887B316A41C for ; Sat, 25 Jun 2005 13:31:05 +0000 (GMT) (envelope-from pho@holm.cc) Received: from relay02.pair.com (relay02.pair.com [209.68.5.16]) by mx1.FreeBSD.org (Postfix) with SMTP id E5C2143D1D for ; Sat, 25 Jun 2005 13:31:04 +0000 (GMT) (envelope-from pho@holm.cc) Received: (qmail 734 invoked from network); 25 Jun 2005 13:31:03 -0000 Received: from unknown (HELO peter.osted.lan) (unknown) by unknown with SMTP; 25 Jun 2005 13:31:03 -0000 X-pair-Authenticated: 80.161.118.233 Received: from peter.osted.lan (localhost.osted.lan [127.0.0.1]) by peter.osted.lan (8.13.1/8.13.1) with ESMTP id j5PDV29V023620; Sat, 25 Jun 2005 15:31:02 +0200 (CEST) (envelope-from pho@peter.osted.lan) Received: (from pho@localhost) by peter.osted.lan (8.13.1/8.13.1/Submit) id j5PDUqor023619; Sat, 25 Jun 2005 15:30:52 +0200 (CEST) (envelope-from pho) Date: Sat, 25 Jun 2005 15:30:52 +0200 From: Peter Holm To: Mike Silbersack Message-ID: <20050625133052.GA23599@peter.osted.lan> References: <20050624212729.C537@odysseus.silby.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20050624212729.C537@odysseus.silby.com> User-Agent: Mutt/1.4.2.1i Cc: current@freebsd.org, Thierry Herbelot Subject: Re: Mbuf double-free guilty party detection patch X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 25 Jun 2005 13:31:05 -0000 On Fri, Jun 24, 2005 at 09:31:28PM -0500, Mike Silbersack wrote: > > The attached patch stores the address of who freed an > mbuf/cluster/whatever inside it, then prints that address when panicing. > You can then feed that address into "x 0xwhatever" in DDB to see who the > semi-guilty party is. > > Two flaws in the patch as is: > > - It's messy and not compatible with non-i386, cleanups are needed. > > - If the mbuf in question is part of a mbuf chain, we'll see m_freem as > the guilty party, because it called m_free. > > So, if you're one of the people seeing panics due to mbufs being used > after free, please try applying the patch and see what results you get. > If you keep getting m_freem as the previous user, then I'll have to > enhance it to work around that. > > Mike "Silby" Silbersack Didn't have much luck with your patch: x.123b:This memory last freed by: 0 x.123b-panic: Memory modified after free 0xc25a7100(256) val=0 @ 0xc25a7100 -- x.123c:This memory last freed by: 0 x.123c-panic: Memory modified after free 0xc1f60e00(256) val=0 @ 0xc1f60e00 -- x.123e:This memory last freed by: 0xc2fa6c00 x.123e-panic: Memory modified after free 0xc2fa6a00(256) val=c2fa6c00 @ 0xc2fa6a00 - Peter > diff -u -r /usr/src/sys.old/kern/uipc_mbuf.c /usr/src/sys/kern/uipc_mbuf.c > --- /usr/src/sys.old/kern/uipc_mbuf.c Fri Jun 24 20:13:59 2005 > +++ /usr/src/sys/kern/uipc_mbuf.c Fri Jun 24 20:50:16 2005 > @@ -219,7 +219,7 @@ > * storage attached to them if the reference count hits 0. > */ > void > -mb_free_ext(struct mbuf *m) > +mb_free_ext(struct mbuf *m, void *arg) > { > u_int cnt; > int dofree; > @@ -249,10 +249,10 @@ > * Do the free, should be safe. > */ > if (m->m_ext.ext_type == EXT_PACKET) { > - uma_zfree(zone_pack, m); > + uma_zfree_arg(zone_pack, m, arg); > return; > } else if (m->m_ext.ext_type == EXT_CLUSTER) { > - uma_zfree(zone_clust, m->m_ext.ext_buf); > + uma_zfree_arg(zone_clust, m->m_ext.ext_buf, arg); > m->m_ext.ext_buf = NULL; > } else { > (*(m->m_ext.ext_free))(m->m_ext.ext_buf, > @@ -266,7 +266,7 @@ > m->m_ext.ext_buf = NULL; > } > } > - uma_zfree(zone_mbuf, m); > + uma_zfree_arg(zone_mbuf, m, arg); > } > > /* > @@ -1381,4 +1381,19 @@ > if (m_final) > m_freem(m_final); > return (NULL); > +} > + > +struct mbuf * > +m_free(struct mbuf *m) > +{ > + struct mbuf *n = m->m_next; > + > +#ifdef INVARIANTS > + m->m_flags |= M_FREELIST; > +#endif > + if (m->m_flags & M_EXT) > + mb_free_ext(m, __builtin_return_address(0)); > + else > + uma_zfree_arg(zone_mbuf, m, __builtin_return_address(0)); > + return n; > } > diff -u -r /usr/src/sys.old/sys/mbuf.h /usr/src/sys/sys/mbuf.h > --- /usr/src/sys.old/sys/mbuf.h Fri Jun 24 20:17:31 2005 > +++ /usr/src/sys/sys/mbuf.h Fri Jun 24 20:53:07 2005 > @@ -350,10 +350,10 @@ > static __inline struct mbuf *m_gethdr(int how, short type); > static __inline struct mbuf *m_getcl(int how, short type, int flags); > static __inline struct mbuf *m_getclr(int how, short type); /* XXX */ > -static __inline struct mbuf *m_free(struct mbuf *m); > +struct mbuf *m_free(struct mbuf *m); > static __inline void m_clget(struct mbuf *m, int how); > static __inline void m_chtype(struct mbuf *m, short new_type); > -void mb_free_ext(struct mbuf *); > +void mb_free_ext(struct mbuf *, void *arg); > > static __inline > struct mbuf * > @@ -404,7 +404,8 @@ > return (uma_zalloc_arg(zone_pack, &args, how)); > } > > -static __inline > +#if 0 > +static > struct mbuf * > m_free(struct mbuf *m) > { > @@ -414,11 +415,12 @@ > m->m_flags |= M_FREELIST; > #endif > if (m->m_flags & M_EXT) > - mb_free_ext(m); > + mb_free_ext(m, __builtin_return_address(0)); > else > - uma_zfree(zone_mbuf, m); > + uma_zfree_arg(zone_mbuf, m, __builtin_return_address(0)); > return n; > } > +#endif > > static __inline > void > diff -u -r /usr/src/sys.old/vm/uma_dbg.c /usr/src/sys/vm/uma_dbg.c > --- /usr/src/sys.old/vm/uma_dbg.c Fri Jun 24 20:13:27 2005 > +++ /usr/src/sys/vm/uma_dbg.c Fri Jun 24 21:11:05 2005 > @@ -66,11 +66,14 @@ > u_int32_t *p; > > cnt = size / sizeof(uma_junk); > + cnt -= sizeof(void *); > > for (p = mem; cnt > 0; cnt--, p++) > - if (*p != uma_junk) > + if (*p != uma_junk) { > + printf("This memory last freed by: %p\n", (void *)*p); > panic("Memory modified after free %p(%d) val=%x @ %p\n", > mem, size, *p, p); > + } > return (0); > } > > @@ -87,9 +90,11 @@ > u_int32_t *p; > > cnt = size / sizeof(uma_junk); > + cnt -= sizeof(void *); > > for (p = mem; cnt > 0; cnt--, p++) > *p = uma_junk; > + *p = (int)arg; > } > > /*