Re: [PATCH user-cr 2/2] add nsexeccwp to test clone-with-pids

["Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
> On Mon, 2009-11-16 at 05:12 -0600, Serge E. Hallyn wrote:
> > Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
> > > On Thu, 2009-11-12 at 23:24 -0600, serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
> > > > +	if (use_clone) {
> > > > +		int stacksize = 4*getpagesize();
> > > > +		void *stack = malloc(stacksize);
> > > > +
> > > > +		if (!stack) {
> > > > +			perror("malloc");
> > > > +			return -1;
> > > > +		}
> > > > +
> > > > +		printf("about to clone with %lx\n", flags);
> > > > +		if (chosen_pid)
> > > > +			printf("Will choose pid %d\n", chosen_pid);
> > > > +		flags |= SIGCHLD;
> > > > +		pid = clone_with_pids(do_child, stack, flags, &pid_set,
> > > > +					(void *)argv);
> > > 
> > > The stack argument should be adjusted with the usual stack += stacksize
> > > - 1 or similar, right?
> > 
> > the clone_with_pids() helper in user-cr/clone_s390x.c (and IIRC the
> > x86 one by Suka also) does this implicitly, by doing:
> > 
> > 	s = child_stack;
> > 	*--s = arg;
> > 	*--s = fn;
> > 	child_stack -= 16
> 
> That's setting up arguments for the function to run in the child, and
> afaict that code assumes the value of child_stack is the _end_ of the
> stack region.

Yes.

> The code I quoted above is passing the beginning of the
> region (the return value from malloc).

Holy cow, that was a snafu in my switching to sending (stack_base,stack_size)
for the previous version, and then back again.  It was meant to send
stack_base+stack_size now.

I say 'holy cow' because it doesn't segfault on s390x.  And it certainly
should!

> On powerpc the segfaults went away when I made the following change.
> 
> diff --git a/nsexeccwp.c b/nsexeccwp.c
> index a71d9a4..92eb092 100644
> --- a/nsexeccwp.c
> +++ b/nsexeccwp.c
> @@ -309,8 +309,8 @@ int main(int argc, char *argv[])
>                 if (chosen_pid)
>                         printf("Will choose pid %d\n", chosen_pid);
>                 flags |= SIGCHLD;
> -               pid = clone_with_pids(do_child, stack, flags, &pid_set,
> -                                       (void *)argv);
> +               pid = clone_with_pids(do_child, stack + stacksize - 1,
> +                                     flags, &pid_set, (void *)argv);

Yes I don't think the -1 should be needed, but certainly the
+stacksize is.

thanks,
-serge