Date: Thu, 12 Apr 2007 12:33:07 +0100 From: "Bruce M. Simpson" <bms@FreeBSD.org> To: Andrew Thompson <thompsa@freebsd.org> Cc: freebsd-net@freebsd.org Subject: Re: ipv6 multicast refcnt panic Message-ID: <461E18F3.6000905@FreeBSD.org> In-Reply-To: <20070412010707.GC9390@heff.fud.org.nz> References: <20070412010707.GC9390@heff.fud.org.nz>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
Andrew Thompson wrote:
> I have come across this panic which appears to be from incorrect
> refcounting on the inet6 multicast code.
>
I can reproduce this panic, however I don't entirely understand what's
going on. When the same IPv6 unicast address is configured twice on the
edsc0 interface, the ifmcstat(8) utility reports that the refcnt for two
IPv6 multicast addresses changed. I do not understand why the duplicate
unicast address isn't rejected, or why groups are being joined twice for
the same address.
I strongly suspect this is a bug in KAME the kind of which existed in
netinet (whereby the 224.0.0.1 address was being joined more than once
per ifnet) which the refcounting change has exposed as a panic, a very
brief look at the ifaddr code in netinet6 suggests this is the case.
Before second address assignment:
edsc0:
inet6 f00f::1
group ff01::1%edsc0 refcnt 1
mcast-macaddr 33:33:00:00:00:01 refcnt 1
group ff02::2:f23c:3567%edsc0 refcnt 1
mcast-macaddr 33:33:f2:3c:35:67 refcnt 1
group ff02::1%edsc0 refcnt 1
mcast-macaddr 33:33:00:00:00:01 refcnt 1
group ff02::1:ff00:1%edsc0 refcnt 1
mcast-macaddr 33:33:ff:00:00:01 refcnt 1
After second address assignment:
edsc0:
inet6 f00f::1
group ff02::1:ff00:1%edsc0 refcnt 1
mcast-macaddr 33:33:ff:00:00:01 refcnt 1
group ff01::1%edsc0 refcnt 2
mcast-macaddr 33:33:00:00:00:01 refcnt 1
group ff02::2:f23c:3567%edsc0 refcnt 2
mcast-macaddr 33:33:f2:3c:35:67 refcnt 1
group ff02::1%edsc0 refcnt 2
mcast-macaddr 33:33:00:00:00:01 refcnt 1
The order of the addresses in the list has flipped around, which makes
visual comparison that much more difficult. Flipping those around to the
same order as the first sample yields:
edsc0:
inet6 f00f::1
group ff01::1%edsc0 refcnt 2
mcast-macaddr 33:33:00:00:00:01 refcnt 1
group ff02::2:f23c:3567%edsc0 refcnt 2
mcast-macaddr 33:33:f2:3c:35:67 refcnt 1
group ff02::1%edsc0 refcnt 2
mcast-macaddr 33:33:00:00:00:01 refcnt 1
group ff02::1:ff00:1%edsc0 refcnt 1
mcast-macaddr 33:33:ff:00:00:01 refcnt 1
So we can be sure the addresses themselves haven't changed, but the
refcount on the IPv6 multicast entries has gone up by 1. The refcount is
no longer proxied to the ifnet-level ifma object since the code was changed.
I don't entirely understand the relationship between the protocol-level
multicast addresses and the unicast address in netinet6, or why
attempting to configure the same unicast address on the same interface
more than once wasn't rejected with an error.
As far as I can tell the code is correct for the single address case.
I've attached a patch which makes the netinet6 detach path more like the
netinet one, though this isn't going to make a great deal of difference
apart from code style; the net code already calls in6_ifdetach() in the
right order.
We can weaken the error checking in if_delmulti() to get an operational
kernel, but this kind of defeats the point of doing the error checking
(which is there to expose such problems). When reporting problems with
the networking code it is helpful to use ifmcstat, INVARIANTS, and the
DIAGNOSTIC kernel option as I tend to add code to catch cases like this.
Regards,
BMS
[-- Attachment #2 --]
==== //depot/user/bms/netdev/sys/netinet6/in6_ifattach.c#1 - /home/bms/p4/netdev/sys/netinet6/in6_ifattach.c ====
--- /tmp/tmp.3746.0 Thu Apr 12 12:32:23 2007
+++ /home/bms/p4/netdev/sys/netinet6/in6_ifattach.c Thu Apr 12 12:25:55 2007
@@ -76,6 +76,7 @@
static int get_ifid __P((struct ifnet *, struct ifnet *, struct in6_addr *));
static int in6_ifattach_linklocal __P((struct ifnet *, struct ifnet *));
static int in6_ifattach_loopback __P((struct ifnet *));
+static void in6_purgemaddrs __P((struct ifnet *));
#define EUI64_GBIT 0x01
#define EUI64_UBIT 0x02
@@ -731,8 +732,6 @@
struct rtentry *rt;
short rtflags;
struct sockaddr_in6 sin6;
- struct in6_multi *in6m;
- struct in6_multi *in6m_next;
/* remove neighbor management table */
nd6_purge(ifp);
@@ -790,18 +789,10 @@
IFAFREE(&oia->ia_ifa);
}
- /* leave from all multicast groups joined */
-
in6_pcbpurgeif0(&udbinfo, ifp);
in6_pcbpurgeif0(&ripcbinfo, ifp);
-
- for (in6m = LIST_FIRST(&in6_multihead); in6m; in6m = in6m_next) {
- in6m_next = LIST_NEXT(in6m, in6m_entry);
- if (in6m->in6m_ifp != ifp)
- continue;
- in6_delmulti(in6m);
- in6m = NULL;
- }
+ /* leave from all multicast groups joined */
+ in6_purgemaddrs(ifp);
/*
* remove neighbor management table. we call it twice just to make
@@ -889,4 +880,23 @@
}
splx(s);
+}
+
+static void
+in6_purgemaddrs(ifp)
+ struct ifnet *ifp;
+{
+ struct in6_multi *in6m;
+ struct in6_multi *oin6m;
+
+#ifdef DIAGNOSTIC
+ printf("%s: purging ifp %p\n", __func__, ifp);
+#endif
+
+ IFF_LOCKGIANT(ifp);
+ LIST_FOREACH_SAFE(in6m, &in6_multihead, in6m_entry, oin6m) {
+ if (in6m->in6m_ifp == ifp)
+ in6_delmulti(in6m);
+ }
+ IFF_UNLOCKGIANT(ifp);
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?461E18F3.6000905>