Download raw body.
RFC 6724 rule 5.5
On 2024-04-15 22:26 +02, Alexander Bluhm <bluhm@openbsd.org> 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.
RFC 6724 rule 5.5