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:
> > > On Wed, Aug 22, 2018 at 12:12:10AM +0200, Remi Locherer wrote:
> > > > On Tue, Aug 21, 2018 at 05:54:18PM +0100, Stuart Henderson wrote:
> > > > > On 2018/08/21 17:16, Remi Locherer wrote:
> > > > > > Hi tech,
> > > > > >
> > > > > > recently we had a short outage in our network. A script started an
> > > > > > additional
> > > > > > ospfd instance because the -n flag for config test was missing.
> > > > >
> > > > > This is a problem with bgpd as well, last time I did this it killed
> > > > > one of the
> > > > > *other* routers on the network (i.e. not just the one where I
> > > > > accidentally ran
> > > > > 2x bgpd...).
> > > > >
> > > > > > What then happend was not nice:
> > > > > > - The new ospfd unlinked the control socket of the first ospfd
> > > > > > - The new ospfd removed all routes from the first ospfd
> > > > > > - The new ospfd was not able to build up an adjacency and therefore
> > > > > > could
> > > > > > not install the routes needed for a recovery.
> > > > > > - Both ospfd instances were running but non-functional.
> > > > > >
> > > > > > Of course the faulty script is fixed by now. ;-)
> > > > > >
> > > > > > It would be nice if ospfd could prevent such a situation.
> > > > > >
> > > > > > Below diff does these things:
> > > > > > - Detect a running ospfd by first doing a connect on the control
> > > > > > socket.
> > > > > > - Do not delete the control socket on exit.
> > > > > > - This could delete the socket of another instance.
> > > > > > - Unlinking the socket on shutdown will be in the way once we add
> > > > > > pledge
> > > > > > to the main process. It was removed recently from various
> > > > > > daemons.
> > > > >
> > > > > This all sounds very sensible.
> > > > >
> > > > > > - Do not delete routes added by another process even if they have
> > > > > > prio RTP_OSPF. Without this the new ospfd will remove all the
> > > > > > routes
> > > > > > of the first one.
> > > > >
> > > > > I'm unsure about this, the above changes stop the new ospfd from
> > > > > running
> > > > > don't they, so that shouldn't be a problem?
> > > >
> > > > It stops to late. kr_init happens before and kill all existing routes
> > > > with
> > > > priority 32. And again in the shutdown function of ospfd.
> > > > >
> > > > > If an ospfd blows up for whatever reason, it would be quite
> > > > > inconvenient
> > > > > if it needs manual route tweaks rather than just 'rcctl start ospfd'
> > > > > to fix it ..
> > > >
> > > > Yes, this is not optimal.
> > > >
> > > > The new diff below defers kr_init until the ospf engine notifies the
> > > > parent
> > > > that the control socket is ready. In case the ospf engine exits because
> > > > the
> > > > control socket is already in use no routes are known that could be
> > > > removed.
> > > >
> > > > With this ospfd keeps the behaviour of removing foreign routes with
> > > > priority 32.
> > > >
> > > > Better?
> > > >
> > >
> > > 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.
>
> Remi
>
>
> 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 27 Aug 2018 21:17:42 -0000
> @@ -42,19 +42,29 @@ int
> control_init(char *path)
> {
> struct sockaddr_un sun;
> - int fd;
> + int fd, fd_check;
> mode_t old_umask;
>
> + bzero(&sun, sizeof(sun));
> + sun.sun_family = AF_UNIX;
> + strlcpy(sun.sun_path, path, sizeof(sun.sun_path));
> +
> + if ((fd_check = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
> + log_warn("control_init: socket check");
> + return (-1);
> + }
> + if (connect(fd_check, (struct sockaddr *)&sun, sizeof(sun)) == 0) {
> + log_warnx("control_init: socket in use");
You may want to close(fd_check) here.
> + return (-1);
> + }
> + close(fd_check);
> +
> if ((fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK,
> 0)) == -1) {
> log_warn("control_init: socket");
> return (-1);
> }
>
> - bzero(&sun, sizeof(sun));
> - sun.sun_family = AF_UNIX;
> - strlcpy(sun.sun_path, path, sizeof(sun.sun_path));
> -
> if (unlink(path) == -1)
> if (errno != ENOENT) {
> log_warn("control_init: unlink %s", path);
> @@ -78,9 +88,7 @@ control_init(char *path)
> return (-1);
> }
>
> - control_state.fd = fd;
> -
> - return (0);
> + return (fd);
> }
>
> int
> 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 27 Aug 2018 21:19:28 -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;
> @@ -218,6 +219,9 @@ main(int argc, char *argv[])
>
> log_info("startup");
>
> + if ((control_fd = control_init(ospfd_conf->csock)) == -1)
> + fatalx("control socket setup failed");
> +
I would prefer if the check happens before the daemon() call since then
the rc script notice this easily. 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.
> if (socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK,
> PF_UNSPEC, pipe_parent2ospfe) == -1)
> fatal("socketpair");
> @@ -270,6 +274,8 @@ main(int argc, char *argv[])
> iev_rde->handler, iev_rde);
> event_add(&iev_rde->ev, NULL);
>
> + 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 +489,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 27 Aug 2018 20:27:46 -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,15 @@ 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();
It is possible to drop the recvfd pledge after this call.
> break;
> default:
> log_debug("ospfe_dispatch_main: error handling imsg %d",
>
--
:wq Claudio
