implement s390 clone_with_pids syscall
PATCH 1/1 implement s390 clone_with_pids syscall
This does the s390 hook for v11 of clone-with-pids. This patch is on top of the linux-cr tree. A pair of patches against user-cr is coming next to (a) make restart work with the new clone-with-pids, and (b) demonstrate clone-with-pids more generally using the ns_exec (namespace manipulation) program.
The full kernel tree/changelog can be seen as branch cwp.2 at
http://git.kernel.org/gitweb.cgi?p=linux/kernel/git/sergeh/linux-cr.git
The user-space clone-with-pids glue for s390 (clone_s390x.c from the user-cr package) is now:
struct pid_set {
int num_pids;
pid_t *pids;
};typedef unsigned long long u64;
typedef unsigned int u32;
typedef int pid_t;
struct clone_args {
u64 clone_flags_high;
u64 child_stack_base;
u64 child_stack_size;
u64 parent_tid_ptr;
u64 child_tid_ptr;
u32 nr_pids;
u32 reserved0;
u64 reserved1;
}; #define do_cwp(flags, pids, args, sz) \
( { \
register unsigned long int __r1 asm ("1") = (unsigned long int)(__NR_clone_with_pids); \
register unsigned long int __r2 asm ("2") = (unsigned long int)(flags); \
register unsigned long int __r3 asm ("3") = (unsigned long int)(args); \
register unsigned long int __r4 asm ("4") = (unsigned long int)(sz); \
register unsigned long int __r5 asm ("5") = (unsigned long int)(pids); \
register long int __result asm ("2"); \
__asm__ __volatile__( \ " svc 0\n" /* do __NR_cwp syscall */ \ " ltgr %%r2,%%r2\n" /* returned 0? */ \ " jnz 1f\n" /* if not goto label 1 */ \ " lg %%r3,0(%%r15)\n" /* get fnarg off stack into arg 1 */ \ " lg %%r2,8(%%r15)\n" /* get fn off stack int r3 basr*/ \ " lgr %%r1,%%r15\n" /* tmp store old stack pointer */ \ " aghi %%r15,-160\n" /* move the stack */ \ " stg %%r1,0(%%r15)\n" /* and save old stack pointer */ \ " basr %%r14,%%r3\n" /* call fn(arg) */ \ " svc 1\n" /* call exit */ \ " 1:\n" \ : "=d" (__result) \ : "d" (__r1), "0" (__r2), "d" (__r3), "d" (__r4), "d" (__r5) \ : "memory"); \ __result; \ } )
int clone_with_pids(int (*fn)(void *), void *child_stack,
unsigned long stack_size, unsigned long flags, struct pid_set *target_pids, void *arg)
{
struct clone_args clone_args, *ca = &clone_args;
u64 *s;
memset(ca, 0, sizeof(struct clone_args));
ca->nr_pids = target_pids->num_pids;
ca->child_stack_size = stack_size - 16;
ca->child_stack_base = (u64) child_stack;
if (child_stack) {
s = (u64 *) (ca->child_stack_base + ca->child_stack_size);
*--s = (u64) arg;
*--s = (u64) fn;
ca->child_stack_size -= 16;
}
return do_cwp(flags, target_pids->pids, ca,
sizeof(struct clone_args));
}Changelog:
Nov 09: fix compat code (thanks, Suka) Nov 10: use orig_gpr2, not gprs[2] for input arg 1
--- arch/s390/kernel/compat_linux.c | 63 ++++++++++++++++++++++++++++++-------- arch/s390/kernel/process.c | 58 +++++++++++++++++++++++++++-------- 2 files changed, 94 insertions(+), 27 deletions(-)
arch/s390/kernel/compat_linux.c +49 -14
@@ -821,20 +821,55 @@ asmlinkage long sys32_clone(void)asmlinkage long sys32_clone_with_pids (void ) {struct pt_regs *regs =task_pt_regs (current); -unsigned long clone_flags; -unsigned long newsp; -int __user *parent_tidptr, *child_tidptr; -void __user *upid_setp; - - clone_flags = regs->gprs[3 ] &0xffffffffUL ; - newsp = regs->orig_gpr2 &0x7fffffffUL ; - parent_tidptr =compat_ptr (regs->gprs[4 ]); - child_tidptr =compat_ptr (regs->gprs[5 ]); - upid_setp =compat_ptr (regs->gprs[7 ]); -if (!newsp) - newsp = regs->gprs[15 ]; -return do_fork_with_pids (clone_flags, newsp, regs,0 , - parent_tidptr, child_tidptr, upid_setp); +int rc; +int args_size; +struct clone_args kca; +unsigned long flags; +int __user *parent_tid_ptr; +int __user *child_tid_ptr; +unsigned long __user child_stack; +unsigned long stack_size; +unsigned int flags_low; +struct clone_args __user *uca; +pid_t __user *pids; + + flags_low = regs->orig_gpr2 &0xffffffffUL ; + uca =compat_ptr (regs->gprs[3 ]); + args_size = regs->gprs[4 ] &0xffffffffUL ; + pids =compat_ptr (regs->gprs[5 ]); + + rc =fetch_clone_args_from_user (uca, args_size, &kca); +if (rc) +return rc; + +/* +* TODO: Convert 'clone-flags' to 64-bits on all architectures. +* TODO: When ->clone_flags_high is non-zero, copy it in to the +* higher word(s) of 'flags': +* +* flags = (kca.clone_flags_high << 32) | flags_low; +*/ + flags = flags_low; + parent_tid_ptr = (int *)kca.parent_tid_ptr; + child_tid_ptr = (int *)kca.child_tid_ptr; + + stack_size = (unsigned long )kca.child_stack_size; + child_stack = (unsigned long )kca.child_stack_base; +if (child_stack) + child_stack += stack_size; +else + child_stack = regs->gprs[15 ]; + +if (!child_stack) + child_stack = regs->gprs[15 ]; + +/* +* TODO: On 32-bit systems, clone_flags is passed in as 32-bit value +* to several functions. Need to convert clone_flags to 64-bit. +*/ +return do_fork_with_pids (flags, child_stack, regs, stack_size, + parent_tid_ptr, child_tid_ptr, kca.nr_pids, + pids); }/*
arch/s390/kernel/process.c +45 -13
65 lines not on this page · raw message
Signature · 1 line
1.6.1
A message that is not in the archive (sent off-list or not yet received): 20091110163708.GA19122-r/[email protected]
arch/s390/kernel/compat_linux.c asmlinkage long sys32_clone(void) · line 859
Quoted 8 lines
arch/s390/kernel/compat_linux.c +7
-0+ parent_tid_ptr = (int *)kca.parent_tid_ptr; + child_tid_ptr = (int *)kca.child_tid_ptr; + + stack_size = (unsigned long )kca.child_stack_size; + child_stack = (unsigned long )kca.child_stack_base; +if (child_stack) + child_stack += stack_size;
Should this calculation not be of the form:child_stack = arch_dependent_alignment(child_stack + stack_size - 1) ?
Is overflow a concern?
Same questions apply to the x86 version.
A message that is not in the archive (sent off-list or not yet received): 1257924442.7132.467.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]
Quoted 13 lines · Nathan Lynch
arch/s390/kernel/compat_linux.c asmlinkage long sys32_clone(void) · line 859
Quoted 8 lines
arch/s390/kernel/compat_linux.c +7
-0+ parent_tid_ptr = (int *)kca.parent_tid_ptr; + child_tid_ptr = (int *)kca.child_tid_ptr; + + stack_size = (unsigned long )kca.child_stack_size; + child_stack = (unsigned long )kca.child_stack_base; +if (child_stack) + child_stack += stack_size;Should this calculation not be of the form:
child_stack=arch_dependent_alignment(child_stack + stack_size - 1)?
Quoted 3 lines
Is overflow a concern?
Same questions apply to the x86 version.
Hmm... if the stack isn't valid, the task will just segfault, so it's not dangerous for the kernel, right? Note that for instance arch/s390/kernel/process.c:SYS_clone() doesn't check the validity of the new stack pointer passed in either.
-serge
A message that is not in the archive (sent off-list or not yet received): 20091111144600.GA6925-r/[email protected]
Quoted 14 lines · Serge E. Hallyn
Quoted 13 lines · Nathan Lynch
arch/s390/kernel/compat_linux.c asmlinkage long sys32_clone(void) · line 859
Quoted 8 lines
arch/s390/kernel/compat_linux.c +7
-0+ parent_tid_ptr = (int *)kca.parent_tid_ptr; + child_tid_ptr = (int *)kca.child_tid_ptr; + + stack_size = (unsigned long )kca.child_stack_size; + child_stack = (unsigned long )kca.child_stack_base; +if (child_stack) + child_stack += stack_size;Should this calculation not be of the form:
child_stack=arch_dependent_alignment(child_stack + stack_size - 1)?
Quoted 8 lines
Quoted 3 lines
Is overflow a concern?
Same questions apply to the x86 version.
Hmm... if the stack isn't valid, the task will just segfault, so it's not dangerous for the kernel, right? Note that for instance arch/s390/kernel/process.c:SYS_clone() doesn't check the validity of the new stack pointer passed in either.
clone expects the stack argument to be the desired value of the stack pointer in the child. cwp is different in that the clone_args struct specifies the base and size of the region the child is to use for stack, meaning that the kernel must derive from these a sane value for the child's stack pointer (on every arch where the stack grows down).
Your current calculation results in an unaligned SP outside of the region that the caller has presumably allocated for the child stack. How is that useful behavior?
A message that is not in the archive (sent off-list or not yet received): 1257958793.7132.1068.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]
Quoted 19 lines · Nathan Lynch
Quoted 18 lines · Serge E. Hallyn
Quoted 16 lines · Nathan Lynch
Review
Quoted 8 lines
(quoted diff) +7
-0+ parent_tid_ptr = (int *)kca.parent_tid_ptr; + child_tid_ptr = (int *)kca.child_tid_ptr; + + stack_size = (unsigned long)kca.child_stack_size; + child_stack = (unsigned long)kca.child_stack_base; + if (child_stack) + child_stack += stack_size;Should this calculation not be of the form:
child_stack=arch_dependent_alignment(child_stack + stack_size - 1)?Is overflow a concern?
Same questions apply to the x86 version.
Quoted 7 lines
Quoted 4 lines
Hmm... if the stack isn't valid, the task will just segfault, so it's not dangerous for the kernel, right? Note that for instance arch/s390/kernel/process.c:SYS_clone() doesn't check the validity of the new stack pointer passed in either.
clone expects the stack argument to be the desired value of the stack pointer in the child.
And doesn't verify it.
Quoted 4 lines
cwp is different in that the
clone_argsstruct
specifies the base and size of the region the child is to use for stack, meaning that the kernel must derive from these a sane value for the child's stack pointer (on every arch where the stack grows down).
And with regular clone, the kernel must expect userspace to do that calculation correctly! Userspace always still has to do
base = malloc(size); base += size - 1;
Quoted 1 line
Your current calculation results in an unaligned SP outside of the
Can you send the patch to align it properly?
Quoted 2 lines
region that the caller has presumably allocated for the child stack. How is that useful behavior?
It's useful because stack_size still gets passed through copy_process to the arch-dependent copy_thread(). That then mostly ignores the size, but in theory we could start tracking it.
Anyway I'm not totally opposed to expecting stack_top in most architectures, I just object to your assertion that somehow it makes more sense for the kernel to expect userspace to do a valid stack_top calculation than for the kernel to take the stack_base and stack_size and comput it itself.
-serge
Quoted 1 line
A message that is not in the archive (sent off-list or not yet received): 20091111183303.GA8761-r/[email protected]
Quoted 25 lines · Serge E. Hallyn
Quoted 24 lines · Nathan Lynch
Quoted 22 lines · Serge E. Hallyn
Quoted 13 lines · Nathan Lynch
Review
Quoted 8 lines
(quoted diff) +7
-0+ parent_tid_ptr = (int *)kca.parent_tid_ptr; + child_tid_ptr = (int *)kca.child_tid_ptr; + + stack_size = (unsigned long)kca.child_stack_size; + child_stack = (unsigned long)kca.child_stack_base; + if (child_stack) + child_stack += stack_size;Should this calculation not be of the form:
child_stack=arch_dependent_alignment(child_stack + stack_size - 1)?Quoted 3 lines
Is overflow a concern?
Same questions apply to the x86 version.
Hmm... if the stack isn't valid, the task will just segfault, so it's not dangerous for the kernel, right? Note that for instance arch/s390/kernel/process.c:SYS_clone() doesn't check the validity of the new stack pointer passed in either.
Quoted 4 lines
Quoted 2 lines
clone expects the stack argument to be the desired value of the stack pointer in the child.
And doesn't verify it.
That's not the point. Garbage inputs will get garbage behavior with either interface and that's fine.
Quoted 5 lines
Quoted 4 lines
cwp is different in that the
clone_argsstruct
specifies the base and size of the region the child is to use for stack, meaning that the kernel must derive from these a sane value for the child's stack pointer (on every arch where the stack grows down).
Quoted 8 lines
And with regular clone, the kernel must expect userspace to do that calculation correctly! Userspace always still has to do
base = malloc(size); base += size - 1;Quoted 1 line
Your current calculation results in an unaligned SP outside of the
Can you send the patch to align it properly?
For powerpc I have
usp = _ALIGN_DOWN((stack_base + stack_sz - 1), 16);
Afraid I don't have s390 ABI docs at hand. But my remarks were based on the incorrect assumption that kca.child_stack_size conveys, well, the stack size; see below.
Quoted 7 lines
Quoted 3 lines
region that the caller has presumably allocated for the child stack. How is that useful behavior?
It's useful because
stack_sizestill gets passed throughcopy_processto the arch-dependentcopy_thread(). That then mostly ignores the size, but in theory we could start tracking it.
Something I missed earlier is that the stack_size you are passing in from user space is not actually the size of the stack. It's adjusted to account for arguments that have been placed at the end of the stack region. So stack_size becomes a value that you want the kernel to add to stack_base to get the desired stack pointer value in the child -- it's not a size at all. At this point we may as well communicate the desired stack pointer value directly (which could be denoted by stack_size == 0, or we could add another member to clone_args), or rename stack_size to stack_offset or similar.
A message that is not in the archive (sent off-list or not yet received): 1257970778.7132.1364.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]
Quoted 9 lines · Nathan Lynch
Something I missed earlier is that the
stack_sizeyou are passing in from user space is not actually the size of the stack. It's adjusted to account for arguments that have been placed at the end of the stack region. Sostack_sizebecomes a value that you want the kernel to add tostack_baseto get the desired stack pointer value in the child -- it's not a size at all. At this point we may as well communicate the desired stack pointer value directly (which could be denoted bystack_size== 0, or we could add another member toclone_args), or renamestack_sizetostack_offsetor similar.
So do I understand correctly that the agreement (reached on irc) is to keep passing in a stack_size, but enforce that it ==0 for all but ia64?
-serge
A message that is not in the archive (sent off-list or not yet received): 20091111203739.GD8761-r/[email protected]
Quoted 11 lines · Serge E. Hallyn
Quoted 9 lines · Nathan Lynch
Something I missed earlier is that the
stack_sizeyou are passing in from user space is not actually the size of the stack. It's adjusted to account for arguments that have been placed at the end of the stack region. Sostack_sizebecomes a value that you want the kernel to add tostack_baseto get the desired stack pointer value in the child -- it's not a size at all. At this point we may as well communicate the desired stack pointer value directly (which could be denoted bystack_size== 0, or we could add another member toclone_args), or renamestack_sizetostack_offsetor similar.
Quoted 3 lines
So do I understand correctly that the agreement (reached on irc) is to keep passing in a
stack_size, but enforce that it ==0 for all but ia64?
That is the approach I prefer.