Introduce new IMSG_CTL_PROCREADY which is used to signal that all pipes

are set up by child processes. The parent sends a ping to all children
and only starts once it has received an acknowledgement from all of them.
This fixes a race condition on process startup when the parent starts
running before all children are ready.

From markus@

diff --git a/sbin/iked/iked.c b/sbin/iked/iked.c
index 7e9f6ad..00bd3f6 100644 (file)
--- a/sbin/iked/iked.c
+++ b/sbin/iked/iked.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: iked.c,v 1.69 2024/02/15 19:04:12 tobhe Exp $ */
+/*     $OpenBSD: iked.c,v 1.70 2024/02/15 20:10:45 tobhe Exp $ */
 
 /*
  * Copyright (c) 2019 Tobias Heider <tobias.heider@stusta.de>
@@ -45,6 +45,7 @@ void   parent_sig_handler(int, short, void *);
 int     parent_dispatch_ca(int, struct privsep_proc *, struct imsg *);
 int     parent_dispatch_control(int, struct privsep_proc *, struct imsg *);
 int     parent_dispatch_ikev2(int, struct privsep_proc *, struct imsg *);
+void    parent_connected(struct privsep *);
 int     parent_configure(struct iked *);
 
 struct iked    *iked_env;
@@ -219,12 +220,9 @@ main(int argc, char *argv[])
        signal_add(&ps->ps_evsigpipe, NULL);
        signal_add(&ps->ps_evsigusr1, NULL);
 
-       proc_connect(ps);
-
        vroute_init(env);
 
-       if (parent_configure(env) == -1)
-               fatalx("configuration failed");
+       proc_connect(ps, parent_connected);
 
        event_dispatch();
 
@@ -234,6 +232,15 @@ main(int argc, char *argv[])
        return (0);
 }
 
+void
+parent_connected(struct privsep *ps)
+{
+       struct iked     *env = ps->ps_env;
+
+       if (parent_configure(env) == -1)
+               fatalx("configuration failed");
+}
+
 int
 parent_configure(struct iked *env)
 {

diff --git a/sbin/iked/iked.h b/sbin/iked/iked.h
index f13e6a0..5b65a16 100644 (file)
--- a/sbin/iked/iked.h
+++ b/sbin/iked/iked.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: iked.h,v 1.228 2024/02/15 19:11:00 tobhe Exp $        */
+/*     $OpenBSD: iked.h,v 1.229 2024/02/15 20:10:45 tobhe Exp $        */
 
 /*
  * Copyright (c) 2019 Tobias Heider <tobias.heider@stusta.de>
@@ -730,6 +730,8 @@ struct privsep {
        struct event                     ps_evsigusr1;
 
        struct iked                     *ps_env;
+       unsigned int                     ps_connecting;
+       void                            (*ps_connected)(struct privsep *);
 };
 
 struct privsep_proc {
@@ -1192,7 +1194,7 @@ void       timer_del(struct iked *, struct iked_timer *);
 void    proc_init(struct privsep *, struct privsep_proc *, unsigned int, int,
            int, char **, enum privsep_procid);
 void    proc_kill(struct privsep *);
-void    proc_connect(struct privsep *);
+void    proc_connect(struct privsep *, void (*)(struct privsep *));
 void    proc_dispatch(int, short event, void *);
 void    proc_run(struct privsep *, struct privsep_proc *,
            struct privsep_proc *, unsigned int,

diff --git a/sbin/iked/proc.c b/sbin/iked/proc.c
index 6907884..534427b 100644 (file)
--- a/sbin/iked/proc.c
+++ b/sbin/iked/proc.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: proc.c,v 1.41 2024/02/15 19:04:12 tobhe Exp $ */
+/*     $OpenBSD: proc.c,v 1.42 2024/02/15 20:10:45 tobhe Exp $ */
 
 /*
  * Copyright (c) 2010 - 2016 Reyk Floeter <reyk@openbsd.org>
@@ -155,14 +155,19 @@ proc_exec(struct privsep *ps, struct privsep_proc *procs, unsigned int nproc,
 }
 
 void
-proc_connect(struct privsep *ps)
+proc_connect(struct privsep *ps, void (*connected)(struct privsep *))
 {
        struct imsgev           *iev;
        unsigned int             src, dst, inst;
 
        /* Don't distribute any sockets if we are not really going to run. */
-       if (ps->ps_noaction)
+       if (ps->ps_noaction) {
+               if (connected == NULL)
+                       fatalx("%s: missing callback", __func__);
+               connected(ps);
                return;
+       }
+       ps->ps_connected = connected;
 
        for (dst = 0; dst < PROC_MAX; dst++) {
                /* We don't communicate with ourselves. */
@@ -187,6 +192,27 @@ proc_connect(struct privsep *ps)
 
                        proc_open(ps, src, dst);
                }
+
+       /*
+        * Finally, send a ready message to everyone:
+        * When this message is processed by the receiver, it has
+        * already processed all IMSG_CTL_PROCFD messages and all
+        * pipes are ready.
+        */
+       for (dst = 0; dst < PROC_MAX; dst++) {
+               if (dst == PROC_PARENT)
+                       continue;
+               for (inst = 0; inst < ps->ps_instances[dst]; inst++) {
+                       if (proc_compose_imsg(ps, dst, inst, IMSG_CTL_PROCREADY,
+                           -1, -1, NULL, 0) == -1)
+                               fatal("%s: proc_compose_imsg", __func__);
+                       ps->ps_connecting++;
+#if DEBUG
+                       log_debug("%s: #%d %s %d", __func__,
+                           ps->ps_connecting, ps->ps_title[dst], inst + 1);
+#endif
+               }
+       }
 }
 
 void
@@ -663,6 +689,33 @@ proc_dispatch(int fd, short event, void *arg)
                        proc_accept(ps, imsg_get_fd(&imsg), pf.pf_procid,
                            pf.pf_instance);
                        break;
+               case IMSG_CTL_PROCREADY:
+#if DEBUG
+                       log_debug("%s: ready-%s: #%d %s %d -> %s %d", __func__,
+                           p->p_id == PROC_PARENT ? "req" : "ack",
+                           ps->ps_connecting, p->p_title, imsg.hdr.pid,
+                           title, ps->ps_instance + 1);
+#endif
+                       if (p->p_id == PROC_PARENT) {
+                               /* ack that we are ready */
+                               if (proc_compose_imsg(ps, PROC_PARENT, 0,
+                                   IMSG_CTL_PROCREADY, -1, -1, NULL, 0) == -1)
+                                       fatal("%s: proc_compose_imsg", __func__);
+                       } else {
+                               /* parent received ack */
+                               if (ps->ps_connecting == 0)
+                                       fatalx("%s: wrong acks", __func__);
+                               if (ps->ps_instance != 0)
+                                       fatalx("%s: wrong instance %d",
+                                           __func__, ps->ps_instance);
+                               if (ps->ps_connected == NULL)
+                                       fatalx("%s: missing callback", __func__);
+                               if (--ps->ps_connecting == 0) {
+                                       log_debug("%s: all connected", __func__);
+                                       ps->ps_connected(ps);
+                               }
+                       }
+                       break;
                default:
                        fatalx("%s: %s %d got invalid imsg %d peerid %d "
                            "from %s %d",

diff --git a/sbin/iked/types.h b/sbin/iked/types.h
index fd8add5..6690a4a 100644 (file)
--- a/sbin/iked/types.h
+++ b/sbin/iked/types.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: types.h,v 1.53 2024/01/15 15:29:00 tobhe Exp $        */
+/*     $OpenBSD: types.h,v 1.54 2024/02/15 20:10:45 tobhe Exp $        */
 
 /*
  * Copyright (c) 2019 Tobias Heider <tobias.heider@stusta.de>
@@ -132,6 +132,7 @@ enum imsg_type {
        IMSG_CTL_SHOW_CERTSTORE,
        IMSG_CTL_SHOW_STATS,
        IMSG_CTL_PROCFD,
+       IMSG_CTL_PROCREADY,
 };
 
 enum privsep_procid {