Re: [PATCH 2/2] pasta: Clean up waiting pasta child on failures

[Paul Holzinger <pholzing@redhat.com>]

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 <david@gibson.dropbear.id.au>
>>> ---
>>>    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 <sys/random.h>
>>>    #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.

>
>> I feel like using something like PR_SET_PDEATHSIG might be a
>> safer option in case pasta crashes without going through passt_exit.
> Oh, good idea.  I forgot PR_SET_PDEATHSIG was a thing.
>
>> And if
>> you don't want to propagate that into the user command we could unset it
>> right again before the exec as well.
>>
>>> +
>>>    	/* Make sure we don't leave the pcap file truncated */
>>>    	if (pcap_fd != -1 && fsync(pcap_fd))
>>>    		warn_perror("Failed to flush pcap file, it might be truncated");
>> -- 
>> Paul Holzinger
>>
-- 
Paul Holzinger