On Fri, Nov 01, 2019 at 01:32:57PM +0100, Oleg Nesterov wrote:
On 11/01, Christian Brauner wrote:
On Thu, Oct 31, 2019 at 05:46:53PM +0100, Oleg Nesterov wrote:
On 10/31, Christian Brauner wrote:
--- a/include/uapi/linux/sched.h +++ b/include/uapi/linux/sched.h @@ -51,6 +51,10 @@
sent when the child exits.- @stack: Specify the location of the stack for the
child process.
Note, @stack is expected to point to the
lowest address. The stack direction will be
determined by the kernel and set up
appropriately based on @stack_size.I can't review this patch, I have no idea what does stack_size mean if !arch/x86.
In short: nothing at all if it weren't for ia64 (and maybe parisc). But let me provide some (hopefully useful) context.
Thanks...
(Probably most of that is well-know,
Certainly not to me ;) Thanks.
+static inline bool clone3_stack_valid(struct kernel_clone_args *kargs) +{
- if (kargs->stack == 0) {
if (kargs->stack_size > 0)return false;- } else {
if (kargs->stack_size == 0)return false;So to implement clone3_wrapper(void *bottom_of_stack) you need to do
clone3_wrapper(void *bottom_of_stack) { struct clone_args args = { ... // make clone3_stack_valid() happy .stack = bottom_of_stack - 1, .stack_size = 1, }; }
looks a bit strange. OK, I agree, this example is very artificial. But why do you think clone3() should nack stack_size == 0 ?
In short, consistency.
And in my opinion this stack_size == 0 check destroys the consistency, see below.
But just in case, let me say that overall I personally like this change.
The best thing imho, is to clearly communicate to userspace that stack needs to point to the lowest address and stack_size to the initial range of the stack pointer
Agreed.
But the kernel can't verify that "stack" actually points to the lowest address and stack_size is actually the stack size. Consider another artificial
Sure, but that's the similar to other structs that are passed via a pointer and come with a size. You could pass:
setxattr(..., ..., value - size, size, ...);
and the kernel would be confused as well.
clone3_wrapper(void *bottom_of_stack, unsigned long offs) { struct clone_args args = { ... // make clone3_stack_valid() happy .stack = bottom_of_stack - offs, .stack_size = offs, }; sys_clone3(args); }Now,
clone3_wrapper(bottom_of_stack, offs);
is same thing for _any_ offs except offs == 0 will fail. Why? To me this is not consistent, I think the "stack_size == 0" check buys nothing and only adds some confusion.
I disagree. It's a very easy contract: pass a stack and a size or request copy-on-write by passing both as 0. Sure, you can flaunt that contract but that's true of every other pointer + size api. The point is: the api we endorse should be simple and stack + stack_size is very simple.
Say, stack_size == 1 is "obviously wrong" too, this certainly means that "stack" doesn't point to the lowest address (or the child will corrupt the memory), but it works.
OK, I won't insist. Perhaps it can help to detect the case when a user forgets to pass the correct stack size.
if (!access_ok((void __user *)kargs->stack, kargs->stack_size))return false;Why?
It's nice of us to tell userspace _before_ we have created a thread that it messed up its parameters instead of starting a thread that then immediately crashes.
Heh. Then why this code doesn't verify that at least stack + stack_size is properly mmaped with PROT_READ|WRITE?
access_ok() is uncomplicated. The other check makes a lot more assumptions. Theare are users that might want to have a PROT_NONE part of their stack as their own "private" guard page (Jann just made that point) and there are other corner cases.
Christian