Limits: Fix a table-channel race condition

The limit structure belongs to channel but its routines are called
not only from channel context (when reconfiguring) but also from rtable
context (when importing, recalculating or pruning).

We are introducing a separate domain on the attrs level for the limit
data structures to allow safe access from channel and rtable.

diff --git a/nest/limit.h b/nest/limit.h
index f8d4b212e3ee3e15b84f7ba370ea830b476d15ab..3c50c7b3be75122ce13d87bee11b2c62f1fd58e9 100644 (file)
--- a/nest/limit.h
+++ b/nest/limit.h
@@ -10,18 +10,38 @@
 #ifndef _BIRD_LIMIT_H_
 #define _BIRD_LIMIT_H_
 
-struct limit {
+
+#define LIMIT_PUBLIC                                                                    \
+  DOMAIN(attrs) lock;          /* Lock to take to access the private parts */          \
+  int (*action)(struct limit_private *, void *data);                                    \
+
+struct limit_private {
+  /* Once more the public part */
+  struct { LIMIT_PUBLIC; };
+  struct limit_private **locked_at;
   u32 max;
   u32 count;
-  int (*action)(struct limit *, void *data);
 };
 
-static inline int limit_do_action(struct limit *l, void *data)
+typedef union limit {
+  struct { LIMIT_PUBLIC; };
+  struct limit_private priv;
+} limit;
+
+
+LOBJ_UNLOCK_CLEANUP(limit, attrs);
+#define LIMIT_LOCK_SIMPLE(lim) LOBJ_LOCK_SIMPLE(lim, attrs)
+#define LIMIT_UNLOCK_SIMPLE(lim)       LOBJ_UNLOCK_SIMPLE(lim, attrs)
+
+#define LIMIT_LOCKED(lim, lm)  LOBJ_LOCKED(lim, lm, limit, attrs)
+#define LIMIT_LOCK(lim, tp)    LOBJ_LOCK(lim, lm, limit, attrs)
+
+static inline int limit_do_action(struct limit_private *l, void *data)
 {
   return l->action ? l->action(l, data) : 1;
 }
 
-static inline int limit_push(struct limit *l, void *data)
+static inline int limit_push(struct limit_private *l, void *data)
 {
   if ((l->count >= l->max) && limit_do_action(l, data))
     return 1;
@@ -30,18 +50,29 @@ static inline int limit_push(struct limit *l, void *data)
   return 0;
 }
 
-static inline void limit_pop(struct limit *l)
+static inline int limit_lock_push(limit *lim, void *data)
+{
+  int ret;
+  LIMIT_LOCKED(lim, l)
+  {
+    ret = limit_push(l, data);
+  }
+  return ret;
+}
+
+static inline void limit_pop(struct limit_private *l)
 {
   ASSERT_DIE(l->count > 0);
   --l->count;
 }
 
-static inline void limit_reset(struct limit *l)
+
+static inline void limit_reset(struct limit_private *l)
 {
   l->count = 0;
 }
 
-static inline void limit_update(struct limit *l, void *data, u32 max)
+static inline void limit_update(struct limit_private *l, void *data, u32 max)
 {
   if (l->count > (l->max = max))
     limit_do_action(l, data);

diff --git a/nest/proto.c b/nest/proto.c
index caf99829b5171bed2a38c15e45537b6616144646..53238610671d1ca190f36147321bc957a442ae31 100644 (file)
--- a/nest/proto.c
+++ b/nest/proto.c
@@ -44,9 +44,9 @@ extern struct protocol proto_unix_iface;
 static void proto_rethink_goal(struct proto *p);
 static char *proto_state_name(struct proto *p);
 void proto_journal_item_cleanup(struct lfjour * journal UNUSED, struct lfjour_item *i);
-static void channel_init_limit(struct channel *c, struct limit *l, int dir, struct channel_limit *cf);
-static void channel_update_limit(struct channel *c, struct limit *l, int dir, struct channel_limit *cf);
-static void channel_reset_limit(struct channel *c, struct limit *l, int dir);
+static void channel_init_limit(struct channel *c, limit *l, int dir, struct channel_limit *cf);
+static void channel_update_limit(struct channel *c, limit *l, int dir, struct channel_limit *cf);
+static void channel_reset_limit(struct channel *c, limit *l, int dir);
 static void channel_stop_export(struct channel *c);
 static void channel_check_stopped(struct channel *c);
 static inline void channel_reimport(struct channel *c, struct rt_feeding_request *rfr)
@@ -90,14 +90,18 @@ channel_export_fed(struct rt_export_request *req)
 {
   SKIP_BACK_DECLARE(struct channel, c, out_req, req);
 
-  struct limit *l = &c->out_limit;
-  if ((c->limit_active & (1 << PLD_OUT)) && (l->count <= l->max))
+  limit *limit = &c->out_limit;
+
+  LIMIT_LOCKED(limit, lim)
   {
-    c->limit_active &= ~(1 << PLD_OUT);
-    channel_request_full_refeed(c);
+    if ((c->limit_active & (1 << PLD_OUT)) && (lim->count <= lim->max))
+    {
+      c->limit_active &= ~(1 << PLD_OUT);
+      channel_request_full_refeed(c);
+    }
+    else
+      CALL(c->proto->export_fed, c);
   }
-  else
-    CALL(c->proto->export_fed, c);
 }
 
 void
@@ -2295,7 +2299,7 @@ static const char * channel_limit_name[] = {
 
 
 static void
-channel_log_limit(struct channel *c, struct limit *l, int dir)
+channel_log_limit(struct channel *c, struct limit_private *l, int dir)
 {
   const char *dir_name[PLD_MAX] = { "receive", "import" , "export" };
   log(L_WARN "Channel %s.%s hits route %s limit (%d), action: %s",
@@ -2303,7 +2307,7 @@ channel_log_limit(struct channel *c, struct limit *l, int dir)
 }
 
 static void
-channel_activate_limit(struct channel *c, struct limit *l, int dir)
+channel_activate_limit(struct channel *c, struct limit_private *l, int dir)
 {
   if (c->limit_active & (1 << dir))
     return;
@@ -2313,7 +2317,7 @@ channel_activate_limit(struct channel *c, struct limit *l, int dir)
 }
 
 static int
-channel_limit_warn(struct limit *l, void *data)
+channel_limit_warn(struct limit_private *l, void *data)
 {
   struct channel_limit_data *cld = data;
   struct channel *c = cld->c;
@@ -2325,7 +2329,7 @@ channel_limit_warn(struct limit *l, void *data)
 }
 
 static int
-channel_limit_block(struct limit *l, void *data)
+channel_limit_block(struct limit_private *l, void *data)
 {
   struct channel_limit_data *cld = data;
   struct channel *c = cld->c;
@@ -2339,7 +2343,7 @@ channel_limit_block(struct limit *l, void *data)
 static const byte chl_dir_down[PLD_MAX] = { PDC_RX_LIMIT_HIT, PDC_IN_LIMIT_HIT, PDC_OUT_LIMIT_HIT };
 
 static int
-channel_limit_down(struct limit *l, void *data)
+channel_limit_down(struct limit_private *l, void *data)
 {
   struct channel_limit_data *cld = data;
   struct channel *c = cld->c;
@@ -2362,7 +2366,7 @@ channel_limit_down(struct limit *l, void *data)
   return 1;
 }
 
-static int (*channel_limit_action[])(struct limit *, void *) = {
+static int (*channel_limit_action[])(struct limit_private *, void *) = {
   [PLA_NONE] = NULL,
   [PLA_WARN] = channel_limit_warn,
   [PLA_BLOCK] = channel_limit_block,
@@ -2371,27 +2375,32 @@ static int (*channel_limit_action[])(struct limit *, void *) = {
 };
 
 static void
-channel_update_limit(struct channel *c, struct limit *l, int dir, struct channel_limit *cf)
+channel_update_limit(struct channel *c, limit *lim, int dir, struct channel_limit *cf)
 {
-  l->action = channel_limit_action[cf->action];
+  lim->action = channel_limit_action[cf->action];
   c->limit_actions[dir] = cf->action;
 
   struct channel_limit_data cld = { .c = c, .dir = dir };
-  limit_update(l, &cld, cf->action ? cf->limit : ~((u32) 0));
+  LIMIT_LOCKED(lim, l)
+      limit_update(l, &cld, cf->action ? cf->limit : ~((u32) 0));
 }
 
 static void
-channel_init_limit(struct channel *c, struct limit *l, int dir, struct channel_limit *cf)
+channel_init_limit(struct channel *c, limit *lim, int dir, struct channel_limit *cf)
 {
-  channel_reset_limit(c, l, dir);
-  channel_update_limit(c, l, dir, cf);
+  lim->lock = DOMAIN_NEW(attrs);
+  channel_reset_limit(c, lim, dir);
+  channel_update_limit(c, lim, dir, cf);
 }
 
 static void
-channel_reset_limit(struct channel *c, struct limit *l, int dir)
+channel_reset_limit(struct channel *c, limit *lim, int dir)
 {
-  limit_reset(l);
-  c->limit_active &= ~(1 << dir);
+  LIMIT_LOCKED(lim, l)
+  {
+    limit_reset(l);
+    c->limit_active &= ~(1 << dir);
+  }
 }
 
 static inline void
@@ -2576,9 +2585,10 @@ channel_show_stats(struct channel *c)
 #define SRI(item) SON(rt_is, item)
 #define SRE(item) SON(rt_es, item)
 
-  u32 rx_routes = c->rx_limit.count;
-  u32 in_routes = c->in_limit.count;
-  u32 out_routes = c->out_limit.count;
+  u32 rx_routes, in_routes, out_routes;
+  LIMIT_LOCKED((&c->rx_limit), l) rx_routes = l->count;
+  LIMIT_LOCKED((&c->in_limit), l) in_routes = l->count;
+  LIMIT_LOCKED((&c->out_limit), l) out_routes = l->count;
 
   if (c->in_keep)
     cli_msg(-1006, "    Routes:         %u imported, %u filtered, %u exported, %u preferred",
@@ -2610,12 +2620,14 @@ channel_show_stats(struct channel *c)
 }
 
 void
-channel_show_limit(struct limit *l, const char *dsc, int active, int action)
+channel_show_limit(limit *lim, const char *dsc, int active, int action)
 {
-  if (!l->action)
+  if (!lim->action)
     return;
 
-  cli_msg(-1006, "    %-16s%d%s", dsc, l->max, active ? " [HIT]" : "");
+  LIMIT_LOCKED(lim, l)
+      cli_msg(-1006, "    %-16s%d%s", dsc, l->max, active ? " [HIT]" : "");
+
   cli_msg(-1006, "      Action:       %s", channel_limit_name[action]);
 }
 

diff --git a/nest/protocol.h b/nest/protocol.h
index ec561b2636084d30488f51d909b924514ab9bc49..a8b91e2f50de4a951c9a605d171926562c44429a 100644 (file)
--- a/nest/protocol.h
+++ b/nest/protocol.h
@@ -262,7 +262,7 @@ static inline event_list *proto_work_list(struct proto *p)
 static inline void proto_send_event(struct proto *p, event *e)
 { ev_send(proto_event_list(p), e); }
 
-void channel_show_limit(struct limit *l, const char *dsc, int active, int action);
+void channel_show_limit(limit *l, const char *dsc, int active, int action);
 void channel_show_info(struct channel *c);
 void channel_cmd_debug(struct channel *c, uint mask);
 
@@ -539,8 +539,8 @@ struct channel_limit_data {
 #define CHANNEL_LIMIT_LOG(_c, _dir, _op)
 #endif
 
-#define CHANNEL_LIMIT_PUSH(_c, _dir)  ({ CHANNEL_LIMIT_LOG(_c, _dir, "push from"); struct channel_limit_data cld = { .c = (_c), .dir = PLD_##_dir }; limit_push(CLP__##_dir(_c), &cld); })
-#define CHANNEL_LIMIT_POP(_c, _dir)   ({ limit_pop(CLP__##_dir(_c)); CHANNEL_LIMIT_LOG(_c, _dir, "pop to"); })
+#define CHANNEL_LIMIT_PUSH(_c, _dir)  ({ CHANNEL_LIMIT_LOG(_c, _dir, "push from"); struct channel_limit_data cld = { .c = (_c), .dir = PLD_##_dir }; limit_lock_push(CLP__##_dir(_c), &cld); })
+#define CHANNEL_LIMIT_POP(_c, _dir)   ({ LIMIT_LOCKED(CLP__##_dir(_c), l) limit_pop(l); CHANNEL_LIMIT_LOG(_c, _dir, "pop to"); })
 
 /*
  *     Channels
@@ -617,9 +617,9 @@ struct channel {
   struct bmap export_accepted_map;     /* Keeps track which routes were really exported */
   struct bmap export_rejected_map;     /* Keeps track which routes were rejected by export filter */
 
-  struct limit rx_limit;               /* Receive limit (for in_keep & RIK_REJECTED) */
-  struct limit in_limit;               /* Input limit */
-  struct limit out_limit;              /* Output limit */
+  limit rx_limit;              /* Receive limit (for in_keep & RIK_REJECTED) */
+  limit in_limit;              /* Input limit */
+  limit out_limit;             /* Output limit */
 
   u8 limit_actions[PLD_MAX];           /* Limit actions enum */
   u8 limit_active;                     /* Flags for active limits */

diff --git a/proto/pipe/pipe.c b/proto/pipe/pipe.c
index 4e9f8d8eb8801b832a30b8bbe48679f74ceeffd8..2be5a61517ec628bc0e684612fbc78f3466cc9f0 100644 (file)
--- a/proto/pipe/pipe.c
+++ b/proto/pipe/pipe.c
@@ -217,8 +217,9 @@ pipe_show_stats(struct pipe_proto *p)
   struct rt_import_stats *rs2i = p->sec->in_req.hook ? &p->sec->in_req.hook->stats : NULL;
   struct rt_export_stats *rs2e = &p->sec->out_req.stats;
 
-  u32 pri_routes = p->pri->in_limit.count;
-  u32 sec_routes = p->sec->in_limit.count;
+  u32 pri_routes, sec_routes;
+  LIMIT_LOCKED(&p->pri->in_limit, l) pri_routes = l->count;
+  LIMIT_LOCKED(&p->sec->in_limit, l) sec_routes = l->count;
 
   /*
    * Pipe stats (as anything related to pipes) are a bit tricky. There