From: Katerina Kubecova Date: Wed, 2 Apr 2025 12:15:40 +0000 (+0200) Subject: Limits: Fix a table-channel race condition X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=af57de0a7e0d0325ef96e32fa524680ac3fdcd03;p=thirdparty%2Fbird.git 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 f8d4b212e..3c50c7b3b 100644 --- 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 caf99829b..532386106 100644 --- 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 ec561b263..a8b91e2f5 100644 --- 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 4e9f8d8eb..2be5a6151 100644 --- 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