Anyone? I have received no feedback on this. I'd really like to get review because fixing IPv6CP has turned out to be tricker than I expected.
On Tue, Dec 17, 2013 at 04:32:03PM +0100, Stefan Sperling wrote: > Our IPv6CP implementation has never handled IFID collisions. The old > code (from before r1.112 of if_spppsubr.c) punted and left the address > unchanged. The new code (as of r1.112) is supposed to handle collisions, > but instead goes into an endless conf-nak loop with the peer. > > The problem is that I made wrong assumptions about in6_update_ifa(). > It doesn't change existing addresses. It only updates parameters of > existing addresses (such as the destination address on the p2p link). > IPv6CP currently keeps suggesting the existing address which collides > with the peer's address. > > Additionally, we need to update several routing table entries when > changing the link-local address (e.g. pinging the peer doesn't work > if we don't do this). > > With the diff below we change our link-local address if it collides > with the address used by the peer. > > In this logged session, I start out using the link-local address > used by the peer (fe80::218:82ff:fe22:a563) and switch over to > an address suggested by the peer. > > Dec 17 16:01:38 jack /bsd: pppoe0: ipv6cp open(initial) > Dec 17 16:01:38 jack /bsd: pppoe0: ipv6cp initial->starting > Dec 17 16:01:38 jack /bsd: pppoe0: ipv6cp up(starting) > Dec 17 16:01:38 jack /bsd: pppoe0: ipv6cp starting->req-sent > Dec 17 16:01:38 jack /bsd: pppoe0: ipv6cp input(req-sent): <conf-req id=0x1 > len=14 > 01-0a-02-18-82-ff-fe-22-a5-63-28-29-2a-2b-2c-2d-2e-2f-30-31-32-33-34-35-36-37-00-00-00-00-00-00-00-00> > Dec 17 16:01:38 jack /bsd: pppoe0: ipv6cp parse opts: ifid > Dec 17 16:01:38 jack /bsd: pppoe0: ipv6cp parse opt values: ifid > fe80::218:82ff:fe22:a563 [conf-nak] send conf-nak suggest > fe80::18:82ff:fe22:722f > Dec 17 16:01:38 jack /bsd: pppoe0: ipv6cp input(req-sent): <conf-nak id=0x19 > len=14 > 01-0a-02-0a-e4-65-99-3e-f1-4e-9f-c0-00-03-00-7f-00-1a-3f-00-00-00-03-10-00-00-00-00-00-00-00-00-00-00> > Dec 17 16:01:38 jack /bsd: pppoe0: ipv6cp nak opts: ifid [suggestaddr > fe80::20a:e465:993e:f14e] [agree] > Dec 17 16:01:38 jack /bsd: pppoe0: ipv6cp input(req-sent): <conf-req id=0x2 > len=14 > 01-0a-02-18-82-ff-fe-22-a5-63-dd-de-df-e0-e1-e2-e3-e4-e5-e6-e7-e8-e9-ea-eb-ec-00-00-00-00-00-00-00-00> > Dec 17 16:01:38 jack /bsd: pppoe0: ipv6cp parse opts: ifid > Dec 17 16:01:38 jack /bsd: pppoe0: ipv6cp parse opt values: ifid > fe80::218:82ff:fe22:a563 [conf-ack] send conf-ack > Dec 17 16:01:38 jack /bsd: pppoe0: ipv6cp req-sent->ack-sent > Dec 17 16:01:38 jack /bsd: pppoe0: ipv6cp input(ack-sent): <conf-ack id=0x1a > len=14 > 01-0a-02-0a-e4-65-99-3e-f1-4e-00-04-00-01-ff-ff-ff-ff-ff-ff-ff-ff-80-00-00-1f-00-00-00-00-00-00-00-00> > Dec 17 16:01:38 jack /bsd: pppoe0: ipv6cp ack-sent->opened > Dec 17 16:01:38 jack /bsd: pppoe0: ipv6cp tlu > > This shows the corresponding routing table changes: > > Destination Gateway Flags > Refs Use Mtu Prio Iface > ::/104 ::1 UGRS > 0 0 - 8 lo0 > ::/96 ::1 UGRS > 0 0 - 8 lo0 > +default fe80::218:82ff:fe22:a563%pppoe0 UG > 0 0 - 56 pppoe0 > ::1 ::1 UH > 14 4 33192 4 lo0 > ::127.0.0.0/104 ::1 UGRS > 0 0 - 8 lo0 > ::224.0.0.0/100 ::1 UGRS > 0 0 - 8 lo0 > ::255.0.0.0/104 ::1 UGRS > 0 0 - 8 lo0 > ::ffff:0.0.0.0/96 ::1 UGRS > 0 0 - 8 lo0 > 2002::/24 ::1 UGRS > 0 0 - 8 lo0 > 2002:7f00::/24 ::1 UGRS > 0 0 - 8 lo0 > 2002:e000::/20 ::1 UGRS > 0 0 - 8 lo0 > 2002:ff00::/24 ::1 UGRS > 0 0 - 8 lo0 > -fe80::/10 ::1 UGRS > 0 0 - 8 lo0 > +fe80::/10 ::1 UGRS > 2 0 - 8 lo0 > fe80::%em0/64 link#1 UC > 0 0 - 4 em0 > fe80::20a:e4ff:fe3e:f14e%em0 00:0a:e4:3e:f1:4e HL > 0 0 - 4 lo0 > fe80::%lo0/64 fe80::1%lo0 U > 0 0 - 4 lo0 > fe80::1%lo0 link#4 UHL > 0 0 - 4 lo0 > -fe80::%pppoe0/64 fe80::218:82ff:fe22:a563%pppoe0 > 0 0 - 4 pppoe0 > -fe80::218:82ff:fe22:a563%pppoe0 link#6 HL > 0 0 - 4 lo0 > +fe80::%pppoe0/64 fe80::20a:e468:123e:f14e%pppoe0 U > 1 0 - 4 pppoe0 > +fe80::20a:e468:123e:f14e%pppoe0 link#6 UHL > 0 0 - 4 lo0 > +fe80::218:82ff:fe22:a563%pppoe0 link#6 UHL > 0 0 - 4 pppoe0 > fec0::/10 ::1 UGRS > 0 0 - 8 lo0 > ff01::/16 ::1 UGRS > 1 0 - 8 lo0 > ff01::%em0/32 link#1 UC > 0 0 - 4 em0 > ff01::%lo0/32 fe80::1%lo0 UC > 0 0 - 4 lo0 > -ff01::%pppoe0/32 fe80::218:82ff:fe22:a563%pppoe0 C > 0 0 - 4 pppoe0 > +ff01::%pppoe0/32 fe80::20a:e468:123e:f14e%pppoe0 UC > 0 0 - 4 pppoe0 > ff02::/16 ::1 UGRS > 1 0 - 8 lo0 > ff02::%em0/32 link#1 UC > 0 0 - 4 em0 > ff02::%lo0/32 fe80::1%lo0 UC > 0 0 - 4 lo0 > -ff02::%pppoe0/32 fe80::218:82ff:fe22:a563%pppoe0 C > 0 0 - 4 pppoe0 > +ff02::%pppoe0/32 fe80::20a:e468:123e:f14e%pppoe0 UC > 0 0 - 4 pppoe0 > > To make the netinet6 code use the IFID determined by IPv6CP, I need to > have in6_ifattach_linklocal() accept a provided IFID. I decided to replace > the altifp parameter, which wasn't used anywhere, with a new ifid parameter > that has the same purpose but suits the new sppp caller better (it's an > address, not an ifp). > > Other bug fixes contained in here are: > > - Make IPv6CP always use the latest suggested address, > (ipv6cp.req_ifid.ifra_addr.sin6_addr), so we use it even if the task > that configures the new address on the interface didn't run yet. > > - Clear the ifindex (KAME hack) in addresses suggested for IPv6CP. > An ifindex in the suggested address might confuse the peer. > > - Make in6_ifdetach() remove route to interface local allnodes > (also here: http://marc.info/?l=openbsd-tech&m=138729124402857&w=2) > This makes the above routing table changes work properly. > > I'm considering committing the above fixes separately. > > Ok? > > Index: net/if_spppsubr.c > =================================================================== > RCS file: /cvs/src/sys/net/if_spppsubr.c,v > retrieving revision 1.113 > diff -u -p -r1.113 if_spppsubr.c > --- net/if_spppsubr.c 20 Nov 2013 08:21:33 -0000 1.113 > +++ net/if_spppsubr.c 17 Dec 2013 14:56:06 -0000 > @@ -69,6 +69,10 @@ > #include <netinet/if_ether.h> > #endif > > +#ifdef INET6 > +#include <netinet6/in6_ifattach.h> > +#endif > + > #include <net/if_sppp.h> > > # define UNTIMEOUT(fun, arg, handle) \ > @@ -3222,7 +3226,10 @@ sppp_ipv6cp_RCR(struct sppp *sp, struct > addlog("\n"); > > /* pass 2: parse option values */ > - sppp_get_ip6_addrs(sp, &myaddr, NULL, NULL); > + if (sp->ipv6cp.flags & IPV6CP_MYIFID_DYN) > + myaddr = sp->ipv6cp.req_ifid.ifra_addr.sin6_addr; > + else > + sppp_get_ip6_addrs(sp, &myaddr, NULL, NULL); > if (debug) > log(LOG_DEBUG, "%s: ipv6cp parse opt values: ", > SPP_ARGS(ifp)); > @@ -3455,7 +3462,10 @@ sppp_ipv6cp_scr(struct sppp *sp) > int i = 0; > > if (sp->ipv6cp.opts & (1 << IPV6CP_OPT_IFID)) { > - sppp_get_ip6_addrs(sp, &ouraddr, NULL, NULL); > + if (sp->ipv6cp.flags & IPV6CP_MYIFID_DYN) > + ouraddr = sp->ipv6cp.req_ifid.ifra_addr.sin6_addr; > + else > + sppp_get_ip6_addrs(sp, &ouraddr, NULL, NULL); > opt[i++] = IPV6CP_OPT_IFID; > opt[i++] = 10; > bcopy(&ouraddr.s6_addr[8], &opt[i], 8); > @@ -4768,6 +4778,25 @@ sppp_update_ip6_addr(void *arg1, void *a > return; > } > > + /* > + * Changing the link-local address requires purging all > + * existing addresses and routes for the interface first. > + */ > + if (sp->ipv6cp.flags & IPV6CP_MYIFID_DYN) { > + in6_ifdetach(ifp); > + error = in6_ifattach_linklocal(ifp, &ifra->ifra_addr.sin6_addr); > + if (error) > + log(LOG_ERR, SPP_FMT > + "could not update IPv6 address (error %d)\n", > + SPP_ARGS(ifp), error); > + splx(s); > + return; > + } > + > + /* > + * Code below changes address parameters only, not the address itself. > + */ > + > /* Destination address can only be set for /128. */ > if (!in6_are_prefix_equal(&ia->ia_prefixmask.sin6_addr, &mask, 128)) { > ifra->ifra_dstaddr.sin6_len = 0; > @@ -4843,6 +4872,7 @@ sppp_suggest_ip6_addr(struct sppp *sp, s > myaddr.s6_addr[14] ^= (random & 0xff); > myaddr.s6_addr[15] ^= ((random & 0xff00) >> 8); > } > + myaddr.s6_addr16[1] = 0; /* KAME hack: clear ifindex */ > bcopy(&myaddr, suggest, sizeof(myaddr)); > } > #endif /*INET6*/ > Index: netinet6/in6.c > =================================================================== > RCS file: /cvs/src/sys/netinet6/in6.c,v > retrieving revision 1.126 > diff -u -p -r1.126 in6.c > --- netinet6/in6.c 28 Nov 2013 10:16:44 -0000 1.126 > +++ netinet6/in6.c 17 Dec 2013 14:24:05 -0000 > @@ -2220,7 +2220,7 @@ in6_if_up(struct ifnet *ifp) > /* > * special cases, like 6to4, are handled in in6_ifattach > */ > - in6_ifattach(ifp, NULL); > + in6_ifattach(ifp); > > dad_delay = 0; > TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) { > Index: netinet6/in6_ifattach.c > =================================================================== > RCS file: /cvs/src/sys/netinet6/in6_ifattach.c,v > retrieving revision 1.64 > diff -u -p -r1.64 in6_ifattach.c > --- netinet6/in6_ifattach.c 28 Nov 2013 10:16:44 -0000 1.64 > +++ netinet6/in6_ifattach.c 17 Dec 2013 14:27:28 -0000 > @@ -67,7 +67,7 @@ int ip6_auto_linklocal = 1; /* enable by > > int get_last_resort_ifid(struct ifnet *, struct in6_addr *); > int get_hw_ifid(struct ifnet *, struct in6_addr *); > -int get_ifid(struct ifnet *, struct ifnet *, struct in6_addr *); > +int get_ifid(struct ifnet *, struct in6_addr *); > int in6_ifattach_loopback(struct ifnet *); > > #define EUI64_GBIT 0x01 > @@ -257,11 +257,9 @@ found: > * Get interface identifier for the specified interface. If it is not > * available on ifp0, borrow interface identifier from other information > * sources. > - * > - * altifp - secondary EUI64 source > */ > int > -get_ifid(struct ifnet *ifp0, struct ifnet *altifp, struct in6_addr *in6) > +get_ifid(struct ifnet *ifp0, struct in6_addr *in6) > { > struct ifnet *ifp; > > @@ -272,13 +270,6 @@ get_ifid(struct ifnet *ifp0, struct ifne > goto success; > } > > - /* try secondary EUI64 source. this basically is for ATM PVC */ > - if (altifp && get_hw_ifid(altifp, in6) == 0) { > - nd6log((LOG_DEBUG, "%s: got interface identifier from %s\n", > - ifp0->if_xname, altifp->if_xname)); > - goto success; > - } > - > /* next, try to get it from some other hardware interface */ > TAILQ_FOREACH(ifp, &ifnet, if_list) { > if (ifp == ifp0) > @@ -318,11 +309,11 @@ success: > } > > /* > - * altifp - secondary EUI64 source > + * ifid - used as EUI64 if not NULL, overrides other EUI64 sources > */ > > int > -in6_ifattach_linklocal(struct ifnet *ifp, struct ifnet *altifp) > +in6_ifattach_linklocal(struct ifnet *ifp, struct in6_addr *ifid) > { > struct in6_ifaddr *ia; > struct in6_aliasreq ifra; > @@ -348,8 +339,15 @@ in6_ifattach_linklocal(struct ifnet *ifp > if ((ifp->if_flags & IFF_LOOPBACK) != 0) { > ifra.ifra_addr.sin6_addr.s6_addr32[2] = 0; > ifra.ifra_addr.sin6_addr.s6_addr32[3] = htonl(1); > + } else if (ifid) { > + ifra.ifra_addr.sin6_addr = *ifid; > + ifra.ifra_addr.sin6_addr.s6_addr16[0] = htons(0xfe80); > + ifra.ifra_addr.sin6_addr.s6_addr16[1] = htons(ifp->if_index); > + ifra.ifra_addr.sin6_addr.s6_addr32[1] = 0; > + ifra.ifra_addr.sin6_addr.s6_addr[8] &= ~EUI64_GBIT; > + ifra.ifra_addr.sin6_addr.s6_addr[8] |= EUI64_UBIT; > } else { > - if (get_ifid(ifp, altifp, &ifra.ifra_addr.sin6_addr) != 0) { > + if (get_ifid(ifp, &ifra.ifra_addr.sin6_addr) != 0) { > nd6log((LOG_ERR, > "%s: no ifid available\n", ifp->if_xname)); > return (-1); > @@ -565,11 +563,9 @@ in6_nigroup(struct ifnet *ifp, const cha > * XXX multiple loopback interface needs more care. for instance, > * nodelocal address needs to be configured onto only one of them. > * XXX multiple link-local address case > - * > - * altifp - secondary EUI64 source > */ > void > -in6_ifattach(struct ifnet *ifp, struct ifnet *altifp) > +in6_ifattach(struct ifnet *ifp) > { > struct in6_ifaddr *ia; > struct in6_addr in6; > @@ -634,7 +630,7 @@ in6_ifattach(struct ifnet *ifp, struct i > if (ip6_auto_linklocal) { > ia = in6ifa_ifpforlinklocal(ifp, 0); > if (ia == NULL) { > - if (in6_ifattach_linklocal(ifp, altifp) == 0) { > + if (in6_ifattach_linklocal(ifp, NULL) == 0) { > /* linklocal address assigned */ > } else { > /* failed to assign linklocal address. bark? */ > @@ -687,6 +683,26 @@ in6_ifdetach(struct ifnet *ifp) > * (Or can we just delay calling nd6_purge until at this point?) > */ > nd6_purge(ifp); > + > + /* remove route to interface local allnodes multicast (ff01::1) */ > + bzero(&sin6, sizeof(sin6)); > + sin6.sin6_len = sizeof(struct sockaddr_in6); > + sin6.sin6_family = AF_INET6; > + sin6.sin6_addr = in6addr_intfacelocal_allnodes; > + sin6.sin6_addr.s6_addr16[1] = htons(ifp->if_index); > + rt = rtalloc1(sin6tosa(&sin6), 0, ifp->if_rdomain); > + if (rt && rt->rt_ifp == ifp) { > + struct rt_addrinfo info; > + > + bzero(&info, sizeof(info)); > + info.rti_flags = rt->rt_flags; > + info.rti_info[RTAX_DST] = rt_key(rt); > + info.rti_info[RTAX_GATEWAY] = rt->rt_gateway; > + info.rti_info[RTAX_NETMASK] = rt_mask(rt); > + rtrequest1(RTM_DELETE, &info, rt->rt_priority, NULL, > + ifp->if_rdomain); > + rtfree(rt); > + } > > /* remove route to link-local allnodes multicast (ff02::1) */ > bzero(&sin6, sizeof(sin6)); > Index: netinet6/in6_ifattach.h > =================================================================== > RCS file: /cvs/src/sys/netinet6/in6_ifattach.h,v > retrieving revision 1.5 > diff -u -p -r1.5 in6_ifattach.h > --- netinet6/in6_ifattach.h 31 Aug 2006 12:37:31 -0000 1.5 > +++ netinet6/in6_ifattach.h 9 Dec 2013 19:33:15 -0000 > @@ -34,10 +34,10 @@ > #define _NETINET6_IN6_IFATTACH_H_ > > #ifdef _KERNEL > -void in6_ifattach(struct ifnet *, struct ifnet *); > +void in6_ifattach(struct ifnet *); > void in6_ifdetach(struct ifnet *); > int in6_nigroup(struct ifnet *, const char *, int, struct sockaddr_in6 *); > -int in6_ifattach_linklocal(struct ifnet *, struct ifnet *); > +int in6_ifattach_linklocal(struct ifnet *, struct in6_addr *); > #endif /* _KERNEL */ > > #endif /* _NETINET6_IN6_IFATTACH_H_ */
