From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: bgpd: initial steps for RFC 8950 support
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Thu, 9 Jan 2025 10:27:37 +0100

On Thu, Jan 09, 2025 at 10:17:27AM +0100, Theo Buehler wrote:
> 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

Will remove the extra (). At least for simple returns I really prefer
that.
 
> > +	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:
 
We need to add AID_EVPN to this at some point but that is just another or
condition. Apart from that I think nothing else will happen. So I switch
it to 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;
> 	}
> 

-- 
:wq Claudio