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",
