DragonFlyBSD

:Another page fault kernel panic, but it doesn't look the same as issue530.
:
:The sources are from July 11:
:
:DragonFly ylem.theshell.com 1.9.0-DEVELOPMENT DragonFly 1.9.0-DEVELOPMENT #26: Wed Jul 11 20:21:15 EDT 2007 root@ylem.theshell.com:/usr/obj/usr/src/sys/YLEM i386
:
:Fatal trap 12: page fault while in kernel mode
:mp_lock = 00000000; cpuid = 0; lapic.id = 00000000
:fault virtual address = 0x8000008b

It's dying trying to traverse the in_ifaddrhead list in netinet/in.c.

(kgdb) frame 7
#7 0xc0207a9b in in_localaddr (in={s_addr = 997540561})
at /usr/src/sys/netinet/in.c:97
97 if ((i & ia->ia_subnetmask) == ia->ia_subnet)
(kgdb)

ia is 0x7fffffff.  When I look at entry there's a lot of 0xdeadc0de
    data in there, which means the structure got freed without being removed
    from the list.

(kgdb) print in_ifaddrhead.tqh_first->ia_link.tqe_next->ia_link.tqe_next->ia_link.tqe_next
$12 = (struct in_ifaddr *) 0xe82ed840
(kgdb) print *$12
(kgdb) print *$12
$13 = {
ia_ifa = {
ifa_addr = 0xe82ed920,
ifa_dstaddr = 0xdeadc0de,
ifa_netmask = 0xdeadc0de,
if_data = {
ifi_type = 222 'Þ',
ifi_physical = 192 'À',
ifi_addrlen = 173 '­',

...
ia_link = {
tqe_next = 0x7fffffff,
tqe_prev = 0xffffffff
}

So we need to audit all the code that manipulates the in_ifaddrhead 
    list and check for structures that might get freed without being removed
    from the list.

-Matt

I'm having no luck. Got any pointers/ideas?

--Peter

:> list and check for structures that might get freed without being remo=
:ved
:> from the list.
:>=20
:
:I'm having no luck. Got any pointers/ideas?
:
:--Peter

I'll do a code audit today to try to find it.

One question: Did you try to unload any modules or ifconfig any interfaces
    down or do anything that might cause a network interface to detach before
    the panic?

-Matt

No. In addition to the normal network traffic (web, ftp, email, etc.) I
was in a vkernel and pinging yahoo.com to see if the network was
working. I received 1 reply before it paniced. The setup was exactly
like the example in vkernel(7).

--Peter

:No. In addition to the normal network traffic (web, ftp, email, etc.) I
:was in a vkernel and pinging yahoo.com to see if the network was
:working. I received 1 reply before it paniced. The setup was exactly
:like the example in vkernel(7).
:
:--Peter

Was it the vkernel that paniced or the real kernel?

This is a good clue.  If it was the real kernel then I'll bet there's
    a bug in the network interface the vkernel uses to pass data to the
    real kernel (if_tap I think).

-Matt

It was the real kernel.

Yes, I was using if_tap in the real kernel. I've been unable to
reproduce the panic though.

--Peter

I think there a ref-counting problem with the ifaddr structure. This
structure is embedded in the ifnet structure and used all over the place.
I can't find the exact cause so here's a patch which will hopefully
force a panic (with INVARIANTS turned on of course) if any attempt is
made to free the structure before it has been removed from the address
list. If we can catch it here it should become obvious where the bug
is.

-Matt

Index: net/if_var.h ===================================================================
RCS file: /cvs/src/sys/net/if_var.h,v
retrieving revision 1.39
diff u -p -r1.39 if_var.h
-- net/if_var.h 16 Aug 2007 20:03:57 -0000 1.39
+++ net/if_var.h 3 Sep 2007 21:02:41 -0000
@ -375,6 +375,7 @ (struct ifaddr *, struct sockaddr *); ===================================================================
RCS file: /cvs/src/sys/netinet/in.c,v
retrieving revision 1.20
diff u -p -r1.20 in.c
-- netinet/in.c 30 Sep 2006 22:38:21 -0000 1.20
+++ netinet/in.c 3 Sep 2007 21:00:54 -0000
@ -274,11 +274,15 @ bzero(ia, sizeof ia);
/ * Protect from ipintr() traversing address list * while we're modifying it.
+ *
+ * IFA_XLIST applies to the in_ifaddrhead list,
+ * set it when the ifa has been added.
*/
crit_enter();

• Protect from ipintr() traversing address list while we're modifying
• it.
  + *
  + * IFA_XLIST applies to the in_ifaddrhead list, clear it when the ifa
  + * has been removed.
  */
  lwkt_serialize_enter(ifp->if_serializer);
  TAILQ_REMOVE(&ifp->if_addrhead, &ia->ia_ifa, ifa_link);
  TAILQ_REMOVE(&in_ifaddrhead, ia, ia_link);
  + ia->ia_ifa.ifa_flags &= ~IFA_XLIST;
  LIST_REMOVE(ia, ia_hash);
  IFAFREE;
  lwkt_serialize_exit(ifp->if_serializer);

ifa->ifa_addr = (struct sockaddr )&ia->ia_addr;
@ -444,10 +448,14 @ 
     /

It happened again. I was running it with the provided patch, but it looks
like the same panic. The kernel are core will be on leaf:~/crash/4/.

Again, I was messing around with vkernels, and it seemed to crash as the
vkernel was booting (looked like it had just configured the network
interfaces). Just a reminder that this is the real kernel that's panicing,
not the vkernel.

--Peter

:...
:> made to free the structure before it has been removed from the address
:> list. If we can catch it here it should become obvious where the bug
:> is.
:>=20
:
:It happened again. I was running it with the provided patch, but it looks
:like the same panic. The kernel are core will be on leaf:~/crash/4/.
:
:Again, I was messing around with vkernels, and it seemed to crash as the
:vkernel was booting (looked like it had just configured the network
:interfaces). Just a reminder that this is the real kernel that's panicing,
:not the vkernel.
:
:--Peter

Hmm.  Were you running my patch on the real kernel?  It should have caught
    the problem but if you were running my patch it doesn't look like it did.

-Matt

Yes. Any ideas?

--Peter

Looking around FreeBSD source I saw this, and was wondering if it could
help?

http://www.theshell.com/cgi-bin/gitweb.cgi?p=freebsd-src.git;a=commitdiff;h=16f96b132d344c7682df05b3bf74278150a5b3b1

commit 16f96b132d344c7682df05b3bf74278150a5b3b1
Author: peadar <peadar>
Date: Wed May 25 13:52:03 2005 +0000

Separate out address-detaching part of if_detach into if_purgeaddrs,
    so if_tap doesn't need to rely on locally-rolled code to do same.

The observable symptom of if_tap's bzero'ing the address details
    was a crash in "ifconfig tap0" after an if_tap device was closed.

Reported By: Matti Saarinen (mjsaarin at cc dot helsinki dot fi)

diff --git a/sys/net/if.c b/sys/net/if.c
index 8aff428..f056329 100644
--- a/sys/net/if.c
+++ b/sys/net/if.c
@ -27,7 +27,7 @ * SUCH DAMAGE. * * @(#)if.c 8.5 (Berkeley) 1/9/95
- * $FreeBSD: src/sys/net/if.c,v 1.227 2005/04/20 09:30:54 glebius Exp $
+ * $FreeBSD: src/sys/net/if.c,v 1.228 2005/05/25 13:52:03 peadar Exp $
*/

#include "opt_compat.h" 
@ -530,13 +530,52 @ if_attachdomain1(struct ifnet *ifp)
 }

/*
+ * Remove any network addresses from an interface.
+ /

+void
+if_purgeaddrs(struct ifnet *ifp)
{
+    struct ifaddr *ifa, *next;

    TAILQ_FOREACH_SAFE(ifa, &ifp->if_addrhead, ifa_link, next) {

        if (ifa->ifa_addr->sa_family  AF_LINK)
+            continue;
#ifdef INET
        / XXX: Ugly!! ad hoc just for INET /
+        if (ifa->ifa_addr && ifa->ifa_addr->sa_family  AF_INET) {
+            struct ifaliasreq ifr;

            bzero(&ifr, sizeof(ifr));
+            ifr.ifra_addr = *ifa->ifa_addr;
+            if (ifa->ifa_dstaddr)
+                ifr.ifra_broadaddr = *ifa->ifa_dstaddr;
+            if (in_control(NULL, SIOCDIFADDR, (caddr_t)&ifr, ifp,
+                NULL)  0)
+                continue;
+        }
#endif / INET /
#ifdef INET6
+        if (ifa->ifa_addr && ifa->ifa_addr->sa_family  AF_INET6) {
+            in6_purgeaddr(ifa);
+            / ifp_addrhead is already updated /
+            continue;
+        }
#endif / INET6 /
        TAILQ_REMOVE(&ifp->if_addrhead, ifa, ifa_link);
+        IFAFREE(ifa);
+    }
}

+/
 * Detach an interface, removing it from the
 * list of "active" interfaces.
  */
 void
 if_detach(struct ifnet *ifp)
{
-    struct ifaddr *ifa, *next;
+    struct ifaddr *ifa;
     struct radix_node_head    *rnh;
     int s;
     int i;
@ -568,35 +607,7 @ if_detach(struct ifnet *ifp)
         altq_detach(&ifp->if_snd);
 #endif

- for (ifa = TAILQ_FIRST(&ifp->if_addrhead); ifa; ifa = next) {
- next = TAILQ_NEXT(ifa, ifa_link);

if (ifa->ifa_addr->sa_family AF_LINK)
- continue;
#ifdef INET
/* XXX: Ugly!! ad hoc just for INET /
- if (ifa->ifa_addr && ifa->ifa_addr->sa_family AF_INET) {
- struct ifaliasreq ifr;

bzero(&ifr, sizeof(ifr));
- ifr.ifra_addr = *ifa->ifa_addr;
- if (ifa->ifa_dstaddr)
- ifr.ifra_broadaddr = *ifa->ifa_dstaddr;
- if (in_control(NULL, SIOCDIFADDR, (caddr_t)&ifr, ifp,
- NULL) 0)
- continue;
- }
#endif / INET /
#ifdef INET6
- if (ifa->ifa_addr && ifa->ifa_addr->sa_family AF_INET6) {
- in6_purgeaddr(ifa);
- / ifp_addrhead is already updated /
- continue;
- }
#endif / INET6 */
TAILQ_REMOVE(&ifp->if_addrhead, ifa, ifa_link);
- IFAFREE;
- }
+ if_purgeaddrs(ifp);

#ifdef INET6
     /*
diff --git a/sys/net/if_tap.c b/sys/net/if_tap.c
index 232c96d..a807bd5 100644
--- a/sys/net/if_tap.c
+++ b/sys/net/if_tap.c
@ -31,7 +31,7 @
  */

/*
- * $FreeBSD: src/sys/net/if_tap.c,v 1.53 2005/05/04 18:55:02 emax Exp $
+ * $FreeBSD: src/sys/net/if_tap.c,v 1.54 2005/05/25 13:52:03 peadar Exp $
 * $Id: if_tap.c,v 0.21 2000/07/23 21:46:02 max Exp $
  */

@ -408,6 +408,7 @ tapclose(dev, foo, bar, td)
int bar;
struct thread *td; {
+ struct ifaddr ifa;
struct tap_softc *tp = dev->si_drv1;
struct ifnet *ifp = &tp->tap_if;
int s;
@ -426,24 +427,10 @ tapclose(dev, foo, bar, td)
s = splimp();
if_down(ifp);
if (ifp->if_flags & IFF_RUNNING) {
- / find internet addresses and delete routes /
- struct ifaddr *ifa = NULL;

/ In desparate need of ifaddr locking. /
TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) {
- if (ifa->ifa_addr->sa_family == AF_INET) {
- rtinit(ifa, (int)RTM_DELETE, 0);

/ remove address from interface /
- bzero(ifa->ifa_addr,
- sizeof((ifa->ifa_addr)));
- bzero(ifa->ifa_dstaddr,
- sizeof());
- bzero(ifa->ifa_netmask,
- sizeof((ifa->ifa_netmask)));
- }
+ rtinit(ifa, (int)RTM_DELETE, 0);
}

+ if_purgeaddrs(ifp);
ifp>if_flags &= ~IFF_RUNNING;
}
splx(s);
diff --git a/sys/net/if_var.h b/sys/net/if_var.h
index ab8d389..6d9cbed 100644
--- a/sys/net/if_var.h
+++ b/sys/net/if_var.h
@ -27,7 +27,7 @ * SUCH DAMAGE. * * From: @(#)if.h 8.1 (Berkeley) 6/10/93
- * $FreeBSD: src/sys/net/if_var.h,v 1.95 2005/04/20 09:30:54 glebius Exp $
+ * $FreeBSD: src/sys/net/if_var.h,v 1.96 2005/05/25 13:52:03 peadar Exp $
*/

#ifndef    NET_IF_VAR_H
@ -629,6 +629,7 @ int    if_allmulti(struct ifnet *, int);
 void    if_attach(struct ifnet *);
 int    if_delmulti(struct ifnet *, struct sockaddr *);
 void    if_detach(struct ifnet *);
+void    if_purgeaddrs(struct ifnet *);
 void    if_down(struct ifnet *);
 void    if_initname(struct ifnet *, const char *, int);
 void    if_link_state_change(struct ifnet *, int);

My question about the panic is:
Have you assigned IP address to the tap(4) device via -I option to vkernel?

The patch looks good to me; it still needs little modification tho.
If you had not assign IP address to tap(4) then I think this patch
probably won't fix your panic.

Best Regards,
sephe

Since ifa_addr is points at the correct memory position but rest of
the pointers are 'deadc0de', the on-list dead in_ifaddr may go through
following code and become dead:
netinet/in.c:

in_ifinit() {
...
if (scrub) {
ia->ia_ifa.ifa_addr = (struct sockaddr *)&oldaddr;
in_ifscrub(ifp, ia);
ia->ia_ifa.ifa_addr = (struct sockaddr *)&ia->ia_addr;
}
...
}

I think in_ifscrub() killed the last reference, but this code path is
rarely stressed:
1) From SIOCSIFADDR, which is not used by ifconfig(8) or vkernel(7)
2) From sppp_set_ip_addr(), is it used on the panic system?

Best Regards,
sephe

sppp is not used.

--Peter

Yes.

Thanks for checking this out. I'm not super comfortable with network code,
so if you have some time, I'd love if you could perform those "little
modifications."

--Peter

:Since ifa_addr is points at the correct memory position but rest of
:the pointers are 'deadc0de', the on-list dead in_ifaddr may go through
:following code and become dead:
:netinet/in.c:
:
:in_ifinit()
:{
:...
: if (scrub) {
: ia->ia_ifa.ifa_addr =3D (struct sockaddr *)&oldaddr;
: in_ifscrub(ifp, ia);
: ia->ia_ifa.ifa_addr =3D (struct sockaddr *)&ia->ia_addr;
: }
:...
:}
:
:I think in_ifscrub() killed the last reference, but this code path is
:rarely stressed:
:1) From SIOCSIFADDR, which is not used by ifconfig(8) or vkernel(7)
:2) From sppp_set_ip_addr(), is it used on the panic system?
:
:Best Regards,
:sephe

I think you are on to something here.  The more I look at that code,
    not only in_ifscrub() but just in_ifinit() generally, particularly
    the fact that the ia is manipulated both inside and outside the
    serializer, the more it looks wrong.

There seem to be a ton of places where the ia can be freed.  Frankly
    the code paths are almost unreadable to me but I am guessing it is
    a call to IFAFREE() somewhere in that mess that is causing the problem.
    (see IFAFREE() in net/if_var.h line 44).

The ia is supposed to be protected by a reference count in the ia
    structure.  The governing route table entry uses a mirror flag RTF_UP
    to mean the same thing.  When an interface goes down RTM_DELETE is
    called and RTF_UP is cleared in the route entry (net/route.c line 808),
    but the IFA_ROUTE flag in the ia itself should not be cleared except
    by the interface code.

I think what is happening is that the ref count that IFAFREE() depends
    on is not being properly incremented or decremented somewhere.  There
    is either an extra IFAFREE() call being made, or an IFAREF() call is
    not being made where it should be.

I don't know where the missing call is but it is either in the route
    table code mismatching an IFAREF/IFAFREE (based I think on the RTF_UP
    flag but I don't know for sure), or the interface code mismatching an
    IFAREF/IFAFREE.

-Matt

What ever happened with this? Was this resolved, Peter?

Is there a chance the work done for:
http://www.dragonflybsd.org/cvsweb/src/sys/netinet/in.c?rev=1.26&content-type=text/x-cvsweb-markup
could have fixed this issue?

-- Dion

I haven't seen it in awhile, so I'm going to call it resolved.