]> git.ipfire.org Git - thirdparty/bird.git/commitdiff
Nest: Removed rte_get_temp(), rte_update() now does its own copy.
authorJan Maria Matejka <mq@ucw.cz>
Fri, 9 Feb 2018 14:49:34 +0000 (15:49 +0100)
committerJan Maria Matejka <mq@ucw.cz>
Wed, 26 Sep 2018 14:23:28 +0000 (16:23 +0200)
This changes behaviour of rte_update() and rte_update2().

Formerly, the caller of rte_update2() took care of caching rta and
allocating rte; then rte_update2() freed the structures if it didn't use them.

Now rte_update2() does the permanent/global copies of rte+rta itself.
Allocation of rte+rta passed to rte_update2() is considered temporary
and the caller is also responsible to free these structures properly.
This is true for any kind of allocation: rte_update2() will always
allocate its own copy of the data passed (either really or just by
raising usecount in rta cache).

It is now possible (and in many protocols now used) to allocate rte+rta
on stack and pass them to rte_update(). It will deal with them correctly.

14 files changed:
nest/route.h
nest/rt-attr.c
nest/rt-dev.c
nest/rt-table.c
proto/babel/babel.c
proto/bgp/packets.c
proto/ospf/rt.c
proto/pipe/pipe.c
proto/rip/rip.c
proto/rpki/rpki.c
proto/static/static.c
sysdep/bsd/krt-sock.c
sysdep/linux/netlink.c
sysdep/unix/krt.c

index c0590f6e69c7607ad43acbafd83434df8dcfaf81..439ee23ff9d5c66d493a7380f82b703f7b9db4a1 100644 (file)
@@ -306,6 +306,7 @@ void rte_free(rte *);
 rte *rte_do_cow(rte *);
 static inline rte * rte_cow(rte *r) { return (r->flags & REF_COW) ? rte_do_cow(r) : r; }
 rte *rte_cow_rta(rte *r, linpool *lp);
+rte *rte_clone(rte *);
 void rt_dump(rtable *);
 void rt_dump_all(void);
 int rt_feed_channel(struct channel *c);
@@ -617,6 +618,7 @@ static inline size_t rta_size(const rta *a) { return sizeof(rta) + sizeof(u32)*a
 rta *rta_lookup(rta *);                        /* Get rta equivalent to this one, uc++ */
 static inline int rta_is_cached(rta *r) { return r->aflags & RTAF_CACHED; }
 static inline rta *rta_clone(rta *r) { r->uc++; return r; }
+int rta_same(rta *x, rta *y);
 void rta__free(rta *r);
 static inline void rta_free(rta *r) { if (r && !--r->uc) rta__free(r); }
 rta *rta_do_cow(rta *o, linpool *lp);
index 73ca47486dc28703de69847bbda336d441ba0c70..5bf1c885f8c0edf6027e6d5aed60b330868a01d4 100644 (file)
@@ -1062,7 +1062,7 @@ rta_hash(rta *a)
   return mem_hash_value(&h) ^ nexthop_hash(&(a->nh)) ^ ea_hash(a->eattrs);
 }
 
-static inline int
+int
 rta_same(rta *x, rta *y)
 {
   return (x->src == y->src &&
index 61f025cea8a28a974e9542dc579db83934f5184e..e33428025e06a5be0019ec4891f31b53cd1bdf8a 100644 (file)
@@ -71,9 +71,6 @@ dev_ifa_notify(struct proto *P, uint flags, struct ifa *ad)
     }
   else if (flags & IF_CHANGE_UP)
     {
-      rta *a;
-      rte *e;
-
       DBG("dev_if_notify: %s:%I going up\n", ad->iface->name, ad->ip);
 
       if (cf->check_link && !(ad->iface->flags & IF_LINK_UP))
@@ -90,10 +87,12 @@ dev_ifa_notify(struct proto *P, uint flags, struct ifa *ad)
        .nh.iface = ad->iface,
       };
 
-      a = rta_lookup(&a0);
-      e = rte_get_temp(a);
-      e->pflags = 0;
-      rte_update2(c, net, e, src);
+      rte e = {
+       .pflags = 0,
+       .attrs = &a0,
+      };
+
+      rte_update2(c, net, &e, src);
     }
 }
 
index 5f6f5720d09cd6542c8651d9f1bd9f90468d7b0c..85ba1d8b4328e9fa73c421b9133e2ad7e40d12a6 100644 (file)
@@ -261,34 +261,33 @@ rte_find(net *net, struct rte_src *src)
   return e;
 }
 
-/**
- * rte_get_temp - get a temporary &rte
- * @a: attributes to assign to the new route (a &rta; in case it's
- * un-cached, rte_update() will create a cached copy automatically)
- *
- * Create a temporary &rte and bind it with the attributes @a.
- * Also set route preference to the default preference set for
- * the protocol.
- */
 rte *
-rte_get_temp(rta *a)
+rte_do_cow(rte *r)
 {
   rte *e = sl_alloc(rte_slab);
 
-  e->attrs = a;
+  memcpy(e, r, sizeof(rte));
+  e->attrs = rta_clone(r->attrs);
   e->flags = 0;
-  e->pref = 0;
   return e;
 }
 
+/**
+ * rte_clone - do a complete copy of given rte
+ * @e: route to clone
+ */
+
 rte *
-rte_do_cow(rte *r)
+rte_clone(rte *r)
 {
   rte *e = sl_alloc(rte_slab);
-
   memcpy(e, r, sizeof(rte));
-  e->attrs = rta_clone(r->attrs);
-  e->flags = 0;
+
+  if (rta_is_cached(r->attrs))
+    e->attrs = rta_clone(r->attrs);
+  else
+    e->attrs = rta_lookup(r->attrs);
+
   return e;
 }
 
@@ -1274,11 +1273,8 @@ rte_unhide_dummy_routes(net *net, rte **dummy)
  *
  * This function is called by the routing protocols whenever they discover
  * a new route or wish to update/remove an existing route. The right announcement
- * sequence is to build route attributes first (either un-cached with @aflags set
- * to zero or a cached one using rta_lookup(); in this case please note that
- * you need to increase the use count of the attributes yourself by calling
- * rta_clone()), call rte_get_temp() to obtain a temporary &rte, fill in all
- * the appropriate data and finally submit the new &rte by calling rte_update().
+ * sequence is to build route attributes first, allocate a temporary &rte, fill in all
+ * the appropriate data and submit the new &rte by calling rte_update().
  *
  * @src specifies the protocol that originally created the route and the meaning
  * of protocol-dependent data of @new. If @new is not %NULL, @src have to be the
@@ -1367,9 +1363,8 @@ rte_update2(struct channel *c, const net_addr *n, rte *new, struct rte_src *src)
                src->proto->store_tmp_attrs(new);
            }
        }
-      if (!rta_is_cached(new->attrs)) /* Need to copy attributes */
-       new->attrs = rta_lookup(new->attrs);
-      new->flags |= REF_COW;
+
+      new = rte_clone(new);
     }
   else
     {
@@ -1391,7 +1386,6 @@ rte_update2(struct channel *c, const net_addr *n, rte *new, struct rte_src *src)
   return;
 
  drop:
-  rte_free(new);
   new = NULL;
   goto recalc;
 }
index afa482bb9254d48e1a1bd549999bd1893bb32576..2ad4a65feeff888a66b26565fa59a4c179e23fce 100644 (file)
@@ -640,15 +640,18 @@ babel_announce_rte(struct babel_proto *p, struct babel_entry *e)
       .nh.iface = r->neigh->ifa->iface,
     };
 
-    rta *a = rta_lookup(&a0);
-    rte *rte = rte_get_temp(a);
-    rte->u.babel.seqno = r->seqno;
-    rte->u.babel.metric = r->metric;
-    rte->u.babel.router_id = r->router_id;
-    rte->pflags = 0;
+    rte rte = {
+      .attrs = &a0,
+      .u.babel = {
+         .seqno = r->seqno,
+         .metric = r->metric,
+         .router_id = r->router_id,
+      },
+      .pflags = 0,
+    };
 
     e->unreachable = 0;
-    rte_update2(c, e->n.addr, rte, p->p.main_source);
+    rte_update2(c, e->n.addr, &rte, p->p.main_source);
   }
   else if (e->valid && (e->router_id != p->router_id))
   {
@@ -660,14 +663,15 @@ babel_announce_rte(struct babel_proto *p, struct babel_entry *e)
       .dest = RTD_UNREACHABLE,
     };
 
-    rta *a = rta_lookup(&a0);
-    rte *rte = rte_get_temp(a);
-    memset(&rte->u.babel, 0, sizeof(rte->u.babel));
-    rte->pflags = 0;
-    rte->pref = 1;
+    rte rte = {
+      .attrs = &a0,
+      .u.babel = {},
+      .pflags = 0,
+      .pref = 1,
+    };
 
     e->unreachable = 1;
-    rte_update2(c, e->n.addr, rte, p->p.main_source);
+    rte_update2(c, e->n.addr, &rte, p->p.main_source);
   }
   else
   {
index dc65ec9e9ad345588c8afa32047c772182faabe0..d81955d866a51680b8de7796916530dd3ad3378b 100644 (file)
@@ -1190,17 +1190,18 @@ bgp_rte_update(struct bgp_parse_state *s, net_addr *n, u32 path_id, rta *a0)
 
     /* Workaround for rta_lookup() breaking eattrs */
     ea_list *ea = a0->eattrs;
-    s->cached_rta = rta_lookup(a0);
+    s->cached_rta = rta_lookup(a0); /* This rta use is finally freed in bgp_decode_nlri() */
     a0->eattrs = ea;
   }
 
-  rta *a = rta_clone(s->cached_rta);
-  rte *e = rte_get_temp(a);
+  rte e = {
+    .attrs = s->cached_rta,
+    .pflags = 0,
+    .u.bgp.suppressed = 0,
+    .u.bgp.stale = -1,
+  };
 
-  e->pflags = 0;
-  e->u.bgp.suppressed = 0;
-  e->u.bgp.stale = -1;
-  rte_update2(&s->channel->c, n, e, s->last_src);
+  rte_update2(&s->channel->c, n, &e, s->last_src);
 }
 
 static void
@@ -2285,7 +2286,7 @@ bgp_decode_nlri(struct bgp_parse_state *s, u32 afi, byte *nlri, uint len, ea_lis
 
   c->desc->decode_nlri(s, nlri, len, a);
 
-  rta_free(s->cached_rta);
+  rta_free(s->cached_rta); /* Freeing the rta used in bgp_rte_update */
   s->cached_rta = NULL;
 }
 
index 4549ce2a227e0203614c2643a36e97f49b345964..97ba5c6adb4ed15f30218a4c20902e2424c47b3f 100644 (file)
@@ -2013,19 +2013,25 @@ again1:
       if (reload || ort_changed(nf, &a0))
       {
        rta *a = rta_lookup(&a0);
-       rte *e = rte_get_temp(a);
 
        rta_free(nf->old_rta);
        nf->old_rta = rta_clone(a);
-       e->u.ospf.metric1 = nf->old_metric1 = nf->n.metric1;
-       e->u.ospf.metric2 = nf->old_metric2 = nf->n.metric2;
-       e->u.ospf.tag = nf->old_tag = nf->n.tag;
-       e->u.ospf.router_id = nf->old_rid = nf->n.rid;
-       e->pflags = 0;
+
+       rte e = {
+         .attrs = a,
+         .u.ospf = {
+           .metric1 = nf->old_metric1 = nf->n.metric1,
+           .metric2 = nf->old_metric2 = nf->n.metric2,
+           .tag = nf->old_tag = nf->n.tag,
+           .router_id = nf->old_rid = nf->n.rid,
+         },
+         .pflags = 0,
+       };
 
        DBG("Mod rte type %d - %N via %I on iface %s, met %d\n",
            a0.source, nf->fn.addr, a0.gw, a0.iface ? a0.iface->name : "(none)", nf->n.metric1);
-       rte_update(&p->p, nf->fn.addr, e);
+       rte_update(&p->p, nf->fn.addr, &e);
+       rta_free(a);
       }
     }
     else if (nf->old_rta)
index 82ccf38a38bf55941ecc66dd45c49c126a170e31..38e1394bf03649789ef7c8f60d5a559b68435403 100644 (file)
@@ -50,6 +50,7 @@ pipe_rt_notify(struct proto *P, struct channel *src_ch, net *n, rte *new, rte *o
   struct channel *dst = (src_ch == p->pri) ? p->sec : p->pri;
   struct rte_src *src;
 
+  rte ee = {};
   rte *e;
   rta *a;
 
@@ -70,8 +71,10 @@ pipe_rt_notify(struct proto *P, struct channel *src_ch, net *n, rte *new, rte *o
 
       a->aflags = 0;
       a->hostentry = NULL;
-      e = rte_get_temp(a);
+
+      e = &ee;
       e->pflags = 0;
+      e->attrs = a;
 
       /* Copy protocol specific embedded attributes. */
       memcpy(&(e->u), &(new->u), sizeof(e->u));
index 2838d425b1793b36bf61f821ea79b6536919d808..9a5d021b4efc6b6c16a8fba71db65b5250030a6a 100644 (file)
@@ -187,16 +187,17 @@ rip_announce_rte(struct rip_proto *p, struct rip_entry *en)
       a0.nh.iface = rt->from->nbr->iface;
     }
 
-    rta *a = rta_lookup(&a0);
-    rte *e = rte_get_temp(a);
-
-    e->u.rip.from = a0.nh.iface;
-    e->u.rip.metric = rt_metric;
-    e->u.rip.tag = rt_tag;
-
-    e->pflags = 0;
+    rte e = {
+      .attrs = &a0,
+      .u.rip = {
+       .from = a0.nh.iface,
+       .metric = rt_metric,
+       .tag = rt_tag,
+      },
+      .pflags = 0,
+    };
 
-    rte_update(&p->p, en->n.addr, e);
+    rte_update(&p->p, en->n.addr, &e);
   }
   else
   {
index 36097dc515fa7ac295c2fec40dbeea23fecc5caf..9f244d1e06be4321c3865635dd0f59d4965a4254 100644 (file)
@@ -126,13 +126,13 @@ rpki_table_add_roa(struct rpki_cache *cache, struct channel *channel, const net_
     .scope = SCOPE_UNIVERSE,
     .dest = RTD_NONE,
   };
+  
+  rte e = {
+    .attrs = &a0,
+    .pflags = 0,
+  };
 
-  rta *a = rta_lookup(&a0);
-  rte *e = rte_get_temp(a);
-
-  e->pflags = 0;
-
-  rte_update2(channel, &pfxr->n, e, a0.src);
+  rte_update2(channel, &pfxr->n, &e, a0.src);
 }
 
 void
index 6984db64fdf3d080898758d1dfa0df336493bf69..a3bbbcfa9c92ffe62b1058a1086a102db4c0d00e 100644 (file)
@@ -98,14 +98,17 @@ static_announce_rte(struct static_proto *p, struct static_route *r)
   if (r->state == SRS_CLEAN)
     return;
 
-  /* We skip rta_lookup() here */
-  rte *e = rte_get_temp(a);
-  e->pflags = 0;
+  rte e = {
+    .attrs = a,
+    .pflags = 0
+  };
+  
+  rte *ep = &e;
 
   if (r->cmds)
-    f_eval_rte(r->cmds, &e, static_lp);
+    f_eval_rte(r->cmds, &ep, static_lp);
 
-  rte_update(&p->p, r->net, e);
+  rte_update(&p->p, r->net, ep);
   r->state = SRS_CLEAN;
 
   if (r->cmds)
index e646c41429741ae1b88a3d1545a5f16c6121e123..b9fb4769cf813dcd695232c34b4cc80f98ab2c11 100644 (file)
@@ -371,7 +371,6 @@ krt_read_route(struct ks_msg *msg, struct krt_proto *p, int scan)
   /* p is NULL iff KRT_SHARED_SOCKET and !scan */
 
   int ipv6;
-  rte *e;
   net *net;
   sockaddr dst, gate, mask;
   ip_addr idst, igate, imask;
@@ -497,6 +496,15 @@ krt_read_route(struct ks_msg *msg, struct krt_proto *p, int scan)
     .scope = SCOPE_UNIVERSE,
   };
 
+  rte e = {
+    .attrs = &a,
+    .net = net,
+    .u.krt = {
+      .src = src,
+      .proto = src2,
+    },
+  };
+
   /* reject/blackhole routes have also set RTF_GATEWAY,
      we wil check them first. */
 
@@ -547,18 +555,10 @@ krt_read_route(struct ks_msg *msg, struct krt_proto *p, int scan)
   }
 
  done:
-  e = rte_get_temp(&a);
-  e->net = net;
-  e->u.krt.src = src;
-  e->u.krt.proto = src2;
-  e->u.krt.seen = 0;
-  e->u.krt.best = 0;
-  e->u.krt.metric = 0;
-
   if (scan)
-    krt_got_route(p, e);
+    krt_got_route(p, &e);
   else
-    krt_got_route_async(p, e, new);
+    krt_got_route_async(p, &e, new);
 }
 
 static void
index 834504d07b5d2ee4c81740c31c6084e6b922afd8..1f47687bd24f548e6fc78208ead44eab589ee25c 100644 (file)
@@ -1413,18 +1413,22 @@ nl_mergable_route(struct nl_parse_state *s, net *net, struct krt_proto *p, uint
 static void
 nl_announce_route(struct nl_parse_state *s)
 {
-  rte *e = rte_get_temp(s->attrs);
-  e->net = s->net;
-  e->u.krt.src = s->krt_src;
-  e->u.krt.proto = s->krt_proto;
-  e->u.krt.seen = 0;
-  e->u.krt.best = 0;
-  e->u.krt.metric = s->krt_metric;
+  rte e = {
+    .attrs = s->attrs,
+    .net = s->net,
+    .u.krt = {
+      .src = s->krt_src,
+      .proto = s->krt_proto,
+      .seen = 0,
+      .best = 0,
+      .metric = s->krt_metric,
+    },
+  };
 
   if (s->scan)
-    krt_got_route(s->proto, e);
+    krt_got_route(s->proto, &e);
   else
-    krt_got_route_async(s->proto, e, s->new);
+    krt_got_route_async(s->proto, &e, s->new);
 
   s->net = NULL;
   s->attrs = NULL;
index 794ebcd014b47e3a5890fb7cbdbd12fd720cecd5..0638654f048b588c7de39771fe56e1fe72022bbb 100644 (file)
@@ -286,7 +286,7 @@ krt_same_key(rte *a, rte *b)
 static inline int
 krt_uptodate(rte *a, rte *b)
 {
-  if (a->attrs != b->attrs)
+  if (!rta_same(a->attrs, b->attrs))
     return 0;
 
   if (a->u.krt.proto != b->u.krt.proto)
@@ -298,12 +298,7 @@ krt_uptodate(rte *a, rte *b)
 static void
 krt_learn_announce_update(struct krt_proto *p, rte *e)
 {
-  net *n = e->net;
-  rta *aa = rta_clone(e->attrs);
-  rte *ee = rte_get_temp(aa);
-  ee->pflags = 0;
-  ee->u.krt = e->u.krt;
-  rte_update(&p->p, n->n.addr, ee);
+  rte_update(&p->p, e->net->n.addr, e);
 }
 
 static void
@@ -312,7 +307,8 @@ krt_learn_announce_delete(struct krt_proto *p, net *n)
   rte_update(&p->p, n->n.addr, NULL);
 }
 
-/* Called when alien route is discovered during scan */
+/* Called when alien route is discovered during scan.
+ * The route is a temporary rte. */
 static void
 krt_learn_scan(struct krt_proto *p, rte *e)
 {
@@ -320,17 +316,15 @@ krt_learn_scan(struct krt_proto *p, rte *e)
   net *n = net_get(&p->krt_table, n0->n.addr);
   rte *m, **mm;
 
-  e->attrs = rta_lookup(e->attrs);
-
   for(mm=&n->routes; m = *mm; mm=&m->next)
     if (krt_same_key(m, e))
       break;
-  if (m)
+
+  if (m) /* Route found */
     {
       if (krt_uptodate(m, e))
        {
          krt_trace_in_rl(&rl_alien, p, e, "[alien] seen");
-         rte_free(e);
          m->u.krt.seen = 1;
        }
       else
@@ -343,11 +337,13 @@ krt_learn_scan(struct krt_proto *p, rte *e)
     }
   else
     krt_trace_in(p, e, "[alien] created");
-  if (!m)
+
+  if (!m) /* Route created or updated -> inserting the new route. */
     {
-      e->next = n->routes;
-      n->routes = e;
-      e->u.krt.seen = 1;
+      rte *ee = rte_clone(e);
+      ee->next = n->routes;
+      n->routes = ee;
+      ee->u.krt.seen = 1;
     }
 }
 
@@ -426,6 +422,11 @@ again:
   p->reload = 0;
 }
 
+/*
+ * This is called when the low-level code learns a route asynchronously.
+ * The given route is considered temporary.
+ */
+
 static void
 krt_learn_async(struct krt_proto *p, rte *e, int new)
 {
@@ -433,8 +434,6 @@ krt_learn_async(struct krt_proto *p, rte *e, int new)
   net *n = net_get(&p->krt_table, n0->n.addr);
   rte *g, **gg, *best, **bestp, *old_best;
 
-  e->attrs = rta_lookup(e->attrs);
-
   old_best = n->routes;
   for(gg=&n->routes; g = *gg; gg = &g->next)
     if (krt_same_key(g, e))
@@ -446,7 +445,6 @@ krt_learn_async(struct krt_proto *p, rte *e, int new)
          if (krt_uptodate(g, e))
            {
              krt_trace_in(p, e, "[alien async] same");
-             rte_free(e);
              return;
            }
          krt_trace_in(p, e, "[alien async] updated");
@@ -456,20 +454,19 @@ krt_learn_async(struct krt_proto *p, rte *e, int new)
       else
        krt_trace_in(p, e, "[alien async] created");
 
+      e = rte_clone(e);
       e->next = n->routes;
       n->routes = e;
     }
   else if (!g)
     {
       krt_trace_in(p, e, "[alien async] delete failed");
-      rte_free(e);
       return;
     }
   else
     {
       krt_trace_in(p, e, "[alien async] removed");
       *gg = g->next;
-      rte_free(e);
       rte_free(g);
     }
   best = n->routes;
@@ -639,10 +636,7 @@ krt_got_route(struct krt_proto *p, rte *e)
       if (KRT_CF->learn)
        krt_learn_scan(p, e);
       else
-       {
-         krt_trace_in_rl(&rl_alien, p, e, "[alien] ignored");
-         rte_free(e);
-       }
+       krt_trace_in_rl(&rl_alien, p, e, "[alien] ignored");
       return;
     }
 #endif
@@ -652,7 +646,6 @@ krt_got_route(struct krt_proto *p, rte *e)
     {
       /* Route to this destination was already seen. Strange, but it happens... */
       krt_trace_in(p, e, "already seen");
-      rte_free(e);
       return;
     }
 
@@ -691,15 +684,12 @@ krt_got_route(struct krt_proto *p, rte *e)
   net->n.flags = (net->n.flags & ~KRF_VERDICT_MASK) | verdict;
   if (verdict == KRF_UPDATE || verdict == KRF_DELETE)
     {
-      /* Get a cached copy of attributes and temporarily link the route */
-      rta *a = e->attrs;
-      a->source = RTS_DUMMY;
-      e->attrs = rta_lookup(a);
-      e->next = net->routes;
-      net->routes = e;
+      /* Get a cached copy of route and temporarily link it */
+      e->attrs->source = RTS_DUMMY;
+      rte *ee = rte_clone(e);
+      ee->next = net->routes;
+      net->routes = ee;
     }
-  else
-    rte_free(e);
 }
 
 static void
@@ -776,6 +766,11 @@ krt_prune(struct krt_proto *p)
     p->initialized = 1;
 }
 
+/*
+ * This is called when the low-level code gets a route asynchronously.
+ * The given route is considered temporary.
+ */
+
 void
 krt_got_route_async(struct krt_proto *p, rte *e, int new)
 {
@@ -805,7 +800,6 @@ krt_got_route_async(struct krt_proto *p, rte *e, int new)
        }
 #endif
     }
-  rte_free(e);
 }
 
 /*