OK florian@ On Tue, Aug 28, 2018 at 01:19:39PM +0200, Remi Locherer wrote: > On Tue, Aug 28, 2018 at 07:56:43AM +0200, Claudio Jeker wrote: > > On Mon, Aug 27, 2018 at 11:33:19PM +0200, Remi Locherer wrote: > > > On Fri, Aug 24, 2018 at 12:21:31PM +0200, Remi Locherer wrote: > > > > On Fri, Aug 24, 2018 at 08:58:12AM +0200, Claudio Jeker wrote: > > [ snip ] > > > > > > Why are we not checking the control socket in the parent? > > > > > Also it may be better to create the control socket in the parent and > > > > > pass > > > > > it to the ospfe. This is what bgpd is doing and allows to change the > > > > > path > > > > > during runtime with a config reload. > > > > > > > > This makes sense to me. I'll come up with a new diff once I found some > > > > time for it. > > > > > > > > But I'm not sure about changing the socket path with a reload. I plan to > > > > pledge (stdio rpath sendfd wroute) and eventually unveil (read > > > > ospfd.conf) > > > > the main process. > > > > > > New diff below creates the control socket in the main process and passes > > > it > > > to the ospf engine later on. The connect check on the control socket now > > > happens very early. > > > > > > The diff in action looks like this: > > > > > > typhoon ..sbin/ospfd$ doas obj/ospfd -dv > > > startup > > > control_init: socket in use > > > fatal in ospfd: control socket setup failed > > > typhoon 1 ..sbin/ospfd$ > > > > > > > > > I borrowed the fd passing code from slaacd. > > > > > > > > > > > > > > > > > Could there be a case where this causes ospfd to hang on start in the > > > > > connect? Not sure if we can sleep doing a connect() to a AF_UNIX > > > > > socket. > > > > > > I never observed a hangin ospfctl which also does a connect on the control > > > socket. But I could not find the definitiv answer. > > > > > [ snip ] > > > I would prefer if the check happens before the daemon() call since then > > the rc script notice this easily. > > sure > > > Also between here and sending the socket > > we spawn off the rde and ospfe processes. So currently you are leaking > > control_fd into those processes. > > You could probably just add the fd as argument to rde() and ospfe() and > > not use the fd passing at all. But the moment ospfd is using fork&exec > > then the fd passing will be needed again. > > How about the new diff below: I moved the check from control_init into its > own function control_check and call only this before daemon(). control_init > happens later. With this the childs do not have the control fd. > > The time frame where another process can start using the socket is a little > bit bigger this way. We can reduce this again when implementing fork&exec > for ospfd. > > One could also argue that with control_check as separate function fd passing > is not strictly needed. But I think this a step towards fork&exec. > > The diff should also address the other suggestions. > > > > Index: control.c > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/control.c,v > retrieving revision 1.44 > diff -u -p -r1.44 control.c > --- control.c 24 Jan 2017 04:24:25 -0000 1.44 > +++ control.c 28 Aug 2018 09:42:11 -0000 > @@ -39,6 +39,32 @@ struct ctl_conn *control_connbypid(pid_t > void control_close(int); > > int > +control_check(char *path) > +{ > + struct sockaddr_un sun; > + int fd; > + > + bzero(&sun, sizeof(sun)); > + sun.sun_family = AF_UNIX; > + strlcpy(sun.sun_path, path, sizeof(sun.sun_path)); > + > + if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) { > + log_warn("control_check: socket check"); > + return (-1); > + } > + > + if (connect(fd, (struct sockaddr *)&sun, sizeof(sun)) == 0) { > + log_warnx("control_check: socket in use"); > + close(fd); > + return (-1); > + } > + > + close(fd); > + > + return (0); > +} > + > +int > control_init(char *path) > { > struct sockaddr_un sun; > @@ -78,9 +104,7 @@ control_init(char *path) > return (-1); > } > > - control_state.fd = fd; > - > - return (0); > + return (fd); > } > > int > Index: control.h > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/control.h,v > retrieving revision 1.6 > diff -u -p -r1.6 control.h > --- control.h 10 Feb 2015 05:24:48 -0000 1.6 > +++ control.h 28 Aug 2018 09:43:17 -0000 > @@ -34,6 +34,7 @@ struct ctl_conn { > struct imsgev iev; > }; > > +int control_check(char *); > int control_init(char *); > int control_listen(void); > void control_accept(int, short, void *); > Index: ospfd.c > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v > retrieving revision 1.99 > diff -u -p -r1.99 ospfd.c > --- ospfd.c 11 Jul 2018 12:09:34 -0000 1.99 > +++ ospfd.c 28 Aug 2018 10:34:48 -0000 > @@ -116,6 +116,7 @@ main(int argc, char *argv[]) > int mib[4]; > size_t len; > char *sockname = NULL; > + int control_fd; > > conffile = CONF_FILE; > ospfd_process = PROC_MAIN; > @@ -213,6 +214,9 @@ main(int argc, char *argv[]) > log_init(debug, LOG_DAEMON); > log_setverbose(ospfd_conf->opts & OSPFD_OPT_VERBOSE); > > + if ((control_check(ospfd_conf->csock)) == -1) > + fatalx("control socket check failed"); > + > if (!debug) > daemon(1, 0); > > @@ -270,6 +274,10 @@ main(int argc, char *argv[]) > iev_rde->handler, iev_rde); > event_add(&iev_rde->ev, NULL); > > + if ((control_fd = control_init(ospfd_conf->csock)) == -1) > + fatalx("control socket setup failed"); > + main_imsg_compose_ospfe_fd(IMSG_CONTROLFD, 0, control_fd); > + > if (kr_init(!(ospfd_conf->flags & OSPFD_FLAG_NO_FIB_UPDATE), > ospfd_conf->rdomain, ospfd_conf->redist_label_or_prefix) == -1) > fatalx("kr_init failed"); > @@ -483,6 +491,13 @@ main_imsg_compose_ospfe(int type, pid_t > { > if (iev_ospfe) > imsg_compose_event(iev_ospfe, type, 0, pid, -1, data, datalen); > +} > + > +void > +main_imsg_compose_ospfe_fd(int type, pid_t pid, int fd) > +{ > + if (iev_ospfe) > + imsg_compose_event(iev_ospfe, type, 0, pid, fd, NULL, 0); > } > > void > Index: ospfd.h > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.h,v > retrieving revision 1.101 > diff -u -p -r1.101 ospfd.h > --- ospfd.h 25 Jun 2018 22:16:53 -0000 1.101 > +++ ospfd.h 27 Aug 2018 15:49:58 -0000 > @@ -101,6 +101,7 @@ enum imsg_type { > IMSG_CTL_IFINFO, > IMSG_CTL_END, > IMSG_CTL_LOG_VERBOSE, > + IMSG_CONTROLFD, > IMSG_KROUTE_CHANGE, > IMSG_KROUTE_DELETE, > IMSG_IFINFO, > @@ -605,6 +606,7 @@ void rtlabel_tag(u_int16_t, u_int32_t) > > /* ospfd.c */ > void main_imsg_compose_ospfe(int, pid_t, void *, u_int16_t); > +void main_imsg_compose_ospfe_fd(int, pid_t, int); > void main_imsg_compose_rde(int, pid_t, void *, u_int16_t); > int ospf_redistribute(struct kroute *, u_int32_t *); > void merge_config(struct ospfd_conf *, struct ospfd_conf *); > Index: ospfe.c > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/ospfe.c,v > retrieving revision 1.101 > diff -u -p -r1.101 ospfe.c > --- ospfe.c 25 Jun 2018 22:16:53 -0000 1.101 > +++ ospfe.c 28 Aug 2018 07:32:15 -0000 > @@ -90,10 +90,6 @@ ospfe(struct ospfd_conf *xconf, int pipe > /* cleanup a bit */ > kif_clear(); > > - /* create ospfd control socket outside chroot */ > - if (control_init(xconf->csock) == -1) > - fatalx("control socket setup failed"); > - > /* create the raw ip socket */ > if ((xconf->ospf_socket = socket(AF_INET, > SOCK_RAW | SOCK_CLOEXEC | SOCK_NONBLOCK, > @@ -136,7 +132,7 @@ ospfe(struct ospfd_conf *xconf, int pipe > setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid)) > fatal("can't drop privileges"); > > - if (pledge("stdio inet mcast", NULL) == -1) > + if (pledge("stdio inet mcast recvfd", NULL) == -1) > fatal("pledge"); > > event_init(); > @@ -189,10 +185,6 @@ ospfe(struct ospfd_conf *xconf, int pipe > } > } > > - /* listen on ospfd control socket */ > - TAILQ_INIT(&ctl_conns); > - control_listen(); > - > if ((pkt_ptr = calloc(1, READ_BUF_SIZE)) == NULL) > fatal("ospfe"); > > @@ -464,6 +456,17 @@ ospfe_dispatch_main(int fd, short event, > case IMSG_CTL_IFINFO: > case IMSG_CTL_END: > control_imsg_relay(&imsg); > + break; > + case IMSG_CONTROLFD: > + if ((fd = imsg.fd) == -1) > + fatalx("%s: expected to receive imsg control" > + "fd but didn't receive any", __func__); > + control_state.fd = fd; > + /* Listen on control socket. */ > + TAILQ_INIT(&ctl_conns); > + control_listen(); > + if (pledge("stdio inet mcast", NULL) == -1) > + fatal("pledge"); > break; > default: > log_debug("ospfe_dispatch_main: error handling imsg %d", >
-- I'm not entirely sure you are real.