On Tue, Mar 23, 2010 at 10:06 PM, Russell King - ARM Linux
[off-list ref] wrote:

On Sun, Mar 21, 2010 at 09:06:04PM -0400, Christoffer Dall wrote:

quoted

In addition to doing everything that clone() system call does, the
eclone() system call:

Some comments...

quoted

+sys_eclone_wrapper:
+ ? ? ? ? ? ? add ? ? ip, sp, #S_OFF
+ ? ? ? ? ? ? str ? ? ip, [sp, #0]
+ ? ? ? ? ? ? b ? ? ? sys_eclone
+ENDPROC(sys_eclone_wrapper)

I'm curious why, if you want the entire set of registers, you don't just
do:
? ? ? ? ? ? ? ?add ? ? r0, sp, #S_OFF
? ? ? ? ? ? ? ?b ? ? ? sys_eclone

and load the syscall arguments out of regs->ARM_foo. ?This avoids the need
for additional stores.

I simply copied the code from sys_clone. Do you prefer that I change
it in both places?

quoted

+
?sys_sigreturn_wrapper:
? ? ? ? ? ? ? add ? ? r0, sp, #S_OFF
? ? ? ? ? ? ? b ? ? ? sys_sigreturn

diff --git a/arch/arm/kernel/sys_arm.c b/arch/arm/kernel/sys_arm.c
index ae4027b..fd8199d 100644
--- a/arch/arm/kernel/sys_arm.c
+++ b/arch/arm/kernel/sys_arm.c

@@ -183,6 +183,45 @@ asmlinkage int sys_clone(unsigned long clone_flags, unsigned long newsp,

? ? ? return do_fork(clone_flags, newsp, regs, 0, parent_tidptr, child_tidptr);
?}

+asmlinkage int sys_eclone(unsigned flags_low, struct clone_args __user *uca,
+ ? ? ? ? ? ? ? ? ? ? ? int args_size, pid_t __user *pids,
+ ? ? ? ? ? ? ? ? ? ? ? struct pt_regs *regs)
+{
+ ? ? int rc;
+ ? ? struct clone_args kca;
+ ? ? unsigned long flags;
+ ? ? int __user *parent_tidp;
+ ? ? int __user *child_tidp;
+ ? ? unsigned long __user stack;

__user on an integer type doesn't make any sense; integer types do not
have address spaces.

thanks, will follow Sukadev's changes...

quoted

+ ? ? unsigned long stack_size;
+
+ ? ? 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_tidp = (int *)(unsigned long)kca.parent_tid_ptr;
+ ? ? child_tidp = (int *)(unsigned long)kca.child_tid_ptr;

This will produce sparse errors. ?Is there a reason why 'clone_args'
tid pointers aren't already pointers marked with __user ?

quoted

+
+ ? ? stack_size = (unsigned long)kca.child_stack_size;

Shouldn't this already be of integer type?

quoted

+ ? ? if (stack_size)
+ ? ? ? ? ? ? return -EINVAL;

So the stack must have a zero size? ?Is this missing a '!' ?

quoted

+
+ ? ? stack = (unsigned long)kca.child_stack;
+ ? ? if (!stack)
+ ? ? ? ? ? ? stack = regs->ARM_sp;
+
+ ? ? return do_fork_with_pids(flags, stack, regs, stack_size, parent_tidp,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? child_tidp, kca.nr_pids, pids);

Hmm, so let me get this syscall interface right. ?We have some arguments
passed in registers and others via a (variable sized?) structure. ?It seems
really weird to have, eg, a pointer to the pids and the number of pids
passed in two separate ways.

The grouping between what's passed in registers and via this clone_args
structure seems to be random. ?Can it be sanitized?

Thanks for you feedback. I will let the people behind eclone deal with
the eclone specifics.