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>

3 files changed, 17 insertions, 6 deletions

diff --git a/bgpd/bgp_nexthop.c b/bgpd/bgp_nexthop.c
index ab0c3a3f1..8e43801d2 100644
--- 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 af4c0bc04..4a25c83cd 100644
--- 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 ad089d9d2..4d2271782 100644
--- 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