linux-kernel - Re: [PATCH v2 1/4] introduce for_each_thread() to replace the buggy while_each

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: <20131204132828.GC4530@localhost.localdomain>
Date:	Wed, 4 Dec 2013 14:28:29 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	David Rientjes <rientjes@...gle.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Mandeep Singh Baines <msb@...omium.org>,
	"Ma, Xindong" <xindong.ma@...el.com>,
	Michal Hocko <mhocko@...e.cz>,
	Sameer Nanda <snanda@...omium.org>,
	Sergey Dyasly <dserrg@...il.com>,
	"Tu, Xiaobing" <xiaobing.tu@...el.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/4] introduce for_each_thread() to replace the buggy
 while_each_thread()

On Wed, Dec 04, 2013 at 02:04:09PM +0100, Oleg Nesterov wrote:
> while_each_thread() and next_thread() should die, almost every
> lockless usage is wrong.
> 
> 1. Unless g == current, the lockless while_each_thread() is not safe.
> 
>    while_each_thread(g, t) can loop forever if g exits, next_thread()
>    can't reach the unhashed thread in this case. Note that this can
>    happen even if g is the group leader, it can exec.
> 
> 2. Even if while_each_thread() itself was correct, people often use
>    it wrongly.
> 
>    It was never safe to just take rcu_read_lock() and loop unless
>    you verify that pid_alive(g) == T, even the first next_thread()
>    can point to the already freed/reused memory.
> 
> This patch adds signal_struct->thread_head and task->thread_node
> to create the normal rcu-safe list with the stable head. The new
> for_each_thread(g, t) helper is always safe under rcu_read_lock()
> as long as this task_struct can't go away.

Thanks, it looks indeed much saner to put the head in the signal struct!

> 
> Note: of course it is ugly to have both task_struct->thread_node
> and the old task_struct->thread_group, we will kill it later, after
> we change the users of while_each_thread() to use for_each_thread().
> 
> Perhaps we can kill it even before we convert all users, we can
> reimplement next_thread(t) using the new thread_head/thread_node.
> But we can't do this right now because this will lead to subtle
> behavioural changes. For example, do/while_each_thread() always
> sees at least one task, while for_each_thread() can do nothing if
> the whole thread group has died.

Would it be safe to have for_each_thread_continue() instead?

> Or thread_group_empty(), currently
> its semantics is not clear unless thread_group_leader(p) and we
> need to audit the callers before we can change it.
> 
> So this patch adds the new interface which has to coexist with the
> old one for some time, hopefully the next changes will be more or
> less straightforward and the old one will go away soon.
> 
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> Reviewed-and-Tested-by: Sergey Dyasly <dserrg@...il.com>
> Reviewed-by: Sameer Nanda <snanda@...omium.org>
> ---

Yeah if the conversion needs careful audit, it makes sense to switch incrementally.
--
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/