bgpd: Refine multiaccess check for next hop resetting

A BGP update-group is dynamically created to group together a set of peers
such that any BGP updates can be formed just once for the entire group and
only the next hop attribute may need to be modified when the update is sent
out to each peer in the group. The update formation code attempts to
determine as much as possible if the next hop will be set to our own IP
address for every peer in the group. This helps to avoid additional checks
at the point of sending the update (which happens on a per-peer basis) and
also because some other attributes may/could vary depending on whether the
next hop is set to our own IP or not. Resetting the next hop to our own IP
address is the most common behavior for EBGP peerings in the absence of
other user-configured or internal (e.g., for l2vpn/evpn) settings and
peerings on a shared subnet.

The code had a flaw in the multiaccess check to see if there are peers in
the update group which are on a shared subnet as the next hop of the path
being announced - the source peer could itself be in the same update group
and cause the check to give an incorrect result. Modify the check to skip
the source peer so that the check is more accurate.

Signed-off-by: Vivek Venkatraman <vivek@cumulusnetworks.com>
Reviewed-by:   Donald Sharp <sharpd@cumulusnetworks.com>
Reviewed-by:   Don Slice <dslice@cumulusnetworks.com>

diff --git a/bgpd/bgp_nexthop.c b/bgpd/bgp_nexthop.c
index ab0c3a3f113aa2b21c87aa8f6da3386ba8961f90..8e43801d2d46bbc15605aa5583a03ee4ab9a1689 100644 (file)
--- a/bgpd/bgp_nexthop.c
+++ b/bgpd/bgp_nexthop.c
@@ -612,7 +612,8 @@ int bgp_multiaccess_check_v6(struct in6_addr nexthop, struct peer *peer)
 }
 
 int bgp_subgrp_multiaccess_check_v6(struct in6_addr nexthop,
-                       struct update_subgroup *subgrp)
+                                   struct update_subgroup *subgrp,
+                                   struct peer *exclude)
 {
        struct bgp_node *rn1 = NULL, *rn2 = NULL;
        struct peer_af *paf = NULL;
@@ -632,6 +633,9 @@ int bgp_subgrp_multiaccess_check_v6(struct in6_addr nexthop,
                return 0;
 
        SUBGRP_FOREACH_PEER (subgrp, paf) {
+               /* Skip peer we're told to exclude - e.g., source of route. */
+               if (paf->peer == exclude)
+                       continue;
 
                p.u.prefix6 = paf->peer->su.sin6.sin6_addr;
                rn2 = bgp_node_match(bgp->connected_table[AFI_IP6], &p);
@@ -650,7 +654,8 @@ int bgp_subgrp_multiaccess_check_v6(struct in6_addr nexthop,
 }
 
 int bgp_subgrp_multiaccess_check_v4(struct in_addr nexthop,
-                                   struct update_subgroup *subgrp)
+                                   struct update_subgroup *subgrp,
+                                   struct peer *exclude)
 {
        struct bgp_node *rn1, *rn2;
        struct peer_af *paf;
@@ -670,6 +675,10 @@ int bgp_subgrp_multiaccess_check_v4(struct in_addr nexthop,
                return 0;
 
        SUBGRP_FOREACH_PEER (subgrp, paf) {
+               /* Skip peer we're told to exclude - e.g., source of route. */
+               if (paf->peer == exclude)
+                       continue;
+
                p.u.prefix4 = paf->peer->su.sin.sin_addr;
 
                rn2 = bgp_node_match(bgp->connected_table[AFI_IP], &p);

diff --git a/bgpd/bgp_nexthop.h b/bgpd/bgp_nexthop.h
index af4c0bc0470595c6feee0af243d2516c623b3151..4a25c83cd5ef21cd38bcc7a4941e44c15a8deabb 100644 (file)
--- a/bgpd/bgp_nexthop.h
+++ b/bgpd/bgp_nexthop.h
@@ -82,9 +82,11 @@ struct bgp_addrv6 {
 extern void bgp_connected_add(struct bgp *bgp, struct connected *c);
 extern void bgp_connected_delete(struct bgp *bgp, struct connected *c);
 extern int bgp_subgrp_multiaccess_check_v4(struct in_addr nexthop,
-                                          struct update_subgroup *subgrp);
+                                          struct update_subgroup *subgrp,
+                                          struct peer *exclude);
 extern int bgp_subgrp_multiaccess_check_v6(struct in6_addr nexthop,
-                                          struct update_subgroup *subgrp);
+                                          struct update_subgroup *subgrp,
+                                          struct peer *exclude);
 extern int bgp_multiaccess_check_v4(struct in_addr nexthop, struct peer *peer);
 extern int bgp_multiaccess_check_v6(struct in6_addr nexthop, struct peer *peer);
 extern int bgp_config_write_scan_time(struct vty *);

diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index ad089d9d25e7b9573b0bfebb080ac0c9896d331d..4d22717825fb7eec1dcb8e21ef796519b2c127a8 100644 (file)
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -2006,7 +2006,7 @@ int subgroup_announce_check(struct bgp_node *rn, struct bgp_path_info *pi,
                        if ((p->family == AF_INET) &&
                                (!bgp_subgrp_multiaccess_check_v4(
                                        piattr->nexthop,
-                                       subgrp)))
+                                       subgrp, from)))
                                subgroup_announce_reset_nhop(
                                        (peer_cap_enhe(peer, afi, safi)
                                                 ? AF_INET6
@@ -2016,7 +2016,7 @@ int subgroup_announce_check(struct bgp_node *rn, struct bgp_path_info *pi,
                        if ((p->family == AF_INET6) &&
                                (!bgp_subgrp_multiaccess_check_v6(
                                        piattr->mp_nexthop_global,
-                                       subgrp)))
+                                       subgrp, from)))
                                subgroup_announce_reset_nhop(
                                        (peer_cap_enhe(peer, afi, safi)
                                                ? AF_INET6