From: Martijn van Duren Subject: snmpd: work on top of iked's proc.c To: tech@openbsd.org Cc: Tobias Heider Date: Thu, 13 Nov 2025 23:53:16 +0100 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 /usr/src path + /usr/src commit - 0377483fd63d89703211981e944e9e01c48a851d blob - bbf7335275bd1a95037ce8a1ff46e55dded65db9 file + usr.sbin/snmpd/proc.c --- 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 @@ -17,13 +17,14 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ +#include +#include #include #include #include #include #include -#include #include #include #include @@ -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) { commit - 0377483fd63d89703211981e944e9e01c48a851d blob - 60af191fb6f00ff295542f65ed9c733b66038830 file + usr.sbin/snmpd/snmpd.c --- 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) { @@ -242,7 +240,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) commit - 0377483fd63d89703211981e944e9e01c48a851d blob - bc1b9ff7d6b8fab49046e4fe3bec9ba5fca31a35 file + usr.sbin/snmpd/snmpd.h --- usr.sbin/snmpd/snmpd.h +++ usr.sbin/snmpd/snmpd.h @@ -90,6 +90,7 @@ enum imsg_type { IMSG_NONE, IMSG_CTL_VERBOSE, IMSG_CTL_PROCFD, + IMSG_CTL_PROCREADY, IMSG_TRAP_EXEC, IMSG_AX_FD }; @@ -144,6 +145,8 @@ struct privsep { struct event ps_evsigusr1; void *ps_env; + unsigned int ps_connecting; + void (*ps_connected)(struct privsep *); }; struct privsep_proc { @@ -157,6 +160,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 { @@ -488,7 +496,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, @@ -507,6 +515,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 * commit - 0377483fd63d89703211981e944e9e01c48a851d blob - d444c8d9b8a47d7da9242e9372d1b88241dece5b file + usr.sbin/snmpd/snmpe.c --- 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",