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]

Message-ID: <20100622143402.GA5463@redhat.com>
Date:	Tue, 22 Jun 2010 16:34:02 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Roland McGrath <roland@...hat.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	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>,
	linux-kernel@...r.kernel.org, stable@...nel.org
Subject: Re: while_each_thread() under rcu_read_lock() is broken?

On 06/21, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@...hat.com> writes:
>
> >> If
> >> that's so, then just changing it to avoid the situation seems like it
> >> would be less invasive overall.
> >
> > How? We should change ->group_leader uner write_lock_irq(tasklist),
> > synchronize_rcu() is not an option. We can't do call_rcu(release_task),
> > we can't take tasklist for writing in the softirq context. But even
> > if we could, this can't help in fact or I missed something.
>
> We already do: call_rcu(&p->rcu, delayed_put_task_struct); in release_task.
> We don't call release_task until after we have removed it as leader and
> dropped the write lock.

Yes. I meant that while this is needed to ensure that next_thread/etc
returns the rcu-safe data, this (or more rcu_call's) can help to fix
while_each_thread.

I think I was unclear. de_thread() changes ->group_leader, but this does
not matter at all. I mentioned this only because we discussed the possibility
to check ->group_leader in while_each_thread.

What does matter is the single line in __unhash_process()

	list_del_rcu(&p->thread_group);

this breaks while_each_thread().

IOW. Why list_for_each_rcu(head) actually works? It works because this
head itself can't be removed from list.

while_each_thread(g, t) is almost equal to list_for_each_rcu() and it
can only work if g can't be removed from list (OK, strictly speaking
until other sub-threads go away, but this doesn't matter).

However, g can be removed if a) it is not ->group_leader and exits,
or b) its subthread execs.

> At first glance it sounds like the group leader is safe as a stopping
> point for a rcu while_each_thread, and I expect the fact that
> de_thread takes everything down to a single thread, could have nice
> properties here.  If pid_alive were only to fail on the group leader
> when de_thread is called I think we could legitimately say that an event
> we won't worry about.  It is close enough to a new thread being created
> anyway.

Not sure I understand exactly... I mean, I don't understand whether
you agree or not with the fix which adds pid_alive() check into
next_thread_careful().

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/