From: NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org>
To: Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>,
Ben Blum <bblum-OM76b2Iv3yLQjUSlxSEPGw@public.gmane.org>,
Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>,
Oleg Nesterov <oleg-6lXkIZvqkOAvJsYlp49lxw@public.gmane.org>
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org,
"Paul E.McKenney"
<paulmck-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>,
"linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Possible race between cgroup_attach_proc and de_thread, and questionable code in de_thread.
Date: Wed, 27 Jul 2011 17:11:01 +1000 [thread overview]
Message-ID: <20110727171101.5e32d8eb@notabene.brown> (raw)
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.
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().
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.
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
next reply other threads:[~2011-07-27 7:11 UTC|newest]
Thread overview: 63+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-07-27 7:11 NeilBrown [this message]
2011-08-14 17:40 ` Possible race between cgroup_attach_proc and de_thread, and questionable code in de_thread Oleg Nesterov
2011-08-15 0:11 ` NeilBrown
[not found] ` <20110815101144.39812e9f-wvvUuzkyo1EYVZTmpyfIwg@public.gmane.org>
2011-08-15 19:09 ` Oleg Nesterov
2011-08-15 19:09 ` Oleg Nesterov
[not found] ` <20110814174000.GA2381-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2011-08-15 0:11 ` NeilBrown
[not found] ` <20110727171101.5e32d8eb-wvvUuzkyo1EYVZTmpyfIwg@public.gmane.org>
2011-07-27 15:07 ` Ben Blum
2011-07-27 15:07 ` Ben Blum
2011-07-27 23:42 ` Paul E. McKenney
[not found] ` <20110727234235.GA2318-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
2011-07-28 1:08 ` NeilBrown
2011-07-28 1:08 ` NeilBrown
2011-07-28 6:26 ` Ben Blum
2011-07-28 7:13 ` NeilBrown
[not found] ` <20110728171345.67d3797d-wvvUuzkyo1EYVZTmpyfIwg@public.gmane.org>
2011-07-29 14:28 ` [PATCH][BUGFIX] cgroups: more safe tasklist locking in cgroup_attach_proc Ben Blum
2011-07-29 14:28 ` Ben Blum
2011-08-01 19:31 ` Paul Menage
[not found] ` <20110729142842.GA8462-japSPQJXeIlCM1neWV3AGuCmf2DRS9x2@public.gmane.org>
2011-08-01 19:31 ` Paul Menage
2011-08-15 18:49 ` Oleg Nesterov
2011-08-15 18:49 ` Oleg Nesterov
2011-08-15 22:50 ` Frederic Weisbecker
2011-08-15 23:04 ` Ben Blum
2011-08-15 23:09 ` Ben Blum
2011-08-15 23:19 ` Frederic Weisbecker
[not found] ` <20110815230900.GB6867-japSPQJXeIlCM1neWV3AGuCmf2DRS9x2@public.gmane.org>
2011-08-15 23:19 ` Frederic Weisbecker
[not found] ` <20110815230415.GA6867-japSPQJXeIlCM1neWV3AGuCmf2DRS9x2@public.gmane.org>
2011-08-15 23:09 ` Ben Blum
2011-08-15 23:04 ` Ben Blum
2011-08-15 23:11 ` [PATCH][BUGFIX] cgroups: fix ordering of calls " Ben Blum
2011-08-15 23:20 ` Frederic Weisbecker
[not found] ` <20110815231156.GC6867-japSPQJXeIlCM1neWV3AGuCmf2DRS9x2@public.gmane.org>
2011-08-15 23:20 ` Frederic Weisbecker
2011-08-15 23:31 ` Paul Menage
2011-08-15 23:31 ` Paul Menage
2011-08-15 23:11 ` Ben Blum
[not found] ` <20110815184957.GA16588-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2011-08-15 22:50 ` [PATCH][BUGFIX] cgroups: more safe tasklist locking " Frederic Weisbecker
2011-09-01 21:46 ` Ben Blum
2011-09-01 21:46 ` Ben Blum
[not found] ` <20110901214643.GD10401-japSPQJXeIlCM1neWV3AGuCmf2DRS9x2@public.gmane.org>
2011-09-02 12:32 ` Oleg Nesterov
2011-09-02 12:32 ` Oleg Nesterov
[not found] ` <20110902123251.GA26764-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2011-09-08 2:11 ` Ben Blum
2011-09-08 2:11 ` Ben Blum
2011-10-14 0:31 ` [PATCH 1/2] cgroups: use sighand lock instead of tasklist_lock " Ben Blum
2011-10-14 12:15 ` Frederic Weisbecker
2011-10-14 0:36 ` [PATCH 2/2] cgroups: convert ss->attach to use whole threadgroup flex_array (cpuset, memcontrol) Ben Blum
2011-10-14 12:21 ` Frederic Weisbecker
2011-10-14 13:53 ` Ben Blum
2011-10-14 13:54 ` Ben Blum
2011-10-14 15:22 ` Frederic Weisbecker
2011-10-17 19:11 ` Ben Blum
2011-10-14 15:21 ` Frederic Weisbecker
2011-10-19 5:43 ` Paul Menage
[not found] ` <20110728062616.GC15204-japSPQJXeIlCM1neWV3AGuCmf2DRS9x2@public.gmane.org>
2011-07-28 7:13 ` Possible race between cgroup_attach_proc and de_thread, and questionable code in de_thread NeilBrown
2011-07-28 12:17 ` Paul E. McKenney
[not found] ` <20110728121741.GB2427-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
2011-08-14 17:51 ` Oleg Nesterov
2011-08-14 17:51 ` Oleg Nesterov
[not found] ` <20110814175119.GC2381-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2011-08-14 23:58 ` NeilBrown
2011-08-15 18:01 ` Paul E. McKenney
2011-08-14 23:58 ` NeilBrown
2011-08-15 18:01 ` Paul E. McKenney
[not found] ` <20110728110813.7ff84b13-wvvUuzkyo1EYVZTmpyfIwg@public.gmane.org>
2011-07-28 6:26 ` Ben Blum
2011-07-28 12:17 ` Paul E. McKenney
2011-08-14 17:45 ` Oleg Nesterov
2011-08-14 17:45 ` Oleg Nesterov
[not found] ` <20110727150710.GB5242-japSPQJXeIlCM1neWV3AGuCmf2DRS9x2@public.gmane.org>
2011-07-27 23:42 ` Paul E. McKenney
2011-08-14 17:40 ` Oleg Nesterov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20110727171101.5e32d8eb@notabene.brown \
--to=neilb-l3a5bk7wagm@public.gmane.org \
--cc=bblum-OM76b2Iv3yLQjUSlxSEPGw@public.gmane.org \
--cc=containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org \
--cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org \
--cc=menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org \
--cc=oleg-6lXkIZvqkOAvJsYlp49lxw@public.gmane.org \
--cc=paulmck-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.