If the RDE process dies it takes the session engine along. The problem
is that the ibuf structures to the RDE become invalid but the SE still
tries to send the RDE messages which ends up in tears and SIGSEGV.
Wrap the imsg_compose() to ibuf_rde in a new helper function which does
a NULL check before. Additionally add an extra check in session_stop()
which is called in the exit handler of the SE and is triggering this
in most cases.
OK?
--
:wq Claudio
Index: session.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
retrieving revision 1.366
diff -u -p -r1.366 session.c
--- session.c 4 Sep 2018 12:00:29 -0000 1.366
+++ session.c 19 Sep 2018 12:28:38 -0000
@@ -94,6 +94,7 @@ int capa_neg_calc(struct peer *);
void session_dispatch_imsg(struct imsgbuf *, int, u_int *);
void session_up(struct peer *);
void session_down(struct peer *);
+int imsg_rde(int, u_int32_t, void *, u_int16_t);
void session_demote(struct peer *, int);
int session_link_state_is_up(int, int, int);
@@ -1379,8 +1380,7 @@ session_sendmsg(struct bgp_msg *msg, str
ibuf_close(&p->wbuf, msg->buf);
if (!p->throttled && p->wbuf.queued > SESS_MSG_HIGH_MARK) {
- if (imsg_compose(ibuf_rde, IMSG_XOFF, p->conf.id, 0, -1,
- NULL, 0) == -1)
+ if (imsg_rde(IMSG_XOFF, p->conf.id, NULL, 0) == -1)
log_peer_warn(&p->conf, "imsg_compose XOFF");
p->throttled = 1;
}
@@ -1671,16 +1671,16 @@ session_graceful_restart(struct peer *p)
for (i = 0; i < AID_MAX; i++) {
if (p->capa.neg.grestart.flags[i] & CAPA_GR_PRESENT) {
- if (imsg_compose(ibuf_rde, IMSG_SESSION_STALE,
- p->conf.id, 0, -1, &i, sizeof(i)) == -1)
+ if (imsg_rde(IMSG_SESSION_STALE, p->conf.id,
+ &i, sizeof(i)) == -1)
return (-1);
log_peer_warnx(&p->conf,
"graceful restart of %s, keeping routes",
aid2str(i));
p->capa.neg.grestart.flags[i] |= CAPA_GR_RESTARTING;
} else if (p->capa.neg.mp[i]) {
- if (imsg_compose(ibuf_rde, IMSG_SESSION_FLUSH,
- p->conf.id, 0, -1, &i, sizeof(i)) == -1)
+ if (imsg_rde(IMSG_SESSION_FLUSH, p->conf.id,
+ &i, sizeof(i)) == -1)
return (-1);
log_peer_warnx(&p->conf,
"graceful restart of %s, flushing routes",
@@ -1704,8 +1704,8 @@ session_graceful_stop(struct peer *p)
if (p->capa.neg.grestart.flags[i] & CAPA_GR_RESTARTING) {
log_peer_warnx(&p->conf, "graceful restart of %s, "
"time-out, flushing", aid2str(i));
- if (imsg_compose(ibuf_rde, IMSG_SESSION_FLUSH,
- p->conf.id, 0, -1, &i, sizeof(i)) == -1)
+ if (imsg_rde(IMSG_SESSION_FLUSH, p->conf.id,
+ &i, sizeof(i)) == -1)
return (-1);
}
p->capa.neg.grestart.flags[i] &= ~CAPA_GR_RESTARTING;
@@ -1771,8 +1771,7 @@ session_dispatch_msg(struct pollfd *pfd,
return (1);
}
if (p->throttled && p->wbuf.queued < SESS_MSG_LOW_MARK) {
- if (imsg_compose(ibuf_rde, IMSG_XON, p->conf.id, 0, -1,
- NULL, 0) == -1)
+ if (imsg_rde(IMSG_XON, p->conf.id, NULL, 0) == -1)
log_peer_warn(&p->conf, "imsg_compose XON");
p->throttled = 0;
}
@@ -2183,8 +2182,7 @@ parse_update(struct peer *peer)
p += MSGSIZE_HEADER; /* header is already checked */
datalen -= MSGSIZE_HEADER;
- if (imsg_compose(ibuf_rde, IMSG_UPDATE, peer->conf.id, 0, -1, p,
- datalen) == -1)
+ if (imsg_rde(IMSG_UPDATE, peer->conf.id, p, datalen) == -1)
return (-1);
return (0);
@@ -2221,8 +2219,7 @@ parse_refresh(struct peer *peer)
return (0);
}
- if (imsg_compose(ibuf_rde, IMSG_REFRESH, peer->conf.id, 0, -1, &aid,
- sizeof(aid)) == -1)
+ if (imsg_rde(IMSG_REFRESH, peer->conf.id, &aid, sizeof(aid)) == -1)
return (-1);
return (0);
@@ -2551,8 +2548,8 @@ capa_neg_calc(struct peer *p)
if (negflags & CAPA_GR_RESTARTING) {
if (!(p->capa.peer.grestart.flags[i] &
CAPA_GR_FORWARD)) {
- if (imsg_compose(ibuf_rde, IMSG_SESSION_FLUSH,
- p->conf.id, 0, -1, &i, sizeof(i)) == -1)
+ if (imsg_rde(IMSG_SESSION_FLUSH, p->conf.id,
+ &i, sizeof(i)) == -1)
return (-1);
log_peer_warnx(&p->conf, "graceful restart of "
"%s, not restarted, flushing", aid2str(i));
@@ -2657,9 +2654,8 @@ session_dispatch_imsg(struct imsgbuf *ib
/* sync the RDE in case we keep the peer */
if (reconf == RECONF_KEEP) {
- if (imsg_compose(ibuf_rde, IMSG_SESSION_ADD,
- p->conf.id, 0, -1, &p->conf,
- sizeof(struct peer_config)) == -1)
+ if (imsg_rde(IMSG_SESSION_ADD, p->conf.id,
+ &p->conf, sizeof(struct peer_config)) == -1)
fatalx("imsg_compose error");
if (p->conf.template) {
/* apply the conf to all clones */
@@ -2670,10 +2666,8 @@ session_dispatch_imsg(struct imsgbuf *ib
session_template_clone(np,
NULL, np->conf.id,
np->conf.remote_as);
- if (imsg_compose(ibuf_rde,
- IMSG_SESSION_ADD,
- np->conf.id, 0, -1,
- &np->conf,
+ if (imsg_rde(IMSG_SESSION_ADD,
+ np->conf.id, &np->conf,
sizeof(struct peer_config))
== -1)
fatalx("imsg_compose
error");
@@ -2965,9 +2959,8 @@ session_dispatch_imsg(struct imsgbuf *ib
timer_stop(p, Timer_RestartTimeout);
/* signal back to RDE to cleanup stale routes */
- if (imsg_compose(ibuf_rde,
- IMSG_SESSION_RESTARTED, imsg.hdr.peerid, 0,
- -1, &aid, sizeof(aid)) == -1)
+ if (imsg_rde(IMSG_SESSION_RESTARTED,
+ imsg.hdr.peerid, &aid, sizeof(aid)) == -1)
fatal("imsg_compose: "
"IMSG_SESSION_RESTARTED");
}
@@ -3188,8 +3181,14 @@ session_down(struct peer *peer)
{
bzero(&peer->capa.neg, sizeof(peer->capa.neg));
peer->stats.last_updown = time(NULL);
- if (imsg_compose(ibuf_rde, IMSG_SESSION_DOWN, peer->conf.id, 0, -1,
- NULL, 0) == -1)
+ /*
+ * session_down is called in the exit code path so check
+ * if the RDE is still around, if not there is no need to
+ * send the message.
+ */
+ if (ibuf_rde == NULL)
+ return;
+ if (imsg_rde(IMSG_SESSION_DOWN, peer->conf.id, NULL, 0) == -1)
fatalx("imsg_compose error");
}
@@ -3198,7 +3197,7 @@ session_up(struct peer *p)
{
struct session_up sup;
- if (imsg_compose(ibuf_rde, IMSG_SESSION_ADD, p->conf.id, 0, -1,
+ if (imsg_rde(IMSG_SESSION_ADD, p->conf.id,
&p->conf, sizeof(p->conf)) == -1)
fatalx("imsg_compose error");
@@ -3209,8 +3208,7 @@ session_up(struct peer *p)
sup.short_as = p->short_as;
memcpy(&sup.capa, &p->capa.neg, sizeof(sup.capa));
p->stats.last_updown = time(NULL);
- if (imsg_compose(ibuf_rde, IMSG_SESSION_UP, p->conf.id, 0, -1,
- &sup, sizeof(sup)) == -1)
+ if (imsg_rde(IMSG_SESSION_UP, p->conf.id, &sup, sizeof(sup)) == -1)
fatalx("imsg_compose error");
}
@@ -3224,11 +3222,27 @@ imsg_ctl_parent(int type, u_int32_t peer
int
imsg_ctl_rde(int type, pid_t pid, void *data, u_int16_t datalen)
{
+ if (ibuf_rde_ctl == NULL) {
+ log_warnx("Can't send message %u to RDE, ctl pipe closed",
+ type);
+ return (0);
+ }
/*
* Use control socket to talk to RDE to bypass the queue of the
* regular imsg socket.
*/
return (imsg_compose(ibuf_rde_ctl, type, 0, pid, -1, data, datalen));
+}
+
+int
+imsg_rde(int type, uint32_t peerid, void *data, u_int16_t datalen)
+{
+ if (ibuf_rde == NULL) {
+ log_warnx("Can't send message %u to RDE, pipe closed", type);
+ return (0);
+ }
+
+ return (imsg_compose(ibuf_rde, type, peerid, 0, -1, data, datalen));
}
void
