It turns out that, actually, the can_attach(), cancel_attach() and
attach() code is broken -- we're incrementing a ref on the old_css of
a task in can_attach(). Then we decrement the ref on a *different* css
(because the task has been migrated). This is clearly a bad thing.
Should we make cgroup_migrate() deal with the accounting for us (by
getting it to grab a ref before can_attach() and dropping it after the
attach succeeds or fails?).
--
Aleksa Sarai (cyphar)
http://www.cyphar.com
I've discovered a bug in the PIDs cgroup. We aren't properly dealing
with css references (we grab a reference to a css in ->can_attach() and
then drop a reference to a *different* css in ->{cancel_,}attach(). This
is a quick patch I wrote which I believe fixes the problem by getting
cgroup_migrate() to grab a reference before ->can_attach() and once the
attach has failed or succeded.
Aleksa Sarai (2):
cgroup: get a ref to source csses when migrating
cgroup: pids: fix invalid get/put usage
kernel/cgroup.c | 21 +++++++++++++++++++--
kernel/cgroup_pids.c | 13 +------------
2 files changed, 20 insertions(+), 14 deletions(-)
--
2.5.0
Grab a ref to each source css being migrated from, otherwise it's
possible for the refcount to reach zero between ->can_attach() and
->cancel_attach(). This means that operations on the task's old css
(such as container_of(...)) become unsafe, as we may be operating on a
different css.
Signed-off-by: Aleksa Sarai <[email protected]>
---
kernel/cgroup.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 4ec1b7ee5de8..6cbfbe36284d 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2305,6 +2305,17 @@ static int cgroup_migrate(struct cgroup *cgrp, struct task_struct *leader,
if (list_empty(&tset.src_csets))
return 0;
+ /*
+ * Fetch a ref of each css, so that the old task's css doesn't get reaped
+ * between ->can_attach() and ->cancel_attach().
+ */
+ down_read(&css_set_rwsem);
+ list_for_each_entry(cset, &tset.src_csets, mg_node) {
+ for_each_e_css(css, i, cgrp)
+ css_get(cset->subsys[i]);
+ }
+ up_read(&css_set_rwsem);
+
/* check that we can legitimately attach to the cgroup */
for_each_e_css(css, i, cgrp) {
if (css->ss->can_attach) {
@@ -2341,7 +2352,7 @@ static int cgroup_migrate(struct cgroup *cgrp, struct task_struct *leader,
css->ss->attach(css, &tset);
ret = 0;
- goto out_release_tset;
+ goto out_deref_src;
out_cancel_attach:
for_each_e_css(css, i, cgrp) {
@@ -2350,7 +2361,13 @@ out_cancel_attach:
if (css->ss->cancel_attach)
css->ss->cancel_attach(css, &tset);
}
-out_release_tset:
+out_deref_src:
+ down_read(&css_set_rwsem);
+ list_for_each_entry(cset, &tset.src_csets, mg_node) {
+ for_each_e_css(css, i, cgrp)
+ css_put(cset->subsys[i]);
+ }
+ up_read(&css_set_rwsem);
down_write(&css_set_rwsem);
list_splice_init(&tset.dst_csets, &tset.src_csets);
list_for_each_entry_safe(cset, tmp_cset, &tset.src_csets, mg_node) {
--
2.5.0
Fix incorrect usage of css_get and css_put to put a different css in
pids_{cancel_,}attach() than the one grabbed in pids_can_attach(). This
could lead to quite serious memory leakage (and unsafe operations on the
putted css).
Signed-off-by: Aleksa Sarai <[email protected]>
---
kernel/cgroup_pids.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/kernel/cgroup_pids.c b/kernel/cgroup_pids.c
index d75488824ae2..f116ca77a658 100644
--- a/kernel/cgroup_pids.c
+++ b/kernel/cgroup_pids.c
@@ -177,7 +177,7 @@ static int pids_can_attach(struct cgroup_subsys_state *css,
* we either fail and hit ->cancel_attach() or succeed and hit
* ->attach().
*/
- old_css = task_get_css(task, pids_cgrp_id);
+ old_css = task_css(task, pids_cgrp_id);
old_pids = css_pids(old_css);
pids_charge(pids, 1);
@@ -202,19 +202,9 @@ static void pids_cancel_attach(struct cgroup_subsys_state *css,
pids_charge(old_pids, 1);
pids_uncharge(pids, 1);
- css_put(old_css);
}
}
-static void pids_attach(struct cgroup_subsys_state *css,
- struct cgroup_taskset *tset)
-{
- struct task_struct *task;
-
- cgroup_taskset_for_each(task, tset)
- css_put(task_css(task, pids_cgrp_id));
-}
-
static int pids_can_fork(struct task_struct *task, void **priv_p)
{
struct cgroup_subsys_state *css;
@@ -354,7 +344,6 @@ static struct cftype pids_files[] = {
struct cgroup_subsys pids_cgrp_subsys = {
.css_alloc = pids_css_alloc,
.css_free = pids_css_free,
- .attach = pids_attach,
.can_attach = pids_can_attach,
.cancel_attach = pids_cancel_attach,
.can_fork = pids_can_fork,
--
2.5.0
Thanks to Nikolay Borisov who discovered this.
--
Aleksa Sarai (cyphar)
http://www.cyphar.com
On Sun, Aug 23, 2015 at 11:10:31PM +1000, Aleksa Sarai wrote:
> Grab a ref to each source css being migrated from, otherwise it's
> possible for the refcount to reach zero between ->can_attach() and
> ->cancel_attach(). This means that operations on the task's old css
> (such as container_of(...)) become unsafe, as we may be operating on a
> different css.
>
> Signed-off-by: Aleksa Sarai <[email protected]>
> ---
> kernel/cgroup.c | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 4ec1b7ee5de8..6cbfbe36284d 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -2305,6 +2305,17 @@ static int cgroup_migrate(struct cgroup *cgrp, struct task_struct *leader,
> if (list_empty(&tset.src_csets))
> return 0;
>
> + /*
> + * Fetch a ref of each css, so that the old task's css doesn't get reaped
> + * between ->can_attach() and ->cancel_attach().
> + */
> + down_read(&css_set_rwsem);
> + list_for_each_entry(cset, &tset.src_csets, mg_node) {
> + for_each_e_css(css, i, cgrp)
> + css_get(cset->subsys[i]);
> + }
> + up_read(&css_set_rwsem);
Have you verified that the scenario you're describing can actually
happen? AFAICS, cgroup_migrate_add_src() already does the pinning.
Thanks.
--
tejun
Hello,
On Sun, Aug 23, 2015 at 11:10:32PM +1000, Aleksa Sarai wrote:
> Fix incorrect usage of css_get and css_put to put a different css in
> pids_{cancel_,}attach() than the one grabbed in pids_can_attach(). This
> could lead to quite serious memory leakage (and unsafe operations on the
> putted css).
So, this patch looks correct to me but can you update the description
so that it notes that the source css doesn't go away while migration
is in progress and thus pinning is unnecessary to begin with?
Thanks.
--
tejun
> Have you verified that the scenario you're describing can actually
> happen? AFAICS, cgroup_migrate_add_src() already does the pinning.
Hmmm. Looking at it, group_migrate_add_src() does grab a ref on the
css_set which contains the css, and the comments mention that grabbing
a ref on the css_set will stop the css from being dropped. I must've
misunderstood one of your previous emails (where you said that such
code was not safe in the ->can_fork(), ->cancel_fork() and ->fork())
path.
You also mentioned that depending on the css_set's ref being bumped to
protect the contained csses is "sort of implementation detail. It can
be implemented in different ways." Which made me think that depending
on that is not a good idea.
But if it's safe to depend on the cgroup_migrate_add_src() pinning the
ref on the css_set, I'll drop this patch and fix up the other one
accordingly.
--
Aleksa Sarai (cyphar)
http://www.cyphar.com
Hello, Aleksa.
On Tue, Aug 25, 2015 at 12:00:54PM +1000, Aleksa Sarai wrote:
> You also mentioned that depending on the css_set's ref being bumped to
> protect the contained csses is "sort of implementation detail. It can
> be implemented in different ways." Which made me think that depending
> on that is not a good idea.
>
> But if it's safe to depend on the cgroup_migrate_add_src() pinning the
> ref on the css_set, I'll drop this patch and fix up the other one
> accordingly.
Yeah, let's go w/o the explicit refcnting.
Thanks.
--
tejun