Skip to content

Commit

Permalink
bgpd: fix missing addpath withdrawal race condition
Browse files Browse the repository at this point in the history
There is a race condition with addpath withdrawal. The withdrawal never
happens if it was request when coalesce timer running.

It can be demonstrated by adding to bgp_snmp_bgp4v2mib/rr/bgpd.conf:

> router bgp 65004
> + coalesce-time 10000

But it also happens in other conditions. For example, when using
"vtysh -f" to load the configuration.

It results in having two identical paths but with different addpath on
r2:

> r2# sh bgp ipv4 10.0.0.0/31
> BGP routing table entry for 10.0.0.0/31, version 3
> Paths: (3 available, best #1, table default)
>   65004
>     192.168.12.4 from 192.168.12.4 (192.168.12.4)
>       Origin IGP, metric 1, valid, external, multipath, best (AS Path)
>       AddPath ID: RX 0, TX-All 2 TX-Best-Per-AS 0 TX-Best-Selected 0
>       Advertised to: 192.168.12.4
>       Last update: Thu Sep 12 16:13:59 2024
>   65004
>     192.168.12.4 from 192.168.12.4 (192.168.12.4)
>       Origin IGP, metric 1, valid, external, multipath
>       AddPath ID: RX 3, TX-All 4 TX-Best-Per-AS 0 TX-Best-Selected 0
>       Advertised to: 192.168.12.4
>       Last update: Thu Sep 12 16:13:59 2024

The first path has been created first but should be withdrawn later.

Withdraw the stale addpath even the coalesce timer is running.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
  • Loading branch information
louis-6wind committed Sep 14, 2024
1 parent f3fc33e commit f407abe
Showing 1 changed file with 6 additions and 2 deletions.
8 changes: 6 additions & 2 deletions bgpd/bgp_updgrp_adv.c
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,12 @@ static int group_announce_route_walkcb(struct update_group *updgrp, void *arg)
afi2str(afi), safi2str(safi), ctx->dest);

UPDGRP_FOREACH_SUBGRP (updgrp, subgrp) {
/* withdraw stale addpath without waiting for the coalesce timer timeout.
* Otherwise, since adj->addpath_tx_id is overwritten, the code never
* notice anymore it has to do a withdrawal.
*/
if (addpath_capable)
subgrp_withdraw_stale_addpath(ctx, subgrp);
/*
* Skip the subgroups that have coalesce timer running. We will
* walk the entire prefix table for those subgroups when the
Expand All @@ -237,8 +243,6 @@ static int group_announce_route_walkcb(struct update_group *updgrp, void *arg)

/* An update-group that uses addpath */
if (addpath_capable) {
subgrp_withdraw_stale_addpath(ctx, subgrp);

subgrp_announce_addpath_best_selected(ctx->dest,
subgrp);

Expand Down

0 comments on commit f407abe

Please sign in to comment.