Download raw body.
snmpd: work on top of iked's proc.c
ping
I've also been looking at my diff to store the engine boots[0] again,
and concluded that I'd rather have such filesystem interacting code in
its own process, but since it doesn't wait for the socketpairs to be
distributed it fails to push the stored data. The engine boots diff
is required to fix the 2038 unix timestamp overflow issue for USM.
martijn@
[0] https://marc.info/?l=openbsd-tech&m=176269107324150&w=2
On 11/13/25 11:53 PM, Martijn van Duren wrote:
> Hello all,
>
> One eyesore for a long time for me is the recvfd pledge inside snmpe.c.
> This one is only there for the creating of socketpairs between multiple
> children. While strictly speaking not needed anymore since the removal
> of the traphandler process, the structure of proc.c kind of enforces
> its presence since proc_run() calls run() (snmpe_init()) right before
> calling event_dispatch(), where the socketpair fds are send through
> here. Since there's no callback after all socketpairs have been send
> it's impossible to remove the recvfd pledge at a trustworthy point.
>
> Looking at the other proc.c versions, I found that iked's version has
> support for IMSG_CTL_PROCREADY, although with a slightly different
> approach (a finalize function in PROC_PARENT after all children have
> answered). The diff below pulls in iked's proc.c with the following
> changes:
> - snmpd doesn't have a control process, so remove those calls (this
> was already removed from snmpd's proc.c)
> - Make proc_connect's conntected callback NULL-able. snmpd doesn't
> need it.
> - store proc_run's run() and arg inside struct privsep_proc, and
> delay their calls until IMSG_CTL_PROCREADY has been received.
> This way we know that all socketpair's have been received.
>
> Thoughts? OK?
>
> martijn@
>
diff refs/heads/master refs/heads/snmpd/proc_ready
commit - d620432c9d9a6e02affa0bfbb7d573fef66aa5ea
commit + bb36d6fa9c06c75551d887893a4725c5e0e42ef4
blob - bbf7335275bd1a95037ce8a1ff46e55dded65db9
blob + 6ac509f4a5252aa00d65f9713baf93bf861ce5c6
--- usr.sbin/snmpd/proc.c
+++ usr.sbin/snmpd/proc.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: proc.c,v 1.39 2024/11/21 13:38:45 claudio Exp $ */
+/* $OpenBSD: proc.c,v 1.51 2024/11/21 13:35:20 claudio Exp $ */
/*
* Copyright (c) 2010 - 2016 Reyk Floeter <reyk@openbsd.org>
@@ -17,13 +17,14 @@
* OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*/
+#include <sys/types.h>
+#include <sys/queue.h>
#include <sys/socket.h>
#include <sys/wait.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
-#include <stdint.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>
@@ -36,6 +37,8 @@
#include "log.h"
#include "snmpd.h"
+enum privsep_procid privsep_process;
+
void proc_exec(struct privsep *, struct privsep_proc *, unsigned int, int,
char **);
void proc_setup(struct privsep *, struct privsep_proc *, unsigned int);
@@ -139,14 +142,18 @@ proc_exec(struct privsep *ps, struct privsep_proc *pro
}
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)
+ connected(ps);
return;
+ }
+ ps->ps_connected = connected;
for (dst = 0; dst < PROC_MAX; dst++) {
/* We don't communicate with ourselves. */
@@ -157,7 +164,7 @@ proc_connect(struct privsep *ps)
iev = &ps->ps_ievs[dst][inst];
if (imsgbuf_init(&iev->ibuf,
ps->ps_pp->pp_pipes[dst][inst]) == -1)
- fatal("imsgbuf_init");
+ fatal("%s: imsgbuf_init", __func__);
imsgbuf_allow_fdpass(&iev->ibuf);
event_set(&iev->ev, iev->ibuf.fd, iev->events,
iev->handler, iev->data);
@@ -174,6 +181,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
@@ -268,7 +296,7 @@ proc_accept(struct privsep *ps, int fd, enum privsep_p
iev = &ps->ps_ievs[dst][n];
if (imsgbuf_init(&iev->ibuf, fd) == -1)
- fatal("imsgbuf_init");
+ fatal("%s: imsgbuf_init", __func__);
imsgbuf_allow_fdpass(&iev->ibuf);
event_set(&iev->ev, iev->ibuf.fd, iev->events, iev->handler, iev->data);
event_add(&iev->ev, NULL);
@@ -553,11 +581,14 @@ proc_run(struct privsep *ps, struct privsep_proc *p,
proc_setup(ps, procs, nproc);
proc_accept(ps, PROC_PARENT_SOCK_FILENO, PROC_PARENT, 0);
- DPRINTF("%s: %s %d/%d, pid %d", __func__, p->p_title,
+
+#if DEBUG
+ log_debug("%s: %s %d/%d, pid %d", __func__, p->p_title,
ps->ps_instance + 1, ps->ps_instances[p->p_id], getpid());
+#endif
- if (run != NULL)
- run(ps, p, arg);
+ p->p_run = run;
+ p->p_arg = arg;
event_dispatch();
@@ -593,13 +624,12 @@ proc_dispatch(int fd, short event, void *arg)
if (event & EV_WRITE) {
if (imsgbuf_write(ibuf) == -1) {
- if (errno == EPIPE) {
- /* this pipe is dead, remove the handler */
+ if (errno == EPIPE) { /* Connection closed. */
event_del(&iev->ev);
event_loopexit(NULL);
return;
- }
- fatal("%s: imsgbuf_write", __func__);
+ } else
+ fatal("imsgbuf_write");
}
}
@@ -639,6 +669,36 @@ 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__);
+ if (p->p_run != NULL) {
+ p->p_run(ps, p, p->p_arg);
+ p->p_run = NULL;
+ }
+ } 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_connecting == 0) {
+ log_debug("%s: all connected", __func__);
+ if (ps->ps_connected != NULL)
+ ps->ps_connected(ps);
+ }
+ }
+ break;
default:
fatalx("%s: %s %d got invalid imsg %d peerid %d "
"from %s %d",
@@ -762,6 +822,14 @@ proc_composev(struct privsep *ps, enum privsep_procid
return (proc_composev_imsg(ps, id, -1, type, -1, -1, iov, iovcnt));
}
+int
+proc_forward_imsg(struct privsep *ps, struct imsg *imsg,
+ enum privsep_procid id, int n)
+{
+ return (proc_compose_imsg(ps, id, n, imsg->hdr.type,
+ imsg->hdr.peerid, -1, imsg->data, IMSG_DATA_SIZE(imsg)));
+}
+
struct imsgbuf *
proc_ibuf(struct privsep *ps, enum privsep_procid id, int n)
{
blob - 28250dcac64845b2e14dcad58d8ff3850ccb0626
blob + 0570c5cd519fa1c534b46a85a8912c9211137c60
--- usr.sbin/snmpd/snmpd.c
+++ usr.sbin/snmpd/snmpd.c
@@ -46,8 +46,6 @@ static struct privsep_proc procs[] = {
{ "snmpe", PROC_SNMPE, snmpd_dispatch_snmpe, snmpe, snmpe_shutdown },
};
-enum privsep_procid privsep_process;
-
void
snmpd_sig_handler(int sig, short event, void *arg)
{
@@ -241,7 +239,7 @@ main(int argc, char *argv[])
signal_add(&ps->ps_evsigpipe, NULL);
signal_add(&ps->ps_evsigusr1, NULL);
- proc_connect(ps);
+ proc_connect(ps, NULL);
snmpd_backend(env);
if (pledge("stdio dns sendfd proc exec id", NULL) == -1)
blob - acef12a885ac8401e24b2624c5aa8c2645f744af
blob + 0327f3d6ecf9cd2eb64517cc90e747b3869e2739
--- usr.sbin/snmpd/snmpd.h
+++ usr.sbin/snmpd/snmpd.h
@@ -89,6 +89,7 @@ enum imsg_type {
IMSG_NONE,
IMSG_CTL_VERBOSE,
IMSG_CTL_PROCFD,
+ IMSG_CTL_PROCREADY,
IMSG_TRAP_EXEC,
IMSG_AX_FD
};
@@ -143,6 +144,8 @@ struct privsep {
struct event ps_evsigusr1;
void *ps_env;
+ unsigned int ps_connecting;
+ void (*ps_connected)(struct privsep *);
};
struct privsep_proc {
@@ -156,6 +159,11 @@ struct privsep_proc {
const char *p_chroot;
struct privsep *p_ps;
struct passwd *p_pw;
+
+ /* private */
+ void (*p_run)(struct privsep *,
+ struct privsep_proc *, void *);
+ void *p_arg;
};
struct privsep_fd {
@@ -492,7 +500,7 @@ enum privsep_procid
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 (*connected)(struct privsep *));
void proc_dispatch(int, short event, void *);
void proc_run(struct privsep *, struct privsep_proc *,
struct privsep_proc *, u_int,
@@ -511,6 +519,8 @@ int proc_composev_imsg(struct privsep *, enum privsep
u_int16_t, u_int32_t, int, const struct iovec *, int);
int proc_composev(struct privsep *, enum privsep_procid,
uint16_t, const struct iovec *, int);
+int proc_forward_imsg(struct privsep *, struct imsg *,
+ enum privsep_procid, int);
struct imsgbuf *
proc_ibuf(struct privsep *, enum privsep_procid, int);
struct imsgev *
blob - 9c0fee911b3b83ab56d4a284c6cd825de79d9a67
blob + d54a63b3ef24e905469be4efdf217ebb0b34483c
--- usr.sbin/snmpd/snmpe.c
+++ usr.sbin/snmpd/snmpe.c
@@ -110,7 +110,7 @@ snmpe_init(struct privsep *ps, struct privsep_proc *p,
/* no filesystem visibility */
if (unveil("/", "") == -1)
fatal("unveil /");
- if (pledge("stdio recvfd inet unix", NULL) == -1)
+ if (pledge("stdio inet unix", NULL) == -1)
fatal("pledge");
log_info("snmpe %s: ready",
snmpd: work on top of iked's proc.c