From 6cb6380686f2867c9548ddb168ca7dfdd387892e Mon Sep 17 00:00:00 2001 From: Ondrej Zajicek Date: Wed, 3 Jun 2015 11:58:46 +0200 Subject: [PATCH 1/3] KRT: Fixes some minor bugs in kernel protocol Conflicts: sysdep/linux/netlink.c --- sysdep/linux/netlink.c | 5 ++ sysdep/unix/krt.c | 107 +++++++++++++++++++++++++---------------- 2 files changed, 71 insertions(+), 41 deletions(-) diff --git a/sysdep/linux/netlink.c b/sysdep/linux/netlink.c index e8cfb2fa..a6d8a421 100644 --- a/sysdep/linux/netlink.c +++ b/sysdep/linux/netlink.c @@ -649,9 +649,14 @@ nl_send_route(struct krt_proto *p, rte *e, struct ea_list *eattrs, int new) r.r.rtm_scope = RT_SCOPE_UNIVERSE; nl_add_attr_ipa(&r.h, sizeof(r), RTA_DST, net->n.prefix); + /* For route delete, we do not specify route attributes */ + if (!new) + return nl_exchange(&r.h); + u32 metric = 0; if (new && e->attrs->source == RTS_INHERIT) metric = e->u.krt.metric; + if (ea = ea_find(eattrs, EA_KRT_METRIC)) metric = ea->u.data; if (metric != 0) diff --git a/sysdep/unix/krt.c b/sysdep/unix/krt.c index 7ea43159..a1d4e68c 100644 --- a/sysdep/unix/krt.c +++ b/sysdep/unix/krt.c @@ -592,6 +592,44 @@ krt_flush_routes(struct krt_proto *p) FIB_WALK_END; } +static struct rte * +krt_export_net(struct krt_proto *p, net *net, rte **rt_free, ea_list **tmpa) +{ + struct filter *filter = p->p.main_ahook->out_filter; + rte *rt; + + rt = net->routes; + *rt_free = NULL; + + if (!rte_is_valid(rt)) + return NULL; + + if (filter == FILTER_REJECT) + return NULL; + + struct proto *src = rt->attrs->src->proto; + *tmpa = src->make_tmp_attrs ? src->make_tmp_attrs(rt, krt_filter_lp) : NULL; + + /* We could run krt_import_control() here, but it is already handled by KRF_INSTALLED */ + + if (filter == FILTER_ACCEPT) + goto accept; + + if (f_run(filter, &rt, tmpa, krt_filter_lp, FF_FORCE_TMPATTR) > F_ACCEPT) + goto reject; + + +accept: + if (rt != net->routes) + *rt_free = rt; + return rt; + +reject: + if (rt != net->routes) + rte_free(rt); + return NULL; +} + static int krt_same_dest(rte *k, rte *e) { @@ -620,7 +658,6 @@ krt_same_dest(rte *k, rte *e) void krt_got_route(struct krt_proto *p, rte *e) { - rte *old; net *net = e->net; int verdict; @@ -663,15 +700,26 @@ krt_got_route(struct krt_proto *p, rte *e) goto sentenced; } - old = net->routes; - if ((net->n.flags & KRF_INSTALLED) && rte_is_valid(old)) + if (net->n.flags & KRF_INSTALLED) { - /* There may be changes in route attributes, we ignore that. - Also, this does not work well if gw is changed in export filter */ - if ((net->n.flags & KRF_SYNC_ERROR) || ! krt_same_dest(e, old)) + rte *new, *rt_free; + ea_list *tmpa; + + new = krt_export_net(p, net, &rt_free, &tmpa); + + /* TODO: There also may be changes in route eattrs, we ignore that for now. */ + + if (!new) + verdict = KRF_DELETE; + else if ((net->n.flags & KRF_SYNC_ERROR) || !krt_same_dest(e, new)) verdict = KRF_UPDATE; else verdict = KRF_SEEN; + + if (rt_free) + rte_free(rt_free); + + lp_flush(krt_filter_lp); } else verdict = KRF_DELETE; @@ -692,25 +740,6 @@ krt_got_route(struct krt_proto *p, rte *e) rte_free(e); } -static inline int -krt_export_rte(struct krt_proto *p, rte **new, ea_list **tmpa) -{ - struct filter *filter = p->p.main_ahook->out_filter; - - if (! *new) - return 0; - - if (filter == FILTER_REJECT) - return 0; - - if (filter == FILTER_ACCEPT) - return 1; - - struct proto *src = (*new)->attrs->src->proto; - *tmpa = src->make_tmp_attrs ? src->make_tmp_attrs(*new, krt_filter_lp) : NULL; - return f_run(filter, new, tmpa, krt_filter_lp, FF_FORCE_TMPATTR) <= F_ACCEPT; -} - static void krt_prune(struct krt_proto *p) { @@ -721,7 +750,7 @@ krt_prune(struct krt_proto *p) { net *n = (net *) f; int verdict = f->flags & KRF_VERDICT_MASK; - rte *new, *new0, *old; + rte *new, *old, *rt_free = NULL; ea_list *tmpa = NULL; if (verdict == KRF_UPDATE || verdict == KRF_DELETE) @@ -733,23 +762,18 @@ krt_prune(struct krt_proto *p) else old = NULL; - new = new0 = n->routes; if (verdict == KRF_CREATE || verdict == KRF_UPDATE) { /* We have to run export filter to get proper 'new' route */ - if (! krt_export_rte(p, &new, &tmpa)) - { - /* Route rejected, should not happen (KRF_INSTALLED) but to be sure .. */ - verdict = (verdict == KRF_CREATE) ? KRF_IGNORE : KRF_DELETE; - } + new = krt_export_net(p, n, &rt_free, &tmpa); + + if (!new) + verdict = (verdict == KRF_CREATE) ? KRF_IGNORE : KRF_DELETE; else - { - ea_list **x = &tmpa; - while (*x) - x = &((*x)->next); - *x = new ? new->attrs->eattrs : NULL; - } + tmpa = ea_append(tmpa, new->attrs->eattrs); } + else + new = NULL; switch (verdict) { @@ -778,8 +802,8 @@ krt_prune(struct krt_proto *p) if (old) rte_free(old); - if (new != new0) - rte_free(new); + if (rt_free) + rte_free(rt_free); lp_flush(krt_filter_lp); f->flags &= ~KRF_VERDICT_MASK; } @@ -974,7 +998,8 @@ krt_import_control(struct proto *P, rte **new, ea_list **attrs, struct linpool * * We will remove KRT_INSTALLED flag, which stops such withdraw to be * processed in krt_rt_notify() and krt_replace_rte(). */ - e->net->n.flags &= ~KRF_INSTALLED; + if (e == e->net->routes) + e->net->n.flags &= ~KRF_INSTALLED; #endif return -1; } From 0c793237cf6ffc3c7813951692baf7df87337a5a Mon Sep 17 00:00:00 2001 From: "Ondrej Zajicek (work)" Date: Wed, 23 Mar 2016 18:25:15 +0100 Subject: [PATCH 2/3] KRT: Fix route learn scan when route changed When a kernel route changed, function krt_learn_scan() noticed that and replaced the route in internal kernel FIB, but after that, function krt_learn_prune() failed to propagate the new route to the nest, because it confused the new route with the (removed) old best route and decided that the best route did not changed. Wow, the original code (and the bug) is almost 17 years old. Conflicts: sysdep/linux/netlink.c --- nest/route.h | 2 +- sysdep/bsd/krt-sock.c | 5 ++--- sysdep/linux/netlink.c | 5 ++++- sysdep/unix/krt.c | 46 ++++++++++++++++++++++++++---------------- 4 files changed, 36 insertions(+), 22 deletions(-) diff --git a/nest/route.h b/nest/route.h index 80369cc3..6c46d1dc 100644 --- a/nest/route.h +++ b/nest/route.h @@ -217,8 +217,8 @@ typedef struct rte { struct { /* Routes generated by krt sync (both temporary and inherited ones) */ s8 src; /* Alleged route source (see krt.h) */ u8 proto; /* Kernel source protocol ID */ - u8 type; /* Kernel route type */ u8 seen; /* Seen during last scan */ + u8 best; /* Best route in network, propagated to core */ u32 metric; /* Kernel metric */ } krt; } u; diff --git a/sysdep/bsd/krt-sock.c b/sysdep/bsd/krt-sock.c index aaf3b23c..ae810562 100644 --- a/sysdep/bsd/krt-sock.c +++ b/sysdep/bsd/krt-sock.c @@ -493,9 +493,8 @@ krt_read_route(struct ks_msg *msg, struct krt_proto *p, int scan) e->net = net; e->u.krt.src = src; e->u.krt.proto = src2; - - /* These are probably too Linux-specific */ - e->u.krt.type = 0; + e->u.krt.seen = 0; + e->u.krt.best = 0; e->u.krt.metric = 0; if (scan) diff --git a/sysdep/linux/netlink.c b/sysdep/linux/netlink.c index a6d8a421..91f2cd69 100644 --- a/sysdep/linux/netlink.c +++ b/sysdep/linux/netlink.c @@ -911,7 +911,10 @@ nl_parse_route(struct nlmsghdr *h, int scan) e->net = net; e->u.krt.src = src; e->u.krt.proto = i->rtm_protocol; - e->u.krt.type = i->rtm_type; + /*e->u.krt.type = i->rtm_type;*/ + e->u.krt.seen = 0; + e->u.krt.best = 0; + e->u.krt.metric = 0; if (a[RTA_PRIORITY]) memcpy(&e->u.krt.metric, RTA_DATA(a[RTA_PRIORITY]), sizeof(e->u.krt.metric)); diff --git a/sysdep/unix/krt.c b/sysdep/unix/krt.c index a1d4e68c..64cd4a6e 100644 --- a/sysdep/unix/krt.c +++ b/sysdep/unix/krt.c @@ -416,46 +416,58 @@ krt_learn_prune(struct krt_proto *p) net *n = (net *) f; rte *e, **ee, *best, **pbest, *old_best; - old_best = n->routes; + /* + * Note that old_best may be NULL even if there was an old best route in + * the previous step, because it might be replaced in krt_learn_scan(). + * But in that case there is a new valid best route. + */ + + old_best = NULL; best = NULL; pbest = NULL; ee = &n->routes; while (e = *ee) { + if (e->u.krt.best) + old_best = e; + if (!e->u.krt.seen) { *ee = e->next; rte_free(e); continue; } + if (!best || best->u.krt.metric > e->u.krt.metric) { best = e; pbest = ee; } + e->u.krt.seen = 0; + e->u.krt.best = 0; ee = &e->next; } if (!n->routes) { DBG("%I/%d: deleting\n", n->n.prefix, n->n.pxlen); if (old_best) - { - krt_learn_announce_delete(p, n); - n->n.flags &= ~KRF_INSTALLED; - } + krt_learn_announce_delete(p, n); + FIB_ITERATE_PUT(&fit, f); fib_delete(fib, f); goto again; } + + best->u.krt.best = 1; *pbest = best->next; best->next = n->routes; n->routes = best; - if (best != old_best || !(n->n.flags & KRF_INSTALLED) || p->reload) + + if ((best != old_best) || p->reload) { DBG("%I/%d: announcing (metric=%d)\n", n->n.prefix, n->n.pxlen, best->u.krt.metric); krt_learn_announce_update(p, best); - n->n.flags |= KRF_INSTALLED; } else DBG("%I/%d: uptodate (metric=%d)\n", n->n.prefix, n->n.pxlen, best->u.krt.metric); @@ -514,31 +526,31 @@ krt_learn_async(struct krt_proto *p, rte *e, int new) best = n->routes; bestp = &n->routes; for(gg=&n->routes; g=*gg; gg=&g->next) + { if (best->u.krt.metric > g->u.krt.metric) { best = g; bestp = gg; } + + g->u.krt.best = 0; + } + if (best) { + best->u.krt.best = 1; *bestp = best->next; best->next = n->routes; n->routes = best; } + if (best != old_best) { DBG("krt_learn_async: distributing change\n"); if (best) - { - krt_learn_announce_update(p, best); - n->n.flags |= KRF_INSTALLED; - } + krt_learn_announce_update(p, best); else - { - n->routes = NULL; - krt_learn_announce_delete(p, n); - n->n.flags &= ~KRF_INSTALLED; - } + krt_learn_announce_delete(p, n); } } @@ -563,7 +575,7 @@ krt_dump(struct proto *P) static void krt_dump_attrs(rte *e) { - debug(" [m=%d,p=%d,t=%d]", e->u.krt.metric, e->u.krt.proto, e->u.krt.type); + debug(" [m=%d,p=%d]", e->u.krt.metric, e->u.krt.proto); } #endif From 205aa82fa23d05702ae11b4fa7fb09135e9955e1 Mon Sep 17 00:00:00 2001 From: Neil Jerram Date: Mon, 3 Oct 2016 15:30:17 +0100 Subject: [PATCH 3/3] Fix issue with broken tunnel routes after rebooting host Now interface equality for filtered routes with tunnel attribute is checked when the verdict for route action is being determined. See https://github.com/projectcalico/calico-containers/issues/947 This fix was developed by Aleks Kuznetsov @aleks-v-k and then just simplified and committed by me - see PR discussion at https://github.com/projectcalico/calico-bird/pull/21. --- sysdep/unix/krt.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/sysdep/unix/krt.c b/sysdep/unix/krt.c index 64cd4a6e..fb6ee92d 100644 --- a/sysdep/unix/krt.c +++ b/sysdep/unix/krt.c @@ -715,7 +715,9 @@ krt_got_route(struct krt_proto *p, rte *e) if (net->n.flags & KRF_INSTALLED) { rte *new, *rt_free; - ea_list *tmpa; + ea_list *tmpa = NULL; + eattr *ea = NULL; + struct iface* i = NULL; new = krt_export_net(p, net, &rt_free, &tmpa); @@ -726,7 +728,22 @@ krt_got_route(struct krt_proto *p, rte *e) else if ((net->n.flags & KRF_SYNC_ERROR) || !krt_same_dest(e, new)) verdict = KRF_UPDATE; else - verdict = KRF_SEEN; + { + /* Calico-BIRD specific: we check if the export filter set a tunnel + * attribute for the route. If it did, and the tunnel interface is + * different from the outgoing interface that is already programmed + * for the route, generate KRF_UPDATE here so that the route gets + * updated to have the tunnel as its outgoing interface. + */ + if (tmpa && + (e->attrs->dest == RTD_ROUTER) && + (ea = ea_find(tmpa, EA_KRT_TUNNEL)) && + (i = if_find_by_name(ea->u.ptr->data)) && + strcmp(i->name, e->attrs->iface->name)) + verdict = KRF_UPDATE; + else + verdict = KRF_SEEN; + } if (rt_free) rte_free(rt_free);