From de86040b2cf4ec9bfbb64f0e208a19d4d7e51adc Mon Sep 17 00:00:00 2001
From: Maria Matejka <mq@ucw.cz>
Date: Sun, 10 Apr 2022 14:11:46 +0200
Subject: [PATCH] Attribute list normalization cleanup

---
 filter/f-inst.c   |  8 ++----
 lib/attrs.h       | 10 ++++++++
 lib/birdlib.h     |  1 +
 lib/route.h       | 18 ++++---------
 nest/rt-attr.c    | 64 +++++++++++++++++++++++++++++++----------------
 nest/rt-show.c    |  2 +-
 proto/bgp/attrs.c | 47 ++++++++--------------------------
 proto/mrt/mrt.c   |  2 +-
 8 files changed, 73 insertions(+), 79 deletions(-)

diff --git a/filter/f-inst.c b/filter/f-inst.c
index e0bad833..7b3db1c7 100644
--- a/filter/f-inst.c
+++ b/filter/f-inst.c
@@ -717,12 +717,8 @@
 	runtime( "Setting opaque attribute is not allowed" );
 	break;
 
-      case T_IP:;
-	int len = sizeof(ip_addr);
-	struct adata *ad = lp_alloc(fs->pool, sizeof(struct adata) + len);
-	ad->length = len;
-	(* (ip_addr *) ad->data) = v1.val.ip;
-	l->attrs[0].u.ptr = ad;
+      case T_IP:
+	l->attrs[0].u.ptr = lp_store_adata(fs->pool, &v1.val.ip, sizeof(ip_addr));
 	break;
 
       default:
diff --git a/lib/attrs.h b/lib/attrs.h
index 97a9630f..fcb70230 100644
--- a/lib/attrs.h
+++ b/lib/attrs.h
@@ -17,6 +17,8 @@ typedef struct adata {
   byte data[0];
 } adata;
 
+#define ADATA_SIZE(s)	BIRD_CPU_ALIGN(sizeof(struct adata) + s)
+
 extern const adata null_adata;		/* adata of length 0 */
 
 static inline struct adata *
@@ -27,6 +29,14 @@ lp_alloc_adata(struct linpool *pool, uint len)
   return ad;
 }
 
+static inline struct adata *
+lp_store_adata(struct linpool *pool, const void *buf, uint len)
+{
+  struct adata *ad = lp_alloc_adata(pool, len);
+  memcpy(ad->data, buf, len);
+  return ad;
+}
+
 static inline int adata_same(const struct adata *a, const struct adata *b)
 { return (a->length == b->length && !memcmp(a->data, b->data, a->length)); }
 
diff --git a/lib/birdlib.h b/lib/birdlib.h
index 81d4908a..6f0bab96 100644
--- a/lib/birdlib.h
+++ b/lib/birdlib.h
@@ -19,6 +19,7 @@ struct align_probe { char x; long int y; };
 #define SKIP_BACK(s, i, p) ((s *)((char *)p - OFFSETOF(s, i)))
 #define BIRD_ALIGN(s, a) (((s)+a-1)&~(a-1))
 #define CPU_STRUCT_ALIGN (sizeof(struct align_probe))
+#define BIRD_CPU_ALIGN(s) BIRD_ALIGN((s), CPU_STRUCT_ALIGN)
 
 /* Utility macros */
 
diff --git a/lib/route.h b/lib/route.h
index 8e60f749..eda5b9ad 100644
--- a/lib/route.h
+++ b/lib/route.h
@@ -204,24 +204,16 @@ ea_get_int(ea_list *e, unsigned id, u32 def)
 }
 
 void ea_dump(ea_list *);
-void ea_sort(ea_list *);		/* Sort entries in all sub-lists */
-unsigned ea_scan(ea_list *);		/* How many bytes do we need for merged ea_list */
-void ea_merge(ea_list *from, ea_list *to); /* Merge sub-lists to allocated buffer */
 int ea_same(ea_list *x, ea_list *y);	/* Test whether two ea_lists are identical */
 uint ea_hash(ea_list *e);	/* Calculate 16-bit hash value */
 ea_list *ea_append(ea_list *to, ea_list *what);
 void ea_format_bitfield(const struct eattr *a, byte *buf, int bufsize, const char **names, int min, int max);
 
-#define ea_normalize(ea) do { \
-  if (ea->next) { \
-    ea_list *t = alloca(ea_scan(ea)); \
-    ea_merge(ea, t); \
-    ea = t; \
-  } \
-  ea_sort(ea); \
-  if (ea->count == 0) \
-    ea = NULL; \
-} while(0) \
+/* Normalize ea_list; allocates the result from tmp_linpool */
+ea_list *ea_normalize(const ea_list *e);
+
+uint ea_list_size(ea_list *);
+void ea_list_copy(ea_list *dest, ea_list *src, uint size);
 
 struct ea_one_attr_list {
   ea_list l;
diff --git a/nest/rt-attr.c b/nest/rt-attr.c
index 2fa9b673..afa95eec 100644
--- a/nest/rt-attr.c
+++ b/nest/rt-attr.c
@@ -626,7 +626,7 @@ ea_do_prune(ea_list *e)
  * If an attribute occurs multiple times in a single &ea_list,
  * ea_sort() leaves only the first (the only significant) occurrence.
  */
-void
+static void
 ea_sort(ea_list *e)
 {
   while (e)
@@ -650,8 +650,8 @@ ea_sort(ea_list *e)
  * This function calculates an upper bound of the size of
  * a given &ea_list after merging with ea_merge().
  */
-unsigned
-ea_scan(ea_list *e)
+static unsigned
+ea_scan(const ea_list *e)
 {
   unsigned cnt = 0;
 
@@ -677,8 +677,8 @@ ea_scan(ea_list *e)
  * segments with ea_merge() and finally sort and prune the result
  * by calling ea_sort().
  */
-void
-ea_merge(ea_list *e, ea_list *t)
+static void
+ea_merge(const ea_list *e, ea_list *t)
 {
   eattr *d = t->attrs;
 
@@ -694,6 +694,16 @@ ea_merge(ea_list *e, ea_list *t)
     }
 }
 
+ea_list *
+ea_normalize(const ea_list *e)
+{
+  ea_list *t = tmp_alloc(ea_scan(e));
+  ea_merge(e, t);
+  ea_sort(t);
+
+  return t->count ? t : NULL;
+}
+
 /**
  * ea_same - compare two &ea_list's
  * @x: attribute list
@@ -729,33 +739,38 @@ ea_same(ea_list *x, ea_list *y)
   return 1;
 }
 
-static inline ea_list *
-ea_list_copy(ea_list *o)
+uint
+ea_list_size(ea_list *o)
 {
-  ea_list *n;
-  unsigned i, adpos, elen;
+  unsigned i, elen;
 
-  if (!o)
-    return NULL;
-  ASSERT(!o->next);
-  elen = adpos = sizeof(ea_list) + sizeof(eattr) * o->count;
+  ASSERT_DIE(o);
+  ASSERT_DIE(!o->next);
+  elen = BIRD_CPU_ALIGN(sizeof(ea_list) + sizeof(eattr) * o->count);
 
   for(i=0; i<o->count; i++)
     {
       eattr *a = &o->attrs[i];
       if (!(a->type & EAF_EMBEDDED))
-	elen += sizeof(struct adata) + a->u.ptr->length;
+	elen += ADATA_SIZE(a->u.ptr->length);
     }
 
-  n = mb_alloc(rta_pool, elen);
+  return elen;
+}
+
+void
+ea_list_copy(ea_list *n, ea_list *o, uint elen)
+{
+  uint adpos = sizeof(ea_list) + sizeof(eattr) * o->count;
   memcpy(n, o, adpos);
-  n->flags |= EALF_CACHED;
-  for(i=0; i<o->count; i++)
+  adpos = BIRD_CPU_ALIGN(adpos);
+
+  for(uint i=0; i<o->count; i++)
     {
       eattr *a = &n->attrs[i];
       if (!(a->type & EAF_EMBEDDED))
 	{
-	  unsigned size = sizeof(struct adata) + a->u.ptr->length;
+	  unsigned size = ADATA_SIZE(a->u.ptr->length);
 	  ASSERT_DIE(adpos + size <= elen);
 
 	  struct adata *d = ((void *) n) + adpos;
@@ -765,8 +780,8 @@ ea_list_copy(ea_list *o)
 	  adpos += size;
 	}
     }
+
   ASSERT_DIE(adpos == elen);
-  return n;
 }
 
 static inline void
@@ -1029,6 +1044,7 @@ ea_hash(ea_list *e)
 
   if (e)			/* Assuming chain of length 1 */
     {
+      ASSERT_DIE(!e->next);
       for(i=0; i<e->count; i++)
 	{
 	  struct eattr *a = &e->attrs[i];
@@ -1135,7 +1151,13 @@ rta_copy(rta *o)
   memcpy(r, o, rta_size(o));
   r->uc = 1;
   r->nh.next = nexthop_copy(o->nh.next);
-  r->eattrs = ea_list_copy(o->eattrs);
+  if (!r->eattrs)
+    return r;
+
+  uint elen = ea_list_size(o->eattrs);
+  r->eattrs = mb_alloc(rta_pool, elen);
+  ea_list_copy(r->eattrs, o->eattrs, elen);
+  r->eattrs->flags |= EALF_CACHED;
   return r;
 }
 
@@ -1191,7 +1213,7 @@ rta_lookup(rta *o)
 
   ASSERT(!o->cached);
   if (o->eattrs)
-    ea_normalize(o->eattrs);
+    o->eattrs = ea_normalize(o->eattrs);
 
   h = rta_hash(o);
   for(r=rta_hash_table[h & rta_cache_mask]; r; r=r->next)
diff --git a/nest/rt-show.c b/nest/rt-show.c
index 464e5f1b..4e0c5602 100644
--- a/nest/rt-show.c
+++ b/nest/rt-show.c
@@ -55,7 +55,7 @@ rt_show_rte(struct cli *c, byte *ia, rte *e, struct rt_show_data *d, int primary
 
   /* Need to normalize the extended attributes */
   if (d->verbose && !rta_is_cached(a) && a->eattrs)
-    ea_normalize(a->eattrs);
+    a->eattrs = ea_normalize(a->eattrs);
 
   get_route_info = e->src->proto->proto->get_route_info;
   if (get_route_info)
diff --git a/proto/bgp/attrs.c b/proto/bgp/attrs.c
index 2c0d011f..1c49e76a 100644
--- a/proto/bgp/attrs.c
+++ b/proto/bgp/attrs.c
@@ -1191,12 +1191,11 @@ bgp_export_attr(struct bgp_export_state *s, eattr *a, ea_list *to)
  * Result: one sorted attribute list segment, or NULL if attributes are unsuitable.
  */
 static inline ea_list *
-bgp_export_attrs(struct bgp_export_state *s, ea_list *attrs)
+bgp_export_attrs(struct bgp_export_state *s, const ea_list *a)
 {
   /* Merge the attribute list */
-  ea_list *new = lp_alloc(s->pool, ea_scan(attrs));
-  ea_merge(attrs, new);
-  ea_sort(new);
+  ea_list *new = ea_normalize(a);
+  ASSERT_DIE(new);
 
   uint i, count;
   count = new->count;
@@ -1498,6 +1497,7 @@ bgp_free_bucket_table(struct bgp_channel *c)
 static struct bgp_bucket *
 bgp_get_bucket(struct bgp_channel *c, ea_list *new)
 {
+
   /* Hash and lookup */
   u32 hash = ea_hash(new);
   struct bgp_bucket *b = HASH_FIND(c->bucket_hash, RBH, new, hash);
@@ -1505,45 +1505,18 @@ bgp_get_bucket(struct bgp_channel *c, ea_list *new)
   if (b)
     return b;
 
-  uint ea_size = sizeof(ea_list) + new->count * sizeof(eattr);
-  uint ea_size_aligned = BIRD_ALIGN(ea_size, CPU_STRUCT_ALIGN);
-  uint size = sizeof(struct bgp_bucket) + ea_size_aligned;
-  uint i;
-  byte *dest;
+  /* Scan the list for total size */
+  uint ea_size = BIRD_CPU_ALIGN(ea_list_size(new));
+  uint size = sizeof(struct bgp_bucket) + ea_size;
 
-  /* Gather total size of non-inline attributes */
-  for (i = 0; i < new->count; i++)
-  {
-    eattr *a = &new->attrs[i];
-
-    if (!(a->type & EAF_EMBEDDED))
-      size += BIRD_ALIGN(sizeof(struct adata) + a->u.ptr->length, CPU_STRUCT_ALIGN);
-  }
-
-  /* Create the bucket */
+  /* Allocate the bucket */
   b = mb_alloc(c->pool, size);
   *b = (struct bgp_bucket) { };
   init_list(&b->prefixes);
   b->hash = hash;
 
-  /* Copy list of extended attributes */
-  memcpy(b->eattrs, new, ea_size);
-  dest = ((byte *) b->eattrs) + ea_size_aligned;
-
-  /* Copy values of non-inline attributes */
-  for (i = 0; i < new->count; i++)
-  {
-    eattr *a = &b->eattrs->attrs[i];
-
-    if (!(a->type & EAF_EMBEDDED))
-    {
-      const struct adata *oa = a->u.ptr;
-      struct adata *na = (struct adata *) dest;
-      memcpy(na, oa, sizeof(struct adata) + oa->length);
-      a->u.ptr = na;
-      dest += BIRD_ALIGN(sizeof(struct adata) + na->length, CPU_STRUCT_ALIGN);
-    }
-  }
+  /* Copy the ea_list */
+  ea_list_copy(b->eattrs, new, ea_size);
 
   /* Insert the bucket to send queue and bucket hash */
   add_tail(&c->bucket_queue, &b->send_node);
diff --git a/proto/mrt/mrt.c b/proto/mrt/mrt.c
index 321c6395..760cfa73 100644
--- a/proto/mrt/mrt.c
+++ b/proto/mrt/mrt.c
@@ -431,7 +431,7 @@ mrt_rib_table_entry_bgp_attrs(struct mrt_table_dump_state *s, rte *r)
 
   /* Attribute list must be normalized for bgp_encode_attrs() */
   if (!rta_is_cached(r->attrs))
-    ea_normalize(eattrs);
+    eattrs = ea_normalize(eattrs);
 
   mrt_buffer_need(b, MRT_ATTR_BUFFER_SIZE);
   byte *pos = b->pos;