Skip site navigation (1)Skip section navigation (2)
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>