Re: [PATCH v2][RFC] BFD: Labeling a manually configured neighbor for tracking

30 Mar 2023

      For BFD controlled Static routes, the BFD sessions are created
 to the route next hop. However, in some scenarios, e.g. when the
 next hop itself does not support BFD, or when it might be more
 meaningful to create the session to a remote monitoring station,
 we need more flexible relationship between a static route nexthop
 and its controlling BFD session.

 In this patch, a numeric label value can be specified with manually
 configured BFD neighbors, and a static route nexthop can be
 configured to track a certain BFD session using that label. The
 detailed behavior is updated in the user's guide.

 This feature may also be added to other protocols, but it seems not
 so meaningful.

Updates from V1:

- Add the missing UNUSED attribute to newly added label parameter of
   bfd_request_session() stub when BFD is not enabled at compile time.
- When cleaning all BFD requests tracking a request created by a 
   manually configured neighbor, skip the later request itself.
- When reconfigure BFD neighbors, if there are labels with added
   neighbors, call bfd_take_requests() to see if there are any matching
   requests in bfd_wait_list.
- When starting a BFD protocol instance, setup the manually configured
   neighbors before calling bfd_take_requests(), so that those requests
   in bfd_wait_list which were tracking non-existence labels can then
   be scanned to see if they can match the newly created neighbors.

In case the patch is broken, this patch can also be found at [1].

[1]: https://github.com/shankerwangmiao/bird/commit/58f6649.patch

 ---
  doc/bird.sgml         | 23 ++++++++++++--
  nest/bfd.h            |  5 +--
  nest/config.Y         | 11 ++++++-
  proto/bfd/bfd.c       | 73 +++++++++++++++++++++++++++++++++++++------
  proto/bfd/bfd.h       |  1 +
  proto/bfd/config.Y    | 22 +++++++++++--
  proto/bgp/bgp.c       |  2 +-
  proto/ospf/neighbor.c |  2 +-
  proto/rip/rip.c       |  2 +-
  proto/static/config.Y |  8 +++--
  proto/static/static.c |  6 ++--
  proto/static/static.h |  1 +
  12 files changed, 131 insertions(+), 25 deletions(-)

 diff --git a/doc/bird.sgml b/doc/bird.sgml
 index 5ac32e9e..80aaa24f 100644
 --- a/doc/bird.sgml
 +++ b/doc/bird.sgml
 @@ -2198,7 +2198,7 @@ protocol bfd [<name>] {
  		multiplier <num>;
  		passive <switch>;
  	};
 -	neighbor <ip> [dev "<interface>"] [local <ip>] [multihop <switch>];
 +	neighbor <ip> [dev "<interface>"] [local <ip>] [multihop <switch>] [label <num>];
  }
  </code>

 @@ -2233,7 +2233,7 @@ protocol bfd [<name>] {
  	connected sessions. Currently only one such definition (for all multihop
  	sessions) could be used.

 -	<tag><label id="bfd-neighbor">neighbor <m/ip/ [dev "<m/interface/"] [local <m/ip/] [multihop <m/switch/]</tag>
 +	<tag><label id="bfd-neighbor">neighbor <m/ip/ [dev "<m/interface/"] [local <m/ip/] [multihop <m/switch/] [label <m/num/]</tag>
  	BFD sessions are usually created on demand as requested by other
  	protocols (like OSPF or BGP). This option allows to explicitly add
  	a BFD session to the specified neighbor regardless of such requests.
 @@ -2242,7 +2242,11 @@ protocol bfd [<name>] {
  	optional specification of used interface and local IP. By default
  	the neighbor must be directly connected, unless the session is
  	configured as multihop. Note that local IP must be specified for
 -	multihop sessions.
 +	multihop sessions. If the optional <cf/label/ is specified, it is
 +	used to identify the session in the BFD protocol instance and can
 +	be later tracked explicitly by other protocols. The choice of the
 +	label is up to the user other than zero, but it should be unique
 +	within all BFD protocol instances.
  </descrip>

  <p>Session specific options (part of <cf/interface/ and <cf/multihop/ definitions):
 @@ -2338,6 +2342,7 @@ protocol bfd {
  	neighbor 192.168.1.10;
  	neighbor 192.168.2.2 dev "eth2";
  	neighbor 192.168.10.1 local 192.168.1.1 multihop;
 +	neighbor 192.168.10.2 local 192.168.1.1 multihop label 10;
  }
  </code>

 @@ -5314,6 +5319,17 @@ options (<cf/bfd/ and <cf/weight 1/), the second nexthop has just <cf/weight 2/.
  	that BFD protocol also has to be configured, see <ref id="bfd" name="BFD">
  	section for details. Default value is no.

 +	<tag><label id="static-route-bfd-track">bfd track <m/num/</tag>
 +	Like <cf/bfd/ option, but the BFD session is not created by the Static
 +	protocol, but it is expected to be already created by one of the neighbors
 +	labeled by the given number in the <cf/neighbor/ statement of the
 +	<ref id="bfd" name="BFD"> protocol. If such a BFD session is not configured,
 +	the route is not installed until a matching session is created later by a
 +	config update and established. It is useful when the next hop liveness
 +	is confirmed with another BFD (maybe multihop) peer other than the next hop
 +	itself. It is not possible to use both <cf/bfd/ and <cf/bfd track/ options
 +	for the same nexthop. Default value is not to track any BFD session.
 +
  	<tag><label id="static-route-mpls">mpls <m/num/[/<m/num/[/<m/num/[...]]]</tag>
  	MPLS labels that should be pushed to packets forwarded by the route.
  	The option could be used for both IP routes (on MPLS ingress routers)
 @@ -5516,6 +5532,7 @@ protocol static {
  	route 2001:db8:40::/48 via "eth2";		# Direct route to eth2
  	route 2001:db8::/32 unreachable;		# Unreachable route
  	route ::/0 via 2001:db8:1::1 bfd;		# BFD-controlled default route
 +	route 2001:db8:50::/48 via fe80::1%'eth1.70' bfd track 10;		# BFD-controlled default route, track bfd session labeled 10
  }
  </code>

 diff --git a/nest/bfd.h b/nest/bfd.h
 index 37561266..e558aad8 100644
 --- a/nest/bfd.h
 +++ b/nest/bfd.h
 @@ -42,6 +42,7 @@ struct bfd_request {
    u8 diag;
    u8 old_state;
    u8 down;
 +  u32 label;
  };

  #define BGP_BFD_GRACEFUL	2	/* BFD down triggers graceful restart */
 @@ -57,14 +58,14 @@ static inline struct bfd_options * bfd_new_options(void)

  #ifdef CONFIG_BFD

 -struct bfd_request * bfd_request_session(pool *p, ip_addr addr, ip_addr local, struct iface *iface, struct iface *vrf, void (*hook)(struct bfd_request *), void *data, const struct bfd_options *opts);
 +struct bfd_request * bfd_request_session(pool *p, ip_addr addr, ip_addr local, struct iface *iface, struct iface *vrf, u32 label, void (*hook)(struct bfd_request *), void *data, const struct bfd_options *opts);
  void bfd_update_request(struct bfd_request *req, const struct bfd_options *opts);

  static inline void cf_check_bfd(int use UNUSED) { }

  #else

 -static inline struct bfd_request * bfd_request_session(pool *p UNUSED, ip_addr addr UNUSED, ip_addr local UNUSED, struct iface *iface UNUSED, struct iface *vrf UNUSED, void (*hook)(struct bfd_request *) UNUSED, void *data UNUSED, const struct bfd_options *opts UNUSED) { return NULL; }
 +static inline struct bfd_request * bfd_request_session(pool *p UNUSED, ip_addr addr UNUSED, ip_addr local UNUSED, struct iface *iface UNUSED, struct iface *vrf UNUSED, u32 label UNUSED, void (*hook)(struct bfd_request *) UNUSED, void *data UNUSED, const struct bfd_options *opts UNUSED) { return NULL; }
  static inline void bfd_update_request(struct bfd_request *req UNUSED, const struct bfd_options *opts UNUSED) { };

  static inline void cf_check_bfd(int use) { if (use) cf_error("BFD not available"); }
 diff --git a/nest/config.Y b/nest/config.Y
 index e78350ca..dea898d1 100644
 --- a/nest/config.Y
 +++ b/nest/config.Y
 @@ -143,7 +143,7 @@ CF_ENUM_PX(T_ENUM_AF, AF_, AFI_, IPV4, IPV6)
  %type <s> optproto
  %type <ra> r_args
  %type <sd> sym_args
 -%type <i> proto_start echo_mask echo_size debug_mask debug_list debug_flag mrtdump_mask mrtdump_list mrtdump_flag export_mode limit_action net_type tos password_algorithm
 +%type <i> proto_start echo_mask echo_size debug_mask debug_list debug_flag mrtdump_mask mrtdump_list mrtdump_flag export_mode limit_action net_type tos password_algorithm bfd_neigh_label
  %type <ps> proto_patt proto_patt2
  %type <cc> channel_start proto_channel
  %type <cl> limit_spec
 @@ -605,6 +605,15 @@ bfd_opts:
     '{' bfd_items '}'
   ;

 +bfd_neigh_label:
 + expr {
 +  if ($1 == 0){
 +    cf_error("BFD label cannot be zero");
 +  }
 +  $$ = $1;
 + }
 + ;
 +
  /* Core commands */
  CF_CLI_HELP(SHOW, ..., [[Show status information]])

 diff --git a/proto/bfd/bfd.c b/proto/bfd/bfd.c
 index a96c1c3f..5e553d4c 100644
 --- a/proto/bfd/bfd.c
 +++ b/proto/bfd/bfd.c
 @@ -661,6 +661,23 @@ bfd_add_request(struct bfd_proto *p, struct bfd_request *req)
  {
    struct bfd_config *cf = (struct bfd_config *) (p->p.cf);

 +  struct bfd_request *ref_req = NULL;
 +  if (req->label != 0 && ipa_zero(req->addr)){
 +    struct bfd_neighbor *nei;
 +    WALK_LIST(nei, cf->neigh_list){
 +      if (nei->label == req->label){
 +        ref_req = nei->req;
 +        break;
 +      }
 +    }
 +    if (ref_req == NULL){
 +      return 0;
 +    }
 +  }
 +  struct bfd_request *actual_req = req;
 +  if (ref_req != NULL)
 +    req = ref_req;
 +
    if (p->p.vrf_set && (p->p.vrf != req->vrf))
      return 0;

 @@ -677,6 +694,8 @@ bfd_add_request(struct bfd_proto *p, struct bfd_request *req)
    if (!s)
      s = bfd_add_session(p, req->addr, req->local, req->iface, &req->opts);

 +  req = actual_req;
 +
    rem_node(&req->n);
    add_tail(&s->request_list, &req->n);
    req->session = s;
 @@ -733,7 +752,7 @@ static struct resclass bfd_request_class;

  struct bfd_request *
  bfd_request_session(pool *p, ip_addr addr, ip_addr local,
 -		    struct iface *iface, struct iface *vrf,
 +		    struct iface *iface, struct iface *vrf, u32 label,
  		    void (*hook)(struct bfd_request *), void *data,
  		    const struct bfd_options *opts)
  {
 @@ -746,6 +765,7 @@ bfd_request_session(pool *p, ip_addr addr, ip_addr local,
    req->local = local;
    req->iface = iface;
    req->vrf = vrf;
 +  req->label = label;

    if (opts)
      req->opts = *opts;
 @@ -773,9 +793,8 @@ bfd_update_request(struct bfd_request *req, const struct bfd_options *opts)
  }

  static void
 -bfd_request_free(resource *r)
 +bfd_request_detach_from_session(struct bfd_request *req)
  {
 -  struct bfd_request *req = (struct bfd_request *) r;
    struct bfd_session *s = req->session;

    rem_node(&req->n);
 @@ -787,6 +806,13 @@ bfd_request_free(resource *r)
      bfd_remove_session(s->ifa->bfd, s);
  }

 +static void
 +bfd_request_free(resource *r)
 +{
 +  struct bfd_request *req = (struct bfd_request *) r;
 +  return bfd_request_detach_from_session(req);
 +}
 +
  static void
  bfd_request_dump(resource *r)
  {
 @@ -821,7 +847,7 @@ bfd_neigh_notify(struct neighbor *nb)
    if ((nb->scope > 0) && !n->req)
    {
      ip_addr local = ipa_nonzero(n->local) ? n->local : nb->ifa->ip;
 -    n->req = bfd_request_session(p->p.pool, n->addr, local, nb->iface, p->p.vrf, NULL, NULL, NULL);
 +    n->req = bfd_request_session(p->p.pool, n->addr, local, nb->iface, p->p.vrf, n->label, NULL, NULL, NULL);
    }

    if ((nb->scope <= 0) && n->req)
 @@ -838,7 +864,7 @@ bfd_start_neighbor(struct bfd_proto *p, struct bfd_neighbor *n)

    if (n->multihop)
    {
 -    n->req = bfd_request_session(p->p.pool, n->addr, n->local, NULL, p->p.vrf, NULL, NULL, NULL);
 +    n->req = bfd_request_session(p->p.pool, n->addr, n->local, NULL, p->p.vrf, n->label, NULL, NULL, NULL);
      return;
    }

 @@ -870,16 +896,37 @@ bfd_stop_neighbor(struct bfd_proto *p UNUSED, struct bfd_neighbor *n)
    if (n->neigh)
      n->neigh->data = NULL;
    n->neigh = NULL;
 +  list tmp_list;
 +  init_list(&tmp_list);
 +  if (n->label != 0){
 +    if (n->req && n->req->session){
 +      struct bfd_session *s = n->req->session;
 +      struct node *nd, *ndn;
 +      WALK_LIST_DELSAFE(nd, ndn, s->request_list){
 +        struct bfd_request *req = SKIP_BACK(struct bfd_request, n, nd);
 +        if (req->label == n->label && ipa_zero(req->addr)){
 +          bfd_request_detach_from_session(req);
 +          add_tail(&tmp_list, &req->n);
 +        }
 +      }
 +    }
 +  }

    rfree(n->req);
    n->req = NULL;
 +
 +  struct node *nd;
 +  WALK_LIST_FIRST(nd, tmp_list){
 +    struct bfd_request *req = SKIP_BACK(struct bfd_request, n, nd);
 +    bfd_submit_request(req);
 +  }
  }

  static inline int
  bfd_same_neighbor(struct bfd_neighbor *x, struct bfd_neighbor *y)
  {
    return ipa_equal(x->addr, y->addr) && ipa_equal(x->local, y->local) &&
 -    (x->iface == y->iface) && (x->multihop == y->multihop);
 +    (x->iface == y->iface) && (x->multihop == y->multihop) && (x->label == y->label);
  }

  static void
 @@ -906,9 +953,17 @@ bfd_reconfigure_neighbors(struct bfd_proto *p, struct bfd_config *new)
    next:;
    }

 +  int have_new_labeled_neigh = 0;
    WALK_LIST(nn, new->neigh_list)
 -    if (!nn->active)
 +    if (!nn->active){
        bfd_start_neighbor(p, nn);
 +      if(nn->label != 0){
 +        have_new_labeled_neigh = 1;
 +      }
 +    }
 +  if (have_new_labeled_neigh){
 +    bfd_take_requests(p);
 +  }
  }

 @@ -1059,12 +1114,12 @@ bfd_start(struct proto *P)

    birdloop_leave(p->loop);

 -  bfd_take_requests(p);
 -
    struct bfd_neighbor *n;
    WALK_LIST(n, cf->neigh_list)
      bfd_start_neighbor(p, n);

 +  bfd_take_requests(p);
 +
    birdloop_start(p->loop);

    return PS_UP;
 diff --git a/proto/bfd/bfd.h b/proto/bfd/bfd.h
 index 011f91dc..c9016157 100644
 --- a/proto/bfd/bfd.h
 +++ b/proto/bfd/bfd.h
 @@ -83,6 +83,7 @@ struct bfd_neighbor

    u8 multihop;
    u8 active;
 +  u32 label;
  };

  struct bfd_proto
 diff --git a/proto/bfd/config.Y b/proto/bfd/config.Y
 index 70461872..e478fc3a 100644
 --- a/proto/bfd/config.Y
 +++ b/proto/bfd/config.Y
 @@ -24,11 +24,11 @@ CF_DECLS
  CF_KEYWORDS(BFD, MIN, IDLE, RX, TX, INTERVAL, MULTIPLIER, PASSIVE,
  	INTERFACE, MULTIHOP, NEIGHBOR, DEV, LOCAL, AUTHENTICATION,
  	NONE, SIMPLE, METICULOUS, KEYED, MD5, SHA1, IPV4, IPV6, DIRECT,
 -	STRICT, BIND)
 +	STRICT, BIND, LABEL)

  %type <iface> bfd_neigh_iface
  %type <a> bfd_neigh_local
 -%type <i> bfd_neigh_multihop bfd_auth_type
 +%type <i> bfd_neigh_multihop bfd_auth_type bfd_neigh_option_label

  CF_GRAMMAR

 @@ -164,7 +164,14 @@ bfd_neigh_multihop:
   | MULTIHOP bool { $$ = $2; }
   ;

 -bfd_neighbor: ipa bfd_neigh_iface bfd_neigh_local bfd_neigh_multihop
 +bfd_neigh_option_label:
 +   /* empty */ { $$ = 0; }
 + | LABEL bfd_neigh_label {
 +  $$ = $2;
 + }
 + ;
 +
 +bfd_neighbor: ipa bfd_neigh_iface bfd_neigh_local bfd_neigh_multihop bfd_neigh_option_label
  {
    this_bfd_neighbor = cfg_allocz(sizeof(struct bfd_neighbor));
    add_tail(&BFD_CFG->neigh_list, NODE this_bfd_neighbor);
 @@ -173,12 +180,21 @@ bfd_neighbor: ipa bfd_neigh_iface bfd_neigh_local bfd_neigh_multihop
    BFD_NEIGHBOR->local = $3;
    BFD_NEIGHBOR->iface = $2;
    BFD_NEIGHBOR->multihop = $4;
 +  BFD_NEIGHBOR->label = $5;

    if ($4 && $2)
      cf_error("Neighbor cannot set both interface and multihop");

    if ($4 && ipa_zero($3))
      cf_error("Multihop neighbor requires specified local address");
 +
 +  if ($5){
 +    struct bfd_neighbor *other_nei;
 +    WALK_LIST(other_nei, BFD_CFG->neigh_list){
 +      if (other_nei != this_bfd_neighbor && other_nei->label == $5)
 +        cf_error("Neighbor label %d already used", $5);
 +    }
 +  }
  };

 diff --git a/proto/bgp/bgp.c b/proto/bgp/bgp.c
 index 9408715e..a3e6becf 100644
 --- a/proto/bgp/bgp.c
 +++ b/proto/bgp/bgp.c
 @@ -1380,7 +1380,7 @@ bgp_update_bfd(struct bgp_proto *p, const struct bfd_options *bfd)
    if (bfd && !p->bfd_req && !bgp_is_dynamic(p))
      p->bfd_req = bfd_request_session(p->p.pool, p->remote_ip, p->local_ip,
  				     p->cf->multihop ? NULL : p->neigh->iface,
 -				     p->p.vrf, bgp_bfd_notify, p, bfd);
 +				     p->p.vrf, 0, bgp_bfd_notify, p, bfd);

    if (!bfd && p->bfd_req)
    {
 diff --git a/proto/ospf/neighbor.c b/proto/ospf/neighbor.c
 index ca369819..a59cef32 100644
 --- a/proto/ospf/neighbor.c
 +++ b/proto/ospf/neighbor.c
 @@ -776,7 +776,7 @@ ospf_neigh_update_bfd(struct ospf_neighbor *n, int use_bfd)

    if (use_bfd && !n->bfd_req)
      n->bfd_req = bfd_request_session(n->pool, n->ip, n->ifa->addr->ip,
 -				     n->ifa->iface, p->p.vrf,
 +				     n->ifa->iface, p->p.vrf, 0,
  				     ospf_neigh_bfd_hook, n, NULL);

    if (!use_bfd && n->bfd_req)
 diff --git a/proto/rip/rip.c b/proto/rip/rip.c
 index 8c2d5aeb..6c559ce8 100644
 --- a/proto/rip/rip.c
 +++ b/proto/rip/rip.c
 @@ -531,7 +531,7 @@ rip_update_bfd(struct rip_proto *p, struct rip_neighbor *n)
       */
      ip_addr saddr = rip_is_v2(p) ? n->ifa->sk->saddr : n->nbr->ifa->ip;
      n->bfd_req = bfd_request_session(p->p.pool, n->nbr->addr, saddr,
 -				     n->nbr->iface, p->p.vrf,
 +				     n->nbr->iface, p->p.vrf, 0,
  				     rip_bfd_notify, n, NULL);
    }

 diff --git a/proto/static/config.Y b/proto/static/config.Y
 index 9d26ee82..6fa47338 100644
 --- a/proto/static/config.Y
 +++ b/proto/static/config.Y
 @@ -46,8 +46,7 @@ static_route_finish(void)
  CF_DECLS

  CF_KEYWORDS(STATIC, ROUTE, VIA, DROP, REJECT, PROHIBIT, PREFERENCE, CHECK, LINK)
 -CF_KEYWORDS(ONLINK, WEIGHT, RECURSIVE, IGP, TABLE, BLACKHOLE, UNREACHABLE, BFD, MPLS)
 -
 +CF_KEYWORDS(ONLINK, WEIGHT, RECURSIVE, IGP, TABLE, BLACKHOLE, UNREACHABLE, BFD, MPLS, TRACK)

  CF_GRAMMAR

 @@ -98,6 +97,11 @@ stat_nexthop:
    }
    | stat_nexthop BFD bool {
      this_snh->use_bfd = $3; cf_check_bfd($3);
 +    this_snh->bfd_label = 0;
 +  }
 +  | stat_nexthop BFD TRACK bfd_neigh_label {
 +    this_snh->use_bfd = 1; cf_check_bfd(1);
 +    this_snh->bfd_label = $4;
    }
  ;

 diff --git a/proto/static/static.c b/proto/static/static.c
 index f2bb98c7..35f6287b 100644
 --- a/proto/static/static.c
 +++ b/proto/static/static.c
 @@ -206,9 +206,10 @@ static_update_bfd(struct static_proto *p, struct static_route *r)
    if (bfd_up && !r->bfd_req)
    {
      // ip_addr local = ipa_nonzero(r->local) ? r->local : nb->ifa->ip;
 -    r->bfd_req = bfd_request_session(p->p.pool, r->via, 
 +    r->bfd_req = bfd_request_session(p->p.pool,
 +				     r->bfd_label != 0 ? IPA_NONE : r->via,
  				     nb->ifa ? nb->ifa->ip : IPA_NONE,
 -				     nb->iface, p->p.vrf,
 +				     nb->iface, p->p.vrf, r->bfd_label,
  				     static_bfd_notify, r, NULL);
    }

 @@ -320,6 +321,7 @@ static_same_dest(struct static_route *x, struct static_route *y)
  	  (x->onlink != y->onlink) ||
  	  (x->weight != y->weight) ||
  	  (x->use_bfd != y->use_bfd) ||
 +	  (x->bfd_label != y->bfd_label) ||
  	  (!x->mls != !y->mls) ||
  	  ((x->mls) && (y->mls) && (x->mls->len != y->mls->len)))
  	return 0;
 diff --git a/proto/static/static.h b/proto/static/static.h
 index fc91f71c..22010d86 100644
 --- a/proto/static/static.h
 +++ b/proto/static/static.h
 @@ -50,6 +50,7 @@ struct static_route {
    byte use_bfd;				/* Configured to use BFD */
    struct bfd_request *bfd_req;		/* BFD request, if BFD is used */
    mpls_label_stack *mls;		/* MPLS label stack; may be NULL */
 +  u32 bfd_label;      /* Label for referred BFD session */
  };

  /*
 -- 
 2.39.0

Re: [PATCH v2][RFC] BFD: Labeling a manually configured neighbor for tracking

Miao Wang