linux-kernel - Re: while_each_thread() under rcu_read

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:	Thu, 24 Jun 2010 23:57:02 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Don Zickus <dzickus@...hat.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...e.hu>,
	Jerome Marchand <jmarchan@...hat.com>,
	Mandeep Singh Baines <msb@...gle.com>,
	Roland McGrath <roland@...hat.com>,
	linux-kernel@...r.kernel.org, stable@...nel.org,
	"Eric W. Biederman" <ebiederm@...ssion.com>
Subject: Re: while_each_thread() under rcu_read_lock() is broken?

On 06/24, Paul E. McKenney wrote:
>
> On Wed, Jun 23, 2010 at 05:24:21PM +0200, Oleg Nesterov wrote:
> > It is very possible that I missed something here, my only point is
> > that I think it would be safer to assume nothing about the leaderness.
>
> It is past time that I list out my assumptions more carefully.  ;-)
>
> First, what "bad things" can happen to a reader scanning a thread
> group?

(I assume you mean the lockless case)

Currently, the only bad thing is that while_each_thread(g) can loop
forever if we race with exec(), or exit() if g is not leader.

And, to simplify, let's consider the same example again

	t = g;
	do {
		printk("pid %d\n", t->pid);
	} while_each_thread(g, t);


> 1.	The thread-group leader might do exec(), destroying the old
> 	list and forming a new one.  In this case, we want any readers
> 	to stop scanning.

I'd say, it is not that we want to stop scanning, it is OK to stop
scanning after we printed g->pid

> 2.	Some other thread might do exec(), destroying the old list and
> 	forming a new one.  In this case, we also want any readers to
> 	stop scanning.

The same.

If the code above runs under for_each_process(g) or it did
"g = find_task_by_pid(tgid)", we will see either new or old leader
and print its pid at least.

> 3.	The thread-group leader might do pthread_exit(), removing itself
> 	from the thread group

No. It can exit, but it won't be removed from thread group. It will
be zombie untill all sub-threads disappear.

> 4.	Some other thread might do pthread_exit(), removing itself
> 	from the thread group, and again might do so while the hapless
> 	reader is referencing that thread.  In this case, we want
> 	the hapless reader to continue scanning the remainder of the
> 	thread group.

Yes.

But, if that thread was used as a starting point g, then

	before the patch:	loop forever
	after the patch:	break

> 5.	The thread-group leader might do exit(), destroying the old
> 	list without forming a new one.  In this case, we want any
> 	readers to stop scanning.
>
> 6.	Some other thread might do exit(), destroying the old list
> 	without forming a new one.  In this case, we also want any
> 	readers to stop scanning.

Yes. But again, it is fine to print more pids as far as we know it
is safe to iterate over the exiting thread group. However,
next_thread_careful() can stop earlier compared to next_thread().
Either way, we can miss none/some/most/all threads if we race with
exit_group().

> Anything else I might be missing?

I think this is all.

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/