bgp proto bug with add paths in bird 1.6.3

[Lennert Buytenhek]

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);