Date: Sat, 25 Jun 2005 15:30:52 +0200 From: Peter Holm <peter@holm.cc> To: Mike Silbersack <silby@silby.com> Cc: current@freebsd.org, Thierry Herbelot <thierry@herbelot.com> Subject: Re: Mbuf double-free guilty party detection patch Message-ID: <20050625133052.GA23599@peter.osted.lan> In-Reply-To: <20050624212729.C537@odysseus.silby.com> References: <20050624212729.C537@odysseus.silby.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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;
> }
>
> /*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20050625133052.GA23599>