On Thu, Dec 11, 2025 at 02:49:50PM +0100, Paul Holzinger wrote: > > On 11/12/2025 00:45, David Gibson wrote: > > On Wed, Dec 10, 2025 at 01:05:10PM +0100, Paul Holzinger wrote: > > > On 10/12/2025 06:15, David Gibson wrote: > > > > When pasta is invoked with a command rather than an existing namespace to > > > > attach to, it spawns a child process to run a shell or other command. We > > > > create that process during conf(), since we need the namespace to exist for > > > > much of our setup. However, we don't want the specified command to run > > > > until the pasta network interface is ready for use. Therefore, > > > > pasta_spawn_cmd() executing in the child waits before exec()ing. main() > > > > signals the child to continue with SIGUSR1 shortly before entering the > > > > main forwarding loop. > > > > > > > > This has the downside that if we exit due to any kind of failure between > > > > conf() and the SIGUSR1, the child process will be around waiting > > > > indefinitely. The user must manually clean this up. > > > > > > > > Make this cleaner, by having passt_exit() terminate the child, when called > > > > during this window. > > > > > > > > Signed-off-by: David Gibson > > > > --- > > > > passt.c | 1 + > > > > pasta.c | 2 ++ > > > > pasta.h | 1 + > > > > util.c | 5 +++++ > > > > 4 files changed, 9 insertions(+) > > > > > > > > diff --git a/passt.c b/passt.c > > > > index cf38822f..955c7091 100644 > > > > --- a/passt.c > > > > +++ b/passt.c > > > > @@ -431,6 +431,7 @@ int main(int argc, char **argv) > > > > if (pasta_child_pid) { > > > > kill(pasta_child_pid, SIGUSR1); > > > > log_stderr = false; > > > > + pasta_child_signalled = true; > > > > } > > > > isolate_postfork(&c); > > > > diff --git a/pasta.c b/pasta.c > > > > index 5c693de1..8ac4511f 100644 > > > > --- a/pasta.c > > > > +++ b/pasta.c > > > > @@ -54,6 +54,8 @@ > > > > /* PID of child, in case we created a namespace */ > > > > int pasta_child_pid; > > > > +/* Has the child been signalled to start a shell or command */ > > > > +bool pasta_child_signalled; > > > > /** > > > > * pasta_child_handler() - Exit once shell exits (if we started it), reap clones > > > > diff --git a/pasta.h b/pasta.h > > > > index 4b063d13..55028c74 100644 > > > > --- a/pasta.h > > > > +++ b/pasta.h > > > > @@ -7,6 +7,7 @@ > > > > #define PASTA_H > > > > extern int pasta_child_pid; > > > > +extern bool pasta_child_signalled; > > > > void pasta_open_ns(struct ctx *c, const char *netns); > > > > void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid, > > > > diff --git a/util.c b/util.c > > > > index e266c396..4744f09e 100644 > > > > --- a/util.c > > > > +++ b/util.c > > > > @@ -35,6 +35,7 @@ > > > > #include "log.h" > > > > #include "pcap.h" > > > > #include "epoll_ctl.h" > > > > +#include "pasta.h" > > > > #ifdef HAS_GETRANDOM > > > > #include > > > > #endif > > > > @@ -1235,6 +1236,10 @@ void abort_with_msg(const char *fmt, ...) > > > > */ > > > > void passt_exit(int status) > > > > { > > > > + /* If we're starting our own namespace, don't leave it in limbo */ > > > > + if (pasta_child_pid && !pasta_child_signalled) > > > > + kill(pasta_child_pid, SIGTERM); > > > Any reason not to use SIGKILL? Then there is no doubt if it might be ignored > > > or not. > > Eh, only that some log might show KILLs on the assumption that they > > indicate a bad thing happening, which isn't really the case here. > > > > > Another thing I assume the goal here is to only kill the process if we > > > didn't exec yet. > > Yes, that was the goal. > > > > > I wonder how much value there is to have the child process > > > outlive pasta. > > That's a good question, and I don't have a strong opinion either way. > > I leant this way based on two factors: > > > > - If this happens later, once the child is established, it's possible > > the user could have started running something there that remains > > valuable even if it loses its network > > > > - If pasta dies once the child has exec()ed, the symptoms are both > > more obvious and easier to clean up: the shell (or whatever) is > > right there, running, and can be exited in the normal way > > (e.g. running to completion or ^C). Before the exec() this leaves > > a process running non-obviously which has to be spotted and killed > > explicitly by the user. > > > > (also note that if we do kill the child after exec(), we definitely > > want to use SIGTERM not SIGKILL to allow it to clean up gracefully). > > sure, I agree though there is bit of a common issue here. The process is in > a new pid namespace so it acts as pid 1. That means unless the process adds > a signal handler explicitly a signal like SIGTERM will get ignored by > default. When running `sleep` as pid 1 it will ignore SIGTERM and you must > kill it. We see that all the time with podman containers. Ah, good point. I guess my earlier draft only worked because we do, in fact have a SIGTERM handler. -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson