From 3e7e4a71868bc519aacc0eb785471b46fc345a5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Toke=20H=C3=B8iland-J=C3=B8rgensen?= Date: Mon, 30 Jan 2023 23:36:39 +0100 Subject: [PATCH 1/3] Babel: Fix missing modulo comparison of seqnos Juliusz noticed there were a couple of places we were doing straight inequality comparisons of seqnos in Babel. This is wrong because seqnos can wrap: so we need to use the modulo-64k comparison function for these cases as well. Introduce a strict-inequality version of the modulo-comparison for this purpose. --- proto/babel/babel.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/proto/babel/babel.c b/proto/babel/babel.c index ff8b6b52..a20bd724 100644 --- a/proto/babel/babel.c +++ b/proto/babel/babel.c @@ -49,6 +49,10 @@ static inline int ge_mod64k(uint a, uint b) { return (u16)(a - b) < 0x8000; } +/* Strict inequality version of the above */ +static inline int gt_mod64k(uint a, uint b) +{ return ge_mod64k(a, b) && a != b; } + static void babel_expire_requests(struct babel_proto *p, struct babel_entry *e); static void babel_select_route(struct babel_proto *p, struct babel_entry *e, struct babel_route *mod); static inline void babel_announce_retraction(struct babel_proto *p, struct babel_entry *e); @@ -559,7 +563,7 @@ babel_is_feasible(struct babel_source *s, u16 seqno, u16 metric) { return !s || (metric == BABEL_INFINITY) || - (seqno > s->seqno) || + gt_mod64k(seqno, s->seqno) || ((seqno == s->seqno) && (metric < s->metric)); } @@ -1013,7 +1017,7 @@ babel_send_update_(struct babel_iface *ifa, btime changed, struct fib *rtable) struct babel_source *s = babel_get_source(p, e, e->router_id); s->expires = current_time() + BABEL_GARBAGE_INTERVAL; - if ((msg.update.seqno > s->seqno) || + if (gt_mod64k(msg.update.seqno, s->seqno) || ((msg.update.seqno == s->seqno) && (msg.update.metric < s->metric))) { s->seqno = msg.update.seqno; From 96d7c4679df49b34be004177b10a99210af5f141 Mon Sep 17 00:00:00 2001 From: Ondrej Zajicek Date: Mon, 30 Jan 2023 23:49:20 +0100 Subject: [PATCH 2/3] Babel: Improve clarity of unfeasible update handling. Add a comment and (unnecessary) check to make correctness obvious. --- proto/babel/babel.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/proto/babel/babel.c b/proto/babel/babel.c index a20bd724..25081c3b 100644 --- a/proto/babel/babel.c +++ b/proto/babel/babel.c @@ -1308,9 +1308,10 @@ babel_handle_update(union babel_msg *m, struct babel_iface *ifa) /* * RFC section 3.8.2.2 - Dealing with unfeasible updates. Generate a one-off - * (not retransmitted) unicast seqno request to the originator of this update + * (not retransmitted) unicast seqno request to the originator of this update. + * Note: !feasible -> s exists, check for 's' is just for clarity / safety. */ - if (!feasible && (metric != BABEL_INFINITY) && + if (!feasible && s && (metric != BABEL_INFINITY) && (!best || (r == best) || (metric < best->metric))) babel_generate_seqno_request(p, e, s->router_id, s->seqno + 1, nbr); From dc4c5f51f83f97100b207136ecfde8ff94e597e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Toke=20H=C3=B8iland-J=C3=B8rgensen?= Date: Tue, 31 Jan 2023 15:52:14 +0100 Subject: [PATCH 3/3] Babel: Initialise source seqno from incoming message When creating a new babel_source object we initialise the seqno to 0. The caller will update the source object with the right metric and seqno value, for both newly created and old source objects. However if we initialise the source object seqno to 0 that may actually turn out to be a valid (higher) seqno than the one in the routing table, because of seqno wrapping. In this case the source metric will not be set properly, which breaks feasibility tracking for subsequent updates. To fix this, add a new initial_seqno argument to babel_get_source() which is used when allocating a new object, and set that to the seqno value of the update we're sending. Thanks to Juliusz Chroboczek for the bugreport. --- proto/babel/babel.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/proto/babel/babel.c b/proto/babel/babel.c index 25081c3b..becff6d0 100644 --- a/proto/babel/babel.c +++ b/proto/babel/babel.c @@ -105,7 +105,8 @@ babel_find_source(struct babel_entry *e, u64 router_id) } static struct babel_source * -babel_get_source(struct babel_proto *p, struct babel_entry *e, u64 router_id) +babel_get_source(struct babel_proto *p, struct babel_entry *e, u64 router_id, + u16 initial_seqno) { struct babel_source *s = babel_find_source(e, router_id); @@ -115,7 +116,7 @@ babel_get_source(struct babel_proto *p, struct babel_entry *e, u64 router_id) s = sl_allocz(p->source_slab); s->router_id = router_id; s->expires = current_time() + BABEL_GARBAGE_INTERVAL; - s->seqno = 0; + s->seqno = initial_seqno; s->metric = BABEL_INFINITY; add_tail(&e->sources, NODE s); @@ -1011,10 +1012,10 @@ babel_send_update_(struct babel_iface *ifa, btime changed, struct fib *rtable) babel_enqueue(&msg, ifa); - /* Update feasibility distance for redistributed routes */ + /* RFC 6126 3.7.3 - update feasibility distance for redistributed routes */ if (e->router_id != p->router_id) { - struct babel_source *s = babel_get_source(p, e, e->router_id); + struct babel_source *s = babel_get_source(p, e, e->router_id, msg.update.seqno); s->expires = current_time() + BABEL_GARBAGE_INTERVAL; if (gt_mod64k(msg.update.seqno, s->seqno) ||