On Thursday, August 16, 2012 6:33:12 pm Randall Stewart wrote:
>
> On Aug 16, 2012, at 3:34 PM, John Baldwin wrote:
>
> > On Thursday, August 16, 2012 1:55:17 pm Randall Stewart wrote:
> >> Author: rrs
> >> Date: Thu Aug 16 17:55:16 2012
> >> New Revision: 239334
> >> URL: http://svn.freebsd.org/changeset/base/239334
> >>
> >> Log:
> >> Its never a good idea to double free the same
> >> address.
> >>
> >> MFC after: 1 week (after the other commits ahead of this gets
> >> MFC'd)
> >>
> >> Modified:
> >> head/sys/netinet/in.c
> >>
> >> Modified: head/sys/netinet/in.c
> >>
> > ==============================================================================
> >> --- head/sys/netinet/in.c Thu Aug 16 17:27:11 2012 (r239333)
> >> +++ head/sys/netinet/in.c Thu Aug 16 17:55:16 2012 (r239334)
> >> @@ -573,7 +573,7 @@ in_control(struct socket *so, u_long cmd
> >> }
> >> TAILQ_REMOVE(&ifp->if_addrhead, &ia->ia_ifa, ifa_link);
> >> IF_ADDR_WUNLOCK(ifp);
> >> - ifa_free(&ia->ia_ifa); /* if_addrhead */
> >> +/* ifa_free(&ia->ia_ifa); - Double free?? */ /* if_addrhead
> >> */
> >
> > This isn't a double free. This is dropping a reference count. In this
> > case
> > as the comment suggests, it is removing the reference held by the per-
> > interface if_addrhead list that it was just removed from two lines above.
> > Later in the function when ifa_free() is invoked:
> >
> > LIST_REMOVE(ia, ia_hash);
> > IN_IFADDR_WUNLOCK();
> > ...
> > ifa_free(&ia->ia_ifa); /* in_ifaddrhead */
> >
> > It is dropping the reference held by the in_ifaddrhead list which the ifa
> > was removed from by the above LIST_REMOVE(). Are you seeing a panic or
> > refcount underflow or some such?
> >
>
> No panic, I wish I were so lucky, I had a lockup/fault at:
>
> in_gif.c line 410 (this is 9 stable)
> -----------------------
> IN_IFADDR_RLOCK();
> TAILQ_FOREACH(ia4, &V_in_ifaddrhead, ia_link) {
> if ((ia4->ia_ifa.ifa_ifp->if_flags & IFF_BROADCAST) == 0)
> <------fault in kernel HERE
> continue;
> if (ip->ip_src.s_addr == ia4->ia_broadaddr.sin_addr.s_addr) {
> IN_IFADDR_RUNLOCK();
> return 0;
> }
> }
> IN_IFADDR_RUNLOCK();
> ------------------------
>
> I went through and made sure first that every reference using V_in_ifaddrhead
> was properly locking, and they were. The only thing I could find is this. From
> the instructions I could see in the assembly the ia4->ia_ifa.ifa_ifp was
> NULL. And
> thus caused a deref of a NULL pointer.
I don't think what you found is a bug. It is clearly dropping two different
references, so I think your fix isn't correct. I suspect there is an extra
ifa_free() in some other path that you are tripping over.
You can clearly see the two references added when an ifa is added to
V_in_ifaddrhead:
ifa_ref(ifa); /* if_addrhead */
IF_ADDR_WLOCK(ifp);
TAILQ_INSERT_TAIL(&ifp->if_addrhead, ifa, ifa_link);
IF_ADDR_WUNLOCK(ifp);
ifa_ref(ifa); /* in_ifaddrhead */
IN_IFADDR_WLOCK();
TAILQ_INSERT_TAIL(&V_in_ifaddrhead, ia, ia_link);
IN_IFADDR_WUNLOCK();
If you can get a crashdump (not sure it sounds like you are able), I would add
KTR traces of all callers to ifa_free() (logging the reference count value and
pointer) and possibly adding an explicit panic for the reference count in
ifa_free
underflowing or overflowing and then look back in history for what ifa_free()
calls were made.
--
John Baldwin
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
