Re: ldpd engine process exits with pledge "cpath"
Hi!
Ricardo Mestre wrote:
> this is getting in my nerves, I made a wrong assumption here and I
> prefer to commit this than backout my previous commit so I'm asking for
> commits for the below.
attached. I hope I understood your intention.
>
> On 14:43 Fri 19 Jun , Ricardo Mestre wrote:
> > mea culpa, but I'd rather just remove the unlink of the socket.
> >
> > OK?
> >
> > Index: control.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ldpd/control.c,v
> > retrieving revision 1.29
> > diff -u -p -u -r1.29 control.c
> > --- control.c 3 Mar 2017 23:30:57 - 1.29
> > +++ control.c 19 Jun 2020 13:40:46 -
> > @@ -98,11 +98,10 @@ control_listen(void)
> > }
> >
> > void
> > -control_cleanup(char *path)
> > +control_cleanup(void)
> > {
> > accept_del(control_fd);
> > close(control_fd);
> > - unlink(path);
> > }
> >
> > /* ARGSUSED */
> > Index: control.h
> > ===
> > RCS file: /cvs/src/usr.sbin/ldpd/control.h,v
> > retrieving revision 1.9
> > diff -u -p -u -r1.9 control.h
> > --- control.h 3 Mar 2017 23:30:57 - 1.9
> > +++ control.h 19 Jun 2020 13:40:46 -
> > @@ -32,7 +32,7 @@ extern struct ctl_conns ctl_conns;
> >
> > intcontrol_init(char *);
> > intcontrol_listen(void);
> > -void control_cleanup(char *);
> > +void control_cleanup(void);
> > intcontrol_imsg_relay(struct imsg *);
> >
> > #endif /* _CONTROL_H_ */
> > Index: ldpe.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ldpd/ldpe.c,v
> > retrieving revision 1.76
> > diff -u -p -u -r1.76 ldpe.c
> > --- ldpe.c 10 Aug 2019 01:30:53 - 1.76
> > +++ ldpe.c 19 Jun 2020 13:40:46 -
> > @@ -171,7 +171,7 @@ ldpe_shutdown(void)
> > msgbuf_clear(_main->ibuf.w);
> > close(iev_main->ibuf.fd);
> >
> > - control_cleanup(global.csock);
> > + control_cleanup();
> > config_clear(leconf);
> >
> > if (sysdep.no_pfkey == 0) {
> >
diff refs/heads/master 1544d4b6d260a07402505aca7822cc1399dacf31
blob - fe0eebdbea6bc8a5090782a5cd91f4c803651caf
blob + 071953e26373fd816177e39876fa1065f7c32835
--- usr.sbin/ldpd/control.c
+++ usr.sbin/ldpd/control.c
@@ -98,11 +98,10 @@ control_listen(void)
}
void
-control_cleanup(char *path)
+control_cleanup(void)
{
accept_del(control_fd);
close(control_fd);
- unlink(path);
}
/* ARGSUSED */
blob - f4f5525707d23ffb059e229177b0840fc1e31230
blob + 7e74ac093d7e8b16e7b9ea19362d3116d3edf2e9
--- usr.sbin/ldpd/control.h
+++ usr.sbin/ldpd/control.h
@@ -32,7 +32,7 @@ extern struct ctl_conns ctl_conns;
intcontrol_init(char *);
intcontrol_listen(void);
-void control_cleanup(char *);
+void control_cleanup(void);
intcontrol_imsg_relay(struct imsg *);
#endif /* _CONTROL_H_ */
blob - 6200d552882ca678e391c59922ccf358439ddac4
blob + a8d2a8a9c842d045588defa9a53dcb1caeda765a
--- usr.sbin/ldpd/ldpe.c
+++ usr.sbin/ldpd/ldpe.c
@@ -171,7 +171,7 @@ ldpe_shutdown(void)
msgbuf_clear(_main->ibuf.w);
close(iev_main->ibuf.fd);
- control_cleanup(global.csock);
+ control_cleanup();
config_clear(leconf);
if (sysdep.no_pfkey == 0) {
Re: ldpd engine process exits with pledge "cpath"
On Fri, Jun 19, 2020 at 02:43:00PM +0100, Ricardo Mestre wrote:
> mea culpa, but I'd rather just remove the unlink of the socket.
>
> OK?
Diff reads OK to me.
We had the same discussion in 2018 for ripd:
https://marc.info/?l=openbsd-tech=154101413029926=2
Note to self: ospfd should get the same treatment ...
>
> Index: control.c
> ===
> RCS file: /cvs/src/usr.sbin/ldpd/control.c,v
> retrieving revision 1.29
> diff -u -p -u -r1.29 control.c
> --- control.c 3 Mar 2017 23:30:57 - 1.29
> +++ control.c 19 Jun 2020 13:40:46 -
> @@ -98,11 +98,10 @@ control_listen(void)
> }
>
> void
> -control_cleanup(char *path)
> +control_cleanup(void)
> {
> accept_del(control_fd);
> close(control_fd);
> - unlink(path);
> }
>
> /* ARGSUSED */
> Index: control.h
> ===
> RCS file: /cvs/src/usr.sbin/ldpd/control.h,v
> retrieving revision 1.9
> diff -u -p -u -r1.9 control.h
> --- control.h 3 Mar 2017 23:30:57 - 1.9
> +++ control.h 19 Jun 2020 13:40:46 -
> @@ -32,7 +32,7 @@ extern struct ctl_conns ctl_conns;
>
> int control_init(char *);
> int control_listen(void);
> -void control_cleanup(char *);
> +void control_cleanup(void);
> int control_imsg_relay(struct imsg *);
>
> #endif /* _CONTROL_H_ */
> Index: ldpe.c
> ===
> RCS file: /cvs/src/usr.sbin/ldpd/ldpe.c,v
> retrieving revision 1.76
> diff -u -p -u -r1.76 ldpe.c
> --- ldpe.c10 Aug 2019 01:30:53 - 1.76
> +++ ldpe.c19 Jun 2020 13:40:46 -
> @@ -171,7 +171,7 @@ ldpe_shutdown(void)
> msgbuf_clear(_main->ibuf.w);
> close(iev_main->ibuf.fd);
>
> - control_cleanup(global.csock);
> + control_cleanup();
> config_clear(leconf);
>
> if (sysdep.no_pfkey == 0) {
>
Re: ldpd engine process exits with pledge "cpath"
I asked a simple question. Does the control program *work ok* with
this proposed change.
With the socket to nowhere:
ldpctl show
ldpctl: connect: /var/run/ldpd.sock: Connection refused
Delete it, and retry:
ldpctl show
ldpctl: connect: /var/run/ldpd.sock: No such file or directory
Yes it looks ok. Please *always show that the change doesn't have
a bad effect*.
OK deraadt
Ricardo Mestre wrote:
> mea culpa, but I'd rather just remove the unlink of the socket.
>
> OK?
>
> Index: control.c
> ===
> RCS file: /cvs/src/usr.sbin/ldpd/control.c,v
> retrieving revision 1.29
> diff -u -p -u -r1.29 control.c
> --- control.c 3 Mar 2017 23:30:57 - 1.29
> +++ control.c 19 Jun 2020 13:40:46 -
> @@ -98,11 +98,10 @@ control_listen(void)
> }
>
> void
> -control_cleanup(char *path)
> +control_cleanup(void)
> {
> accept_del(control_fd);
> close(control_fd);
> - unlink(path);
> }
>
> /* ARGSUSED */
> Index: control.h
> ===
> RCS file: /cvs/src/usr.sbin/ldpd/control.h,v
> retrieving revision 1.9
> diff -u -p -u -r1.9 control.h
> --- control.h 3 Mar 2017 23:30:57 - 1.9
> +++ control.h 19 Jun 2020 13:40:46 -
> @@ -32,7 +32,7 @@ extern struct ctl_conns ctl_conns;
>
> int control_init(char *);
> int control_listen(void);
> -void control_cleanup(char *);
> +void control_cleanup(void);
> int control_imsg_relay(struct imsg *);
>
> #endif /* _CONTROL_H_ */
> Index: ldpe.c
> ===
> RCS file: /cvs/src/usr.sbin/ldpd/ldpe.c,v
> retrieving revision 1.76
> diff -u -p -u -r1.76 ldpe.c
> --- ldpe.c10 Aug 2019 01:30:53 - 1.76
> +++ ldpe.c19 Jun 2020 13:40:46 -
> @@ -171,7 +171,7 @@ ldpe_shutdown(void)
> msgbuf_clear(_main->ibuf.w);
> close(iev_main->ibuf.fd);
>
> - control_cleanup(global.csock);
> + control_cleanup();
> config_clear(leconf);
>
> if (sysdep.no_pfkey == 0) {
>
Re: ldpd engine process exits with pledge "cpath"
Klemens Nanni wrote: > On Fri, Jun 19, 2020 at 11:19:24PM +0100, Ricardo Mestre wrote: > > this is getting in my nerves, I made a wrong assumption here and I > > prefer to commit this than backout my previous commit so I'm asking for > > commits for the below. > Wasn't there another daemon as well that stopped unlinking the socket > in favour of fewer pledge promises? > > Given that control_init() does unlink the control socket early on, I > think this is a sensible approach, OK kn. the test is whether the control-program behaves nicely with a dead control socket
Re: ldpd engine process exits with pledge "cpath"
On Fri, Jun 19, 2020 at 11:19:24PM +0100, Ricardo Mestre wrote: > this is getting in my nerves, I made a wrong assumption here and I > prefer to commit this than backout my previous commit so I'm asking for > commits for the below. Wasn't there another daemon as well that stopped unlinking the socket in favour of fewer pledge promises? Given that control_init() does unlink the control socket early on, I think this is a sensible approach, OK kn.
Re: ldpd engine process exits with pledge "cpath"
this is getting in my nerves, I made a wrong assumption here and I
prefer to commit this than backout my previous commit so I'm asking for
commits for the below.
On 14:43 Fri 19 Jun , Ricardo Mestre wrote:
> mea culpa, but I'd rather just remove the unlink of the socket.
>
> OK?
>
> Index: control.c
> ===
> RCS file: /cvs/src/usr.sbin/ldpd/control.c,v
> retrieving revision 1.29
> diff -u -p -u -r1.29 control.c
> --- control.c 3 Mar 2017 23:30:57 - 1.29
> +++ control.c 19 Jun 2020 13:40:46 -
> @@ -98,11 +98,10 @@ control_listen(void)
> }
>
> void
> -control_cleanup(char *path)
> +control_cleanup(void)
> {
> accept_del(control_fd);
> close(control_fd);
> - unlink(path);
> }
>
> /* ARGSUSED */
> Index: control.h
> ===
> RCS file: /cvs/src/usr.sbin/ldpd/control.h,v
> retrieving revision 1.9
> diff -u -p -u -r1.9 control.h
> --- control.h 3 Mar 2017 23:30:57 - 1.9
> +++ control.h 19 Jun 2020 13:40:46 -
> @@ -32,7 +32,7 @@ extern struct ctl_conns ctl_conns;
>
> int control_init(char *);
> int control_listen(void);
> -void control_cleanup(char *);
> +void control_cleanup(void);
> int control_imsg_relay(struct imsg *);
>
> #endif /* _CONTROL_H_ */
> Index: ldpe.c
> ===
> RCS file: /cvs/src/usr.sbin/ldpd/ldpe.c,v
> retrieving revision 1.76
> diff -u -p -u -r1.76 ldpe.c
> --- ldpe.c10 Aug 2019 01:30:53 - 1.76
> +++ ldpe.c19 Jun 2020 13:40:46 -
> @@ -171,7 +171,7 @@ ldpe_shutdown(void)
> msgbuf_clear(_main->ibuf.w);
> close(iev_main->ibuf.fd);
>
> - control_cleanup(global.csock);
> + control_cleanup();
> config_clear(leconf);
>
> if (sysdep.no_pfkey == 0) {
>
Re: ldpd engine process exits with pledge "cpath"
mea culpa, but I'd rather just remove the unlink of the socket.
OK?
Index: control.c
===
RCS file: /cvs/src/usr.sbin/ldpd/control.c,v
retrieving revision 1.29
diff -u -p -u -r1.29 control.c
--- control.c 3 Mar 2017 23:30:57 - 1.29
+++ control.c 19 Jun 2020 13:40:46 -
@@ -98,11 +98,10 @@ control_listen(void)
}
void
-control_cleanup(char *path)
+control_cleanup(void)
{
accept_del(control_fd);
close(control_fd);
- unlink(path);
}
/* ARGSUSED */
Index: control.h
===
RCS file: /cvs/src/usr.sbin/ldpd/control.h,v
retrieving revision 1.9
diff -u -p -u -r1.9 control.h
--- control.h 3 Mar 2017 23:30:57 - 1.9
+++ control.h 19 Jun 2020 13:40:46 -
@@ -32,7 +32,7 @@ extern struct ctl_conns ctl_conns;
intcontrol_init(char *);
intcontrol_listen(void);
-void control_cleanup(char *);
+void control_cleanup(void);
intcontrol_imsg_relay(struct imsg *);
#endif /* _CONTROL_H_ */
Index: ldpe.c
===
RCS file: /cvs/src/usr.sbin/ldpd/ldpe.c,v
retrieving revision 1.76
diff -u -p -u -r1.76 ldpe.c
--- ldpe.c 10 Aug 2019 01:30:53 - 1.76
+++ ldpe.c 19 Jun 2020 13:40:46 -
@@ -171,7 +171,7 @@ ldpe_shutdown(void)
msgbuf_clear(_main->ibuf.w);
close(iev_main->ibuf.fd);
- control_cleanup(global.csock);
+ control_cleanup();
config_clear(leconf);
if (sysdep.no_pfkey == 0) {
ldpd engine process exits with pledge "cpath"
The problems is that ldpd exits with a pledge violation. However, it happens
when ldpd is terminating during ldpe_shutdown().
OpenBSD 6.7 (GENERIC.MP) #182: Thu May 7 11:11:58 MDT 2020
dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
A demonstration of the problem can be achieved on a newly installed OpenBSD
6.7 as follows:
# (umask -S 0177; touch /etc/ldpd.conf); ldpd -dv & p=$!; sleep 1; kill $p
[1] 38081
startup
accept_add: accepting on fd 4
lde add fec
kernel routing table decoupled
accept_del: 4 removed from queue
waiting for children to terminate
label decision engine exiting
ldp engine terminated; signal 6
terminating
The "cpath" pledge(2) was removed from the engine process, from what I
understood from the commit message, on the false premise that the socket is
unlinked by the main process.
The fix, is I see it, would be to either 1) actually unlink the socket in
the main process or 2) add "cpath" back to the engine process. 2) seems
like a bad option, but 1) makes me doubt my assumptions since this story
contains contradicting statements.
So, I propose that the the call of control_cleanup() is moved from
ldpe_shutdown() to ldpd_shutdown(). This changes the order in which the
unlink(2) call happens relatively to the other steps in ldpe_shutdown(),
however this should be of no concern as I see it.
diff refs/heads/master 098bb599a09e458fcda8f1c5c8b82ddbb0d6970e
blob - b867d7961dc964ad7f7e31cbbe1cd6903d87364b
blob + 99bdd2d52f4898844de5e704448e0e2e41158d6c
--- usr.sbin/ldpd/ldpd.c
+++ usr.sbin/ldpd/ldpd.c
@@ -34,13 +34,13 @@
#include "ldpd.h"
#include "ldpe.h"
#include "lde.h"
+#include "control.h"
#include "log.h"
static void main_sig_handler(int, short, void *);
static __dead void usage(void);
static __dead void ldpd_shutdown(void);
-static pid_tstart_child(enum ldpd_process, char *, int, int, int,
- char *);
+static pid_tstart_child(enum ldpd_process, char *, int, int, int);
static void main_dispatch_ldpe(int, short, void *);
static void main_dispatch_lde(int, short, void *);
static int main_imsg_compose_both(enum imsg_type, void *,
@@ -71,6 +71,7 @@ static struct imsgev *iev_ldpe;
static struct imsgev *iev_lde;
static pid_tldpe_pid;
static pid_tlde_pid;
+static char*sockname;
/* ARGSUSED */
static void
@@ -111,7 +112,6 @@ main(int argc, char *argv[])
char*saved_argv0;
int ch;
int debug = 0, lflag = 0, eflag = 0;
- char*sockname;
int pipe_parent2ldpe[2];
int pipe_parent2lde[2];
@@ -216,11 +216,9 @@ main(int argc, char *argv[])
/* start children */
lde_pid = start_child(PROC_LDE_ENGINE, saved_argv0,
- pipe_parent2lde[1], debug, global.cmd_opts & LDPD_OPT_VERBOSE,
- NULL);
+ pipe_parent2lde[1], debug, global.cmd_opts & LDPD_OPT_VERBOSE);
ldpe_pid = start_child(PROC_LDP_ENGINE, saved_argv0,
- pipe_parent2ldpe[1], debug, global.cmd_opts & LDPD_OPT_VERBOSE,
- sockname);
+ pipe_parent2ldpe[1], debug, global.cmd_opts & LDPD_OPT_VERBOSE);
if (unveil("/", "r") == -1)
fatal("unveil");
@@ -297,6 +295,7 @@ ldpd_shutdown(void)
close(iev_lde->ibuf.fd);
kr_shutdown();
+ control_cleanup(sockname);
config_clear(ldpd_conf);
log_debug("waiting for children to terminate");
@@ -319,8 +318,7 @@ ldpd_shutdown(void)
}
static pid_t
-start_child(enum ldpd_process p, char *argv0, int fd, int debug, int verbose,
-char *sockname)
+start_child(enum ldpd_process p, char *argv0, int fd, int debug, int verbose)
{
char*argv[5];
int argc = 0;
@@ -351,16 +349,14 @@ start_child(enum ldpd_process p, char *argv0, int fd,
break;
case PROC_LDP_ENGINE:
argv[argc++] = "-E";
+ argv[argc++] = "-s";
+ argv[argc++] = sockname;
break;
}
if (debug)
argv[argc++] = "-d";
if (verbose)
argv[argc++] = "-v";
- if (sockname) {
- argv[argc++] = "-s";
- argv[argc++] = sockname;
- }
argv[argc++] = NULL;
execvp(argv0, argv);
blob - 6200d552882ca678e391c59922ccf358439ddac4
blob + 2b769ebd8a42ef11711b46320759741559562ed9
--- usr.sbin/ldpd/ldpe.c
+++ usr.sbin/ldpd/ldpe.c
@@ -171,7 +171,6 @@ ldpe_shutdown(void)
msgbuf_clear(_main->ibuf.w);
close(iev_main->ibuf.fd);
- control_cleanup(global.csock);
config_clear(leconf);
if (sysdep.no_pfkey == 0) {
