linux-kernel - Re: + cgroups-add-functionality-to-read-write-lock-clone_thread-forking-pe r-threadgroup.patch added to -mm tree

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:	Sat, 22 Aug 2009 15:09:52 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Paul Menage <menage@...gle.com>
Cc:	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
	bblum@...gle.com, ebiederm@...ssion.com, lizf@...fujitsu.com,
	matthltc@...ibm.com
Subject: Re: +
	cgroups-add-functionality-to-read-write-lock-clone_thread-forking-pe
	r-threadgroup.patch added to -mm tree

On 08/21, Paul Menage wrote:
>
> On Fri, Aug 21, 2009 at 3:45 AM, Oleg Nesterov<oleg@...hat.com> wrote:
> > In case I wasn't clear.
> >
> > Let's suppose we have subthreads T1 and T2, and we have a reference to T1.
>
> In this case, T1 is also the thread group leader.

You forced me to take a look at the next patch,
cgroups-add-ability-to-move-all-threads-in-a-process-to-a-new-cgroup-atomically.patch
which uses this helper ;) please see below.

> And we hold tasklist_lock around the entire operation. (So the
> rcu_read_lock() call is probably a red herring - Li Zefan already
> suggested that it be removed).

Yes, rcu_read_lock() is not needed under tasklist to iterate over
->thread_group.

> But you're saying that could still be a problem if tsk exits  before
> we even get to this point?
>
> My impression was that if the thread group leader exits, it hangs
> around (still attached to its thread group list) until all its threads
> have exited.

Yes, unless the non-leader execs, in this case the leader can die before
sub-thread, the execing thread becomes the new leader.

IOW. Yes, ->group_leader dies last, but exec can change ->group_leader.

> In which case as long as we're holding tasklist_lock, the
> thread group list should remain valid.

Only if it was valid before we take tasklist.

OK, cgroups-add-ability-to-move-all-threads-in-a-process-to-a-new-cgroup-atomically.patch
does

	threadgroup_sighand = threadgroup_fork_lock(leader);

	rcu_read_lock();
	list_for_each_entry_rcu(tsk, &leader->thread_group, thread_group)

and this is equally wrong afais. Hmm, and other similar
list_for_each_entry_rcu's doesn't look right. This code can do something
like

	rcu_read_lock();
	if (!tsk->sighand)	// tsk is leader or not, doesn't matter
		fail;
	list_for_each_rcu(tsk->thread_group) {}
	rcu_read_unlock();

though.

And why do we need sighand->threadgroup_fork_lock ? I gueess, to protect
against clone(CLONE_THREAD).

But. threadgroup_fork_lock() has another problem. Even if the process
P is singlethreaded, I can't see how ->threadgroup_fork_lock work.

threadgroup_fork_lock() bumps P->sighand->count. If P exec, it will
notice sighand->count != 1 and switch to another ->sighand.

Oleg.

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