linux-kernel - Re: [PATCH v2 1/2] fork: add clone3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 4 Jun 2019 11:56:33 +0200
From:   Christian Brauner <christian@...uner.io>
To:     David Howells <dhowells@...hat.com>
Cc:     viro@...iv.linux.org.uk, linux-kernel@...r.kernel.org,
        torvalds@...ux-foundation.org, jannh@...gle.com,
        keescook@...omium.org, fweimer@...hat.com, oleg@...hat.com,
        arnd@...db.de, Pavel Emelyanov <xemul@...tuozzo.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Adrian Reber <adrian@...as.de>,
        Andrei Vagin <avagin@...il.com>, linux-api@...r.kernel.org
Subject: Re: [PATCH v2 1/2] fork: add clone3

On Tue, Jun 04, 2019 at 11:43:17AM +0200, Christian Brauner wrote:
> On Tue, Jun 04, 2019 at 10:28:12AM +0100, David Howells wrote:
> > Christian Brauner <christian@...uner.io> wrote:
> > 
> > > +#include <linux/compiler_types.h>
> > 
> > I suspect you don't want to include that directly.
> > 
> > Also, to avoid bloating linux/sched/task.h yet further, maybe put this in
> > linux/sched/clone.h?
> 
> Yeah, not the worst idea.
> Though I'd leave the flags where they are and just add struct
> kernel_clone_args in there. But I assume that's what you meant anyway.

Actually, I would like to defer this to the cleanup patch too.
This way the patch stays small and clean and task.h is currently the
right place to put it.

> 
> > 
> > > -extern long _do_fork(unsigned long, unsigned long, unsigned long, int __user *, int __user *, unsigned long);
> > > +extern long _do_fork(struct kernel_clone_args *kargs);
> > >  extern long do_fork(unsigned long, unsigned long, unsigned long, int __user *, int __user *);
> > 
> > Maybe these could move into linux/sched/clone.h too.
> 
> Meh, that could be a separate cleanup patch after clone3() has been
> merged.
> 
> > 
> > > +#define CLONE_MAX ~0U
> > 
> > Can you add a comment summarising the meaning?
> 
> Yes, can do.
> 
> > 
> > > +	u64 clone_flags = args->flags;
> > > +	int __user *child_tidptr = args->child_tid;
> > > +	unsigned long tls = args->tls;
> > > +	unsigned long stack_start = args->stack;
> > > +	unsigned long stack_size = args->stack_size;
> > 
> > Some of these are only used once, so it's probably not worth sticking them in
> > local variables.
> 
> [1]:
> Ok, will double check.
> This was just to minimize copy-paste erros for variables which were used
> multiple times.
> 
> > 
> > > -		if (clone_flags &
> > > -		    (CLONE_DETACHED | CLONE_PARENT_SETTID | CLONE_THREAD))
> > > -			return ERR_PTR(-EINVAL);
> > 
> > Did this error check get lost?  I can see part of it further on, but the check
> > on CLONE_PARENT_SETTID is absent.
> 
> No, it's only relevant for legacy clone() since it uses the
> parent_tidptr argument to return the pidfd. clone3() has a dedicated
> return argument for that in clone_args.
> The check for legacy clone() is now done in legacy clone() directly.
> copy_process() should only do generic checks for all version of
> clone(),fork(),vfork(), etc.
> 
> > 
> > > +	int __user *parent_tidptr = args->parent_tid;
> > 
> > There's only one usage remaining after this patch, so a local var doesn't gain
> > a lot.
> 
> Yes, that leads back to [1].
> 
> > 
> > >  pid_t kernel_thread(int (*fn)(void *), void *arg, unsigned long flags)
> > >  {
> > > -	return _do_fork(flags|CLONE_VM|CLONE_UNTRACED, (unsigned long)fn,
> > > -		(unsigned long)arg, NULL, NULL, 0);
> > > +	struct kernel_clone_args args = {
> > > +		.flags = ((flags | CLONE_VM | CLONE_UNTRACED) & ~CSIGNAL),
> > > +		.exit_signal = (flags & CSIGNAL),
> > 
> > Kernel threads can have exit signals?
> 
> Yes,
> 
> kernel/kthread.c:       pid = kernel_thread(kthread, create, CLONE_FS | CLONE_FILES | SIGCHLD);
> kernel/umh.c:   pid = kernel_thread(call_usermodehelper_exec_async, sub_info, SIGCHLD);
> 
> And even if they couldn't have. This is just to make sure that if they
> ever would we'd be prepared.
> 
> > 
> > > +static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
> > > +				     struct clone_args __user *uargs,
> > > +				     size_t size)
> > 
> > I would make this "noinline".  If it gets inlined, local variable "args" may
> > still be on the stack when _do_fork() gets called.
> 
> Hm, can do.
> 
> Thanks!
> Christian