[Devel] Re: [C/R ARM][PATCH 2/3] ARM: Add the eclone system call

[Christoffer Dall]

On Tue, Mar 23, 2010 at 10:06 PM, Russell King - ARM Linux
<linux at arm.linux.org.uk> wrote:
> On Sun, Mar 21, 2010 at 09:06:04PM -0400, Christoffer Dall wrote:
>> In addition to doing everything that clone() system call does, the
>> eclone() system call:
>
> Some comments...
>
>> +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?

>> +
>>  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...

>> +     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 ?
>
>> +
>> +     stack_size = (unsigned long)kca.child_stack_size;
>
> Shouldn't this already be of integer type?
>
>> +     if (stack_size)
>> +             return -EINVAL;
>
> So the stack must have a zero size?  Is this missing a '!' ?
>
>> +
>> +     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.
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers