From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: RFC 6724 rule 5.5
To: tech <tech@openbsd.org>
Date: Mon, 15 Apr 2024 22:26:13 +0200

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.

>  	/*
>  	 * 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.

> +					goto replace;
> +				}
> +			}
> +
>  			/*
>  			 * Rule 6: Prefer matching label.
>  			 * We do not implement policy tables.

Rest of the diff is OK.