Re: [PATCH] pasta: wait for netns setup before calling exec

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 3 Feb 2023 17:37:03 +0100
Stefano Brivio <sbrivio@redhat.com> wrote:

> On Fri, 3 Feb 2023 15:44:40 +0100
> Paul Holzinger <pholzing@redhat.com> wrote:
> 
> > On 02/02/2023 11:25, Stefano Brivio wrote:  
> > > On Wed,  1 Feb 2023 19:01:16 +0100
> > > Paul Holzinger <pholzing@redhat.com> wrote:
> > >    
> > >> When a user spawns a command with pasta they expect the network to be
> > >> ready. Currently this does not work because pasta will fork/exec
> > >> before it will setup the network config.
> > >>
> > >> This patch fixes it by using a pipe to sync parent and child. The child
> > >> will now block reading from this pipe before the exec call. The parent
> > >> will then unblock the child only after the netns was configured.    
> > > Thanks for the patch! I'm reviewing this in a bit.
> > >
> > > A few considerations meanwhile:
> > >
> > > - there's actually a bigger issue (you're fixing here) than the
> > >    namespace configuration (via netlink) itself: the tap device isn't
> > >    ready (tap_sock_init() hasn't been called yet) when we spawn the
> > >    command in the new namespace. Oops.
> > >
> > >    If you're wondering: we can't just reorder things, because to complete
> > >    the configuration phase (conf()) we need the namespace to be set up,
> > >    and we can't initialise the tap device before it's set up
> > >
> > > - pipes are more commonly used to transfer data around (hence the whole
> > >    code you need to open a communication channel, check it, close it).
> > >    Did you try with a signal? Or is there a reason why it wouldn't work?
> > >
> > >    You could simply SIGSTOP the child, from the child itself:
> > >
> > > 	kill(getpid(), SIGSTOP);
> > >
> > >    and send a SIGCONT to it (we already store the PID of the child in
> > >    pasta_child_pid) once we're ready.
> > >
> > >    SIGCONT is special in that it doesn't need CAP_KILL or the processes
> > >    to run under the same UID -- just in the same session, so it wouldn't
> > >    risk interfering with the isolation_*() calls.
> > >
> > >    I haven't tested this but I think it should lead to simpler code.  
> >
> > Thinking about this more STOP/CONT will not work reliably, it could stop
> > the child forever when the parent sends SIGCONT before the child
> > SIGSTOPs itself. While this is unlikely we have no control over how both
> > processes are scheduled.
> > 
> > With this pipe version there is no problem when the parent closes the fd
> > before the child calls read, read will simply return EOF and the child can
> > continue, thus it will work correctly in all cases.  
> 
> Ah, right, nice catch. Still, you could probably use pause() or
> sigsuspend() instead of the SIGSTOP. Let me try a quick stand-alone
> experiment and I'll get back to you (probably early next week), unless
> you manage to get it working before.

Sorry, forget about it -- it doesn't solve the problem of waiting, in
the parent, that the child is stopped, which is exactly the point you
raised. A waitpid() with WUNTRACED does:

#include <stdio.h>
#include <unistd.h>
#include <signal.h>
#include <sys/wait.h>

#define DELAY_PARENT	0
#define DELAY_CHILD	0

int main()
{
	pid_t pid;
	int i;

	if ((pid = fork())) {
#if DELAY_PARENT
		for (i = 0; i < 10000000; i++);
#endif
		waitpid(pid, NULL, WUNTRACED);
		kill(pid, SIGCONT);
		sleep(1);
		return 0;
	}

#if DELAY_CHILD
	for (i = 0; i < 10000000; i++);
#endif
	raise(SIGSTOP);
	fprintf(stderr, "received SIGCONT\n");
	return 0;
}

I left in some busyloops you can use to check. It's three lines, with
error checks probably 9, still less than the pipe thing (~16) and it
looks simpler (to me).

-- 
Stefano