linux-kernel - Re: [PATCH][BUGFIX] cgroups: more safe tasklist locking in cgroup_attach

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: <20110815225003.GB29942@somewhere>
Date:	Tue, 16 Aug 2011 00:50:06 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Ben Blum <bblum@...rew.cmu.edu>, NeilBrown <neilb@...e.de>,
	paulmck@...ux.vnet.ibm.com, Paul Menage <menage@...gle.com>,
	Li Zefan <lizf@...fujitsu.com>,
	containers@...ts.linux-foundation.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH][BUGFIX] cgroups: more safe tasklist locking in
 cgroup_attach_proc

On Mon, Aug 15, 2011 at 08:49:57PM +0200, Oleg Nesterov wrote:
> On 07/29, Ben Blum wrote:
> >
> > According to this thread - https://lkml.org/lkml/2011/7/27/243 - RCU is
> > not sufficient to guarantee the tasklist is stable w.r.t. de_thread and
> > exit. Taking tasklist_lock for reading, instead of rcu_read_lock,
> > ensures proper exclusion.
> 
> Yes.
> 
> So far I still think we should fix while_each_thread() so that it works
> under rcu_read_lock() "as exepected", I'll try to think more.
> 
> But whatever we do with while_each_thread(), this can't help
> cgroup_attach_proc(), it needs the locking.
> 
> > -	rcu_read_lock();
> > +	read_lock(&tasklist_lock);
> >  	if (!thread_group_leader(leader)) {
> 
> Agreed, this should work.
> 
> But can't we avoid the global list? thread_group_leader() or not, we do
> not really care. We only need to ensure we can safely find all threads.
> 
> How about the patch below?
> 
> 
> With or without this/your patch this leader can die right after we
> drop the lock. ss->can_attach(leader) and ss->attach(leader) look
> suspicious. If a sub-thread execs, this task_struct has nothing to
> do with the threadgroup.
> 
> 
> 
> Also. This is off-topic, but... Why cgroup_attach_proc() and
> cgroup_attach_task() do ->attach_task() + cgroup_task_migrate()
> in the different order? cgroup_attach_proc() looks wrong even
> if currently doesn't matter.

Right. As we concluded in our off-list discussion, if there
is no strong reason for that, I'm going to fix that in my task
counter patchset because there it really matters. If we can't
migrate the thread because it has already exited, we really
don't want to call ->attach_task() but rather cancel_attach_task().

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