From: Florian Obser Subject: Re: RFC 6724 rule 5.5 To: Alexander Bluhm Cc: tech Date: Tue, 16 Apr 2024 01:21:22 +0200 On 2024-04-15 22:26 +02, Alexander Bluhm wrote: > On Mon, Apr 15, 2024 at 08:15:24PM +0200, Florian Obser wrote: >> Rule 5.5: Prefer addresses in a prefix advertised by the next-hop. >> >> For this we have to track the (link-local) address of the advertising >> router per interface address and compare it with the selected route. >> >> We are (ab)using the ifra_dstaddr for this because it will always be >> unused with autoconf addresses since they can't be used on P2P links. >> >> Rule 5.5 is beneficial if multiple address from multiple routers are >> configured with the same interface priority. For example in multi-homing >> setups. >> >> Comments, tests, OKs? > > Some remarks > >> @@ -561,6 +561,16 @@ in6_update_ifa(struct ifnet *ifp, struct in6_aliasreq *ifra, >> if (ifp == NULL || ifra == NULL) /* this maybe redundant */ >> return (EINVAL); >> >> + /* Extract advertising router for SLAAC addresses */ >> + if ((ifra->ifra_flags & IN6_IFF_AUTOCONF) && >> + ifra->ifra_dstaddr.sin6_family == AF_INET6) { >> + gw6 = ifra->ifra_dstaddr; >> + memset(&ifra->ifra_dstaddr, 0, sizeof(ifra->ifra_dstaddr)); >> + error = in6_check_embed_scope(&gw6, ifp->if_index); >> + if (error) >> + return error; >> + } >> + > > I don't like resetting ifra_dstaddr. This is done to select other > paths in the following if conditions. Can you put an (ifra->ifra_flags > & IN6_IFF_AUTOCONF) and some else conditions in the code that deals > with dst6 anyway? I think explicit conditions are more readable > than modifying variables. Sure, the diff at the end is more invasive. I hope I don't mess up p2p links... > >> /* >> * The destination address for a p2p link must have a family >> * of AF_UNSPEC or AF_INET6. >> @@ -706,6 +716,13 @@ in6_update_ifa(struct ifnet *ifp, struct in6_aliasreq *ifra, >> ia6->ia_dstaddr = dst6; >> } >> >> + /* Set or update router from which we learned the address */ >> + if ((ifra->ifra_flags & IN6_IFF_AUTOCONF) && >> + gw6.sin6_family == AF_INET6 && >> + !IN6_ARE_ADDR_EQUAL(&gw6.sin6_addr, &ia6->ia_gwaddr.sin6_addr)) { >> + ia6->ia_gwaddr = gw6; >> + } >> + > > Then this could be some else if ((ifra->ifra_flags & IN6_IFF_AUTOCONF). > And dst6 could be reused instead of new gw6. > > I have not tried to write it that way. Maybe it does not work > and your solution is nicer. > >> @@ -1460,9 +1485,20 @@ in6_ifawithscope(struct ifnet *oifp, struct in6_addr *dst, u_int rdomain) >> /* >> * Rule 5.5: Prefer addresses in a prefix advertised >> * by the next-hop. >> - * We do not track this information. >> */ >> >> + if (gw6) { >> + struct in6_addr *in6_bestgw, *in6_newgw; >> + >> + in6_bestgw = &ia6_best->ia_gwaddr.sin6_addr; >> + in6_newgw = &ifatoia6(ifa)->ia_gwaddr.sin6_addr; >> + if (!IN6_ARE_ADDR_EQUAL(in6_bestgw, gw6) && >> + IN6_ARE_ADDR_EQUAL(in6_newgw, gw6)) { >> + printf("replace\n"); > > This looks like forgotten debug code. nice catch, fixed > >> + goto replace; >> + } >> + } >> + >> /* >> * Rule 6: Prefer matching label. >> * We do not implement policy tables. > > Rest of the diff is OK. > Bonus fix for a missing loopback, although I don't understand where you use a dst_addr for loopback... Before my rewrite: 688 * If a new destination address is specified, scrub the old one and 689 * install the new destination. Note that the interface must be 690 * p2p or loopback (see the check above.) 691 */ 692 if ((ifp->if_flags & IFF_POINTOPOINT) && dst6.sin6_family == AF_INET6 && ^^^^^^^^^^^^^^^ I tried to not fully rewrite in6_update_ifa() but things got complicated, this is what I have. I've dropped a few comments that were stating the obvious before and are now completely wrong. Thoughts? diff --git sbin/slaacd/engine.c sbin/slaacd/engine.c index 000a9fcf413..72798552824 100644 --- sbin/slaacd/engine.c +++ sbin/slaacd/engine.c @@ -2130,6 +2130,7 @@ configure_address(struct address_proposal *addr_proposal) address.if_index = addr_proposal->if_index; memcpy(&address.addr, &addr_proposal->addr, sizeof(address.addr)); + memcpy(&address.gw, &addr_proposal->from, sizeof(address.gw)); memcpy(&address.mask, &addr_proposal->mask, sizeof(address.mask)); address.vltime = addr_proposal->vltime; address.pltime = addr_proposal->pltime; diff --git sbin/slaacd/engine.h sbin/slaacd/engine.h index 7a8551d2c50..079ef9a64e0 100644 --- sbin/slaacd/engine.h +++ sbin/slaacd/engine.h @@ -19,6 +19,7 @@ struct imsg_configure_address { uint32_t if_index; struct sockaddr_in6 addr; + struct sockaddr_in6 gw; struct in6_addr mask; uint32_t vltime; uint32_t pltime; diff --git sbin/slaacd/slaacd.c sbin/slaacd/slaacd.c index 4d1786361f7..d7fcec2601b 100644 --- sbin/slaacd/slaacd.c +++ sbin/slaacd/slaacd.c @@ -632,6 +632,8 @@ configure_interface(struct imsg_configure_address *address) memcpy(&in6_addreq.ifra_addr, &address->addr, sizeof(in6_addreq.ifra_addr)); + memcpy(&in6_addreq.ifra_dstaddr, &address->gw, + sizeof(in6_addreq.ifra_dstaddr)); memcpy(&in6_addreq.ifra_prefixmask.sin6_addr, &address->mask, sizeof(in6_addreq.ifra_prefixmask.sin6_addr)); in6_addreq.ifra_prefixmask.sin6_family = AF_INET6; diff --git sys/netinet6/icmp6.c sys/netinet6/icmp6.c index 2ec50eb06a2..e3c302969f1 100644 --- sys/netinet6/icmp6.c +++ sys/netinet6/icmp6.c @@ -1164,7 +1164,7 @@ icmp6_reflect(struct mbuf **mp, size_t off, struct sockaddr *sa) rtfree(rt); goto bad; } - ia6 = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t, rtableid); + ia6 = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t, rtableid, rt); if (ia6 != NULL) src = &ia6->ia_addr.sin6_addr; if (src == NULL) diff --git sys/netinet6/in6.c sys/netinet6/in6.c index 1266354e202..97ade868341 100644 --- sys/netinet6/in6.c +++ sys/netinet6/in6.c @@ -597,27 +597,19 @@ in6_update_ifa(struct ifnet *ifp, struct in6_aliasreq *ifra, */ plen = in6_mask2len(&ia6->ia_prefixmask.sin6_addr, NULL); } - /* - * If the destination address on a p2p interface is specified, - * and the address is a scoped one, validate/set the scope - * zone identifier. - */ + dst6 = ifra->ifra_dstaddr; - if ((ifp->if_flags & (IFF_POINTOPOINT|IFF_LOOPBACK)) != 0 && - (dst6.sin6_family == AF_INET6)) { + if (dst6.sin6_family == AF_INET6) { + if ((ifp->if_flags & (IFF_POINTOPOINT|IFF_LOOPBACK)) == 0 && + (ifra->ifra_flags & IN6_IFF_AUTOCONF) == 0) + return (EINVAL); + error = in6_check_embed_scope(&dst6, ifp->if_index); if (error) return error; - } - /* - * The destination address can be specified only for a p2p or a - * loopback interface. If specified, the corresponding prefix length - * must be 128. - */ - if (ifra->ifra_dstaddr.sin6_family == AF_INET6) { - if ((ifp->if_flags & (IFF_POINTOPOINT|IFF_LOOPBACK)) == 0) - return (EINVAL); - if (plen != 128) + + if ((ifp->if_flags & (IFF_POINTOPOINT|IFF_LOOPBACK)) != 0 + && plen != 128) return (EINVAL); } /* lifetime consistency check */ @@ -661,6 +653,11 @@ in6_update_ifa(struct ifnet *ifp, struct in6_aliasreq *ifra, } else { ia6->ia_ifa.ifa_dstaddr = NULL; } + if ((ifra->ifra_flags & IN6_IFF_AUTOCONF) == 0) + ia6->ia_gwaddr = dst6; + else + memset(&ia6->ia_gwaddr, 0, sizeof(ia6->ia_gwaddr)); + ia6->ia_ifa.ifa_netmask = sin6tosa(&ia6->ia_prefixmask); ia6->ia_ifp = ifp; @@ -684,26 +681,36 @@ in6_update_ifa(struct ifnet *ifp, struct in6_aliasreq *ifra, ia6->ia_prefixmask = ifra->ifra_prefixmask; } - /* - * If a new destination address is specified, scrub the old one and - * install the new destination. Note that the interface must be - * p2p or loopback (see the check above.) - */ - if ((ifp->if_flags & IFF_POINTOPOINT) && dst6.sin6_family == AF_INET6 && - !IN6_ARE_ADDR_EQUAL(&dst6.sin6_addr, &ia6->ia_dstaddr.sin6_addr)) { - struct ifaddr *ifa = &ia6->ia_ifa; - - if ((ia6->ia_flags & IFA_ROUTE) != 0 && - rt_ifa_del(ifa, RTF_HOST, ifa->ifa_dstaddr, - ifp->if_rdomain) != 0) { - nd6log((LOG_ERR, "%s: failed to remove a route " - "to the old destination: %s\n", __func__, - inet_ntop(AF_INET6, &ia6->ia_addr.sin6_addr, - addr, sizeof(addr)))); - /* proceed anyway... */ - } else - ia6->ia_flags &= ~IFA_ROUTE; - ia6->ia_dstaddr = dst6; + if (dst6.sin6_family == AF_INET6) { + if ((ifra->ifra_flags & IN6_IFF_AUTOCONF) && + !IN6_ARE_ADDR_EQUAL(&dst6.sin6_addr, + &ia6->ia_gwaddr.sin6_addr)) { + /* + * Set or update router from which we learned the + * address + */ + ia6->ia_gwaddr = dst6; + } else if ((ifp->if_flags & (IFF_POINTOPOINT | IFF_LOOPBACK)) + != 0 && !IN6_ARE_ADDR_EQUAL(&dst6.sin6_addr, + &ia6->ia_dstaddr.sin6_addr)) { + /* + * If a new destination address is specified, scrub the + * old one and install the new destination. + */ + struct ifaddr *ifa = &ia6->ia_ifa; + + if ((ia6->ia_flags & IFA_ROUTE) != 0 && + rt_ifa_del(ifa, RTF_HOST, ifa->ifa_dstaddr, + ifp->if_rdomain) != 0) { + nd6log((LOG_ERR, "%s: failed to remove a route " + "to the old destination: %s\n", __func__, + inet_ntop(AF_INET6, &ia6->ia_addr.sin6_addr, + addr, sizeof(addr)))); + /* proceed anyway... */ + } else + ia6->ia_flags &= ~IFA_ROUTE; + ia6->ia_dstaddr = dst6; + } } /* @@ -1329,13 +1336,21 @@ in6_prefixlen2mask(struct in6_addr *maskp, int len) * return the best address out of the same scope */ struct in6_ifaddr * -in6_ifawithscope(struct ifnet *oifp, struct in6_addr *dst, u_int rdomain) +in6_ifawithscope(struct ifnet *oifp, struct in6_addr *dst, u_int rdomain, + struct rtentry *rt) { int dst_scope = in6_addrscope(dst), src_scope, best_scope = 0; int blen = -1; struct ifaddr *ifa; struct ifnet *ifp; struct in6_ifaddr *ia6_best = NULL; + struct in6_addr *gw6 = NULL; + + if (rt) { + if (rt->rt_gateway != NULL && + rt->rt_gateway->sa_family == AF_INET6) + gw6 = &(satosin6(rt->rt_gateway)->sin6_addr); + } if (oifp == NULL) { printf("%s: output interface is not specified\n", __func__); @@ -1460,9 +1475,18 @@ in6_ifawithscope(struct ifnet *oifp, struct in6_addr *dst, u_int rdomain) /* * Rule 5.5: Prefer addresses in a prefix advertised * by the next-hop. - * We do not track this information. */ + if (gw6) { + struct in6_addr *in6_bestgw, *in6_newgw; + + in6_bestgw = &ia6_best->ia_gwaddr.sin6_addr; + in6_newgw = &ifatoia6(ifa)->ia_gwaddr.sin6_addr; + if (!IN6_ARE_ADDR_EQUAL(in6_bestgw, gw6) && + IN6_ARE_ADDR_EQUAL(in6_newgw, gw6)) + goto replace; + } + /* * Rule 6: Prefer matching label. * We do not implement policy tables. diff --git sys/netinet6/in6.h sys/netinet6/in6.h index 642a24b1999..db5896beb79 100644 --- sys/netinet6/in6.h +++ sys/netinet6/in6.h @@ -404,6 +404,7 @@ struct sockaddr_in6; struct ifaddr; struct in6_ifaddr; struct ifnet; +struct rtentry; void ipv6_input(struct ifnet *, struct mbuf *); struct mbuf * @@ -413,7 +414,8 @@ int in6_cksum(struct mbuf *, uint8_t, uint32_t, uint32_t); void in6_proto_cksum_out(struct mbuf *, struct ifnet *); int in6_localaddr(struct in6_addr *); int in6_addrscope(struct in6_addr *); -struct in6_ifaddr *in6_ifawithscope(struct ifnet *, struct in6_addr *, u_int); +struct in6_ifaddr *in6_ifawithscope(struct ifnet *, struct in6_addr *, u_int, + struct rtentry *); int in6_mask2len(struct in6_addr *, u_char *); int in6_nam2sin6(const struct mbuf *, struct sockaddr_in6 **); int in6_sa2sin6(struct sockaddr *, struct sockaddr_in6 **); diff --git sys/netinet6/in6_src.c sys/netinet6/in6_src.c index d6163d254c0..db123c22a2b 100644 --- sys/netinet6/in6_src.c +++ sys/netinet6/in6_src.c @@ -162,7 +162,7 @@ in6_pcbselsrc(const struct in6_addr **in6src, struct sockaddr_in6 *dstsock, if (ifp == NULL) return (ENXIO); /* XXX: better error? */ - ia6 = in6_ifawithscope(ifp, dst, rtableid); + ia6 = in6_ifawithscope(ifp, dst, rtableid, NULL); if_put(ifp); if (ia6 == NULL) @@ -192,7 +192,7 @@ in6_pcbselsrc(const struct in6_addr **in6src, struct sockaddr_in6 *dstsock, if (rt != NULL) { ifp = if_get(rt->rt_ifidx); if (ifp != NULL) { - ia6 = in6_ifawithscope(ifp, dst, rtableid); + ia6 = in6_ifawithscope(ifp, dst, rtableid, rt); if_put(ifp); } if (ia6 == NULL) /* xxx scope error ?*/ @@ -256,7 +256,7 @@ in6_selectsrc(const struct in6_addr **in6src, struct sockaddr_in6 *dstsock, if (ifp == NULL) return (ENXIO); /* XXX: better error? */ - ia6 = in6_ifawithscope(ifp, dst, rtableid); + ia6 = in6_ifawithscope(ifp, dst, rtableid, NULL); if_put(ifp); if (ia6 == NULL) @@ -280,7 +280,7 @@ in6_selectsrc(const struct in6_addr **in6src, struct sockaddr_in6 *dstsock, ifp = if_get(htons(dstsock->sin6_scope_id)); if (ifp) { - ia6 = in6_ifawithscope(ifp, dst, rtableid); + ia6 = in6_ifawithscope(ifp, dst, rtableid, NULL); if_put(ifp); if (ia6 == NULL) diff --git sys/netinet6/in6_var.h sys/netinet6/in6_var.h index 1323eee209a..a97a1d463f7 100644 --- sys/netinet6/in6_var.h +++ sys/netinet6/in6_var.h @@ -93,6 +93,7 @@ struct in6_ifaddr { #define ia_flags ia_ifa.ifa_flags struct sockaddr_in6 ia_addr; /* interface address */ + struct sockaddr_in6 ia_gwaddr; /* router we learned address from */ struct sockaddr_in6 ia_dstaddr; /* space for destination addr */ struct sockaddr_in6 ia_prefixmask; /* prefix mask */ TAILQ_ENTRY(in6_ifaddr) ia_list; /* list of IP6 addresses */ -- In my defence, I have been left unsupervised.