commit 7534432dcc3c654a8671b6b0cdffd1dbdbc73074

The following script gives me a reliable BUG bisected to commit 7534432dcc3c654a8671b6b0cdffd1dbdbc73074, subject "cgroups: remove rcu_read_lock() in cgroupstats_build()". It's not immediately clear to me why that commit should cause this...

thanks,
-serge

==============================================================
script
==============================================================
#!/bin/bash
mount -t cgroup -o freezer none /cgroup
sleep 100 &
pid=jobs -p
mkdir /cgroup/1
echo $pid > /cgroup/1/tasks
umount /cgroup
mount -t cgroup -o freezer,ns none /cgroup
mount -t cgroup -o freezer none /cgroup
#kill %1
sleep 100 &
pid=jobs -p | tail -1
mkdir /cgroup/2
echo $pid > /cgroup/2/tasks
umount /cgroup
==============================================================

==============================================================
The BUG output:
==============================================================

------------[ cut here ]------------
kernel BUG at kernel/cgroup.c:468!
invalid opcode: 0000 [#1] SMP
last sysfs file: /sys/kernel/uevent_seqnum
Modules linked in:

Pid: 2900, comm: sh Not tainted (2.6.28-07513-ge5f6a86 #231)
EIP: 0060:[<c02411bb>] EFLAGS: 00010293 CPU: 2
EIP is at cgroup_attach_task+0x281/0x3ba
EAX: dfb57ee8 EBX: dfbe2914 ECX: df8cda68 EDX: dfb57ef0
ESI: df0ff900 EDI: 00000004 EBP: dfbe2900 ESP: dfb57ec0
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Process sh (pid: 2900, ti=dfb56000 task=df88ee00 task.ti=dfb56000)

Stack:
 dfb57ee8 dfaabde0 df0ff900 df294014 c0725b88 df294000 c0621244 df800150
 df809040 df8cd9c0 df9563e0 c0724b48 df8cda68 dfbe2914 dfbe2900 df0ff900
 00000005 c024135f dfb57f2c fffffff2 c0620968 c0241836 00000b5e 00000000
Call Trace:
 [<c024135f>] cgroup_tasks_write+0x6b/0x91
 [<c0241836>] cgroup_file_write+0xcd/0x1c3
 [<c02606ae>] handle_mm_fault+0x4da/0x52a
 [<c0241769>] cgroup_file_write+0x0/0x1c3
 [<c0271fa1>] vfs_write+0x83/0xf6
 [<c027245f>] sys_write+0x3c/0x63
 [<c0202d1a>] syscall_call+0x7/0xb
 [<c04f0000>] do_nanosleep+0x25/0x8c
Code: 8d 50 08 a3 48 4b 72 c0 89 68 10 8b 4d 14 c7 40 04 48 4b 72 c0 89 48 08 89 51 04 89 55 14 89 5a 04 8d 44 24 28 39 44 24 28 74 04 <0f> 0b eb fe 8d 45 1c ff 05 c8 5b 72 c0 e8 d6 ec ff ff 8d 4d 04
EIP: [<c02411bb>] cgroup_attach_task+0x281/0x3ba SS:ESP 0068:dfb57ec0
---[ end trace d624dca4c4cd0d89 ]---

==============================================================

You can use $! to refer to the most recently started background process.

This command should have failed with EBUSY, since freezer is already part of an existing hierarchy. So I think it's a red herring in this problem.

Is there a reason for this commented-out line?

This is the line that crashes?

What is this line in your tree?

Paul

But in fact taking out this line does stop the crash. So it's a bug in the error handling when you try to mount a hierarchy with a subsystem that's already busy elsewhere.

Paul

It's due to root_count getting out of sync with reality. It gets incremented in cgroup_get_sb() after we've done all error checking, but decremented in cgroup_kill_sb, which can be called on a superblock that we tried to create but then gave up on. I guess the fix is to increment root_count as soon as we have the superblock.

Paul

I see what you mean. I am still mystified how rcu_read_lock() around
the cgroup iterator in cgroupstats_build() hid/prevented the problem though :/.

Incrementing the root_count early dissociates root_count and the
addition to root list, doesn't it? How about only decrementing the count if we're on the list as in the patch below?

Cheers,
-Matt Helsley

Subject: [PATCH][RFC] Fix incorrect root_count decrement when recovering from error in cgroup_get_sb()

We may be recovering from an error in cgroup_get_sb() that occured prior to adding the new hierarchy root to the roots list. Check if root was added to roots and only decrement if it has been.

---
Totally untested. Need to check that &root->root_list is empty whenever root is off the roots list.

 kernel/cgroup.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Index: linux-2.6.29-rc1/kernel/cgroup.c
===================================================================
@@ -1115,8 +1115,11 @@ static void cgroup_kill_sb(struct super_
 	}
 	write_unlock(&css_set_lock);
 
-	list_del(&root->root_list);
-	root_count--;
+	if (!list_empty(&root->root_list)) {
+		list_del(&root->root_list);
+		root_count--;
+	} /* else we're recovering from an error in cgroup_get_sb() and hadn't added to
+	     the root list yet */
 
 	mutex_unlock(&cgroup_mutex);

I think the problem was in git bisect - the problematic commit was a couple later than the one Serge pointed at.

That's basically the patch that I already sent out. That check was there originally, but removed in e5f6a8609bab0c2d7543ab1505105e011832afd7 as a "redundant check".

Paul

Hah - thanks, I actually looked for something like that but couldn't find it.

Sorry, I'd meant to look into whether that needed to be there or not, but decided to put it off until after git bisecting, then forgot, probably because the bisect result was so confusing.

I see you already sent a patch for the *real* problem, thanks!

-serge

I think the problematic commit was e5f6a8609bab0c2d7543ab1505105e011832afd7, which removed a "redundant check" from cgroup_kill_sb(). So the good news is this hasn't made it into a released kernel yet. I sent a patch to put that check back in.

Paul