cgroups: fix 2.6.32 regression causing BUG_ON() in cgroup_diput()

PATCH In review Dave Anderson 3 participants 3 messages Dec 23, 2009 – Dec 24, 2009

lore

Share & tools

lore

Dave Anderson

author

2009-12-23 18:48 UTC #0 lore raw

PATCH 1/1 cgroups: fix 2.6.32 regression causing BUG_ON() in cgroup_diput()

The LTP cgroup test suite generates a "kernel BUG at kernel/cgroup.c:790!" here in cgroup_diput():

                /*
                 * if we're getting rid of the cgroup, refcount should ensure
                 * that there are no pidlists left.
                 */
                BUG_ON(!list_empty(&cgrp->pidlists));

The cgroup pidlist rework in 2.6.32 generates the BUG_ON, which is caused when pidlist_array_load() calls cgroup_pidlist_find():

(1) if a matching cgroup_pidlist is found, it down_write's the mutex of the pre-existing cgroup_pidlist, and increments its use_count.
(2) if no matching cgroup_pidlist is found, then a new one is allocated, it down_write's its mutex, and the use_count is set to 0.
(3) the matching, or new, cgroup_pidlist gets returned back to pidlist_array_load(), which increments its use_count -- regardless whether new or pre-existing -- and up_write's the mutex.

So if a matching list is ever encountered by cgroup_pidlist_find() during the life of a cgroup directory, it results in an inflated use_count value, preventing it from ever getting released by cgroup_release_pid_array(). Then if the directory is subsequently removed, cgroup_diput() hits the BUG_ON() when it finds that the directory's cgroup is still populated with a pidlist.

The patch simply removes the use_count increment when a matching pidlist is found by cgroup_pidlist_find(), because it gets bumped by the calling pidlist_array_load() function while still protected by the list's mutex.

Signed-off-by Dave Anderson <[email protected]>

---

kernel/cgroup.c renamed from kernel/cgroup.c.orig +0 -1

@@ -2468,7 +2468,6 @@ static struct cgroup_pidlist *cgroup_pid
 			/* make sure l doesn't vanish out from under us */
 			down_write(&l->mutex);
 			mutex_unlock(&cgrp->pidlist_mutex);
-			l->use_count++;
 			return l;
 		}
 	}

Li Zefan

Reviewed-by on v1 1/1

2009-12-24 05:47 UTC #1 lore raw

CC Andrew

CC Container list

Quoted 11 lines · Dave Anderson

The LTP cgroup test suite generates a "kernel BUG at kernel/cgroup.c:790!" here in cgroup_diput():
/*
* if we're getting rid of the cgroup, refcount should
ensure
                 * that there are no pidlists left.
                 */
                BUG_ON(!list_empty(&cgrp->pidlists));

Good catch. Thanks!

This BUG can be easily triggered if 2 threads are reading the same cgroup's tasks file at the same time, and then the cgroup gets removed.

And this patch needs to be added to 2.6.32.x.

Quoted 20 lines

The cgroup pidlist rework in 2.6.32 generates the BUG_ON, which is caused when pidlist_array_load() calls cgroup_pidlist_find():
(1) if a matching cgroup_pidlist is found, it down_write's the mutex of the pre-existing cgroup_pidlist, and increments its use_count.
(2) if no matching cgroup_pidlist is found, then a new one is allocated, it down_write's its mutex, and the use_count is set to 0.
(3) the matching, or new, cgroup_pidlist gets returned back to pidlist_array_load(),
which increments its use_count -- regardless whether new or
pre-existing --
and up_write's the mutex.
So if a matching list is ever encountered by cgroup_pidlist_find() during the life of a cgroup directory, it results in an inflated use_count value, preventing it from ever getting released by cgroup_release_pid_array(). Then if the directory is subsequently removed, cgroup_diput() hits the BUG_ON() when it finds that the directory's cgroup is still populated with a pidlist.

Quoted 7 lines

The patch simply removes the use_count increment when a matching pidlist is found by cgroup_pidlist_find(), because it gets bumped by the calling pidlist_array_load() function while still protected by the list's mutex.
Signed-off-by Dave Anderson <[email protected]>

Reviewed-by Li Zefan <[email protected]>

Ben Blum

2009-12-24 08:39 UTC #2 lore raw

In reply to Dave Anderson

Quoted 31 lines · Dave Anderson

The LTP cgroup test suite generates a "kernel BUG at kernel/cgroup.c:790!" here in cgroup_diput():
                /*
                 * if we're getting rid of the cgroup, refcount should 
                 ensure
                 * that there are no pidlists left.
                 */
                BUG_ON(!list_empty(&cgrp->pidlists));
The cgroup pidlist rework in 2.6.32 generates the BUG_ON, which is caused when pidlist_array_load() calls cgroup_pidlist_find():
(1) if a matching cgroup_pidlist is found, it down_write's the mutex of the pre-existing cgroup_pidlist, and increments its use_count.
(2) if no matching cgroup_pidlist is found, then a new one is allocated, it down_write's its mutex, and the use_count is set to 0.
(3) the matching, or new, cgroup_pidlist gets returned back to pidlist_array_load(),
which increments its use_count -- regardless whether new or pre-existing --
and up_write's the mutex.
So if a matching list is ever encountered by cgroup_pidlist_find() during the life of a cgroup directory, it results in an inflated use_count value, preventing it from ever getting released by cgroup_release_pid_array(). Then if the directory is subsequently removed, cgroup_diput() hits the BUG_ON() when it finds that the directory's cgroup is still populated with a pidlist.

Quoted 10 lines

The patch simply removes the use_count increment when a matching pidlist is found by cgroup_pidlist_find(), because it gets bumped by the calling pidlist_array_load() function while still protected by the list's mutex.
Signed-off-by Dave Anderson <[email protected]>
---

Ack! That was probably my fault. Good catch.