[PATCH 1/2] Add IP6_SADR support to Bird Core

[Ondrej Zajicek]

On Sat, Feb 03, 2018 at 09:40:56PM +0100, Toke Høiland-Jørgensen wrote:
> This adds support for source-specific IPv6 routes to Bird core. This is
> based on Dean Luga's original patch, with the review comments addressed.
> Sadr support is added to network address parsing in confbase.Y and to
> the kernel protocol on Linux.

First, some general comments:

Put sadr_ip6 variant consistently above mpls variant. There are several
places where it is in reverse.

If you do not object, i would much prefer *_ip6_sadr than *_sadr_ip6
(also in uppercase variants). It will also be consistent with user
visible channel and table keywords for this type.


> -%type <net> net_ip4_ net_ip6_ net_ip6 net_ip_ net_ip net_or_ipa
> +%type <net> net_ip4_ net_ip6_ net_sadr_ip6_ net_ip6 net_ip_ net_ip net_or_ipa
>  %type <net_ptr> net_ net_any net_vpn4_ net_vpn6_ net_vpn_ net_roa4_ net_roa6_ net_roa_ net_mpls_

net_sadr_ip6_ should be in  <net_ptr> instead of <net>, as net_addr_sadr_ip6
is longer than net_addr, so must be passed as pointer. That fixes the
"Integer expression expected" issue.


>  %type <mls> label_stack_start label_stack
>  
> @@ -96,7 +96,7 @@ CF_DECLS
>  %left '!'
>  %nonassoc '.'
>  
> -CF_KEYWORDS(DEFINE, ON, OFF, YES, NO, S, MS, US, PORT, VPN, MPLS)
> +CF_KEYWORDS(DEFINE, ON, OFF, YES, NO, S, MS, US, PORT, VPN, MPLS, FROM)
>  
>  CF_GRAMMAR
>  
> @@ -206,6 +206,22 @@ net_ip6_: IP6 '/' NUM
>  	     n->prefix, n->pxlen, ip6_and(n->prefix, ip6_mkmask(n->pxlen)), n->pxlen);
>  };
>  
> +net_sadr_ip6_: IP6 '/' NUM FROM IP6 '/' NUM
> +{
> +  if ($3 > IP6_MAX_PREFIX_LENGTH)
> +    cf_error("Invalid prefix length %u", $3);
> +
> +  if ($7 > IP6_MAX_PREFIX_LENGTH)
> +    cf_error("Invalid prefix length %u", $7);
> +
> +  net_fill_sadr_ip6(&($$), $1, $3, $5, $7);
> +
> +  net_addr_sadr_ip6 *n = (void *) &($$);

W.r.t. the change from <net> to <net_ptr> it should be:

  $$ = cfg_alloc(sizeof(net_addr_sadr_ip6));
  net_fill_sadr_ip6($$, $1, $3, $5, $7);

  net_addr_sadr_ip6 *n = (void *) $$;


> -net_ip_: net_ip4_ | net_ip6_ ;
> +net_ip_: net_ip4_ | net_ip6_ | net_sadr_ip6_;

Here net_sadr_ip6_ should be separate

>  net_vpn_: net_vpn4_ | net_vpn6_ ;
>  net_roa_: net_roa4_ | net_roa6_ ;
>  
> diff --git a/lib/net.c b/lib/net.c
> index 9335b78f..a2dad51d 100644
> --- a/lib/net.c
> +++ b/lib/net.c
> @@ -14,6 +14,7 @@ const char * const net_label[] = {
>    [NET_ROA6] 	= "roa6",
>    [NET_FLOW4] 	= "flow4",
>    [NET_FLOW6] 	= "flow6",
> +  [NET_SADR_IP6] = "sadr6",

 "ipv4 sadr" to be consistent with table keywords?

> +typedef struct net_addr_sadr_ip6 {
> +  u8 type;
> +  u8 dst_pxlen;
> +  u16 length;
> +  ip6_addr dst_prefix;
> +  u32 src_pxlen; // if u8, NET_ADDR_SADR_IP6 would leave non-zero padding

Perhaps we should use s32 so it is promoted to same type as u8?

> +int
> +net_compare_sadr_ip6(const net_addr_sadr_ip6 *a, const net_addr_sadr_ip6 *b)
> +{
> +  int dst_cmp = ip6_compare(a->dst_prefix, b->dst_prefix) ?: uint_cmp(a->dst_pxlen, b->dst_pxlen);
> +  if (dst_cmp)
> +    return dst_cmp;
> +  else
> +    return ip6_compare(a->src_prefix, b->src_prefix) ?: uint_cmp(a->src_pxlen, b->src_pxlen);
> +}

Seems too complicated. Perhaps just:

{
  return
     ip6_compare(a->dst_prefix, b->dst_prefix) ?: uint_cmp(a->dst_pxlen, b->dst_pxlen) ?:
     ip6_compare(a->src_prefix, b->src_prefix) ?: uint_cmp(a->src_pxlen, b->src_pxlen);
}


> +static inline void *
> +net_route_sadr_ip6(rtable *t, net_addr_sadr_ip6 *n)

IMHO proper way how to implement this is like net_roa_check_ip6() is
done. As nets are hashed by dst, you can use fib_get_chain() to get hash
chain, walk through it and find the best matching src from nodes with the
exact dst of the current iteration, and decrease dst_pxlen in further
iterations if no match.

Also note that in order to net_route is used in 'show route for', you
should add net_sadr_ip6_ to r_args_for grammar in nest/config.Y


> --- a/sysdep/unix/krt.c
> +++ b/sysdep/unix/krt.c
> @@ -1103,7 +1103,9 @@ krt_start(struct proto *P)
>    switch (p->p.net_type)
>    {
>    case NET_IP4:	p->af = AF_INET; break;
> -  case NET_IP6:	p->af = AF_INET6; break;
> +  case NET_IP6:
> +  case NET_SADR_IP6:
> +    p->af = AF_INET6; break;
>  #ifdef AF_MPLS
>    case NET_MPLS: p->af = AF_MPLS; break;
>  #endif
> @@ -1219,9 +1221,9 @@ struct protocol proto_unix_kernel = {
>    .attr_class =		EAP_KRT,
>    .preference =		DEF_PREF_INHERITED,
>  #ifdef HAVE_MPLS_KERNEL
> -  .channel_mask =	NB_IP | NB_MPLS,
> +  .channel_mask =	NB_IP | NB_SADR_IP6 | NB_MPLS,
>  #else
> -  .channel_mask =	NB_IP,
> +  .channel_mask =	NB_IP | NB_SADR_IP6,
>  #endif

SADR is not supported in BSD, channel_mask should reflect mask. There
should be define for CONFIG_SADR_IP6_KERNEL in sysdef/cf/linux.h, and
then in sysdep/unix/krt.c:


#ifdef CONFIG_SADR_IP6_KERNEL
#define MAYBE_SADR_IP6	NB_SADR_IP
#else
#define MAYBE_SADR_IP6	0
#endif

#ifdef HAVE_MPLS_KERNEL
#define MAYBE_MPLS	NB_MPLS
#else
#define MAYBE_MPLS	0
#endif

struct protocol proto_unix_kernel = {
  ...
  .channel_mask =	NB_IP | MAYBE_SADR_IP6 | MAYBE_MPLS
  ...
};


-- 
Elen sila lumenn' omentielvo

Ondrej 'Santiago' Zajicek (email: santiago at crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <http://trubka.network.cz/pipermail/bird-users/attachments/20180205/b48ac7a8/attachment.sig>