This is the first step of some larger reshuffling of how the RDE is
working. One of the things needed is proper reference counting for
nexthops since I want to kill nexthop_link and nexthop_unlink in the long
run.
Even though an intermediat step the result is IMO a lot cleaner than
before. Caching the nexthop in the filter_set and holding a refcnt while
doing the UPDATE processing makes a lot of the special code disapear.
Please test and review
--
:wq Claudio
Index: bgpd.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
retrieving revision 1.318
diff -u -p -r1.318 bgpd.h
--- bgpd.h 13 Jun 2018 09:33:51 -0000 1.318
+++ bgpd.h 21 Jun 2018 18:38:48 -0000
@@ -915,20 +915,22 @@ enum action_types {
ACTION_SET_ORIGIN
};
+struct nexthop;
struct filter_set {
TAILQ_ENTRY(filter_set) entry;
union {
- u_int8_t prepend;
- u_int16_t id;
- u_int32_t metric;
- int32_t relative;
- struct bgpd_addr nexthop;
- struct filter_community community;
- struct filter_largecommunity large_community;
- struct filter_extcommunity ext_community;
- char pftable[PFTABLE_LEN];
- char rtlabel[RTLABEL_LEN];
- u_int8_t origin;
+ u_int8_t prepend;
+ u_int16_t id;
+ u_int32_t metric;
+ int32_t relative;
+ struct bgpd_addr nexthop;
+ struct nexthop *nh;
+ struct filter_community community;
+ struct filter_largecommunity large_community;
+ struct filter_extcommunity ext_community;
+ char pftable[PFTABLE_LEN];
+ char rtlabel[RTLABEL_LEN];
+ u_int8_t origin;
} action;
enum action_types type;
};
Index: rde.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.380
diff -u -p -r1.380 rde.c
--- rde.c 13 Jun 2018 09:33:51 -0000 1.380
+++ rde.c 21 Jun 2018 17:04:41 -0000
@@ -377,7 +377,6 @@ rde_dispatch_imsg_session(struct imsgbuf
struct rde_peer *peer;
struct rde_aspath *asp;
struct filter_set *s;
- struct nexthop *nh;
u_int8_t *asdata;
ssize_t n;
int verbose;
@@ -570,12 +569,9 @@ badnet:
if ((s = malloc(sizeof(struct filter_set))) == NULL)
fatal(NULL);
memcpy(s, imsg.data, sizeof(struct filter_set));
+ if (s->type == ACTION_SET_NEXTHOP)
+ s->action.nh = nexthop_get(&s->action.nexthop);
TAILQ_INSERT_TAIL(session_set, s, entry);
-
- if (s->type == ACTION_SET_NEXTHOP) {
- nh = nexthop_get(&s->action.nexthop);
- nh->refcnt++;
- }
break;
case IMSG_CTL_SHOW_NETWORK:
case IMSG_CTL_SHOW_RIB:
@@ -668,7 +664,6 @@ rde_dispatch_imsg_parent(struct imsgbuf
struct filter_head *nr;
struct filter_rule *r;
struct filter_set *s;
- struct nexthop *nh;
struct rib *rib;
struct prefixset *ps;
struct prefixset_item *psi;
@@ -907,12 +902,9 @@ rde_dispatch_imsg_parent(struct imsgbuf
if ((s = malloc(sizeof(struct filter_set))) == NULL)
fatal(NULL);
memcpy(s, imsg.data, sizeof(struct filter_set));
+ if (s->type == ACTION_SET_NEXTHOP)
+ s->action.nh = nexthop_get(&s->action.nexthop);
TAILQ_INSERT_TAIL(parent_set, s, entry);
-
- if (s->type == ACTION_SET_NEXTHOP) {
- nh = nexthop_get(&s->action.nexthop);
- nh->refcnt++;
- }
break;
case IMSG_MRT_OPEN:
case IMSG_MRT_REOPEN:
@@ -1263,8 +1255,7 @@ rde_update_dispatch(struct imsg *imsg)
* But first unlock the previously locked nexthop.
*/
if (asp->nexthop) {
- asp->nexthop->refcnt--;
- (void)nexthop_delete(asp->nexthop);
+ (void)nexthop_put(asp->nexthop);
asp->nexthop = NULL;
}
if ((pos = rde_get_mp_nexthop(mpp, mplen, aid, asp)) == -1) {
@@ -1361,11 +1352,6 @@ rde_update_dispatch(struct imsg *imsg)
done:
if (attrpath_len != 0) {
- /* unlock the previously locked entry */
- if (asp->nexthop) {
- asp->nexthop->refcnt--;
- (void)nexthop_delete(asp->nexthop);
- }
/* free allocated attribute memory that is no longer used */
path_put(asp);
}
@@ -1569,12 +1555,6 @@ bad_flags:
return (-1);
}
a->nexthop = nexthop_get(&nexthop);
- /*
- * lock the nexthop because it is not yet linked else
- * withdraws may remove this nexthop which in turn would
- * cause a use after free error.
- */
- a->nexthop->refcnt++;
break;
case ATTR_MED:
if (attr_len != 4)
@@ -1908,12 +1888,6 @@ rde_get_mp_nexthop(u_char *data, u_int16
}
asp->nexthop = nexthop_get(&nexthop);
- /*
- * lock the nexthop because it is not yet linked else
- * withdraws may remove this nexthop which in turn would
- * cause a use after free error.
- */
- asp->nexthop->refcnt++;
/* ignore reserved (old SNPA) field as per RFC4760 */
totlen += nhlen + 1;
Index: rde.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
retrieving revision 1.169
diff -u -p -r1.169 rde.h
--- rde.h 21 Jun 2018 17:26:16 -0000 1.169
+++ rde.h 21 Jun 2018 17:36:23 -0000
@@ -224,7 +224,7 @@ struct nexthop {
*/
u_int32_t costs;
#endif
- int refcnt; /* filterset reference counter */
+ int refcnt;
enum nexthop_state state;
u_int8_t nexthop_netlen;
u_int8_t flags;
@@ -506,13 +506,14 @@ prefix_peer(struct prefix *p)
void nexthop_init(u_int32_t);
void nexthop_shutdown(void);
-void nexthop_modify(struct rde_aspath *, struct bgpd_addr *,
+void nexthop_modify(struct rde_aspath *, struct nexthop *,
enum action_types, u_int8_t);
void nexthop_link(struct rde_aspath *);
void nexthop_unlink(struct rde_aspath *);
-int nexthop_delete(struct nexthop *);
void nexthop_update(struct kroute_nexthop *);
struct nexthop *nexthop_get(struct bgpd_addr *);
+struct nexthop *nexthop_ref(struct nexthop *);
+int nexthop_put(struct nexthop *);
int nexthop_compare(struct nexthop *, struct nexthop *);
/* rde_update.c */
Index: rde_filter.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_filter.c,v
retrieving revision 1.88
diff -u -p -r1.88 rde_filter.c
--- rde_filter.c 21 Jun 2018 17:28:02 -0000 1.88
+++ rde_filter.c 21 Jun 2018 17:29:02 -0000
@@ -130,8 +130,7 @@ rde_apply_set(struct rde_aspath *asp, st
case ACTION_SET_NEXTHOP_BLACKHOLE:
case ACTION_SET_NEXTHOP_NOMODIFY:
case ACTION_SET_NEXTHOP_SELF:
- nexthop_modify(asp, &set->action.nexthop, set->type,
- aid);
+ nexthop_modify(asp, set->action.nh, set->type, aid);
break;
case ACTION_SET_COMMUNITY:
switch (set->action.community.as) {
@@ -607,7 +606,6 @@ void
filterset_free(struct filter_set_head *sh)
{
struct filter_set *s;
- struct nexthop *nh;
if (sh == NULL)
return;
@@ -619,11 +617,8 @@ filterset_free(struct filter_set_head *s
else if (s->type == ACTION_PFTABLE_ID)
pftable_unref(s->action.id);
else if (s->type == ACTION_SET_NEXTHOP &&
- bgpd_process == PROC_RDE) {
- nh = nexthop_get(&s->action.nexthop);
- --nh->refcnt;
- (void)nexthop_delete(nh);
- }
+ bgpd_process == PROC_RDE)
+ nexthop_put(s->action.nh);
free(s);
}
}
Index: rde_rib.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
retrieving revision 1.160
diff -u -p -r1.160 rde_rib.c
--- rde_rib.c 21 Jun 2018 17:26:16 -0000 1.160
+++ rde_rib.c 21 Jun 2018 18:45:59 -0000
@@ -607,7 +607,6 @@ path_destroy(struct rde_aspath *asp)
LIST_REMOVE(asp, path_l);
TAILQ_REMOVE(&asp->peer->path_h, asp, peer_l);
asp->peer = NULL;
- asp->nexthop = NULL;
asp->flags &= ~F_ATTR_LINKED;
path_put(asp);
@@ -654,7 +653,7 @@ path_copy(struct rde_aspath *asp)
nasp->aspath->refcnt++;
rdemem.aspath_refs++;
}
- nasp->nexthop = asp->nexthop;
+ nasp->nexthop = nexthop_ref(asp->nexthop);
nasp->med = asp->med;
nasp->lpref = asp->lpref;
nasp->weight = asp->weight;
@@ -705,6 +704,7 @@ path_put(struct rde_aspath *asp)
rtlabel_unref(asp->rtlabelid);
pftable_unref(asp->pftableid);
aspath_put(asp->aspath);
+ nexthop_put(asp->nexthop);
attr_freeall(asp);
rdemem.path_cnt--;
free(asp);
@@ -1161,10 +1161,14 @@ nexthop_shutdown(void)
nh != NULL; nh = nnh) {
nnh = LIST_NEXT(nh, nexthop_l);
nh->state = NEXTHOP_UNREACH;
- (void)nexthop_delete(nh);
+ (void)nexthop_put(nh);
+ }
+ if (!LIST_EMPTY(&nexthoptable.nexthop_hashtbl[i])) {
+ nh = LIST_FIRST(&nexthoptable.nexthop_hashtbl[i]);
+ log_warnx("nexthop_shutdown: non-free table, "
+ "nexthop %s refcnt %d",
+ log_addr(&nh->exit_nexthop), nh->refcnt);
}
- if (!LIST_EMPTY(&nexthoptable.nexthop_hashtbl[i]))
- log_warnx("nexthop_shutdown: non-free table");
}
free(nexthoptable.nexthop_hashtbl);
@@ -1185,6 +1189,7 @@ nexthop_update(struct kroute_nexthop *ms
}
oldstate = nh->state;
+
if (msg->valid)
nh->state = NEXTHOP_REACH;
else
@@ -1202,9 +1207,10 @@ nexthop_update(struct kroute_nexthop *ms
sizeof(nh->nexthop_net));
nh->nexthop_netlen = msg->netlen;
- if (nexthop_delete(nh))
- /* nexthop no longer used */
- return;
+ if (oldstate == NEXTHOP_LOOKUP)
+ /* drop reference which was hold during the lookup */
+ if (nexthop_put(nh))
+ return;
if (rde_noevaluate())
/*
@@ -1219,12 +1225,13 @@ nexthop_update(struct kroute_nexthop *ms
}
void
-nexthop_modify(struct rde_aspath *asp, struct bgpd_addr *nexthop,
+nexthop_modify(struct rde_aspath *asp, struct nexthop *nh,
enum action_types type, u_int8_t aid)
{
- struct nexthop *nh;
+ if (asp->flags & F_ATTR_LINKED)
+ fatalx("nexthop_modify: trying to modify linked asp");
- if (type == ACTION_SET_NEXTHOP && aid != nexthop->aid)
+ if (type == ACTION_SET_NEXTHOP && aid != nh->exit_nexthop.aid)
return;
asp->flags &= ~F_NEXTHOP_MASK;
@@ -1242,12 +1249,8 @@ nexthop_modify(struct rde_aspath *asp, s
asp->flags |= F_NEXTHOP_SELF;
break;
case ACTION_SET_NEXTHOP:
- nh = nexthop_get(nexthop);
- if (asp->flags & F_ATTR_LINKED)
- nexthop_unlink(asp);
- asp->nexthop = nh;
- if (asp->flags & F_ATTR_LINKED)
- nexthop_link(asp);
+ nexthop_put(asp->nexthop);
+ asp->nexthop = nexthop_ref(nh);
break;
default:
break;
@@ -1273,30 +1276,11 @@ nexthop_unlink(struct rde_aspath *asp)
LIST_REMOVE(asp, nexthop_l);
- /* see if list is empty */
+ /* remove reference to nexthop */
nh = asp->nexthop;
asp->nexthop = NULL;
- (void)nexthop_delete(nh);
-}
-
-int
-nexthop_delete(struct nexthop *nh)
-{
- /* nexthop still used by some other aspath */
- if (!LIST_EMPTY(&nh->path_h))
- return (0);
-
- /* either pinned or in a state where it may not be deleted */
- if (nh->refcnt > 0 || nh->state == NEXTHOP_LOOKUP)
- return (0);
-
- LIST_REMOVE(nh, nexthop_l);
- rde_send_nexthop(&nh->exit_nexthop, 0);
-
- rdemem.nexthop_cnt--;
- free(nh);
- return (1);
+ (void)nexthop_put(nh);
}
struct nexthop *
@@ -1313,6 +1297,7 @@ nexthop_get(struct bgpd_addr *nexthop)
LIST_INIT(&nh->path_h);
nh->state = NEXTHOP_LOOKUP;
+ nexthop_ref(nh); /* take reference for lookup */
nh->exit_nexthop = *nexthop;
LIST_INSERT_HEAD(nexthop_hash(nexthop), nh,
nexthop_l);
@@ -1320,7 +1305,36 @@ nexthop_get(struct bgpd_addr *nexthop)
rde_send_nexthop(&nh->exit_nexthop, 1);
}
- return (nh);
+ return nexthop_ref(nh);
+}
+
+struct nexthop *
+nexthop_ref(struct nexthop *nexthop)
+{
+ if (nexthop)
+ nexthop->refcnt++;
+ return (nexthop);
+}
+
+int
+nexthop_put(struct nexthop *nh)
+{
+ if (nh == NULL)
+ return (0);
+
+ if (--nh->refcnt > 0)
+ return (0);
+
+ /* sanity check */
+ if (!LIST_EMPTY(&nh->path_h) || nh->state == NEXTHOP_LOOKUP)
+ fatalx("nexthop_put: refcnt error");
+
+ LIST_REMOVE(nh, nexthop_l);
+ rde_send_nexthop(&nh->exit_nexthop, 0);
+
+ rdemem.nexthop_cnt--;
+ free(nh);
+ return (1);
}
int
