[PATCH 0/7] Babel: Fixes for version 1.6.0
This patch series fixes a series of small-ish issues uncovered during testing of the 1.6.0 release of Bird. Thanks to Baptiste Jonglez for reporting most of them. Toke Høiland-Jørgensen (7): Babel: Make sure intervals don't overflow. Babel: Immediately send a hello on interface reconfigure. Babel: Rework handling of retractions. Babel: Documentation updates. Babel: Send wildcard retractions on shutdown and startup. Babel: Don't keep an infeasible route as selected. Babel: Don't maintain feasibility distance for our own routes. doc/bird.sgml | 33 ++++++++++-- nest/config.Y | 2 +- nest/rt-attr.c | 2 +- proto/babel/babel.c | 144 +++++++++++++++++++++++++++++++++++++------------- proto/babel/babel.h | 3 ++ proto/babel/config.Y | 9 ++-- proto/babel/packets.c | 42 +++++++++++---- 7 files changed, 179 insertions(+), 56 deletions(-) -- 2.8.0
Intervals are carried as 16-bit centisecond values, but kept internally in 16-bit second values, which causes a potential for overflow. This adds some checks to make sure this doesn't happen. Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk> --- proto/babel/babel.h | 2 ++ proto/babel/config.Y | 9 +++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/proto/babel/babel.h b/proto/babel/babel.h index aea0dd8..67d32ad 100644 --- a/proto/babel/babel.h +++ b/proto/babel/babel.h @@ -50,6 +50,8 @@ #define BABEL_INITIAL_HOP_COUNT 255 #define BABEL_MAX_SEND_INTERVAL 5 #define BABEL_TIME_UNITS 100 /* On-wire times are counted in centiseconds */ +#define BABEL_MAX_INTERVAL 0xFFFF/BABEL_TIME_UNITS /* max interval that won't overflow + * when carried as 16-bit centiseconds */ #define BABEL_SEQNO_REQUEST_EXPIRY 60 #define BABEL_GARBAGE_INTERVAL 300 diff --git a/proto/babel/config.Y b/proto/babel/config.Y index e7ce6a9..fea269d 100644 --- a/proto/babel/config.Y +++ b/proto/babel/config.Y @@ -77,17 +77,18 @@ babel_iface_finish: BABEL_IFACE->rxcost = BABEL_RXCOST_WIRED; } + /* make sure we don't overflow the 16-bit centisec fields */ if (!BABEL_IFACE->update_interval) - BABEL_IFACE->update_interval = BABEL_IFACE->hello_interval*BABEL_UPDATE_INTERVAL_FACTOR; - BABEL_IFACE->ihu_interval = BABEL_IFACE->hello_interval*BABEL_IHU_INTERVAL_FACTOR; + BABEL_IFACE->update_interval = MIN_(BABEL_IFACE->hello_interval*BABEL_UPDATE_INTERVAL_FACTOR, BABEL_MAX_INTERVAL); + BABEL_IFACE->ihu_interval = MIN_(BABEL_IFACE->hello_interval*BABEL_IHU_INTERVAL_FACTOR, BABEL_MAX_INTERVAL); }; babel_iface_item: | PORT expr { BABEL_IFACE->port = $2; if (($2<1) || ($2>65535)) cf_error("Invalid port number"); } | RXCOST expr { BABEL_IFACE->rxcost = $2; if (($2<1) || ($2>65535)) cf_error("Invalid rxcost"); } - | HELLO INTERVAL expr { BABEL_IFACE->hello_interval = $3; if (($3<1) || ($3>65535)) cf_error("Invalid hello interval"); } - | UPDATE INTERVAL expr { BABEL_IFACE->update_interval = $3; if (($3<1) || ($3>65535)) cf_error("Invalid hello interval"); } + | HELLO INTERVAL expr { BABEL_IFACE->hello_interval = $3; if (($3<1) || ($3>BABEL_MAX_INTERVAL)) cf_error("Invalid hello interval"); } + | UPDATE INTERVAL expr { BABEL_IFACE->update_interval = $3; if (($3<1) || ($3>BABEL_MAX_INTERVAL)) cf_error("Invalid hello interval"); } | TYPE WIRED { BABEL_IFACE->type = BABEL_IFACE_TYPE_WIRED; } | TYPE WIRELESS { BABEL_IFACE->type = BABEL_IFACE_TYPE_WIRELESS; } | RX BUFFER expr { BABEL_IFACE->rx_buffer = $3; if (($3<256) || ($3>65535)) cf_error("RX buffer must be in range 256-65535"); } -- 2.8.0
On Mon, May 02, 2016 at 07:07:49PM +0200, Toke Høiland-Jørgensen wrote:
Intervals are carried as 16-bit centisecond values, but kept internally in 16-bit second values, which causes a potential for overflow. This adds some checks to make sure this doesn't happen.
+ /* make sure we don't overflow the 16-bit centisec fields */ if (!BABEL_IFACE->update_interval) - BABEL_IFACE->update_interval = BABEL_IFACE->hello_interval*BABEL_UPDATE_INTERVAL_FACTOR; - BABEL_IFACE->ihu_interval = BABEL_IFACE->hello_interval*BABEL_IHU_INTERVAL_FACTOR; + BABEL_IFACE->update_interval = MIN_(BABEL_IFACE->hello_interval*BABEL_UPDATE_INTERVAL_FACTOR, BABEL_MAX_INTERVAL); + BABEL_IFACE->ihu_interval = MIN_(BABEL_IFACE->hello_interval*BABEL_IHU_INTERVAL_FACTOR, BABEL_MAX_INTERVAL);
This is not completely correct, because IHU interval is not independent of hello_interval in this implementation - IHUs are sent for each BABEL_IHU_INTERVAL_FACTOR hellos even if ihu_interval is limited by this. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Ondrej Zajicek <santiago@crfreenet.org> writes:
On Mon, May 02, 2016 at 07:07:49PM +0200, Toke Høiland-Jørgensen wrote:
Intervals are carried as 16-bit centisecond values, but kept internally in 16-bit second values, which causes a potential for overflow. This adds some checks to make sure this doesn't happen.
+ /* make sure we don't overflow the 16-bit centisec fields */ if (!BABEL_IFACE->update_interval) - BABEL_IFACE->update_interval = BABEL_IFACE->hello_interval*BABEL_UPDATE_INTERVAL_FACTOR; - BABEL_IFACE->ihu_interval = BABEL_IFACE->hello_interval*BABEL_IHU_INTERVAL_FACTOR; + BABEL_IFACE->update_interval = MIN_(BABEL_IFACE->hello_interval*BABEL_UPDATE_INTERVAL_FACTOR, BABEL_MAX_INTERVAL); + BABEL_IFACE->ihu_interval = MIN_(BABEL_IFACE->hello_interval*BABEL_IHU_INTERVAL_FACTOR, BABEL_MAX_INTERVAL);
This is not completely correct, because IHU interval is not independent of hello_interval in this implementation - IHUs are sent for each BABEL_IHU_INTERVAL_FACTOR hellos even if ihu_interval is limited by this.
Yeah, you're right. Hmm, guess it doesn't really make sense to have the IHU interval configurable by itself, then? -Toke
On Tue, Jul 19, 2016 at 06:09:41PM +0200, Toke Høiland-Jørgensen wrote:
Ondrej Zajicek <santiago@crfreenet.org> writes:
On Mon, May 02, 2016 at 07:07:49PM +0200, Toke Høiland-Jørgensen wrote:
Intervals are carried as 16-bit centisecond values, but kept internally in 16-bit second values, which causes a potential for overflow. This adds some checks to make sure this doesn't happen.
+ /* make sure we don't overflow the 16-bit centisec fields */ if (!BABEL_IFACE->update_interval) - BABEL_IFACE->update_interval = BABEL_IFACE->hello_interval*BABEL_UPDATE_INTERVAL_FACTOR; - BABEL_IFACE->ihu_interval = BABEL_IFACE->hello_interval*BABEL_IHU_INTERVAL_FACTOR; + BABEL_IFACE->update_interval = MIN_(BABEL_IFACE->hello_interval*BABEL_UPDATE_INTERVAL_FACTOR, BABEL_MAX_INTERVAL); + BABEL_IFACE->ihu_interval = MIN_(BABEL_IFACE->hello_interval*BABEL_IHU_INTERVAL_FACTOR, BABEL_MAX_INTERVAL);
This is not completely correct, because IHU interval is not independent of hello_interval in this implementation - IHUs are sent for each BABEL_IHU_INTERVAL_FACTOR hellos even if ihu_interval is limited by this.
Yeah, you're right. Hmm, guess it doesn't really make sense to have the IHU interval configurable by itself, then?
Probably not. But it makes sense to have configurable IHU factor (different for wired and wireless networks). And if hello*factor is too large, then just use smaller factor. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Ondrej Zajicek <santiago@crfreenet.org> writes:
On Tue, Jul 19, 2016 at 06:09:41PM +0200, Toke Høiland-Jørgensen wrote:
Ondrej Zajicek <santiago@crfreenet.org> writes:
On Mon, May 02, 2016 at 07:07:49PM +0200, Toke Høiland-Jørgensen wrote:
Intervals are carried as 16-bit centisecond values, but kept internally in 16-bit second values, which causes a potential for overflow. This adds some checks to make sure this doesn't happen.
+ /* make sure we don't overflow the 16-bit centisec fields */ if (!BABEL_IFACE->update_interval) - BABEL_IFACE->update_interval = BABEL_IFACE->hello_interval*BABEL_UPDATE_INTERVAL_FACTOR; - BABEL_IFACE->ihu_interval = BABEL_IFACE->hello_interval*BABEL_IHU_INTERVAL_FACTOR; + BABEL_IFACE->update_interval = MIN_(BABEL_IFACE->hello_interval*BABEL_UPDATE_INTERVAL_FACTOR, BABEL_MAX_INTERVAL); + BABEL_IFACE->ihu_interval = MIN_(BABEL_IFACE->hello_interval*BABEL_IHU_INTERVAL_FACTOR, BABEL_MAX_INTERVAL);
This is not completely correct, because IHU interval is not independent of hello_interval in this implementation - IHUs are sent for each BABEL_IHU_INTERVAL_FACTOR hellos even if ihu_interval is limited by this.
Yeah, you're right. Hmm, guess it doesn't really make sense to have the IHU interval configurable by itself, then?
Probably not. But it makes sense to have configurable IHU factor (different for wired and wireless networks). And if hello*factor is too large, then just use smaller factor.
Right, makes sense. I'll write that up and submit another patch :) -Toke
This makes Bird send a wildcard retraction on all interfaces before shutting down and right after starting up. This helps ensure that neighbours will discard the announced routes as soon as possible, rather than only after the normal timeout procedures. Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk> --- proto/babel/babel.c | 47 ++++++++++++++++++++++++++++++++++++++++------- proto/babel/babel.h | 1 + proto/babel/packets.c | 20 ++++++++++++++++---- 3 files changed, 57 insertions(+), 11 deletions(-) diff --git a/proto/babel/babel.c b/proto/babel/babel.c index e982abb..399d5f4 100644 --- a/proto/babel/babel.c +++ b/proto/babel/babel.c @@ -829,21 +829,29 @@ babel_trigger_update(struct babel_proto *p) /* A retraction is an update with an infinite metric */ static void -babel_send_retraction(struct babel_iface *ifa, ip_addr prefix, int plen) +babel_send_retraction(struct babel_iface *ifa, ip_addr prefix, int plen, u8 wildcard) { struct babel_proto *p = ifa->proto; union babel_msg msg = {}; - TRACE(D_PACKETS, "Sending retraction for %I/%d router-id %lR seqno %d", - prefix, plen, p->router_id, p->update_seqno); msg.type = BABEL_TLV_UPDATE; - msg.update.plen = plen; msg.update.interval = ifa->cf->update_interval; msg.update.seqno = p->update_seqno; msg.update.metric = BABEL_INFINITY; - msg.update.prefix = prefix; - msg.update.router_id = p->router_id; + if (wildcard) + { + TRACE(D_PACKETS, "Sending wildcard retraction on %s", ifa->ifname); + msg.update.wildcard = 1; + } + else + { + TRACE(D_PACKETS, "Sending retraction for %I/%d router-id %lR seqno %d", + prefix, plen, p->router_id, p->update_seqno); + msg.update.prefix = prefix; + msg.update.plen = plen; + msg.update.router_id = p->router_id; + } babel_enqueue(&msg, ifa); } @@ -1204,7 +1212,7 @@ babel_handle_route_request(union babel_msg *m, struct babel_iface *ifa) struct babel_entry *e = babel_find_entry(p, msg->prefix, msg->plen); if (!e) { - babel_send_retraction(ifa, msg->prefix, msg->plen); + babel_send_retraction(ifa, msg->prefix, msg->plen, 0); } else { @@ -1339,6 +1347,7 @@ babel_iface_start(struct babel_iface *ifa) ifa->up = 1; babel_send_hello(ifa, 0); + babel_send_retraction(ifa, IPA_NONE, 0, 1); babel_send_wildcard_request(ifa); babel_send_update(ifa, 0); /* Full update */ } @@ -2051,6 +2060,29 @@ babel_start(struct proto *P) return PS_UP; } +static inline void +babel_iface_shutdown(struct babel_iface *ifa) +{ + if(ifa->sk) + { + babel_send_retraction(ifa, IPA_NONE, 0, 1); + babel_send_queue(ifa); + } +} + +static int +babel_shutdown(struct proto *P) +{ + struct babel_proto *p = (void *) P; + struct babel_iface *ifa; + TRACE(D_EVENTS, "Shutting down interfaces"); + + WALK_LIST(ifa, p->interfaces) + babel_iface_shutdown(ifa); + + return PS_DOWN; +} + static int babel_reconfigure(struct proto *P, struct proto_config *c) { @@ -2078,6 +2110,7 @@ struct protocol proto_babel = { .init = babel_init, .dump = babel_dump, .start = babel_start, + .shutdown = babel_shutdown, .reconfigure = babel_reconfigure, .get_route_info = babel_get_route_info, .get_attr = babel_get_attr diff --git a/proto/babel/babel.h b/proto/babel/babel.h index 67d32ad..b871edd 100644 --- a/proto/babel/babel.h +++ b/proto/babel/babel.h @@ -270,6 +270,7 @@ struct babel_msg_update { u8 type; u8 ae; u8 plen; + u8 wildcard; u16 interval; u16 seqno; u16 metric; diff --git a/proto/babel/packets.c b/proto/babel/packets.c index 60b9cd3..b92ad9c 100644 --- a/proto/babel/packets.c +++ b/proto/babel/packets.c @@ -480,6 +480,7 @@ babel_read_update(struct babel_tlv *hdr, union babel_msg *m, if (tlv->plen > 0) return PARSE_ERROR; + msg->wildcard = 1; msg->prefix = IPA_NONE; break; @@ -558,8 +559,12 @@ babel_write_update(struct babel_tlv *hdr, union babel_msg *m, * When needed, we write Router-ID TLV before Update TLV and return size of * both of them. There is enough space for the Router-ID TLV, because * sizeof(struct babel_tlv_router_id) == sizeof(struct babel_tlv_update). + * + * Router ID is not used for retractions (section 4.4.9), so don't bother with + * it in this case. */ - if (!state->router_id_seen || (msg->router_id != state->router_id)) + if ((!state->router_id_seen || (msg->router_id != state->router_id)) + && msg->metric < BABEL_INFINITY) { len0 = babel_write_router_id(hdr, msg->router_id, state, max_len); tlv = (struct babel_tlv_update *) NEXT_TLV(tlv); @@ -572,12 +577,19 @@ babel_write_update(struct babel_tlv *hdr, union babel_msg *m, memset(tlv, 0, sizeof(struct babel_tlv_update)); TLV_HDR(tlv, BABEL_TLV_UPDATE, len); - tlv->ae = BABEL_AE_IP6; - tlv->plen = msg->plen; + if(msg->wildcard) + { + tlv->ae = BABEL_AE_WILDCARD; + } + else + { + tlv->ae = BABEL_AE_IP6; + tlv->plen = msg->plen; + put_ip6_px(tlv->addr, msg->prefix, msg->plen); + } put_time16(&tlv->interval, msg->interval); put_u16(&tlv->seqno, msg->seqno); put_u16(&tlv->metric, msg->metric); - put_ip6_px(tlv->addr, msg->prefix, msg->plen); return len0 + len; } -- 2.8.0
When a route becomes infeasible it shouldn't be kept as selected; this is forbidden by section 3.6 of the RFC and prevents subsequent updates from the same router ID from replacing it. Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk> --- proto/babel/babel.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/proto/babel/babel.c b/proto/babel/babel.c index 399d5f4..a40d848 100644 --- a/proto/babel/babel.c +++ b/proto/babel/babel.c @@ -565,6 +565,11 @@ babel_select_route(struct babel_entry *e) babel_send_seqno_request(e); babel_announce_rte(p, e); + + /* Section 3.6 of the RFC forbids an infeasible from being selected. This + is cleared after announcing the route to the core to make sure an + unreachable route is propagated first. */ + e->selected_in = NULL; } else { -- 2.8.0
We don't need to maintain feasibility distances for our own router ID (we ignore the updates anyway). Not doing so makes the routes be garbage collected sooner when export filters change. Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk> --- proto/babel/babel.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/proto/babel/babel.c b/proto/babel/babel.c index a40d848..826f718 100644 --- a/proto/babel/babel.c +++ b/proto/babel/babel.c @@ -788,14 +788,17 @@ babel_send_update(struct babel_iface *ifa, bird_clock_t changed) msg.update.prefix = e->n.prefix; msg.update.router_id = r->router_id; - /* Update feasibility distance */ - struct babel_source *s = babel_get_source(e, r->router_id); - s->expires = now + BABEL_GARBAGE_INTERVAL; - if ((msg.update.seqno > s->seqno) || - ((msg.update.seqno == s->seqno) && (msg.update.metric < s->metric))) + /* Update feasibility distance if it's not our own route. */ + if (r->router_id != p->router_id) { - s->seqno = msg.update.seqno; - s->metric = msg.update.metric; + struct babel_source *s = babel_get_source(e, r->router_id); + s->expires = now + BABEL_GARBAGE_INTERVAL; + if ((msg.update.seqno > s->seqno) || + ((msg.update.seqno == s->seqno) && (msg.update.metric < s->metric))) + { + s->seqno = msg.update.seqno; + s->metric = msg.update.metric; + } } babel_enqueue(&msg, ifa); } -- 2.8.0
Toke Høiland-Jørgensen <toke@toke.dk> writes:
This patch series fixes a series of small-ish issues uncovered during testing of the 1.6.0 release of Bird. Thanks to Baptiste Jonglez for reporting most of them.
So I don't think any of these ever got merged. Any chance of getting them merged soonish? :) -Toke
On Sun, Jul 17, 2016 at 02:57:51PM +0200, Toke Høiland-Jørgensen wrote:
Toke Høiland-Jørgensen <toke@toke.dk> writes:
This patch series fixes a series of small-ish issues uncovered during testing of the 1.6.0 release of Bird. Thanks to Baptiste Jonglez for reporting most of them.
So I don't think any of these ever got merged. Any chance of getting them merged soonish? :)
Hi Sorry for not handling that earlier. I will review them ASAP so they could get to 1.6.1, which we will release soon. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
participants (2)
-
Ondrej Zajicek -
Toke Høiland-Jørgensen