[For LKML: CC me on replies, I'm not subscribed]
On Saturday 11 September 2004 20:26, Jeff Dike wrote:
> > Jeff, please fix your patch again - the unused argument is the fourth,
> > not the third:
>
> Crap, I went to the trouble of confirming this in the i386 code, and
> ended up miscounting arguments. It still worked, though.
It worked no worse than current version (which is broken). In fact the 2.4
clone had 2 arguments. So it's obvious.
I checked in the i386 code (the _syscall5 macro and the sys_clone definition).
And the patch from David is the correct one:
This says where args go (from unistd.h, macro _syscall5):
: "0" (__NR_##name),"b" ((long)(arg1)),"c" ((long)(arg2)), \
"d" ((long)(arg3)),"S" ((long)(arg4)),"D" ((long)(arg5))); \
And this is the i386 code, with some comments, especially about the three
remaining problems:
asmlinkage int sys_clone(struct pt_regs regs)
{
unsigned long clone_flags;
unsigned long newsp;
int __user *parent_tidptr, *child_tidptr;
clone_flags = regs.ebx; //arg1
newsp = regs.ecx; //arg2
parent_tidptr = (int __user *)regs.edx; //arg3
child_tidptr = (int __user *)regs.edi; //arg5
/*XXX: Shouldn't UML implement this?*/
if (!newsp)
newsp = regs.esp;
/*XXX: UML forgets the "& ~ CLONE_IDLETASK". */
/*And also UML does not pass regs.*/
return do_fork(clone_flags & ~CLONE_IDLETASK, newsp, ®s, 0,
parent_tidptr, child_tidptr);
}
Now, the CLONE_IDLETASK must be copied straight into our version. Pretty
clear, that flag is for kernelspace callers of do_fork() only.
Security problem? I guess possibly (in the meaning used by OpenBSD, i.e. it is
a security concern, because somehow could discover that this is exploitable).
Instead, luckily, both newsp and regs are passed unchanged to the arch code
(the arch-independent code ignores them), and exactly to copy_thread. And the
Uml version is ready to deal with this API, luckily.
However, this is non-standard. I've added just a comment for now, since you
may have reason to keep the current code, but such behaviour calls for
breakage when things change.
The attached patch replaces the one on your page.
--
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729
On Sun, Sep 12, 2004 at 05:52:44PM +0200, BlaisorBlade wrote:
> It worked no worse than current version (which is broken). In fact the 2.4
> clone had 2 arguments. So it's obvious.
In fact, it worked better. It worked on a modern Debian filesystem, where
my old code didn't. Hence I didn't notice the mistake (although I should
have compared what I did with David's patch and justified the differences,
if any).
> However, this is non-standard. I've added just a comment for now, since you
> may have reason to keep the current code, but such behaviour calls for
> breakage when things change.
Yeah, I not sure why I did things the way I did. That's very old code, and
there may have been some good reason for it which has since disappeared.
Offhand, it looks like doing things in the standard way will clean up
copy_thread a bit.
Jeff
On Monday 13 September 2004 05:10, Jeff Dike wrote:
> On Sun, Sep 12, 2004 at 05:52:44PM +0200, BlaisorBlade wrote:
> > It worked no worse than current version (which is broken). In fact the
> > 2.4 clone had 2 arguments. So it's obvious.
>
> In fact, it worked better. It worked on a modern Debian filesystem, where
> my old code didn't.
Oh, well, your version gets the fifth arg right, indeed.
> > However, this is non-standard. I've added just a comment for now, since
> > you may have reason to keep the current code, but such behaviour calls
> > for breakage when things change.
> Yeah, I not sure why I did things the way I did. That's very old code, and
> there may have been some good reason for it which has since disappeared.
> Offhand, it looks like doing things in the standard way will clean up
> copy_thread a bit.
I agree completely.
--
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729