[RFC][PATCH] clone_with_pids()^w eclone() for x86_64

[RFC][PATCH] clone_with_pids()^w eclone() for x86_64

[Dave Hansen]

This is still a bit rough, but I figured I'd post it for kicks.

Most of the process.c stuff is copy-n-paste with i386 and needs
to get consolidated.  I also need to give this the new name.

I'd appreciate anybody that knows inline assembly well to make
sure that I'm not being a complete doofus with this call below.
This seems to work, but I'm not confident it is the best way.

int clone_with_pids(long flags_low, struct clone_args *clone_args, long args_size,
                 int *pids)
{
        long retval;

        __asm__  __volatile__(
                 "movq %3, %%r10\n\t"           /* pids in r10*/
                 "pushq %%rbp\n\t"              /* save value of ebp */
                :
                :"D" (flags_low), /* rdi */
                 "S" (clone_args),/* rsi */
                 "d" (args_size), /* rdx */
                 "a" (pids)       /* use rax, which gets moved to r10 */
                );

        __asm__ __volatile__(
                 "syscall\n\t"  /* Linux/x86_64 system call */
                 "testq %0,%0\n\t"      /* check return value */
                 "jne 1f\n\t"           /* jump if parent */
                 "popq %%rbx\n\t"       /* get subthread function */
                 "call *%%rbx\n\t"      /* start subthread function */
                 "movq %2,%0\n\t"
                 "syscall\n"            /* exit system call: exit subthread */
                 "1:\n\t"
                 "popq %%rbp\t"         /* restore parent's ebp */
                :"=a" (retval)
                :"0" (__NR_clone3), "i" (__NR_exit)
                :"ebx", "ecx", "edx"
                );

        if (retval < 0) {
                errno = -retval;
                retval = -1;
        }
        return retval;
}


---

 linux-2.6.git-dave/arch/x86/include/asm/syscalls.h  |    5 ++
 linux-2.6.git-dave/arch/x86/include/asm/unistd_64.h |    2 
 linux-2.6.git-dave/arch/x86/kernel/entry_64.S       |    8 +++
 linux-2.6.git-dave/arch/x86/kernel/process_64.c     |   49 ++++++++++++++++++++
 linux-2.6.git-dave/kernel/fork.c                    |   18 +++++++
 5 files changed, 82 insertions(+)

diff -puN arch/x86/include/asm/syscalls.h~cwp-x86_64 arch/x86/include/asm/syscalls.h
--- linux-2.6.git/arch/x86/include/asm/syscalls.h~cwp-x86_64	2009-11-18 16:37:09.000000000 -0800
+++ linux-2.6.git-dave/arch/x86/include/asm/syscalls.h	2009-11-18 16:37:09.000000000 -0800
@@ -78,6 +78,11 @@ asmlinkage long sys_iopl(unsigned int, s
 asmlinkage long sys_clone(unsigned long, unsigned long,
 			  void __user *, void __user *,
 			  struct pt_regs *);
+asmlinkage long sys_clone_with_pids(unsigned int flags_low,
+				struct clone_args * __user cargs,
+				int cargs_size,
+				pid_t * __user pids,
+				struct pt_regs *pt_regs);
 asmlinkage long sys_execve(char __user *, char __user * __user *,
 			   char __user * __user *,
 			   struct pt_regs *);
diff -puN arch/x86/include/asm/unistd_64.h~cwp-x86_64 arch/x86/include/asm/unistd_64.h
--- linux-2.6.git/arch/x86/include/asm/unistd_64.h~cwp-x86_64	2009-11-18 16:37:09.000000000 -0800
+++ linux-2.6.git-dave/arch/x86/include/asm/unistd_64.h	2009-11-18 16:37:09.000000000 -0800
@@ -661,6 +661,8 @@ __SYSCALL(__NR_pwritev, sys_pwritev)
 __SYSCALL(__NR_rt_tgsigqueueinfo, sys_rt_tgsigqueueinfo)
 #define __NR_perf_counter_open			298
 __SYSCALL(__NR_perf_counter_open, sys_perf_counter_open)
+#define __NR_clone_with_pids			299
+__SYSCALL(__NR_clone_with_pids, stub_clone_with_pids)
 
 #ifndef __NO_STUBS
 #define __ARCH_WANT_OLD_READDIR
diff -puN arch/x86/kernel/entry_64.S~cwp-x86_64 arch/x86/kernel/entry_64.S
--- linux-2.6.git/arch/x86/kernel/entry_64.S~cwp-x86_64	2009-11-18 16:37:09.000000000 -0800
+++ linux-2.6.git-dave/arch/x86/kernel/entry_64.S	2009-11-18 16:37:09.000000000 -0800
@@ -684,6 +684,13 @@ END(system_call)
 
 /*
  * Certain special system calls that need to save a complete full stack frame.
+ *
+ * 'arg' should be the register that pt_regs will show up in when
+ * 'func' is called.  Using normal calling conventions, this is:
+ *
+ * 	func(%rdi, %rsi, %rdx, %rcx, %r8, %r9)
+ *
+ * So, if you want pt_regs as the third argument, use %rdx.
  */
 	.macro PTREGSCALL label,func,arg
 ENTRY(\label)
@@ -704,6 +711,7 @@ END(\label)
 	PTREGSCALL stub_vfork, sys_vfork, %rdi
 	PTREGSCALL stub_sigaltstack, sys_sigaltstack, %rdx
 	PTREGSCALL stub_iopl, sys_iopl, %rsi
+	PTREGSCALL stub_clone_with_pids, sys_clone_with_pids, %r8
 
 ENTRY(ptregscall_common)
 	DEFAULT_FRAME 1 8	/* offset 8: return address */
diff -puN arch/x86/kernel/process_64.c~cwp-x86_64 arch/x86/kernel/process_64.c
--- linux-2.6.git/arch/x86/kernel/process_64.c~cwp-x86_64	2009-11-18 16:37:09.000000000 -0800
+++ linux-2.6.git-dave/arch/x86/kernel/process_64.c	2009-11-18 16:37:09.000000000 -0800
@@ -534,6 +534,55 @@ sys_clone(unsigned long clone_flags, uns
 	return do_fork(clone_flags, newsp, regs, 0, parent_tid, child_tid);
 }
 
+asmlinkage long
+sys_clone_with_pids(unsigned int flags_low, struct clone_args * __user cargs,
+		int args_size, pid_t * __user pids, struct pt_regs *pt_regs)
+{
+	int rc;
+	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;
+
+	printk("%s() 0\n", __func__);
+	rc = fetch_clone_args_from_user(cargs, args_size, &kca);
+	if (rc) {
+		printk("%s() 1\n", __func__);
+		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;
+	 */
+	printk("%s() 2\n", __func__);
+	flags = flags_low;
+	parent_tid_ptr = (int *)kca.parent_tid_ptr;
+	child_tid_ptr =  (int *)kca.child_tid_ptr;
+
+	printk("%s() 3\n", __func__);
+	stack_size = (unsigned long)kca.child_stack_size;
+	child_stack = (unsigned long)kca.child_stack_base + stack_size;
+
+	printk("%s() 4\n", __func__);
+	if (!child_stack)
+		child_stack = pt_regs->sp;
+	printk("%s() 5\n", __func__);
+
+	/*
+	 * 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, pt_regs, stack_size,
+				parent_tid_ptr, child_tid_ptr, kca.nr_pids,
+				pids);
+}
+
 unsigned long get_wchan(struct task_struct *p)
 {
 	unsigned long stack;
diff -puN kernel/fork.c~cwp-x86_64 kernel/fork.c
--- linux-2.6.git/kernel/fork.c~cwp-x86_64	2009-11-18 16:37:09.000000000 -0800
+++ linux-2.6.git-dave/kernel/fork.c	2009-11-18 16:37:09.000000000 -0800
@@ -1359,8 +1359,10 @@ static pid_t *copy_target_pids(int unum_
 
 	if (!unum_pids)
 		return NULL;
+	printk("%s(%d, %p) 0\n", __func__, unum_pids, upids);
 
 	knum_pids = task_pid(current)->level + 1;
+	printk("%s(%d, %p) knum_pids: %d\n", __func__, unum_pids, upids, knum_pids);
 	if (unum_pids > knum_pids)
 		return ERR_PTR(-EINVAL);
 
@@ -1407,6 +1409,7 @@ static pid_t *copy_target_pids(int unum_
 	size = unum_pids * sizeof(pid_t);
 
 	rc = copy_from_user(&target_pids[j], upids, size);
+	printk("%s() copy(%p, %p, %d) rc: %d\n", __func__, &target_pids[j], upids, size, rc);
 	if (rc) {
 		rc = -EFAULT;
 		goto out_free;
@@ -1467,6 +1470,8 @@ long do_fork_with_pids(unsigned long clo
 	long nr;
 	pid_t *target_pids;
 
+	if (upids)
+		printk("%s() 0\n", __func__);
 	/*
 	 * Do some preliminary argument and permissions checking before we
 	 * actually start allocating stuff
@@ -1482,6 +1487,8 @@ long do_fork_with_pids(unsigned long clo
 			return -EPERM;
 	}
 
+	if (upids)
+		printk("%s() 1\n", __func__);
 	/*
 	 * We hope to recycle these flags after 2.6.26
 	 */
@@ -1501,6 +1508,7 @@ long do_fork_with_pids(unsigned long clo
 
 	target_pids = copy_target_pids(num_pids, upids);
 	if (target_pids) {
+		printk("%s() 1a\n", __func__);
 		if (IS_ERR(target_pids))
 			return PTR_ERR(target_pids);
 
@@ -1509,6 +1517,8 @@ long do_fork_with_pids(unsigned long clo
 			goto out_free;
 	}
 
+	if (upids)
+		printk("%s() 2\n", __func__);
 	/*
 	 * When called from kernel_thread, don't do user tracing stuff.
 	 */
@@ -1517,12 +1527,16 @@ long do_fork_with_pids(unsigned long clo
 
 	p = copy_process(clone_flags, stack_start, regs, stack_size,
 			 child_tidptr, NULL, target_pids, trace);
+	if (upids)
+		printk("%s() 3\n", __func__);
 	/*
 	 * Do this prior waking up the new thread - the thread pointer
 	 * might get invalid after that point, if the thread exits quickly.
 	 */
 	if (!IS_ERR(p)) {
 		struct completion vfork;
+		if (upids)
+			printk("%s() 4\n", __func__);
 
 		trace_sched_process_fork(current, p);
 
@@ -1571,9 +1585,13 @@ long do_fork_with_pids(unsigned long clo
 		nr = PTR_ERR(p);
 	}
 
+	if (upids)
+		printk("%s() 5\n", __func__);
 out_free:
 	kfree(target_pids);
 
+	if (upids)
+		printk("%s() 6\n", __func__);
 	return nr;
 }
 
_

Re: [RFC][PATCH] clone_with_pids()^w eclone() for x86_64

[Louis Rilling]

[-- Attachment #1.1: Type: text/plain, Size: 3139 bytes --]

Hi Dave,

On 18/11/09 16:48 -0800, Dave Hansen wrote:
> 
> This is still a bit rough, but I figured I'd post it for kicks.
> 
> Most of the process.c stuff is copy-n-paste with i386 and needs
> to get consolidated.  I also need to give this the new name.
> 
> I'd appreciate anybody that knows inline assembly well to make
> sure that I'm not being a complete doofus with this call below.
> This seems to work, but I'm not confident it is the best way.

I'm not a total guru of inline assembly, but I have comments :)

> 
> int clone_with_pids(long flags_low, struct clone_args *clone_args, long args_size,
>                  int *pids)
> {
>         long retval;
> 
>         __asm__  __volatile__(
>                  "movq %3, %%r10\n\t"           /* pids in r10*/
>                  "pushq %%rbp\n\t"              /* save value of ebp */
>                 :
>                 :"D" (flags_low), /* rdi */
>                  "S" (clone_args),/* rsi */
>                  "d" (args_size), /* rdx */
>                  "a" (pids)       /* use rax, which gets moved to r10 */
>                 );

1. The fourth C arg is not in rax, but in rcx.

> 
>         __asm__ __volatile__(
>                  "syscall\n\t"  /* Linux/x86_64 system call */
>                  "testq %0,%0\n\t"      /* check return value */
>                  "jne 1f\n\t"           /* jump if parent */
>                  "popq %%rbx\n\t"       /* get subthread function */
>                  "call *%%rbx\n\t"      /* start subthread function */
>                  "movq %2,%0\n\t"
>                  "syscall\n"            /* exit system call: exit subthread */
>                  "1:\n\t"
>                  "popq %%rbp\t"         /* restore parent's ebp */
>                 :"=a" (retval)
>                 :"0" (__NR_clone3), "i" (__NR_exit)
>                 :"ebx", "ecx", "edx"
>                 );

2. You should probably not separate this into two asm statements. In particular,
   the compiler has no way to know that r10 should be preserved between the two
   statements, and may be confused by the change of rsp.

3. r10 and r11 should be listed as clobbered.

4. I fail to see the magic that puts the subthread function pointer in the
   stack.

5. Maybe rdi should contain the subthread argument before calling the subthread?

6. rdi, rsi, rdx, rcx, r8 and r9 should be added to the clobber list because of
   the call to the subthread function.

7. rsi could be used in place of rbx to hold the function pointer, which would
   allow you to remove ebx from the clobber list.

8. I don't see why rbp should be saved. The ABI says it must be saved by the
   callee.

9. Before calling exit(), maybe put some exit code in rdi?

> 
>         if (retval < 0) {
>                 errno = -retval;
>                 retval = -1;
>         }
>         return retval;
> }

Thanks,

Louis

[...]

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 206 bytes --]

_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [RFC][PATCH] clone_with_pids()^w eclone() for x86_64

[Dave Hansen]

On Thu, 2009-11-19 at 10:58 +0100, Louis Rilling wrote:
> > int clone_with_pids(long flags_low, struct clone_args *clone_args, long args_size,
> >                  int *pids)
> > {
> >         long retval;
> > 
> >         __asm__  __volatile__(
> >                  "movq %3, %%r10\n\t"           /* pids in r10*/
> >                  "pushq %%rbp\n\t"              /* save value of ebp */
> >                 :
> >                 :"D" (flags_low), /* rdi */
> >                  "S" (clone_args),/* rsi */
> >                  "d" (args_size), /* rdx */
> >                  "a" (pids)       /* use rax, which gets moved to r10 */
> >                 );
> 
> 1. The fourth C arg is not in rax, but in rcx.

Hey Louis,

So, try as I might, I couldn't get that to work.  I thought it was rcx,
too.

So, changing that instruction to:

                "movq %3, %%rcx\n\t"           /* pids in r10*/

and putting 0x11111, etc... in for the args the strace output for the
syscall looks like this:

        syscall_299(0x11111, 0x22222, 0x33333, 0x1, 0x1, 0x2, 0, 0, 0,
        0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
        0, 0) = -1 (errno 22)

and I get -EFAULT back from the function doing the copy_from_user() of
the pids argument, even when using good values.

If I use the asm posted above, I get this:
        
        syscall_299(0x11111, 0x22222, 0x33333, 0x44444, 0x1, 0x2, 0, 0,
        0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
        0, 0, 0) = -1 (errno 22)
        
Or, this from a real call:
        
        syscall_299(0x1100011, 0x7fff19f0fd40, 0x38, 0x602070, 0x1, 0x2,
        0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0[2992, 377]: Child:
        
I had to find r10 basically by trial and error.  I have no idea why it
works.

> > 
> >         __asm__ __volatile__(
> >                  "syscall\n\t"  /* Linux/x86_64 system call */
> >                  "testq %0,%0\n\t"      /* check return value */
> >                  "jne 1f\n\t"           /* jump if parent */
> >                  "popq %%rbx\n\t"       /* get subthread function */
> >                  "call *%%rbx\n\t"      /* start subthread function */
> >                  "movq %2,%0\n\t"
> >                  "syscall\n"            /* exit system call: exit subthread */
> >                  "1:\n\t"
> >                  "popq %%rbp\t"         /* restore parent's ebp */
> >                 :"=a" (retval)
> >                 :"0" (__NR_clone3), "i" (__NR_exit)
> >                 :"ebx", "ecx", "edx"
> >                 );
> 
> 2. You should probably not separate this into two asm statements. In particular,
>    the compiler has no way to know that r10 should be preserved between the two
>    statements, and may be confused by the change of rsp.

Yeah, I wondered about that.  Suka, we should probably fix your tests
and the i386 code, too.

> 3. r10 and r11 should be listed as clobbered.

D'oh!  I didn't even touch the bottom registers because it continued to
work from the i386 version that I stole from Suka.  

> 4. I fail to see the magic that puts the subthread function pointer in the
>    stack.
> 
> 5. Maybe rdi should contain the subthread argument before calling the subthread?
> 
> 6. rdi, rsi, rdx, rcx, r8 and r9 should be added to the clobber list because of
>    the call to the subthread function.
> 
> 7. rsi could be used in place of rbx to hold the function pointer, which would
>    allow you to remove ebx from the clobber list.
> 
> 8. I don't see why rbp should be saved. The ABI says it must be saved by the
>    callee.
> 
> 9. Before calling exit(), maybe put some exit code in rdi?

Thanks for looking through this, Louis.  I'll send out another version
in a bit.

-- Dave

Re: [RFC][PATCH] clone_with_pids()^w eclone() for x86_64

[Louis Rilling]

[-- Attachment #1.1: Type: text/plain, Size: 5720 bytes --]

On Thu, Nov 19, 2009 at 09:48:49AM -0800, Dave Hansen wrote:
> On Thu, 2009-11-19 at 10:58 +0100, Louis Rilling wrote:
> > > int clone_with_pids(long flags_low, struct clone_args *clone_args, long args_size,
> > >                  int *pids)
> > > {
> > >         long retval;
> > > 
> > >         __asm__  __volatile__(
> > >                  "movq %3, %%r10\n\t"           /* pids in r10*/
> > >                  "pushq %%rbp\n\t"              /* save value of ebp */
> > >                 :
> > >                 :"D" (flags_low), /* rdi */
> > >                  "S" (clone_args),/* rsi */
> > >                  "d" (args_size), /* rdx */
> > >                  "a" (pids)       /* use rax, which gets moved to r10 */
> > >                 );
> > 
> > 1. The fourth C arg is not in rax, but in rcx.
> 
> Hey Louis,
> 
> So, try as I might, I couldn't get that to work.  I thought it was rcx,
> too.
> 
> So, changing that instruction to:
> 
>                 "movq %3, %%rcx\n\t"           /* pids in r10*/

Hm, no.

I meant (without taking into account my other comments):

         __asm__  __volatile__(
                  "movq %3, %%r10\n\t"           /* pids in r10*/
                  "pushq %%rbp\n\t"              /* save value of ebp */
                 :
                 :"D" (flags_low), /* rdi */
                  "S" (clone_args),/* rsi */
                  "d" (args_size), /* rdx */
                  "c" (pids)       /* use rcx, which gets moved to r10 */
                 );

But actually this is even better :D:

         __asm__  __volatile__(
                  "movq %3, %%r10\n\t"           /* pids in r10*/
                  "pushq %%rbp\n\t"              /* save value of ebp */
                 :
                 :"D" (flags_low), /* rdi */
                  "S" (clone_args),/* rsi */
                  "d" (args_size), /* rdx */
                  "r10" (pids)     /* Linux reads its fourth arg from r10 */
                 );


> 
> and putting 0x11111, etc... in for the args the strace output for the
> syscall looks like this:
> 
>         syscall_299(0x11111, 0x22222, 0x33333, 0x1, 0x1, 0x2, 0, 0, 0,
>         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
>         0, 0) = -1 (errno 22)
> 
> and I get -EFAULT back from the function doing the copy_from_user() of
> the pids argument, even when using good values.
> 
> If I use the asm posted above, I get this:
>         
>         syscall_299(0x11111, 0x22222, 0x33333, 0x44444, 0x1, 0x2, 0, 0,
>         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
>         0, 0, 0) = -1 (errno 22)
>         
> Or, this from a real call:
>         
>         syscall_299(0x1100011, 0x7fff19f0fd40, 0x38, 0x602070, 0x1, 0x2,
>         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
>         0, 0, 0, 0, 0[2992, 377]: Child:
>         
> I had to find r10 basically by trial and error.  I have no idea why it
> works.

r10 is used to pass the fourth arg to the kernel because the syscall instruction
puts next rip (return address) in rcx. Using r10 instead of rcx is defined as part
of Linux ABI for x86_64.

For all the details, read the comments in
arch/x86/kernel/entry_64.S:ENTRY(system_call).

> 
> > > 
> > >         __asm__ __volatile__(
> > >                  "syscall\n\t"  /* Linux/x86_64 system call */
> > >                  "testq %0,%0\n\t"      /* check return value */
> > >                  "jne 1f\n\t"           /* jump if parent */
> > >                  "popq %%rbx\n\t"       /* get subthread function */
> > >                  "call *%%rbx\n\t"      /* start subthread function */
> > >                  "movq %2,%0\n\t"
> > >                  "syscall\n"            /* exit system call: exit subthread */
> > >                  "1:\n\t"
> > >                  "popq %%rbp\t"         /* restore parent's ebp */
> > >                 :"=a" (retval)
> > >                 :"0" (__NR_clone3), "i" (__NR_exit)
> > >                 :"ebx", "ecx", "edx"
> > >                 );
> > 
> > 2. You should probably not separate this into two asm statements. In particular,
> >    the compiler has no way to know that r10 should be preserved between the two
> >    statements, and may be confused by the change of rsp.
> 
> Yeah, I wondered about that.  Suka, we should probably fix your tests
> and the i386 code, too.
> 
> > 3. r10 and r11 should be listed as clobbered.
> 
> D'oh!  I didn't even touch the bottom registers because it continued to
> work from the i386 version that I stole from Suka.  

That's again because of the syscall instruction, which saves EFLAGS to r11
(and sysret restores EFLAGS from r11).

> 
> > 4. I fail to see the magic that puts the subthread function pointer in the
> >    stack.
> > 
> > 5. Maybe rdi should contain the subthread argument before calling the subthread?
> > 
> > 6. rdi, rsi, rdx, rcx, r8 and r9 should be added to the clobber list because of
> >    the call to the subthread function.
> > 
> > 7. rsi could be used in place of rbx to hold the function pointer, which would
> >    allow you to remove ebx from the clobber list.
> > 
> > 8. I don't see why rbp should be saved. The ABI says it must be saved by the
> >    callee.
> > 
> > 9. Before calling exit(), maybe put some exit code in rdi?
> 
> Thanks for looking through this, Louis.  I'll send out another version
> in a bit.

Thanks,

Louis

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 206 bytes --]

_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [RFC][PATCH] clone_with_pids()^w eclone() for x86_64

[Louis Rilling]

[-- Attachment #1.1: Type: text/plain, Size: 6710 bytes --]

On Thu, Nov 19, 2009 at 10:26:46PM +0100, Louis Rilling wrote:
> On Thu, Nov 19, 2009 at 09:48:49AM -0800, Dave Hansen wrote:
> > On Thu, 2009-11-19 at 10:58 +0100, Louis Rilling wrote:
> > > > int clone_with_pids(long flags_low, struct clone_args *clone_args, long args_size,
> > > >                  int *pids)
> > > > {
> > > >         long retval;
> > > > 
> > > >         __asm__  __volatile__(
> > > >                  "movq %3, %%r10\n\t"           /* pids in r10*/
> > > >                  "pushq %%rbp\n\t"              /* save value of ebp */
> > > >                 :
> > > >                 :"D" (flags_low), /* rdi */
> > > >                  "S" (clone_args),/* rsi */
> > > >                  "d" (args_size), /* rdx */
> > > >                  "a" (pids)       /* use rax, which gets moved to r10 */
> > > >                 );
> > > 
> > > 1. The fourth C arg is not in rax, but in rcx.
> > 
> > Hey Louis,
> > 
> > So, try as I might, I couldn't get that to work.  I thought it was rcx,
> > too.
> > 
> > So, changing that instruction to:
> > 
> >                 "movq %3, %%rcx\n\t"           /* pids in r10*/
> 
> Hm, no.
> 
> I meant (without taking into account my other comments):
> 
>          __asm__  __volatile__(
>                   "movq %3, %%r10\n\t"           /* pids in r10*/
>                   "pushq %%rbp\n\t"              /* save value of ebp */
>                  :
>                  :"D" (flags_low), /* rdi */
>                   "S" (clone_args),/* rsi */
>                   "d" (args_size), /* rdx */
>                   "c" (pids)       /* use rcx, which gets moved to r10 */
>                  );
> 
> But actually this is even better :D:
> 
>          __asm__  __volatile__(
>                   "movq %3, %%r10\n\t"           /* pids in r10*/
>                   "pushq %%rbp\n\t"              /* save value of ebp */
>                  :
>                  :"D" (flags_low), /* rdi */
>                   "S" (clone_args),/* rsi */
>                   "d" (args_size), /* rdx */
>                   "r10" (pids)     /* Linux reads its fourth arg from r10 */
>                  );
> 

Grrrr, I sent the email too quickly! This is the better version:

         __asm__  __volatile__(
                  "pushq %%rbp\n\t"              /* save value of ebp */
                 :
                 :"D" (flags_low), /* rdi */
                  "S" (clone_args),/* rsi */
                  "d" (args_size), /* rdx */
                  "r10" (pids)     /* Linux reads its fourth arg from r10 */
                 );

Louis


> > 
> > and putting 0x11111, etc... in for the args the strace output for the
> > syscall looks like this:
> > 
> >         syscall_299(0x11111, 0x22222, 0x33333, 0x1, 0x1, 0x2, 0, 0, 0,
> >         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> >         0, 0) = -1 (errno 22)
> > 
> > and I get -EFAULT back from the function doing the copy_from_user() of
> > the pids argument, even when using good values.
> > 
> > If I use the asm posted above, I get this:
> >         
> >         syscall_299(0x11111, 0x22222, 0x33333, 0x44444, 0x1, 0x2, 0, 0,
> >         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> >         0, 0, 0) = -1 (errno 22)
> >         
> > Or, this from a real call:
> >         
> >         syscall_299(0x1100011, 0x7fff19f0fd40, 0x38, 0x602070, 0x1, 0x2,
> >         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> >         0, 0, 0, 0, 0[2992, 377]: Child:
> >         
> > I had to find r10 basically by trial and error.  I have no idea why it
> > works.
> 
> r10 is used to pass the fourth arg to the kernel because the syscall instruction
> puts next rip (return address) in rcx. Using r10 instead of rcx is defined as part
> of Linux ABI for x86_64.
> 
> For all the details, read the comments in
> arch/x86/kernel/entry_64.S:ENTRY(system_call).
> 
> > 
> > > > 
> > > >         __asm__ __volatile__(
> > > >                  "syscall\n\t"  /* Linux/x86_64 system call */
> > > >                  "testq %0,%0\n\t"      /* check return value */
> > > >                  "jne 1f\n\t"           /* jump if parent */
> > > >                  "popq %%rbx\n\t"       /* get subthread function */
> > > >                  "call *%%rbx\n\t"      /* start subthread function */
> > > >                  "movq %2,%0\n\t"
> > > >                  "syscall\n"            /* exit system call: exit subthread */
> > > >                  "1:\n\t"
> > > >                  "popq %%rbp\t"         /* restore parent's ebp */
> > > >                 :"=a" (retval)
> > > >                 :"0" (__NR_clone3), "i" (__NR_exit)
> > > >                 :"ebx", "ecx", "edx"
> > > >                 );
> > > 
> > > 2. You should probably not separate this into two asm statements. In particular,
> > >    the compiler has no way to know that r10 should be preserved between the two
> > >    statements, and may be confused by the change of rsp.
> > 
> > Yeah, I wondered about that.  Suka, we should probably fix your tests
> > and the i386 code, too.
> > 
> > > 3. r10 and r11 should be listed as clobbered.
> > 
> > D'oh!  I didn't even touch the bottom registers because it continued to
> > work from the i386 version that I stole from Suka.  
> 
> That's again because of the syscall instruction, which saves EFLAGS to r11
> (and sysret restores EFLAGS from r11).
> 
> > 
> > > 4. I fail to see the magic that puts the subthread function pointer in the
> > >    stack.
> > > 
> > > 5. Maybe rdi should contain the subthread argument before calling the subthread?
> > > 
> > > 6. rdi, rsi, rdx, rcx, r8 and r9 should be added to the clobber list because of
> > >    the call to the subthread function.
> > > 
> > > 7. rsi could be used in place of rbx to hold the function pointer, which would
> > >    allow you to remove ebx from the clobber list.
> > > 
> > > 8. I don't see why rbp should be saved. The ABI says it must be saved by the
> > >    callee.
> > > 
> > > 9. Before calling exit(), maybe put some exit code in rdi?
> > 
> > Thanks for looking through this, Louis.  I'll send out another version
> > in a bit.
> 
> Thanks,
> 
> Louis
> 
> -- 
> Dr Louis Rilling			Kerlabs
> Skype: louis.rilling			Batiment Germanium
> Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
> http://www.kerlabs.com/			35700 Rennes



-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 206 bytes --]

_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [RFC][PATCH] clone_with_pids()^w eclone() for x86_64

[Dave Hansen]

On Thu, 2009-11-19 at 22:26 +0100, Louis Rilling wrote:
> But actually this is even better :D:
> 
>          __asm__  __volatile__(
>                   "movq %3, %%r10\n\t"           /* pids in r10*/
>                   "pushq %%rbp\n\t"              /* save value of ebp */
>                  :
>                  :"D" (flags_low), /* rdi */
>                   "S" (clone_args),/* rsi */
>                   "d" (args_size), /* rdx */
>                   "r10" (pids)     /* Linux reads its fourth arg from r10 */
>                  );

That gives me:

clone-lib.c:28: error: matching constraint references invalid operand
number


-- Dave

Re: [RFC][PATCH] clone_with_pids()^w eclone() for x86_64

[Louis Rilling]

[-- Attachment #1.1: Type: text/plain, Size: 1377 bytes --]

On Thu, Nov 19, 2009 at 01:32:36PM -0800, Dave Hansen wrote:
> On Thu, 2009-11-19 at 22:26 +0100, Louis Rilling wrote:
> > But actually this is even better :D:
> > 
> >          __asm__  __volatile__(
> >                   "movq %3, %%r10\n\t"           /* pids in r10*/
> >                   "pushq %%rbp\n\t"              /* save value of ebp */
> >                  :
> >                  :"D" (flags_low), /* rdi */
> >                   "S" (clone_args),/* rsi */
> >                   "d" (args_size), /* rdx */
> >                   "r10" (pids)     /* Linux reads its fourth arg from r10 */
> >                  );
> 
> That gives me:
> 
> clone-lib.c:28: error: matching constraint references invalid operand
> number

?? I know that this syntax works for r8, but I don't remember for r9-r15. I don't
have my handy documentation nearby, so let met check tomorrow (if nobody finds
before).

Anyway, although not optimal, even your first version should be ok with respect
to placing the fourth arg in r10 (assuming that the compiler keeps it until the
next asm statement of course, which any [future?] code instrumentation of the
compiler could break).

Louis

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 206 bytes --]

_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [RFC][PATCH] clone_with_pids()^w eclone() for x86_64

[Sukadev Bhattiprolu]

Louis Rilling [Louis.Rilling-aw0BnHfMbSpBDgjK7y7TUQ@public.gmane.org] wrote:
| > 
| >         __asm__ __volatile__(
| >                  "syscall\n\t"  /* Linux/x86_64 system call */
| >                  "testq %0,%0\n\t"      /* check return value */
| >                  "jne 1f\n\t"           /* jump if parent */
| >                  "popq %%rbx\n\t"       /* get subthread function */
| >                  "call *%%rbx\n\t"      /* start subthread function */
| >                  "movq %2,%0\n\t"
| >                  "syscall\n"            /* exit system call: exit subthread */
| >                  "1:\n\t"
| >                  "popq %%rbp\t"         /* restore parent's ebp */
| >                 :"=a" (retval)
| >                 :"0" (__NR_clone3), "i" (__NR_exit)
| >                 :"ebx", "ecx", "edx"
| >                 );
| 
| 2. You should probably not separate this into two asm statements. In particular,
|    the compiler has no way to know that r10 should be preserved between the two
|    statements, and may be confused by the change of rsp.
| 

Don't know enough asm, but can we leave them as two asm statements if we
add all registers with parameters to the clobbered list ? The i386 code that
Dave is referring to adds 3 (%ebx, %ecx, %edx) to the clobbered list, but is
the missing the fourth, %edi.

Sukadev

| 3. r10 and r11 should be listed as clobbered.
| 
| 4. I fail to see the magic that puts the subthread function pointer in the
|    stack.
| 
| 5. Maybe rdi should contain the subthread argument before calling the subthread?
| 
| 6. rdi, rsi, rdx, rcx, r8 and r9 should be added to the clobber list because of
|    the call to the subthread function.
| 
| 7. rsi could be used in place of rbx to hold the function pointer, which would
|    allow you to remove ebx from the clobber list.
| 
| 8. I don't see why rbp should be saved. The ABI says it must be saved by the
|    callee.
| 
| 9. Before calling exit(), maybe put some exit code in rdi?
| 
| > 
| >         if (retval < 0) {
| >                 errno = -retval;
| >                 retval = -1;
| >         }
| >         return retval;
| > }
| 
| Thanks,
| 
| Louis
| 
| [...]
| 
| -- 
| Dr Louis Rilling			Kerlabs
| Skype: louis.rilling			Batiment Germanium
| Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
| http://www.kerlabs.com/			35700 Rennes



| _______________________________________________
| Containers mailing list
| Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
| https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [RFC][PATCH] clone_with_pids()^w eclone() for x86_64

[Louis Rilling]

[-- Attachment #1.1: Type: text/plain, Size: 3839 bytes --]

On 19/11/09 23:29 -0800, Sukadev Bhattiprolu wrote:
> Louis Rilling [Louis.Rilling-aw0BnHfMbSpBDgjK7y7TUQ@public.gmane.org] wrote:
> | > 
> | >         __asm__ __volatile__(
> | >                  "syscall\n\t"  /* Linux/x86_64 system call */
> | >                  "testq %0,%0\n\t"      /* check return value */
> | >                  "jne 1f\n\t"           /* jump if parent */
> | >                  "popq %%rbx\n\t"       /* get subthread function */
> | >                  "call *%%rbx\n\t"      /* start subthread function */
> | >                  "movq %2,%0\n\t"
> | >                  "syscall\n"            /* exit system call: exit subthread */
> | >                  "1:\n\t"
> | >                  "popq %%rbp\t"         /* restore parent's ebp */
> | >                 :"=a" (retval)
> | >                 :"0" (__NR_clone3), "i" (__NR_exit)
> | >                 :"ebx", "ecx", "edx"
> | >                 );
> | 
> | 2. You should probably not separate this into two asm statements. In particular,
> |    the compiler has no way to know that r10 should be preserved between the two
> |    statements, and may be confused by the change of rsp.
> | 
> 
> Don't know enough asm, but can we leave them as two asm statements if we
> add all registers with parameters to the clobbered list ? The i386 code that
> Dave is referring to adds 3 (%ebx, %ecx, %edx) to the clobbered list, but is
> the missing the fourth, %edi.

No. Nothing prevents the compiler from instrumenting the code and overwriting
registers between the two statements. Moreover I don't see the point of
separating this in two statements. To me it looks weird to separate input
registers setup from syscall call.

The clobbered list just lets the compiler know that those registers may be
overwritten by the asm instructions, so that the compiler should not rely on the
contents of those registers after the asm statement. But as soon as the
statement is executed, the compiler can use the clobbered registers again.

Louis

> 
> Sukadev
> 
> | 3. r10 and r11 should be listed as clobbered.
> | 
> | 4. I fail to see the magic that puts the subthread function pointer in the
> |    stack.
> | 
> | 5. Maybe rdi should contain the subthread argument before calling the subthread?
> | 
> | 6. rdi, rsi, rdx, rcx, r8 and r9 should be added to the clobber list because of
> |    the call to the subthread function.
> | 
> | 7. rsi could be used in place of rbx to hold the function pointer, which would
> |    allow you to remove ebx from the clobber list.
> | 
> | 8. I don't see why rbp should be saved. The ABI says it must be saved by the
> |    callee.
> | 
> | 9. Before calling exit(), maybe put some exit code in rdi?
> | 
> | > 
> | >         if (retval < 0) {
> | >                 errno = -retval;
> | >                 retval = -1;
> | >         }
> | >         return retval;
> | > }
> | 
> | Thanks,
> | 
> | Louis
> | 
> | [...]
> | 
> | -- 
> | Dr Louis Rilling			Kerlabs
> | Skype: louis.rilling			Batiment Germanium
> | Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
> | http://www.kerlabs.com/			35700 Rennes
> 
> 
> 
> | _______________________________________________
> | Containers mailing list
> | Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> | https://lists.linux-foundation.org/mailman/listinfo/containers
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 206 bytes --]

_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [RFC][PATCH] clone_with_pids()^w eclone() for x86_64

[Louis Rilling]

[-- Attachment #1.1: Type: text/plain, Size: 1561 bytes --]

On 19/11/09 22:44 +0100, Louis Rilling wrote:
> On Thu, Nov 19, 2009 at 01:32:36PM -0800, Dave Hansen wrote:
> > On Thu, 2009-11-19 at 22:26 +0100, Louis Rilling wrote:
> > > But actually this is even better :D:
> > > 
> > >          __asm__  __volatile__(
> > >                   "movq %3, %%r10\n\t"           /* pids in r10*/
> > >                   "pushq %%rbp\n\t"              /* save value of ebp */
> > >                  :
> > >                  :"D" (flags_low), /* rdi */
> > >                   "S" (clone_args),/* rsi */
> > >                   "d" (args_size), /* rdx */
> > >                   "r10" (pids)     /* Linux reads its fourth arg from r10 */
> > >                  );
> > 
> > That gives me:
> > 
> > clone-lib.c:28: error: matching constraint references invalid operand
> > number
> 
> ?? I know that this syntax works for r8, but I don't remember for r9-r15. I don't
> have my handy documentation nearby, so let met check tomorrow (if nobody finds
> before).

Ok, I couldn't even find why r8 works, but according to this
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=16331
gcc support for using those registers in inline asm constraints is deliberately
missing.

So the only safe solution is something like your first one, although we could
expect that using rcx instead of rax will optimize it a bit.

Thanks,

Louis

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 206 bytes --]

_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers