linux-kernel - Re: [PATCH v3 1/1] kthread: allocate kthread structure using kmalloc

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]

Message-ID: <alpine.DEB.2.20.1610261650260.4983@nanos>
Date:   Wed, 26 Oct 2016 16:57:42 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Oleg Nesterov <oleg@...hat.com>
cc:     Andy Lutomirski <luto@...capital.net>,
        Roman Pen <roman.penyaev@...fitbricks.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>, Tejun Heo <tj@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/1] kthread: allocate kthread structure using
 kmalloc

On Wed, 26 Oct 2016, Oleg Nesterov wrote:
> Some notes right now. Of course, with this patch we are ready to remove
> put_task_stack() from kthread.c right now. The next change should kill
> to_live_kthread() altogether. And stop using ->vfork_done.
> 
> And. With this patch we do not need another "workqueue: ignore dead tasks
> in a workqueue sleep hook" fix from Roman.

Nice !
 
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index be2cc1f..c6adbde 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -53,14 +53,34 @@ enum KTHREAD_BITS {
>  	KTHREAD_IS_PARKED,
>  };
>  
> -#define __to_kthread(vfork)	\
> -	container_of(vfork, struct kthread, exited)
> +static inline void set_kthread_struct(void *kthread)
> +{
> +	/*
> +	 * We abuse ->set_child_tid to avoid the new member and because it
> +	 * can't be wrongly copied by copy_process(). We also rely on fact
> +	 * that the caller can't exec, so PF_KTHREAD can't be cleared.
> +	 */
> +	current->set_child_tid = (__force void __user *)kthread;

Can we pretty please avoid this type casting? We only have 5 places using
set_child_tid. So we can really make it a proper union and fix up the 5
usage sites as a preparatory patch for this.

> +}
>  
>  static inline struct kthread *to_kthread(struct task_struct *k)
>  {
> -	return __to_kthread(k->vfork_done);
> +	WARN_ON(!(k->flags & PF_KTHREAD));
> +	return (__force void *)k->set_child_tid;
>  }
>  
> +void free_kthread_struct(struct task_struct *k)
> +{
> +	kfree(to_kthread(k)); /* can be NULL if kmalloc() failed */

Can you please not use tail comments? They really stop the reading flow.

> +#define __to_kthread(vfork)	\
> +	container_of(vfork, struct kthread, exited)
> +
> +/*
> + * TODO: kill it and use to_kthread(). But we still need the users
> + * like kthread_stop() which has to sync with the exiting kthread.
> + */
>  static struct kthread *to_live_kthread(struct task_struct *k)
>  {
>  	struct completion *vfork = ACCESS_ONCE(k->vfork_done);
> @@ -181,14 +201,11 @@ static int kthread(void *_create)
>  	int (*threadfn)(void *data) = create->threadfn;
>  	void *data = create->data;
>  	struct completion *done;
> -	struct kthread self;
> +	struct kthread *self;
>  	int ret;
>  
> -	self.flags = 0;
> -	self.data = data;
> -	init_completion(&self.exited);
> -	init_completion(&self.parked);
> -	current->vfork_done = &self.exited;
> +	self = kmalloc(sizeof(*self), GFP_KERNEL);
> +	set_kthread_struct(self);
>  
>  	/* If user was SIGKILLed, I release the structure. */
>  	done = xchg(&create->done, NULL);
> @@ -196,6 +213,19 @@ static int kthread(void *_create)
>  		kfree(create);
>  		do_exit(-EINTR);
>  	}
> +
> +	if (!self) {
> +		create->result = ERR_PTR(-ENOMEM);
> +		complete(done);
> +		do_exit(-ENOMEM);
> +	}
> +
> +	self->flags = 0;
> +	self->data = data;
> +	init_completion(&self->exited);
> +	init_completion(&self->parked);
> +	current->vfork_done = &self->exited;
> +
>  	/* OK, tell user we're spawned, wait for stop or wakeup */
>  	__set_current_state(TASK_UNINTERRUPTIBLE);
>  	create->result = current;
> @@ -203,12 +233,10 @@ static int kthread(void *_create)
>  	schedule();
>  
>  	ret = -EINTR;
> -
> -	if (!test_bit(KTHREAD_SHOULD_STOP, &self.flags)) {
> -		__kthread_parkme(&self);
> +	if (!test_bit(KTHREAD_SHOULD_STOP, &self->flags)) {
> +		__kthread_parkme(self);
>  		ret = threadfn(data);
>  	}
> -	/* we can't just return, we must preserve "self" on stack */
>  	do_exit(ret);
>  }

Other than the above nits, this is the right direction to go.

Thanks,

	tglx