From ed2a7b50ef3333ec932faf0b7ff5c3eaed9e9705 Mon Sep 17 00:00:00 2001 From: Yuan Yuan Date: Thu, 14 Dec 2023 19:06:24 +0000 Subject: [PATCH] bgpd: Fix over-aggressive in de-dup BGP messages When a BGP Update->Withdraw->Update happens in very quick succession, FRR would think the second Update is a duplicate, and not advertise the Update out after the Withdraw. Thus fixing the logic to not execute dedup when 1, there is no match on adj; 2, when matched adj is marked as withdrawl which would still in adj table and have attr info until the Withdraw being sent out; 3, attr_hash matching to confirm dedup is not enough for hash conflict case. Signed-off-by: Yuan Yuan --- bgpd/bgp_advertise.h | 4 ++-- bgpd/bgp_updgrp_adv.c | 17 ++++++++++++----- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/bgpd/bgp_advertise.h b/bgpd/bgp_advertise.h index 7c3b23ab54e9..c6e9aa85cdf7 100644 --- a/bgpd/bgp_advertise.h +++ b/bgpd/bgp_advertise.h @@ -75,8 +75,8 @@ struct bgp_adj_out { /* Advertisement information. */ struct bgp_advertise *adv; - /* Attribute hash */ - uint32_t attr_hash; + /* Withdraw */ + bool is_withdraw; }; RB_HEAD(bgp_adj_out_rb, bgp_adj_out); diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c index c466d8719776..e9fe6ad4da53 100644 --- a/bgpd/bgp_updgrp_adv.c +++ b/bgpd/bgp_updgrp_adv.c @@ -508,7 +508,7 @@ void bgp_adj_out_set_subgroup(struct bgp_dest *dest, struct peer *adv_peer; struct peer_af *paf; struct bgp *bgp; - uint32_t attr_hash = attrhash_key_make(attr); + bool no_dedup = false; peer = SUBGRP_PEER(subgrp); afi = SUBGRP_AFI(subgrp); @@ -533,6 +533,7 @@ void bgp_adj_out_set_subgroup(struct bgp_dest *dest, &path->tx_addpath)); if (!adj) return; + no_dedup = true; subgrp->pscount++; } @@ -543,9 +544,14 @@ void bgp_adj_out_set_subgroup(struct bgp_dest *dest, * the route wasn't changed actually. * Do not suppress BGP UPDATES for route-refresh. */ - if (CHECK_FLAG(bgp->flags, BGP_FLAG_SUPPRESS_DUPLICATES) - && !CHECK_FLAG(subgrp->sflags, SUBGRP_STATUS_FORCE_UPDATES) - && adj->attr_hash == attr_hash) { + if (CHECK_FLAG(bgp->flags, BGP_FLAG_SUPPRESS_DUPLICATES) && + !CHECK_FLAG(subgrp->sflags, SUBGRP_STATUS_FORCE_UPDATES) + /* When the adj entry for dedup check was not there, dedup should not happen */ + && !no_dedup + /* Is not withdraw */ + && !adj->is_withdraw + /* Still have to compare the original attr to make sure nothing has changed */ + && adj->attr && attr && attrhash_cmp(adj->attr, attr)) { if (BGP_DEBUG(update, UPDATE_OUT)) { char attr_str[BUFSIZ] = {0}; @@ -586,7 +592,7 @@ void bgp_adj_out_set_subgroup(struct bgp_dest *dest, adv->baa = bgp_advertise_attr_intern(subgrp->hash, attr); adv->adj = adj; - adj->attr_hash = attr_hash; + adj->is_withdraw = false; /* Add new advertisement to advertisement attribute list. */ bgp_advertise_add(adv->baa, adv); @@ -657,6 +663,7 @@ void bgp_adj_out_unset_subgroup(struct bgp_dest *dest, adv = adj->adv; adv->dest = dest; adv->adj = adj; + adj->is_withdraw = true; /* Note if we need to trigger a packet write */ trigger_write =