[PATCH v3] util, pasta: Add do_clone() wrapper around __clone2() and clone()

[PATCH v3] util, pasta: Add do_clone() wrapper around __clone2() and clone()

[Stefano Brivio]

Spotted in Debian's buildd logs: on ia64, clone(2) is not available:
the glibc wrapper is named __clone2() and it takes, additionally,
the size of the stack area passed by the caller.

Add a do_clone() wrapper handling the different cases, and also
taking care of pointing the child's stack in the middle of the
allocated area: on PA-RISC (hppa), handled by clone(), the stack
grows up, and on ia64 the stack grows down, but the register backing
store grows up -- and I think it might be actually used here.

Suggested-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
v3: Don't call it __clone(), it conflicts with glibc on static builds

v2: Add a wrapper instead of two __ia64__ ifdefs

 pasta.c | 10 +++++-----
 util.c  | 21 +++++++++++++++++++++
 util.h  |  8 +++++---
 3 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/pasta.c b/pasta.c
index db86317..528f02a 100644
--- a/pasta.c
+++ b/pasta.c
@@ -226,11 +226,11 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
 		arg.argv = sh_argv;
 	}
 
-	pasta_child_pid = clone(pasta_spawn_cmd,
-				ns_fn_stack + sizeof(ns_fn_stack) / 2,
-				CLONE_NEWIPC | CLONE_NEWPID | CLONE_NEWNET |
-				CLONE_NEWUTS,
-				(void *)&arg);
+	pasta_child_pid = do_clone(pasta_spawn_cmd, ns_fn_stack,
+				   sizeof(ns_fn_stack),
+				   CLONE_NEWIPC | CLONE_NEWPID | CLONE_NEWNET |
+				   CLONE_NEWUTS,
+				   (void *)&arg);
 
 	if (pasta_child_pid == -1) {
 		perror("clone");
diff --git a/util.c b/util.c
index be102e3..2125a67 100644
--- a/util.c
+++ b/util.c
@@ -482,3 +482,24 @@ int write_file(const char *path, const char *buf)
 	close(fd);
 	return len == 0 ? 0 : -1;
 }
+
+/**
+ * do_clone() - Wrapper of __clone2() for ia64, clone() for other architectures
+ * @fn:		Entry point for child
+ * @stack_area:	Stack area for child: we'll point callees to the middle of it
+ * @stack_size:	Total size of stack area, passed to callee divided by two
+ * @flags:	clone() system call flags
+ * @arg:	Argument to @fn
+ *
+ * Return: thread ID of child, -1 on failure
+ */
+int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
+	     void *arg)
+{
+#ifdef __ia64__
+	return __clone2(fn, stack_area + stack_size / 2, stack_size / 2,
+			flags, arg);
+#else
+	return clone(fn, stack_area + stack_size / 2, flags, arg);
+#endif
+}
diff --git a/util.h b/util.h
index 2d4e1ff..02b55a9 100644
--- a/util.h
+++ b/util.h
@@ -81,13 +81,15 @@
 	(((struct in_addr *)(a))->s_addr == ((struct in_addr *)b)->s_addr)
 
 #define NS_FN_STACK_SIZE	(RLIMIT_STACK_VAL * 1024 / 8)
+int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
+	     void *arg);
 #define NS_CALL(fn, arg)						\
 	do {								\
 		char ns_fn_stack[NS_FN_STACK_SIZE];			\
 									\
-		clone((fn), ns_fn_stack + sizeof(ns_fn_stack) / 2,	\
-		      CLONE_VM | CLONE_VFORK | CLONE_FILES | SIGCHLD,	\
-		      (void *)(arg));					\
+		do_clone((fn), ns_fn_stack, sizeof(ns_fn_stack),	\
+			 CLONE_VM | CLONE_VFORK | CLONE_FILES | SIGCHLD,\
+			 (void *)(arg));				\
 	} while (0)
 
 #if __BYTE_ORDER == __BIG_ENDIAN
-- 
@@ -81,13 +81,15 @@
 	(((struct in_addr *)(a))->s_addr == ((struct in_addr *)b)->s_addr)
 
 #define NS_FN_STACK_SIZE	(RLIMIT_STACK_VAL * 1024 / 8)
+int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
+	     void *arg);
 #define NS_CALL(fn, arg)						\
 	do {								\
 		char ns_fn_stack[NS_FN_STACK_SIZE];			\
 									\
-		clone((fn), ns_fn_stack + sizeof(ns_fn_stack) / 2,	\
-		      CLONE_VM | CLONE_VFORK | CLONE_FILES | SIGCHLD,	\
-		      (void *)(arg));					\
+		do_clone((fn), ns_fn_stack, sizeof(ns_fn_stack),	\
+			 CLONE_VM | CLONE_VFORK | CLONE_FILES | SIGCHLD,\
+			 (void *)(arg));				\
 	} while (0)
 
 #if __BYTE_ORDER == __BIG_ENDIAN
-- 
2.35.1

Re: [PATCH v3] util, pasta: Add do_clone() wrapper around __clone2() and clone()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3890 bytes --]

On Wed, Nov 16, 2022 at 05:30:57PM +0100, Stefano Brivio wrote:
> Spotted in Debian's buildd logs: on ia64, clone(2) is not available:
> the glibc wrapper is named __clone2() and it takes, additionally,
> the size of the stack area passed by the caller.
> 
> Add a do_clone() wrapper handling the different cases, and also
> taking care of pointing the child's stack in the middle of the
> allocated area: on PA-RISC (hppa), handled by clone(), the stack
> grows up, and on ia64 the stack grows down, but the register backing
> store grows up -- and I think it might be actually used here.
> 
> Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Heh, my only comment on v2 was going to be that it's not a good idea
to name identifiers starting with _ - that's reserved for the system
library.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> v3: Don't call it __clone(), it conflicts with glibc on static builds
> 
> v2: Add a wrapper instead of two __ia64__ ifdefs
> 
>  pasta.c | 10 +++++-----
>  util.c  | 21 +++++++++++++++++++++
>  util.h  |  8 +++++---
>  3 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/pasta.c b/pasta.c
> index db86317..528f02a 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -226,11 +226,11 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
>  		arg.argv = sh_argv;
>  	}
>  
> -	pasta_child_pid = clone(pasta_spawn_cmd,
> -				ns_fn_stack + sizeof(ns_fn_stack) / 2,
> -				CLONE_NEWIPC | CLONE_NEWPID | CLONE_NEWNET |
> -				CLONE_NEWUTS,
> -				(void *)&arg);
> +	pasta_child_pid = do_clone(pasta_spawn_cmd, ns_fn_stack,
> +				   sizeof(ns_fn_stack),
> +				   CLONE_NEWIPC | CLONE_NEWPID | CLONE_NEWNET |
> +				   CLONE_NEWUTS,
> +				   (void *)&arg);
>  
>  	if (pasta_child_pid == -1) {
>  		perror("clone");
> diff --git a/util.c b/util.c
> index be102e3..2125a67 100644
> --- a/util.c
> +++ b/util.c
> @@ -482,3 +482,24 @@ int write_file(const char *path, const char *buf)
>  	close(fd);
>  	return len == 0 ? 0 : -1;
>  }
> +
> +/**
> + * do_clone() - Wrapper of __clone2() for ia64, clone() for other architectures
> + * @fn:		Entry point for child
> + * @stack_area:	Stack area for child: we'll point callees to the middle of it
> + * @stack_size:	Total size of stack area, passed to callee divided by two
> + * @flags:	clone() system call flags
> + * @arg:	Argument to @fn
> + *
> + * Return: thread ID of child, -1 on failure
> + */
> +int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
> +	     void *arg)
> +{
> +#ifdef __ia64__
> +	return __clone2(fn, stack_area + stack_size / 2, stack_size / 2,
> +			flags, arg);
> +#else
> +	return clone(fn, stack_area + stack_size / 2, flags, arg);
> +#endif
> +}
> diff --git a/util.h b/util.h
> index 2d4e1ff..02b55a9 100644
> --- a/util.h
> +++ b/util.h
> @@ -81,13 +81,15 @@
>  	(((struct in_addr *)(a))->s_addr == ((struct in_addr *)b)->s_addr)
>  
>  #define NS_FN_STACK_SIZE	(RLIMIT_STACK_VAL * 1024 / 8)
> +int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
> +	     void *arg);
>  #define NS_CALL(fn, arg)						\
>  	do {								\
>  		char ns_fn_stack[NS_FN_STACK_SIZE];			\
>  									\
> -		clone((fn), ns_fn_stack + sizeof(ns_fn_stack) / 2,	\
> -		      CLONE_VM | CLONE_VFORK | CLONE_FILES | SIGCHLD,	\
> -		      (void *)(arg));					\
> +		do_clone((fn), ns_fn_stack, sizeof(ns_fn_stack),	\
> +			 CLONE_VM | CLONE_VFORK | CLONE_FILES | SIGCHLD,\
> +			 (void *)(arg));				\
>  	} while (0)
>  
>  #if __BYTE_ORDER == __BIG_ENDIAN

-- 
David Gibson			| 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] util, pasta: Add do_clone() wrapper around __clone2() and clone()

[Stefano Brivio]

On Thu, 17 Nov 2022 10:00:32 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Nov 16, 2022 at 05:30:57PM +0100, Stefano Brivio wrote:
> > Spotted in Debian's buildd logs: on ia64, clone(2) is not available:
> > the glibc wrapper is named __clone2() and it takes, additionally,
> > the size of the stack area passed by the caller.
> > 
> > Add a do_clone() wrapper handling the different cases, and also
> > taking care of pointing the child's stack in the middle of the
> > allocated area: on PA-RISC (hppa), handled by clone(), the stack
> > grows up, and on ia64 the stack grows down, but the register backing
> > store grows up -- and I think it might be actually used here.
> > 
> > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> 
> Heh, my only comment on v2 was going to be that it's not a good idea
> to name identifiers starting with _ - that's reserved for the system
> library.

Oops, yes, I keep forgetting about that -- and we already have other
offenders in util.c, I'll try to switch them to other names at some
point.

-- 
Stefano