linux-kernel - Re: [PATCH 3/3] make kthread

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: <m14pnjon4m.fsf@ebiederm.dsl.xmission.com>
Date:	Fri, 13 Apr 2007 17:44:41 -0600
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Oleg Nesterov <oleg@...sign.ru>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Davide Libenzi <davidel@...ilserver.org>,
	Ingo Molnar <mingo@...e.hu>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Roland McGrath <roland@...hat.com>,
	Rusty Russell <rusty@...tcorp.com.au>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] make kthread_stop() scalable

Oleg Nesterov <oleg@...sign.ru> writes:

> It's a shame kthread_stop() (may take a while!) runs with a global semaphore
> held. With this patch kthread() allocates all neccesary data (struct kthread)
> on its own stack, globals kthread_stop_xxx are deleted.

Oleg so fare you patches  have been inspiring.  However..

> HACKS:
>
> 	- re-use task_struct->set_child_tid to point to "struct kthread"

	 task_struct->vfork_done is a better cannidate.

> 	- use do_exit() directly to preserve "struct kthread" on stack

Calling do_exit directly like that is not a hack, as it appears the preferred
way to exit is to call do_exit, or complete_and_exit.

While this does improve the scalability and remove a global variable.  It
also introduces a complex special case in the form of struct kthread.

It also doesn't solve the biggest problem with the current kthread interface
in that calling kthread_stop does not cause the code to break out of
interruptible sleeps.

>  static int kthread(void *_create)
>  {
> -	struct kthread_create_info *create = _create;
> -	int (*threadfn)(void *data);
> -	void *data;
> -	int ret = -EINTR;
> +	struct kthread self = {
> +		.task = current,
> +		.err = -EINTR,
> +	};
>  
>  	/* Copy data: it's on kthread's stack */
> -	threadfn = create->threadfn;
> -	data = create->data;
> +	struct kthread_create_info *create = _create;
> +	int (*threadfn)(void *data) = create->threadfn;
> +	void *data = create->data;
> +
> +	/*
> +	 * This should be enough to assure that self is still on
> +	 * stack when we enter do_exit()
> +	 */
> +	set_kthread(&self);
> +	create->result = &self;
>  

> @@ -91,7 +105,7 @@ static void create_kthread(struct kthrea
>  
>  	/* We want our own signal handler (we take no signals by default). */
>  	pid = kernel_thread(kthread, create, CLONE_FS | CLONE_FILES | SIGCHLD);
> -	create->result = pid;
> +	create->result = ERR_PTR(pid);

Ouch.    You have a nasty race here.

If kthread runs before kernel_thread returns then setting
"create->result = ERR_PTR(pid);" could easily stomp 
"create->result = &self".


Eric

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/