From: Jan Maria Matejka <mq@ucw.cz>
Date: Fri, 9 Feb 2018 14:49:34 +0000 (+0100)
Subject: Nest: Removed rte_get_temp(), rte_update() now does its own copy.
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=54ac17dc136ed9acccb797c56ed91f6ffed196e2;p=thirdparty%2Fbird.git

Nest: Removed rte_get_temp(), rte_update() now does its own copy.

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.
---

diff --git a/nest/route.h b/nest/route.h
index c0590f6e6..439ee23ff 100644
--- a/nest/route.h
+++ b/nest/route.h
@@ -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);
diff --git a/nest/rt-attr.c b/nest/rt-attr.c
index 73ca47486..5bf1c885f 100644
--- a/nest/rt-attr.c
+++ b/nest/rt-attr.c
@@ -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 &&
diff --git a/nest/rt-dev.c b/nest/rt-dev.c
index 61f025cea..e33428025 100644
--- a/nest/rt-dev.c
+++ b/nest/rt-dev.c
@@ -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);
     }
 }
 
diff --git a/nest/rt-table.c b/nest/rt-table.c
index 5f6f5720d..85ba1d8b4 100644
--- a/nest/rt-table.c
+++ b/nest/rt-table.c
@@ -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;
 }
diff --git a/proto/babel/babel.c b/proto/babel/babel.c
index afa482bb9..2ad4a65fe 100644
--- a/proto/babel/babel.c
+++ b/proto/babel/babel.c
@@ -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
   {
diff --git a/proto/bgp/packets.c b/proto/bgp/packets.c
index dc65ec9e9..d81955d86 100644
--- a/proto/bgp/packets.c
+++ b/proto/bgp/packets.c
@@ -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;
 }
 
diff --git a/proto/ospf/rt.c b/proto/ospf/rt.c
index 4549ce2a2..97ba5c6ad 100644
--- a/proto/ospf/rt.c
+++ b/proto/ospf/rt.c
@@ -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)
diff --git a/proto/pipe/pipe.c b/proto/pipe/pipe.c
index 82ccf38a3..38e1394bf 100644
--- a/proto/pipe/pipe.c
+++ b/proto/pipe/pipe.c
@@ -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));
diff --git a/proto/rip/rip.c b/proto/rip/rip.c
index 2838d425b..9a5d021b4 100644
--- a/proto/rip/rip.c
+++ b/proto/rip/rip.c
@@ -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
   {
diff --git a/proto/rpki/rpki.c b/proto/rpki/rpki.c
index 36097dc51..9f244d1e0 100644
--- a/proto/rpki/rpki.c
+++ b/proto/rpki/rpki.c
@@ -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
diff --git a/proto/static/static.c b/proto/static/static.c
index 6984db64f..a3bbbcfa9 100644
--- a/proto/static/static.c
+++ b/proto/static/static.c
@@ -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)
diff --git a/sysdep/bsd/krt-sock.c b/sysdep/bsd/krt-sock.c
index e646c4142..b9fb4769c 100644
--- a/sysdep/bsd/krt-sock.c
+++ b/sysdep/bsd/krt-sock.c
@@ -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
diff --git a/sysdep/linux/netlink.c b/sysdep/linux/netlink.c
index 834504d07..1f47687bd 100644
--- a/sysdep/linux/netlink.c
+++ b/sysdep/linux/netlink.c
@@ -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;
diff --git a/sysdep/unix/krt.c b/sysdep/unix/krt.c
index 794ebcd01..0638654f0 100644
--- a/sysdep/unix/krt.c
+++ b/sysdep/unix/krt.c
@@ -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);
 }
 
 /*