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.
>
Looks good to me.
>
> 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",
>
--
:wq Claudio
