From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: initial steps for RFC 8950 support
To: tech@openbsd.org
Date: Thu, 9 Jan 2025 10:17:27 +0100

On Wed, Jan 08, 2025 at 04:20:45PM +0100, Claudio Jeker wrote:
> This are the first bits to support IPv6 nexthop for IPv4 routes which is
> defined by RFC 8950.
> 
> The big change of RFC 8950 is that when enabled some updates will use
> MP_REACH_ATTR even for AID_INET. So this diff kind of implements those
> bits but all of this is currently unreachable since peer_has_ext_nexthop()
> never returns true.
> 
> This diff tries to error hard when a peer uses MP encoding for AID_INET
> unless peer_has_ext_nexthop() is true. Also we never accept a
> MP_UNREACH_ATTR for AID_INET.
> 
> It seems that the standards allow for 48byte nexthops in AID_VPN_IPv6
> so adjust that there as well. We always ignore the link-local address so
> just accept the extra lenght.

Makes sense.

> 
> I rewrote the nexthop handling of up_generate_mp_reach() since fitting
> different nexthops into the old code resulted in a lot of ugly code.

This reads fine to me.  Two nits below, feel free to ignore.

ok tb

> Index: rde_peer.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_peer.c,v
> diff -u -p -r1.43 rde_peer.c
> --- rde_peer.c	7 Jan 2025 17:43:31 -0000	1.43
> +++ rde_peer.c	8 Jan 2025 13:48:52 -0000
> @@ -58,6 +58,20 @@ peer_has_add_path(struct rde_peer *peer,
>  }
>  
>  int
> +peer_has_ext_msg(struct rde_peer *peer)
> +{
> +	return (peer->capa.ext_msg);
> +}
> +
> +int
> +peer_has_ext_nexthop(struct rde_peer *peer, uint8_t aid)
> +{
> +	if (aid >= AID_MAX)
> +		return 0;

Either add parens here or remove those in the other two returns you add
in this hunk

> +	return (peer->capa.ext_nexthop[aid]);
> +}
> +
> +int
>  peer_accept_no_as_set(struct rde_peer *peer)
>  {
>  	return (peer->flags & PEERFLAG_NO_AS_SET);
> Index: rde_update.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
> diff -u -p -r1.172 rde_update.c
> --- rde_update.c	7 Jan 2025 12:11:45 -0000	1.172
> +++ rde_update.c	8 Jan 2025 14:55:10 -0000
> @@ -877,7 +877,7 @@ up_generate_mp_reach(struct ibuf *buf, s
>      struct nexthop *nh, uint8_t aid)
>  {
>  	struct bgpd_addr *nexthop;
> -	size_t off;
> +	size_t off, nhoff;
>  	uint16_t len, afi;
>  	uint8_t safi;
>  
> @@ -898,59 +898,61 @@ up_generate_mp_reach(struct ibuf *buf, s
>  		return -1;
>  	if (ibuf_add_n8(buf, safi) == -1)
>  		return -1;
> +	nhoff = ibuf_size(buf);
> +	if (ibuf_add_zero(buf, 1) == -1)
> +		return -1;
>  
>  	switch (aid) {

Unless there's code that will soon be added here that justifies the
switch, I think this should become an if:

	if (aid == AID_VPN_IPv4 || aid == AID_VPN_IPv6) {
		/* write zero rd */
		if (ibuf_add_zero(buf, sizeof(uint64_t)) == -1)
			return -1;
	}