On Mon, Mar 06, 2017 at 07:41:44PM +0200, Lennert Buytenhek wrote:

Hi!

bgp_init() in proto/bgp/bgp.c does:

P->accept_ra_types = c->secondary ? RA_ACCEPTED : RA_OPTIMAL;

and then bgp_rx_open() in proto/bgp/packets.c does:

if (p->add_path_tx) p->p.accept_ra_types = RA_ANY;

As bgp_init() seems to only be called at configuration time, this means that if you have add path tx, and you have a peer that advertises rx add path and then reconnects without advertising rx add path, you will still have RA_ANY set on the sending side and not have the correct behavior of only sending the optimal route. (This is easy to verify.)

Not entirely sure how you'd want to fix this, but perhaps replicating the RA_ACCEPTED / RA_OPTIONAL assignment in bgp_rx_open() (or just moving it there) would do the trick?

In other words, something like this? (I didn't trace all the code paths, but I'm guessing it should be safe to remove the ->accept_ra_types assignment from bgp_init() based on the fact that we are overriding it in bgp_rx_open() and expecting that to do the trick for the add_path_tx case -- if that's not sufficient then it's not safe to do that reassignment from there to begin with.) diff --git a/proto/bgp/packets.c b/proto/bgp/packets.c index d100b7d..a566d11 100644 --- a/proto/bgp/packets.c +++ b/proto/bgp/packets.c @@ -1039,6 +1039,8 @@ bgp_rx_open(struct bgp_conn *conn, byte *pkt, uint len) if (p->add_path_tx) p->p.accept_ra_types = RA_ANY; + else + p->p.accept_ra_types = p->cf->secondary ? RA_ACCEPTED : RA_OPTIMAL; DBG("BGP: Hold timer set to %d, keepalive to %d, AS to %d, ID to %x, AS4 session to %d\n", conn->hold_time, conn->keepalive_time, p->remote_as, p->remote_id, p->as4_session);