On Wed, Jul 27, 2011 at 05:11:01PM +1000, NeilBrown wrote:
>
> Hi,
> I've been exploring the use of RCU in the kernel, particularly looking for
> things that don't quite look right. I found cgroup_attach_proc which was
> added a few months ago.
Awesome, thanks! :)
>
> It contains:
>
> rcu_read_lock();
> if (!thread_group_leader(leader)) {
> /*
> * a race with de_thread from another thread's exec() may strip
> * us of our leadership, making while_each_thread unsafe to use
> * on this task. if this happens, there is no choice but to
> * throw this task away and try again (from cgroup_procs_write);
> * this is "double-double-toil-and-trouble-check locking".
> */
> rcu_read_unlock();
> retval = -EAGAIN;
> goto out_free_group_list;
> }
>
> (and having the comment helps a lot!)
>
> The comment acknowledges a race with de_thread but seems to assume that
> rcu_read_lock() will protect against that race. It won't.
> It could possibly protect if the racy code in de_thread() contained a call
> to synchronize_rcu(), but it doesn't so there is no obvious exclusion
> between the two.
> I note that some other locks are held and maybe some other lock provides
> the required exclusion - I haven't explored that too deeply - but if that is
> the case, then the use of rcu_read_lock() here is pointless - it isn't
> needed just to call thread_group_leader().
I wrote this code, and I admit to not having a full understanding of RCU
myself. The code was once more complicated (before the patches went in,
mind you), and had a series of checks like that leading up to a
list_for_each_entry over the ->thread_group list (in "step 3", instead
of iterating over the flex_array), and had read_lock(&tasklist_lock)
around it. (...)
(The other locks held are just cgroup_mutex and threadgroup_fork_lock,
which wouldn't provide the exclusion.)
>
> The race as I understand it is with this code:
>
>
> list_replace_rcu(&leader->tasks, &tsk->tasks);
> list_replace_init(&leader->sibling, &tsk->sibling);
>
> tsk->group_leader = tsk;
> leader->group_leader = tsk;
>
>
> which seems to be called with only tasklist_lock held, which doesn't seem to
> be held in the cgroup code.
>
> If the "thread_group_leader(leader)" call in cgroup_attach_proc() runs before
> this chunk is run with the same value for 'leader', but the
> while_each_thread is run after, then the while_read_thread() might loop
> forever. rcu_read_lock doesn't prevent this from happening.
Somehow I was under the impression that holding tasklist_lock (for
writing) provided exclusion from code that holds rcu_read_lock -
probably because there are other points in the kernel which do
while_each_thread with only RCU-read held (and not tasklist):
- kernel/hung_task.c, check_hung_uninterruptible_tasks()
- kernel/posix-cpu-timers.c, thread_group_cputime()
- fs/ioprio.c, ioprio_set() and ioprio_get()
(There are also places, like kernel/signal.c, where code does
while_each_thread with only sighand->siglock held. this also seems
sketchy, since de_thread only takes that lock after the code quoted
above. there's a big comment in fs/exec.c where this is also done, but I
don't quite understand it.)
You seem to imply that rcu_read_lock() doesn't exclude against
write_lock(&tasklist_lock). If that's true, then we can fix the cgroup
code simply by replacing rcu_read_lock/rcu_read_unlock with
read_lock and read_unlocck on tasklist_lock. (I can hurry a bugfix patch
for this together if so.)
Wouldn't this mean that the three places listed above are also wrong?
>
> The code in de_thread() is actually questionable by itself.
> "list_replace_rcu" cannot really be used on the head of a list - it is only
> meant to be used on a member of a list.
> To move a list from one head to another you should be using
> list_splice_init_rcu().
> The ->tasks list doesn't seem to have a clearly distinguished 'head' but
> whatever is passed as 'g' to while_each_thread() is effectively a head and
> removing it from a list can cause a loop using while_each_thread() can not
> find the head and so never complete.
>
> I' not sure how best to fix this, though possibly changing
> while_each_thead to:
>
> while ((t = next_task(t)) != g && !thread_group_leader(t))
>
> might be part of it. We would also need to move
> tsk->group_leader = tsk;
> in the above up to the top, and probably add some memory barrier.
> However I don't know enough about how the list is used to be sure.
>
> Comments?
>
> Thanks,
> NeilBrown
>
>
I barely understand de_thread() from the reader's perspective, let alone
from the author's perspective, so I can't speak for that one.
Thanks for pointing this out!
-- Ben
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
