linux-kernel - Re: Q: cgroup: Questions about possible issues in cgroup locking

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, 20 Mar 2012 20:34:14 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Mandeep Singh Baines <msb@...omium.org>
Cc:	Frederic Weisbecker <fweisbec@...il.com>,
	Li Zefan <lizf@...fujitsu.com>, Tejun Heo <tj@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Containers <containers@...ts.linux-foundation.org>,
	Cgroups <cgroups@...r.kernel.org>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Paul Menage <paul@...lmenage.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: Q: cgroup: Questions about possible issues in cgroup locking

OK, finally we should do something with this problem ;)

On 01/19, Oleg Nesterov wrote:
>
> I'll try to investigate if we can remove
>
> 	leader->group_leader = tsk;
>
> from de_thread(). In fact I already thought about this change a long
> ago without any connection to while_each_thread(). This assignment
> looks "assymetrical" compared to other threads we kill. But we did
> have a reason (reasons?). Hopefully, the only really important reason
> was already removed by 087806b1.

On the second thought, I think we should not do this.

For example, do_prlimit() assumes that tsk->group_leader is correct
under tasklist_lock.

OK, lets return to the thread_group_leader() check. To ensure we do
not visit the same thread twice we can check 'g', not 't'.

This is what I am going to send, after I re-check once again...

I have the problem with the changelog ;) Somehow it should explain
that while_each_thread_rcu(g, t) can't race with do_group_exit().
I think it can't, list_del_rcu(leader->thread_group) happens when
this entry is already "empty", it should be the last thread in group.
If the non-leader thread goes away from the least, we still have
the "path" to reach the leader. But this is not easy to explain.

As for the barrier. If de_thread() changes the leader it drops
and re-acquires tasklist_lock (this implies mb) after it changes
old_leader->exit_signal (used in thread_group_leader) and before
__unhash_process() which does list_del_rcu().

This means that if while_each_thread() sees the result of
list_del_rcu(old_leader) it must also see that
thread_group_leader(old_leader) != T.

What do you think? Do you see any problems?

Oleg.
---

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 7d379a6..f169bfd 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2323,9 +2323,24 @@ extern bool current_is_single_threaded(void);
 #define do_each_thread(g, t) \
 	for (g = t = &init_task ; (g = t = next_task(g)) != &init_task ; ) do

+/*
+ * needs tasklist_lock or ->siglock, or rcu if the caller ensures
+ * that 'g' can't exit or exec.
+ */
 #define while_each_thread(g, t) \
 	while ((t = next_thread(t)) != g)

+/*
+ * rcu-safe, but should start at ->group_leader.
+ * thread_group_leader(g) protects against the race with exec which
+ * removes the leader from list.
+ * smp_rmb() pairs with implicit mb() implied by unlock + lock in
+ * de_thread()->release_task() path.
+ */
+#define while_each_thread_rcu(g, t)				\
+	while ((t = next_thread(t)) != g &&			\
+		({ smp_rmb(); thread_group_leader(g); }))
+
 static inline int get_nr_threads(struct task_struct *tsk)
 {
 	return tsk->signal->nr_threads;

--
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/