[PATCH 1/4] Core changes to support SADR

Ondrej Zajicek santiago at crfreenet.org
Mon May 22 15:30:16 CEST 2017


On Sun, May 21, 2017 at 11:01:34PM +0200, Dean Luga wrote:
> From: dean <dluga93 at gmail.com>
> 
> This patch adds a new network of type NET_SADR_IP6, including new
> structures, constants, and switch cases for the new network type.
> Some existing functions are duplicates to handle the new network
> type, and netlink can now handle SADR routes.
> 
> The net_route_sadr_ip6 function is a bit of a hack.
> Routing in SADR is supposed to match the destination address first,
> then out of the entries with that dst, it should choose the most
> specific matching src address.

Well, net_route() in integrated branch is a bit of a hack generally.
It was carried over from plain-IP branch, it needs some redesign to
better handle net types different than NET_IP.


> For the destination-only part, I use a net_addr_ip6 as the
> destination prefix. Its type is changed to NET_SADR_IP6 to satisfy
> the assertion in fib_find. The fib_find function checks the length
> of the prefix to distinguish between destination-only or full SADR
> searches.
> ---
>  lib/net.c              | 34 ++++++++++++++++++++++++++
>  lib/net.h              | 66 +++++++++++++++++++++++++++++++++++++++++++++++---
>  nest/rt-fib.c          | 16 ++++++++++++
>  nest/rt-table.c        | 35 +++++++++++++++++++++++++-
>  sysdep/linux/netlink.c | 26 ++++++++++++++++++--
>  sysdep/unix/krt.c      |  4 ++-
>  6 files changed, 174 insertions(+), 7 deletions(-)
> 
>  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);
> +}
> +
> +int
>  net_compare(const net_addr *a, const net_addr *b)
>  {
>    if (a->type != b->type)
> @@ -158,6 +177,8 @@ net_compare(const net_addr *a, const net_addr *b)
>      return net_compare_flow6((const net_addr_flow6 *) a, (const net_addr_flow6 *) b);
>    case NET_MPLS:
>      return net_compare_mpls((const net_addr_mpls *) a, (const net_addr_mpls *) b);
> +  case NET_SADR_IP6:
> +    return net_compare_sadr_ip6((const net_addr_sadr_ip6 *) a, (const net_addr_sadr_ip6 *) b);
>    }
>    return 0;
>  }
> @@ -178,6 +199,7 @@ net_hash(const net_addr *n)
>    case NET_FLOW4: return NET_HASH(n, flow4);
>    case NET_FLOW6: return NET_HASH(n, flow6);
>    case NET_MPLS: return NET_HASH(n, mpls);
> +  case NET_SADR_IP6: return NET_HASH(n, ip6);
>    default: bug("invalid type");
>    }
>  }

This should be NET_HASH(n, sadr_ip6) even if it is the same function.

> diff --git a/lib/net.h b/lib/net.h
> index 332f4c9..22eee60 100644
> --- a/lib/net.h
> +++ b/lib/net.h
> @@ -12,7 +12,6 @@
>  
>  #include "lib/ip.h"
>  
> -
>  #define NET_IP4		1
>  #define NET_IP6		2
>  #define NET_VPN4	3
> @@ -22,7 +21,8 @@
>  #define NET_FLOW4	7
>  #define NET_FLOW6	8
>  #define NET_MPLS	9
> -#define NET_MAX		10
> +#define NET_SADR_IP6	10
> +#define NET_MAX		11

I would prefer that NET_SADR_IP6 would be before NET_MPLS, as it is still
IP-family net type,

>  
>  #define NB_IP4		(1 << NET_IP4)
>  #define NB_IP6		(1 << NET_IP6)
> @@ -33,8 +33,9 @@
>  #define NB_FLOW4	(1 << NET_FLOW4)
>  #define NB_FLOW6	(1 << NET_FLOW6)
>  #define NB_MPLS		(1 << NET_MPLS)
> +#define NB_SADR		(1 << NET_SADR_IP6)
>  
> -#define NB_IP		(NB_IP4 | NB_IP6)
> +#define NB_IP		(NB_IP4 | NB_IP6 | NB_SADR)

No, NB_IP should stay as is. It is used by protocols to specify what
network types they support and they do not get SADR support
automatically.

>  #define NB_VPN		(NB_VPN4 | NB_VPN6)
>  #define NB_FLOW		(NB_FLOW4 | NB_FLOW6)
>  #define NB_DEST		(NB_IP | NB_VPN | NB_MPLS)

But you should add it to NB_DEST, so destinations are allowed.

> @@ -120,6 +121,15 @@ typedef struct net_addr_mpls {
>    u32 label;
>  } net_addr_mpls;
>  
> +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
> +  ip6_addr src_prefix;
> +} net_addr_sadr_ip6;
> +
>  typedef union net_addr_union {
>    net_addr n;
>    net_addr_ip4 ip4;
> @@ -131,6 +141,7 @@ typedef union net_addr_union {
>    net_addr_flow4 flow4;
>    net_addr_flow6 flow6;
>    net_addr_mpls mpls;
> +  net_addr_sadr_ip6 sadr_ip6;
>  } net_addr_union;
>  
>  
> @@ -169,6 +180,9 @@ extern const u16 net_max_text_length[];
>  #define NET_ADDR_MPLS(label) \
>    ((net_addr_mpls) { NET_MPLS, 20, sizeof(net_addr_mpls), label })
>  
>  static inline ip4_addr net4_prefix(const net_addr *a)
>  { return ((net_addr_ip4 *) a)->prefix; }
> @@ -243,6 +263,12 @@ static inline ip4_addr net4_prefix(const net_addr *a)
>  static inline ip6_addr net6_prefix(const net_addr *a)
>  { return ((net_addr_ip6 *) a)->prefix; }
>  
> +static inline ip6_addr net6_sadr_dst_prefix(const net_addr *a)
> +{ return ((net_addr_sadr_ip6 *) a)->dst_prefix; }
> +
> +static inline ip6_addr net6_sadr_src_prefix(const net_addr *a)
> +{ return ((net_addr_sadr_ip6 *) a)->src_prefix; }
> +

These accessors seems pointless. dst_prefix is the same as net6_prefix()
(assuming net_addr_sadr_ip6 extends net_addr_ip6 like other IPv6-based
nettypes) and src_prefix is not used outside specialized code, which would
have net typed as net_addr_sadr_ip6 and could access src_prefix directly.

>  static inline ip_addr net_prefix(const net_addr *a)
>  {
>    switch (a->type)
> @@ -259,6 +285,9 @@ static inline ip_addr net_prefix(const net_addr *a)
>    case NET_FLOW6:
>      return ipa_from_ip6(net6_prefix(a));
>  
> +  case NET_SADR_IP6:
> +    return ipa_from_ip6(net6_sadr_dst_prefix(a));
> +

You could handle that by net6_prefix() together with other IPv6-based nettypes.

>    case NET_MPLS:
>    default:
>      return IPA_NONE;
> @@ -279,6 +308,12 @@ static inline uint net4_pxlen(const net_addr *a)
>  static inline uint net6_pxlen(const net_addr *a)
>  { return a->pxlen; }
>  
> +static inline uint net6_sadr_dst_pxlen(const net_addr *a)
> +{ return ((net_addr_sadr_ip6 *) a)->dst_pxlen; }
> +
> +static inline uint net6_sadr_src_pxlen(const net_addr *a)
> +{ return ((net_addr_sadr_ip6 *) a)->src_pxlen; }
> +

Like net6_sadr_*_prefix



> @@ -327,6 +362,13 @@ static inline int net_equal_flow6(const net_addr_flow6 *a, const net_addr_flow6
>  static inline int net_equal_mpls(const net_addr_mpls *a, const net_addr_mpls *b)
>  { return !memcmp(a, b, sizeof(net_addr_mpls)); }
>  
> +typedef net_addr_ip6 net_addr_sadr_dst;
> +static inline int net_equal_sadr_dst(const net_addr_ip6 *a, const net_addr_ip6 *b)
> +{ return net_equal_ip6(a, b); }
> +
> +static inline int net_equal_sadr_ip6(const net_addr_sadr_ip6 *a, const net_addr_sadr_ip6 *b)
> +{ return !memcmp(a, b, sizeof(net_addr_sadr_ip6)); }
> +
>  
> @@ -455,6 +502,12 @@ static inline u32 net_hash_flow6(const net_addr_flow6 *n)
>  static inline u32 net_hash_mpls(const net_addr_mpls *n)
>  { return n->label; }
>  
> +static inline u32 net_hash_sadr_dst(const net_addr_sadr_dst* n)
> +{ return net_hash_ip6(n); }
> +
> +static inline u32 net_hash_sadr_ip6(const net_addr_sadr_ip6 *n)
> +{ return ip6_hash(n->dst_prefix) ^ ((u32) n->dst_pxlen << 26); }
> +

Perhaps unnecessary, see my comment to fib_find() below


> @@ -231,6 +232,20 @@ fib_find(struct fib *f, const net_addr *a)
>    case NET_ROA6: return FIB_FIND(f, a, roa6);
>    case NET_FLOW4: return FIB_FIND(f, a, flow4);
>    case NET_FLOW6: return FIB_FIND(f, a, flow6);
> +  case NET_SADR_IP6:
> +    {
> +      if (a->length != sizeof(net_addr_sadr_ip6))
> +      {
> +        // dst only search
> +        net_addr_ip6 a0;
> +        net_copy((net_addr *)&a0, a);
> +        a0.length = sizeof(net_addr_sadr_ip6);
> +
> +        return FIB_FIND(f, &a0, sadr_dst);
> +      }
> +      else
> +        return FIB_FIND(f, a, sadr_ip6);
> +    }

No. Generic fib_find() should implement exact-match search with matching
net_addr type. If you need/want partial match, it should be done as a
separate function. BTW, where do you need that?

Perhaps we should add some generic function partial-match IP-based search
for network types that support it (e.g., ROA, SADR). But it would need
a different interface, so caller could enumerate all valid matches.


> +static inline void *
> +net_route_sadr_ip6(rtable *t, net_addr_sadr_ip6 *n)
> +{
> +  net *r;
> +
> +  net_addr_ip6 dst = NET_ADDR_IP6(n->dst_prefix, n->dst_pxlen);
> +  dst.type = NET_SADR_IP6;
> +
> +  // search for matching dst
> +  while (r = net_route_ip6(t, &dst))
> +  {
> +    // found a matching dst, start search for entries that match both prefixes
> +    net_addr_sadr_ip6 full_addr = 
> +      NET_ADDR_SADR_IP6(dst.prefix, dst.pxlen, n->src_prefix, n->src_pxlen);
> +
> +    while (r = net_find_valid(t, (net_addr *) &full_addr), (!r) && (full_addr.src_pxlen > 0))
> +    {
> +      full_addr.src_pxlen--;
> +      ip6_clrbit(&full_addr.src_prefix, full_addr.src_pxlen);
> +    }
> +
> +    if (r)
> +      return r;
> +    dst.pxlen--;

You should set dst.pxlen based on r found by net_route_ip6(),
to avoid repeated iteration over the same prefix lengths that
did not match in the inner cycle.

Also be sure that pxlen is not already zero.


> @@ -1747,6 +1779,7 @@ rt_preconfig(struct config *c)
>  
>    rt_new_table(cf_get_symbol("master4"), NET_IP4);
>    rt_new_table(cf_get_symbol("master6"), NET_IP6);
> +  rt_new_table(cf_get_symbol("master_sadr6"), NET_SADR_IP6);
>  }

No, we have only IPv4 and IPv6 master table defined by default.
Other tables are defined in config file if necessary.


> @@ -1980,7 +2013,7 @@ rt_new_table(struct symbol *s, uint addr_type)
>    /* Hack that allows to 'redefine' the master table */
>    if ((s->class == SYM_TABLE) &&
>        (s->def == new_config->def_tables[addr_type]) &&
> -      ((addr_type == NET_IP4) || (addr_type == NET_IP6)))
> +      ((addr_type == NET_IP4) || (addr_type == NET_IP6) || (addr_type == NET_SADR_IP6)))
>      return s->def;

ditto


>    struct rtable_config *c = cfg_allocz(sizeof(struct rtable_config));
> diff --git a/sysdep/linux/netlink.c b/sysdep/linux/netlink.c

I will check KRT/Netlink code later.

-- 
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: 181 bytes
Desc: Digital signature
URL: <http://trubka.network.cz/pipermail/bird-users/attachments/20170522/45cd785d/attachment.asc>


More information about the Bird-users mailing list