From: reyk <reyk@openbsd.org>
Date: Sun, 20 Apr 2014 14:48:29 +0000 (+0000)
Subject: Reimplement the multi-dimensional arrays that are used to set up the
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=9326d639a3dfeb90c54cb9afa9501ba27c09edc4;p=openbsd

Reimplement the multi-dimensional arrays that are used to set up the
process to process imsg communication.  It became a maze after we
added support for multiple relay processes and even worse with the ca
processes.  This change makes it easier to understand.  Now it only
opens socketpairs that are needed - the code previously wasted lots of
fds.

ok blambert@
---

diff --git a/usr.sbin/relayd/proc.c b/usr.sbin/relayd/proc.c
index b8959ce211d..79d188b58ab 100644
--- a/usr.sbin/relayd/proc.c
+++ b/usr.sbin/relayd/proc.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: proc.c,v 1.10 2014/04/18 21:29:20 tedu Exp $	*/
+/*	$OpenBSD: proc.c,v 1.11 2014/04/20 14:48:29 reyk Exp $	*/
 
 /*
  * Copyright (c) 2010 - 2014 Reyk Floeter <reyk@openbsd.org>
@@ -43,49 +43,88 @@
 
 #include "relayd.h"
 
-void	 proc_setup(struct privsep *);
+void	 proc_open(struct privsep *, struct privsep_proc *,
+	    struct privsep_proc *, size_t);
+void	 proc_close(struct privsep *);
 int	 proc_ispeer(struct privsep_proc *, u_int, enum privsep_procid);
 void	 proc_shutdown(struct privsep_proc *);
 void	 proc_sig_handler(int, short, void *);
 void	 proc_range(struct privsep *, enum privsep_procid, int *, int *);
-u_int	 proc_instances(struct privsep *, u_int, u_int);
 
 int
-proc_ispeer(struct privsep_proc *p, u_int nproc, enum privsep_procid type)
+proc_ispeer(struct privsep_proc *procs, u_int nproc, enum privsep_procid type)
 {
 	u_int	i;
 
 	for (i = 0; i < nproc; i++)
-		if (p[i].p_id == type)
+		if (procs[i].p_id == type)
 			return (1);
 	return (0);
 }
 
 void
-proc_init(struct privsep *ps, struct privsep_proc *p, u_int nproc)
+proc_init(struct privsep *ps, struct privsep_proc *procs, u_int nproc)
 {
-	u_int	 i;
+	u_int			 i, j, src, dst;
+	struct privsep_pipes	*pp;
 
 	/*
-	 * Called from parent
+	 * Allocate pipes for all process instances (incl. parent)
+	 *
+	 * - ps->ps_pipes: N:M mapping
+	 * N source processes connected to M destination processes:
+	 * [src][instances][dst][instances], for example
+	 * [PROC_RELAY][3][PROC_CA][3]
+	 *
+	 * - ps->ps_pp: per-process 1:M part of ps->ps_pipes
+	 * Each process instance has a destination array of socketpair fds:
+	 * [dst][instances], for example
+	 * [PROC_PARENT][0]
+	 */
+	for (src = 0; src < PROC_MAX; src++) {
+		/* Allocate destination array for each process */
+		if ((ps->ps_pipes[src] = calloc(ps->ps_ninstances,
+		    sizeof(struct privsep_pipes))) == NULL)
+			fatal("proc_init: calloc");
+
+		for (i = 0; i < ps->ps_ninstances; i++) {
+			pp = &ps->ps_pipes[src][i];
+
+			for (dst = 0; dst < PROC_MAX; dst++) {
+				/* Allocate maximum fd integers */
+				if ((pp->pp_pipes[dst] =
+				    calloc(ps->ps_ninstances,
+				    sizeof(int))) == NULL)
+					fatal("proc_init: calloc");
+
+				/* Mark fd as unused */
+				for (j = 0; j < ps->ps_ninstances; j++)
+					pp->pp_pipes[dst][j] = -1;
+			}
+		}
+	}
+
+	/*
+	 * Setup and run the parent and its children
 	 */
 	privsep_process = PROC_PARENT;
 	ps->ps_instances[PROC_PARENT] = 1;
 	ps->ps_title[PROC_PARENT] = "parent";
 	ps->ps_pid[PROC_PARENT] = getpid();
+	ps->ps_pp = &ps->ps_pipes[privsep_process][0];
 
 	for (i = 0; i < nproc; i++) {
 		/* Default to 1 process instance */
-		if (ps->ps_instances[p[i].p_id] < 1)
-			ps->ps_instances[p[i].p_id] = 1;
-		ps->ps_title[p[i].p_id] = p[i].p_title;
+		if (ps->ps_instances[procs[i].p_id] < 1)
+			ps->ps_instances[procs[i].p_id] = 1;
+		ps->ps_title[procs[i].p_id] = procs[i].p_title;
 	}
 
-	proc_setup(ps);
+	proc_open(ps, NULL, procs, nproc);
 
 	/* Engage! */
 	for (i = 0; i < nproc; i++)
-		ps->ps_pid[p[i].p_id] = (*p[i].p_init)(ps, &p[i]);
+		ps->ps_pid[procs[i].p_id] = (*procs[i].p_init)(ps, &procs[i]);
 }
 
 void
@@ -107,101 +146,113 @@ proc_kill(struct privsep *ps)
 		pid = waitpid(WAIT_ANY, NULL, 0);
 	} while (pid != -1 || (pid == -1 && errno == EINTR));
 
-	proc_clear(ps, 1);
+	proc_close(ps);
 }
 
 void
-proc_setup(struct privsep *ps)
+proc_open(struct privsep *ps, struct privsep_proc *p,
+    struct privsep_proc *procs, size_t nproc)
 {
-	int	 i, j, n, count, sockpair[2];
+	struct privsep_pipes	*pa, *pb;
+	int			 fds[2];
+	u_int			 i, j, src, proc;
 
-	for (i = 0; i < PROC_MAX; i++)
-		for (j = 0; j < PROC_MAX; j++) {
-			if ((count = proc_instances(ps, i, j)) == 0)
-				continue;
-
-			if ((ps->ps_pipes[i][j] =
-			    calloc(count, sizeof(int))) == NULL ||
-			    (ps->ps_pipes[j][i] =
-			    calloc(count, sizeof(int))) == NULL)
-				fatal(NULL);
+	if (p == NULL)
+		src = privsep_process; /* parent */
+	else
+		src = p->p_id;
 
-			for (n = 0; n < count; n++) {
-				if (ps->ps_noaction)
+	/*
+	 * Open socket pairs for our peers
+	 */	
+	for (proc = 0; proc < nproc; proc++) {
+		procs[proc].p_ps = ps;
+		procs[proc].p_env = ps->ps_env;
+
+		for (i = 0; i < ps->ps_instances[src]; i++) {
+			for (j = 0; j < ps->ps_instances[procs[proc].p_id];
+			    j++) {
+				pa = &ps->ps_pipes[src][i];
+				pb = &ps->ps_pipes[procs[proc].p_id][j];
+
+				/* Check if fds are already set by peer */
+				if (pa->pp_pipes[procs[proc].p_id][j] != -1)
 					continue;
-				if (socketpair(AF_UNIX, SOCK_STREAM, PF_UNSPEC,
-				    sockpair) == -1)
+
+				if (socketpair(AF_UNIX, SOCK_STREAM,
+				    PF_UNSPEC, fds) == -1)
 					fatal("socketpair");
-				ps->ps_pipes[i][j][n] = sockpair[0];
-				ps->ps_pipes[j][i][n] = sockpair[1];
-				socket_set_blockmode(
-				    ps->ps_pipes[i][j][n],
-				    BM_NONBLOCK);
-				socket_set_blockmode(
-				    ps->ps_pipes[j][i][n],
-				    BM_NONBLOCK);
+
+				socket_set_blockmode(fds[0], BM_NONBLOCK);
+				socket_set_blockmode(fds[1], BM_NONBLOCK);
+
+				pa->pp_pipes[procs[proc].p_id][j] = fds[0];
+				pb->pp_pipes[src][i] = fds[1];
 			}
 		}
+	}
 }
 
 void
-proc_config(struct privsep *ps, struct privsep_proc *p, u_int nproc)
+proc_listen(struct privsep *ps, struct privsep_proc *procs, size_t nproc)
 {
-	u_int	i, j, src, dst, count, n, instance;
-
-	src = privsep_process;
+	u_int			 i, dst, src, n, m;
+	struct privsep_pipes	*pp;
 
 	/*
-	 * close unused pipes
+	 * Close unused pipes
 	 */
-	for (i = 0; i < PROC_MAX; i++) {
-		for (j = 0; j < PROC_MAX; j++) {
-			if ((count = proc_instances(ps, i, j)) == 0)
+	for (src = 0; src < PROC_MAX; src++) {
+		for (n = 0; n < ps->ps_instances[src]; n++) {
+			/* Ingore current process */
+			if (src == (u_int)privsep_process &&
+			    n == ps->ps_instance)
 				continue;
 
-			for (n = 0; n < count; n++) {
-				instance = ps->ps_instances[i] > 1 ? n : 0;
-				if (i == src &&
-				    proc_ispeer(p, nproc, j) &&
-				    ps->ps_instance == instance)
-					continue;
+			pp = &ps->ps_pipes[src][n];
 
-				if (!ps->ps_noaction)
-					close(ps->ps_pipes[i][j][n]);
-				ps->ps_pipes[i][j][n] = -1;
+			for (dst = 0; dst < PROC_MAX; dst++) {
+				if (src == dst)
+					continue;
+				for (m = 0; m < ps->ps_instances[dst]; m++) {
+					if (pp->pp_pipes[dst][m] == -1)
+						continue;
+
+					/* Close and invalidate fd */
+					close(pp->pp_pipes[dst][m]);
+					pp->pp_pipes[dst][m] = -1;
+				}
 			}
 		}
 	}
 
+	src = privsep_process;
+	ps->ps_pp = pp = &ps->ps_pipes[src][ps->ps_instance];
+
 	/*
-	 * listen on appropriate pipes
+	 * Listen on appropriate pipes
 	 */
 	for (i = 0; i < nproc; i++) {
-		dst = p[i].p_id;
-		p[i].p_ps = ps;
-		p[i].p_env = ps->ps_env;
+		dst = procs[i].p_id;
 
 		if (src == dst)
-			fatal("proc_config: cannot peer with oneself");
+			fatal("proc_listen: cannot peer with oneself");
 
-		if ((count = proc_instances(ps, src, dst)) == 0)
-			continue;
-
-		if ((ps->ps_ievs[dst] = calloc(count,
+		if ((ps->ps_ievs[dst] = calloc(ps->ps_instances[dst],
 		    sizeof(struct imsgev))) == NULL)
-			fatal("proc_config");
+			fatal("proc_open");
 
-		for (n = 0; n < count; n++) {
-			if (ps->ps_pipes[src][dst][n] == -1)
+		for (n = 0; n < ps->ps_instances[dst]; n++) {
+			if (pp->pp_pipes[dst][n] == -1)
 				continue;
 
 			imsg_init(&(ps->ps_ievs[dst][n].ibuf),
-			    ps->ps_pipes[src][dst][n]);
+			    pp->pp_pipes[dst][n]);
 			ps->ps_ievs[dst][n].handler = proc_dispatch;
 			ps->ps_ievs[dst][n].events = EV_READ;
-			ps->ps_ievs[dst][n].proc = &p[i];
+			ps->ps_ievs[dst][n].proc = &procs[i];
 			ps->ps_ievs[dst][n].data = &ps->ps_ievs[dst][n];
-			p[i].p_instance = n;
+			procs[i].p_instance = n;
 
 			event_set(&(ps->ps_ievs[dst][n].ev),
 			    ps->ps_ievs[dst][n].ibuf.fd,
@@ -214,53 +265,34 @@ proc_config(struct privsep *ps, struct privsep_proc *p, u_int nproc)
 }
 
 void
-proc_clear(struct privsep *ps, int purge)
+proc_close(struct privsep *ps)
 {
-	u_int	 src = privsep_process, dst, n, count;
+	u_int			 dst, n;
+	struct privsep_pipes	*pp;
 
 	if (ps == NULL)
 		return;
 
+	pp = ps->ps_pp;
+
 	for (dst = 0; dst < PROC_MAX; dst++) {
-		if ((count = proc_instances(ps, src, dst)) == 0 ||
-		    ps->ps_ievs[dst] == NULL)
+		if (ps->ps_ievs[dst] == NULL)
 			continue;
 
-		for (n = 0; n < count; n++) {
-			if (ps->ps_pipes[src][dst][n] == -1)
+		for (n = 0; n < ps->ps_instances[dst]; n++) {
+			if (pp->pp_pipes[dst][n] == -1)
 				continue;
-			if (purge) {
-				event_del(&(ps->ps_ievs[dst][n].ev));
-				imsg_clear(&(ps->ps_ievs[dst][n].ibuf));
-				close(ps->ps_pipes[src][dst][n]);
-			} else
-				imsg_flush(&(ps->ps_ievs[dst][n].ibuf));
+
+			/* Cancel the fd, close and invalidate the fd */
+			event_del(&(ps->ps_ievs[dst][n].ev));
+			imsg_clear(&(ps->ps_ievs[dst][n].ibuf));
+			close(pp->pp_pipes[dst][n]);
+			pp->pp_pipes[dst][n] = -1;
 		}
-		if (purge)
-			free(ps->ps_ievs[dst]);
+		free(ps->ps_ievs[dst]);
 	}
 }
 
-u_int
-proc_instances(struct privsep *ps, u_int src, u_int dst)
-{
-	u_int	 count;
-
-	if (ps->ps_instances[src] > 1 &&
-	    ps->ps_instances[dst] > 1 &&
-	    ps->ps_instances[src] != ps->ps_instances[dst])
-		fatalx("N:M peering not supported");
-
-	if (src == dst ||
-	    ps->ps_instances[src] == 0 ||
-	    ps->ps_instances[dst] == 0)
-		return (0);
-
-	count = MAX(ps->ps_instances[src], ps->ps_instances[dst]);
-
-	return (count);
-}
-
 void
 proc_shutdown(struct privsep_proc *p)
 {
@@ -272,7 +304,7 @@ proc_shutdown(struct privsep_proc *p)
 	if (p->p_shutdown != NULL)
 		(*p->p_shutdown)();
 
-	proc_clear(ps, 1);
+	proc_close(ps);
 
 	log_info("%s exiting, pid %d", p->p_title, getpid());
 
@@ -313,9 +345,12 @@ proc_run(struct privsep *ps, struct privsep_proc *p,
 	if (ps->ps_noaction)
 		return (0);
 
+	proc_open(ps, p, procs, nproc);
+
+	/* Fork child handlers */
 	switch (pid = fork()) {
 	case -1:
-		fatal("run_proc: cannot fork");
+		fatal("proc_run: cannot fork");
 	case 0:
 		/* Set the process group of the current process */
 		setpgrp(0, getpid());
@@ -326,7 +361,7 @@ proc_run(struct privsep *ps, struct privsep_proc *p,
 
 	pw = ps->ps_pw;
 
-	if (p->p_id == PROC_CONTROL) {
+	if (p->p_id == PROC_CONTROL && ps->ps_instance == 0) {
 		if (control_init(ps, &ps->ps_csock) == -1)
 			fatalx(p->p_title);
 	}
@@ -339,16 +374,16 @@ proc_run(struct privsep *ps, struct privsep_proc *p,
 
 #ifndef DEBUG
 	if (chroot(root) == -1)
-		fatal("run_proc: chroot");
+		fatal("proc_run: chroot");
 	if (chdir("/") == -1)
-		fatal("run_proc: chdir(\"/\")");
+		fatal("proc_run: chdir(\"/\")");
 #else
 #warning disabling privilege revocation and chroot in DEBUG MODE
 	if (p->p_chroot != NULL) {
 		if (chroot(root) == -1)
-			fatal("run_proc: chroot");
+			fatal("proc_run: chroot");
 		if (chdir("/") == -1)
-			fatal("run_proc: chdir(\"/\")");
+			fatal("proc_run: chdir(\"/\")");
 	}
 #endif
 
@@ -360,7 +395,7 @@ proc_run(struct privsep *ps, struct privsep_proc *p,
 	if (setgroups(1, &pw->pw_gid) ||
 	    setresgid(pw->pw_gid, pw->pw_gid, pw->pw_gid) ||
 	    setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid))
-		fatal("run_proc: cannot drop privileges");
+		fatal("proc_run: cannot drop privileges");
 #endif
 
 	/* Fork child handlers */
@@ -390,7 +425,7 @@ proc_run(struct privsep *ps, struct privsep_proc *p,
 	signal_add(&ps->ps_evsighup, NULL);
 	signal_add(&ps->ps_evsigpipe, NULL);
 
-	proc_config(ps, procs, nproc);
+	proc_listen(ps, procs, nproc);
 
 	if (p->p_id == PROC_CONTROL && ps->ps_instance == 0) {
 		TAILQ_INIT(&ctl_conns);
@@ -531,22 +566,9 @@ void
 proc_range(struct privsep *ps, enum privsep_procid id, int *n, int *m)
 {
 	if (*n == -1) {
-		/*
-		 * -1 means that the current process is
-		 * N:1 - one of many processes connected to a single peer,
-		 *       so find the right slot of the peer.
-		 * 1:N - a single process connected to many peers,
-		 *       so find the range of slots of all peers.
-		 */
-		if (ps->ps_instances[privsep_process] <=
-		    ps->ps_instances[id]) {
-			*n = 0;
-			*m = ps->ps_instances[id];
-			return;
-		}
-
-		*n = ps->ps_instance;
-		*m = ps->ps_instance + 1;
+		/* Use a range of all target instances */
+		*n = 0;
+		*m = ps->ps_instances[id];
 	} else {
 		/* Use only a single slot of the specified peer process */
 		*m = *n + 1;
@@ -560,10 +582,11 @@ proc_compose_imsg(struct privsep *ps, enum privsep_procid id, int n,
 	int	 m;
 
 	proc_range(ps, id, &n, &m);
-	for (; n < m; n++)
+	for (; n < m; n++) {
 		if (imsg_compose_event(&ps->ps_ievs[id][n],
 		    type, -1, 0, fd, data, datalen) == -1)
 			return (-1);
+	}
 
 	return (0);
 }
diff --git a/usr.sbin/relayd/relayd.c b/usr.sbin/relayd/relayd.c
index 6051bd55278..afabd002224 100644
--- a/usr.sbin/relayd/relayd.c
+++ b/usr.sbin/relayd/relayd.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: relayd.c,v 1.120 2014/04/18 13:55:26 reyk Exp $	*/
+/*	$OpenBSD: relayd.c,v 1.121 2014/04/20 14:48:29 reyk Exp $	*/
 
 /*
  * Copyright (c) 2007 - 2014 Reyk Floeter <reyk@openbsd.org>
@@ -226,6 +226,8 @@ main(int argc, char *argv[])
 
 	ps->ps_instances[PROC_RELAY] = env->sc_prefork_relay;
 	ps->ps_instances[PROC_CA] = env->sc_prefork_relay;
+	ps->ps_ninstances = env->sc_prefork_relay;
+
 	proc_init(ps, procs, nitems(procs));
 
 	setproctitle("parent");
@@ -244,7 +246,7 @@ main(int argc, char *argv[])
 	signal_add(&ps->ps_evsighup, NULL);
 	signal_add(&ps->ps_evsigpipe, NULL);
 
-	proc_config(ps, procs, nitems(procs));
+	proc_listen(ps, procs, nitems(procs));
 
 	if (load_config(env->sc_conffile, env) == -1) {
 		proc_kill(env->sc_ps);
diff --git a/usr.sbin/relayd/relayd.h b/usr.sbin/relayd/relayd.h
index bce53dff249..559b167deab 100644
--- a/usr.sbin/relayd/relayd.h
+++ b/usr.sbin/relayd/relayd.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: relayd.h,v 1.175 2014/04/18 13:55:26 reyk Exp $	*/
+/*	$OpenBSD: relayd.h,v 1.176 2014/04/20 14:48:29 reyk Exp $	*/
 
 /*
  * Copyright (c) 2006 - 2014 Reyk Floeter <reyk@openbsd.org>
@@ -860,14 +860,21 @@ enum privsep_procid {
 /* Attach the control socket to the following process */
 #define PROC_CONTROL	PROC_PFE
 
+struct privsep_pipes {
+	int				*pp_pipes[PROC_MAX];
+};
+
 struct privsep {
-	int				*ps_pipes[PROC_MAX][PROC_MAX];
+	struct privsep_pipes		*ps_pipes[PROC_MAX];
+	struct privsep_pipes		*ps_pp;
+
 	struct imsgev			*ps_ievs[PROC_MAX];
 	const char			*ps_title[PROC_MAX];
 	pid_t				 ps_pid[PROC_MAX];
 	u_int8_t			 ps_what[PROC_MAX];
 
 	u_int				 ps_instances[PROC_MAX];
+	u_int				 ps_ninstances;
 	u_int				 ps_instance;
 
 	struct control_sock		 ps_csock;
@@ -1178,8 +1185,7 @@ __dead void fatalx(const char *);
 /* proc.c */
 void	 proc_init(struct privsep *, struct privsep_proc *, u_int);
 void	 proc_kill(struct privsep *);
-void	 proc_clear(struct privsep *, int);
-void	 proc_config(struct privsep *, struct privsep_proc *, u_int);
+void	 proc_listen(struct privsep *, struct privsep_proc *, size_t);
 void	 proc_dispatch(int, short event, void *);
 pid_t	 proc_run(struct privsep *, struct privsep_proc *,
 	    struct privsep_proc *, u_int,