WARNING: at fs/namespace.c:636 mntput_no_expire+0xac/0xf2()

Thread 1:
for ((; ;))

  {
      mount -t cgroup -o cpuset xxx /mnt > /dev/null 2>&1
      mkdir /mnt/0 > /dev/null 2>&1
      rmdir /mnt/0 > /dev/null 2>&1
      umount /mnt > /dev/null 2>&1

}

Thread 2:
for ((; ;))
{
mount -t cpuset xxx /mnt > /dev/null 2>&1
umount /mnt > /dev/null 2>&1
}

(Note: Again it is irrelevant which cgroup subsys is used.)

After a while this showed up:

------------[ cut here ]------------
WARNING: at fs/namespace.c:636 mntput_no_expire+0xac/0xf2()
Hardware name: Aspire SA85
Modules linked in: bridge stp llc autofs4 dm_mirror dm_region_hash dm_log dm_mod r8169 parport_pc mii parport sg button sata_sis pata_sis ata_generic libata sd_mod scsi_mod ext3 jbd mbcache uhci_hcd ohci_hcd ehci_hcd [last unloaded: scsi_wait_scan]
Pid: 4745, comm: umount Not tainted 2.6.28 #479
Call Trace:
 [<c042bbe3>] warn_slowpath+0x79/0x8f
 [<c044babf>] ? __lock_acquire+0x69a/0x700
 [<c04ae44e>] ? mntput_no_expire+0x79/0xf2
 [<c04ae481>] mntput_no_expire+0xac/0xf2
 [<c04ae968>] sys_umount+0x26a/0x2b1
 [<c04ae9c1>] sys_oldumount+0x12/0x14
 [<c0403251>] sysenter_do_call+0x12/0x31
---[ end trace 79d0ab4bef01333f ]---

The WARNING is: WARN_ON(atomic_read(&mnt->__mnt_writers));

(cc's added)

Quoted 35 lines · On Mon, 05 Jan 2009 11:23:33 +0800 Li Zefan <[email protected]> wrote:

Thread 1:
for ((; ;))

  {
      mount -t cgroup -o cpuset xxx /mnt > /dev/null 2>&1
      mkdir /mnt/0 > /dev/null 2>&1
      rmdir /mnt/0 > /dev/null 2>&1
      umount /mnt > /dev/null 2>&1

}

Thread 2:
for ((; ;))
{
mount -t cpuset xxx /mnt > /dev/null 2>&1
umount /mnt > /dev/null 2>&1
}

(Note: Again it is irrelevant which cgroup subsys is used.)

After a while this showed up:

------------[ cut here ]------------
WARNING: at fs/namespace.c:636 mntput_no_expire+0xac/0xf2()
Hardware name: Aspire SA85
Modules linked in: bridge stp llc autofs4 dm_mirror dm_region_hash dm_log dm_mod r8169 parport_pc mii parport sg button sata_sis pata_sis ata_generic libata sd_mod scsi_mod ext3 jbd mbcache uhci_hcd ohci_hcd ehci_hcd [last unloaded: scsi_wait_scan]
Pid: 4745, comm: umount Not tainted 2.6.28 #479
Call Trace:
 [<c042bbe3>] warn_slowpath+0x79/0x8f
 [<c044babf>] ? __lock_acquire+0x69a/0x700
 [<c04ae44e>] ? mntput_no_expire+0x79/0xf2
 [<c04ae481>] mntput_no_expire+0xac/0xf2
 [<c04ae968>] sys_umount+0x26a/0x2b1
 [<c04ae9c1>] sys_oldumount+0x12/0x14
 [<c0403251>] sysenter_do_call+0x12/0x31
---[ end trace 79d0ab4bef01333f ]---

Quoted 1 line

The WARNING is: WARN_ON(atomic_read(&mnt->__mnt_writers));

OK, I'm all confused. Here we see a WARN_ON triggered, but in
http://lkml.org/lkml/2009/1/4/352 with the same testcase we're seeing a lockdep warning.

You refer to Arjan's "lockdep: annotate sb ->s_umount" patch - but that's over two years old.

And you say "The changelog said s_umount needs to be classified as per-sb, but actually it made it as per-filesystem." But what is the difference between per-sb and per-fs?

More info here: http://bugzilla.kernel.org/show_bug.cgi?id=12673

This bug report seems to be all over the place.

Is it a post-2.6.28 regression, btw?

	BTW, a trivial note - kfree(root) in your ->kill_sb() is done

earlier than it's nice to do. Shouldn't affect the problem, though.

Quoted 2 lines · Al Viro

	BTW, a trivial note - kfree(root) in your ->kill_sb() is done

earlier than it's nice to do. Shouldn't affect the problem, though.

	Other probably irrelevant notes:

                memcpy(start, cgrp->dentry->d_name.name, len);
                cgrp = cgrp->parent;
                if (!cgrp)
                        break;
                dentry = rcu_dereference(cgrp->dentry);

in cgroup_path(). Why don't we need rcu_dereference on both? Moreover, shouldn't that be

                memcpy(start, dentry->d_name.name, len);

anyway, seeing that we'd just looked at dentry->d_name.len?

In cgroup_rmdir():

        spin_lock(&cgrp->dentry->d_lock);
        d = dget(cgrp->dentry);
        spin_unlock(&d->d_lock);

        cgroup_d_remove_dir(d);
        dput(d);

Er? Comments, please... Unless something very unusual is going on, either that d_lock is pointless or dget() is rather unsafe.

cgroups_clone()

        /* Now do the VFS work to create a cgroup */
        inode = parent->dentry->d_inode;

        /* Hold the parent directory mutex across this operation to
         * stop anyone else deleting the new cgroup */
        mutex_lock(&inode->i_mutex);

Can the parent be in process of getting deleted by somebody else? If yes, we are in trouble here.

BTW, that thing in cgroup_path()... What guarantees that cgroup_rename() won't hit between getting len and doing memcpy()?

That said, cgroup seems to be completely agnostic wrt anything happening on vfsmount level, so I really don't see how it gets to that WARN_ON(). Hell knows; I really want to see the sequence of events - it might be something like fscking up ->s_active handling with interesting results (cgroup code is certainly hitting it in not quite usual ways), it may be genuine VFS-only race. Need more data...

Quoted 9 lines

Thread 1:
for ((; ;))

  {
      mount -t cgroup -o cpuset xxx /mnt > /dev/null 2>&1
      mkdir /mnt/0 > /dev/null 2>&1
      rmdir /mnt/0 > /dev/null 2>&1
      umount /mnt > /dev/null 2>&1

}

Quoted 5 lines

Thread 2:
{
mount -t cpuset xxx /mnt > /dev/null 2>&1
umount /mnt > /dev/null 2>&1
}

Quoted 9 lines · Andrew Morton

Quoted 9 lines

Thread 1:
for ((; ;))

  {
      mount -t cgroup -o cpuset xxx /mnt > /dev/null 2>&1
      mkdir /mnt/0 > /dev/null 2>&1
      rmdir /mnt/0 > /dev/null 2>&1
      umount /mnt > /dev/null 2>&1

}

Quoted 5 lines

Quoted 5 lines

Thread 2:
{
mount -t cpuset xxx /mnt > /dev/null 2>&1
umount /mnt > /dev/null 2>&1
}

How cute... Same mountpoint in both, so these mount(2) will sometimes fail (cgroup picks the same sb on the same options, AFAICS) and fail silently due to these redirects...

That's a lovely way to stress-test a large part of ro-bind stuff *and* umount()-related code. Could you do C equivalent of the above (just the same syscalls in loop, nothing fancier) and do time-stamped strace?

Quoted 19 lines · Al Viro

Quoted 9 lines · Andrew Morton

Quoted 9 lines

Thread 1:
for ((; ;))

  {
      mount -t cgroup -o cpuset xxx /mnt > /dev/null 2>&1
      mkdir /mnt/0 > /dev/null 2>&1
      rmdir /mnt/0 > /dev/null 2>&1
      umount /mnt > /dev/null 2>&1

}

Quoted 5 lines

Quoted 5 lines

Thread 2:
{
mount -t cpuset xxx /mnt > /dev/null 2>&1
umount /mnt > /dev/null 2>&1
}

How cute... Same mountpoint in both, so these mount(2) will sometimes fail (cgroup picks the same sb on the same options, AFAICS) and fail silently due to these redirects...

Quoted 4 lines

That's a lovely way to stress-test a large part of ro-bind stuff *and* umount()-related code. Could you do C equivalent of the above (just the same syscalls in loop, nothing fancier) and do time-stamped strace?

Sure, I'll write a C version and try to reproduce the warning.

Quoted 25 lines · Li Zefan

Quoted 23 lines · Al Viro

Quoted 9 lines · Andrew Morton

Quoted 9 lines

Thread 1:
for ((; ;))

  {
      mount -t cgroup -o cpuset xxx /mnt > /dev/null 2>&1
      mkdir /mnt/0 > /dev/null 2>&1
      rmdir /mnt/0 > /dev/null 2>&1
      umount /mnt > /dev/null 2>&1

}

Quoted 5 lines

Quoted 5 lines

Thread 2:
{
mount -t cpuset xxx /mnt > /dev/null 2>&1
umount /mnt > /dev/null 2>&1
}

How cute... Same mountpoint in both, so these mount(2) will sometimes fail (cgroup picks the same sb on the same options, AFAICS) and fail silently due to these redirects...

That's a lovely way to stress-test a large part of ro-bind stuff *and* umount()-related code. Could you do C equivalent of the above (just the same syscalls in loop, nothing fancier) and do time-stamped strace?

Quoted 2 lines

Sure, I'll write a C version and try to reproduce the warning.

Unfortunately, the C equivalent can't reproduce the warning, I've run the test for the whole night. :( While using the script, often I can trigger the warning in several mins.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

How cute... Same mountpoint in both, so these mount(2) will sometimes fail (cgroup picks the same sb on the same options, AFAICS) and fail silently due to these redirects...

That's a lovely way to stress-test a large part of ro-bind stuff *and* umount()-related code. Could you do C equivalent of the above (just the same syscalls in loop, nothing fancier) and do time-stamped strace?

Quoted 5 lines

Quoted 2 lines

Sure, I'll write a C version and try to reproduce the warning.

Unfortunately, the C equivalent can't reproduce the warning, I've run the test for the whole night. :( While using the script, often I can trigger the warning in several mins.

Ho-hum... I wonder if we are hitting cgroup_clone() in all that fun...

Could you

	a) add a printk to that sucker
	b) independently from (a), see if wrapping these syscalls into
	pid = fork();
	if (!pid) {
		[make a syscall, print something]
		exit(0);
	} else if (pid > 0) {
		waitpid(pid, NULL, 0);
	}

and see what happens...

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 8 lines

How cute... Same mountpoint in both, so these mount(2) will sometimes fail (cgroup picks the same sb on the same options, AFAICS) and fail silently due to these redirects...

That's a lovely way to stress-test a large part of ro-bind stuff *and* umount()-related code. Could you do C equivalent of the above (just the same syscalls in loop, nothing fancier) and do time-stamped strace?

Sure, I'll write a C version and try to reproduce the warning.

Quoted 4 lines

Quoted 3 lines

Unfortunately, the C equivalent can't reproduce the warning, I've run the test for the whole night. :( While using the script, often I can trigger the warning in several mins.

Ho-hum... I wonder if we are hitting cgroup_clone() in all that fun...

I don't think so, I think cgroup_clone() will be called only if namespace is used, like clone(CLONE_NEWNS). Even if cgroup_clone() gets called, it will return before doing any vfs work unless the ns_cgroup subsystem is mounted.

But the following testcase can also trigger the warning:

thread 1:
for ((; ;))

{
	mount -t cgroup -o ns xxx cgroup/ > /dev/null 2>&1
	# remove the dirs generated by cgroup_clone()
	rmdir cgroup/[1-9]* > /dev/null 2>&1
	umount cgroup/ > /dev/null 2>&1
}

thread 2:

int foo(void *arg)
{ return 0; }

char *stack[4096];

int main(int argc, char **argv)

{
        int usec = DEFAULT_USEC;
        while (1) {
                usleep(usec);
		# cgroup_clone() will be called
                clone(foo, stack+4096, CLONE_NEWNS, NULL);
        }

        return 0;
}

Quoted 26 lines

thread 1:
for ((; ;))

{
	mount -t cgroup -o ns xxx cgroup/ > /dev/null 2>&1
	# remove the dirs generated by cgroup_clone()
	rmdir cgroup/[1-9]* > /dev/null 2>&1
	umount cgroup/ > /dev/null 2>&1
}

thread 2:

int foo(void *arg)
{ return 0; }

char *stack[4096];

int main(int argc, char **argv)

{
        int usec = DEFAULT_USEC;
        while (1) {
                usleep(usec);
		# cgroup_clone() will be called
                clone(foo, stack+4096, CLONE_NEWNS, NULL);
        }

Quoted 2 lines

        return 0;
}

Uh-oh... That clone() will do more, actually - it will clone a bunch of vfsmounts. What happens if you create a separate namespace for the first thread, so that the second one would not have our vfsmount to play with?

Alternatively, what if the second thread is doing

	mount --bind cgroup foo
	umount foo

in a loop?

Another one: does turning the umount in the first thread into umount -l affect anything?

I ran following testcase, and triggered the warning in 1 hour:

thread 1:
for ((; ;))

{
        mount --bind /cgroup /mnt > /dev/null 2>&1
        umount /mnt > /dev/null 2>&1
}

tread 2:
for ((; ;))

{
        mount -t cgroup -o cpu xxx /cgroup > /dev/null 2>&1
        mkdir /cgroup/0 > /dev/null 2>&1
        rmdir /cgroup/0 > /dev/null 2>&1
        umount -l /cgroup > /dev/null 2>&1
}

Quoted 9 lines · Li Zefan

I ran following testcase, and triggered the warning in 1 hour:

thread 1:
for ((; ;))

{
        mount --bind /cgroup /mnt > /dev/null 2>&1
        umount /mnt > /dev/null 2>&1
}

Quoted 8 lines

tread 2:
for ((; ;))

{
        mount -t cgroup -o cpu xxx /cgroup > /dev/null 2>&1
        mkdir /cgroup/0 > /dev/null 2>&1
        rmdir /cgroup/0 > /dev/null 2>&1
        umount -l /cgroup > /dev/null 2>&1
}

Wow. You know, at that point these redirects could probably be removed.

If anything in there ends up producing an output, we very much want to see that. Actually, I'd even make that

	mount --bind /cgroup/mnt || (echo mount1: ; date)

etc., so we'd see when do they fail and which one fails (if any)...

Which umount has failed in the above, BTW?

Quoted 19 lines · Al Viro

Quoted 9 lines · Li Zefan

I ran following testcase, and triggered the warning in 1 hour:

thread 1:
for ((; ;))

{
        mount --bind /cgroup /mnt > /dev/null 2>&1
        umount /mnt > /dev/null 2>&1
}

Quoted 8 lines

tread 2:
for ((; ;))

{
        mount -t cgroup -o cpu xxx /cgroup > /dev/null 2>&1
        mkdir /cgroup/0 > /dev/null 2>&1
        rmdir /cgroup/0 > /dev/null 2>&1
        umount -l /cgroup > /dev/null 2>&1
}

Quoted 1 line

Wow. You know, at that point these redirects could probably be removed.

Ah, yes.

Quoted 5 lines

If anything in there ends up producing an output, we very much want to see that. Actually, I'd even make that

	mount --bind /cgroup/mnt || (echo mount1: ; date)

etc., so we'd see when do they fail and which one fails (if any)...

Quoted 3 lines

Which umount has failed in the above, BTW?

the first one sometimes failed, and the second one hasn't failed:

Quoted 20 lines · Al Viro

Quoted 9 lines · Li Zefan

I ran following testcase, and triggered the warning in 1 hour:

thread 1:
for ((; ;))

{
        mount --bind /cgroup /mnt > /dev/null 2>&1
        umount /mnt > /dev/null 2>&1
}

Quoted 8 lines

tread 2:
for ((; ;))

{
        mount -t cgroup -o cpu xxx /cgroup > /dev/null 2>&1
        mkdir /cgroup/0 > /dev/null 2>&1
        rmdir /cgroup/0 > /dev/null 2>&1
        umount -l /cgroup > /dev/null 2>&1
}

Quoted 1 line

Wow. You know, at that point these redirects could probably be removed.

Ah, yes.

Quoted 8 lines

If anything in there ends up producing an output, we very much want to see that. Actually, I'd even make that

	mount --bind /cgroup/mnt || (echo mount1: ; date)

etc., so we'd see when do they fail and which one fails (if any)...

Which umount has failed in the above, BTW?

the first one sometimes failed, and the second one hasn't failed:

mount: wrong fs type, bad option, bad superblock on /cgroup,
missing codepage or helper program, or other error In some cases useful info is found in syslog - try dmesg | tail or so

mount1

On Fri, Feb 13, 2009 at 06:41:35AM +0000, Al Viro wrote:

Aaaargh...

        /*
         * We don't have to hold all of the locks at the
         * same time here because we know that we're the
         * last reference to mnt and that no new writers
         * can come in.
         */
        for_each_possible_cpu(cpu) {
                struct mnt_writer *cpu_writer = &per_cpu(mnt_writers, cpu);
                if (cpu_writer->mnt != mnt)
                        continue;
                spin_lock(&cpu_writer->lock);

is *almost* OK. Modulo SMP cache coherency. We know that nothing should be setting ->mnt to ours anymore, that's fine. But we do not know if we'd seen *earlier* change done on CPU in question (not the one we are running __mntput() on).

I probably would still like to use milder solution in the long run, but for now let's check if turning that into

                struct mnt_writer *cpu_writer = &per_cpu(mnt_writers, cpu);
                spin_lock(&cpu_writer->lock);
                if (cpu_writer->mnt != mnt) {
			spin_unlock(&cpu_writer->lock);
                        continue;
		}

prevents the problem, OK?

Quoted 24 lines · Al Viro

On Fri, Feb 13, 2009 at 06:41:35AM +0000, Al Viro wrote:

Aaaargh...

        /*
         * We don't have to hold all of the locks at the
         * same time here because we know that we're the
         * last reference to mnt and that no new writers
         * can come in.
         */
        for_each_possible_cpu(cpu) {
                struct mnt_writer *cpu_writer = &per_cpu(mnt_writers, cpu);
                if (cpu_writer->mnt != mnt)
                        continue;
                spin_lock(&cpu_writer->lock);

is *almost* OK. Modulo SMP cache coherency. We know that nothing should be setting ->mnt to ours anymore, that's fine. But we do not know if we'd seen *earlier* change done on CPU in question (not the one we are running __mntput() on).

I probably would still like to use milder solution in the long run, but for now let's check if turning that into

Quoted 8 lines

                struct mnt_writer *cpu_writer = &per_cpu(mnt_writers, cpu);
                spin_lock(&cpu_writer->lock);
                if (cpu_writer->mnt != mnt) {
			spin_unlock(&cpu_writer->lock);
                        continue;
		}

prevents the problem, OK?

Sure, I'll try. :)

Quoted 9 lines

Quoted 8 lines

                struct mnt_writer *cpu_writer = &per_cpu(mnt_writers, cpu);
                spin_lock(&cpu_writer->lock);
                if (cpu_writer->mnt != mnt) {
			spin_unlock(&cpu_writer->lock);
                        continue;
		}

prevents the problem, OK?

Quoted 2 lines

Sure, I'll try. :)

Not a single warning for the whole weekend, so I think above change works.

OK... So here's what we really want:

	* we know that nobody will set cpu_writer->mnt to mnt from now on
	* all changes to that sucker are done under cpu_writer->lock
	* we want the laziest equivalent of
		spin_lock(&cpu_writer->lock);
		if (likely(cpu_writer->mnt != mnt)) {
			spin_unlock(&cpu_writer->lock);
			continue;
		}
		/* do stuff */

that would make sure we won't miss earlier setting of ->mnt done by another CPU.

Anyway, for now (HEAD and all -stable starting with 2.6.26) we want this:

namespace.c +4 -2

@@ -614,9 +614,11 @@
 	 */
 	for_each_possible_cpu(cpu) {
 		struct mnt_writer *cpu_writer = &per_cpu(mnt_writers, cpu);
-		if (cpu_writer->mnt != mnt)
-			continue;
 		spin_lock(&cpu_writer->lock);
+		if (cpu_writer->mnt != mnt) {
+			spin_unlock(&cpu_writer->lock);
+			continue;
+		}
 		atomic_add(cpu_writer->count, &mnt->__mnt_writers);
 		cpu_writer->count = 0;
 		/*

And here is my:

Tested-by Li Zefan <[email protected]>

Quoted 9 lines · Andrew Morton

Quoted 9 lines

Thread 1:
for ((; ;))

  {
      mount -t cgroup -o cpuset xxx /mnt > /dev/null 2>&1
      mkdir /mnt/0 > /dev/null 2>&1
      rmdir /mnt/0 > /dev/null 2>&1
      umount /mnt > /dev/null 2>&1

}

Quoted 5 lines

Quoted 5 lines

Thread 2:
{
mount -t cpuset xxx /mnt > /dev/null 2>&1
umount /mnt > /dev/null 2>&1
}

How cute... Same mountpoint in both, so these mount(2) will sometimes fail (cgroup picks the same sb on the same options, AFAICS) and fail silently due to these redirects...

That's a lovely way to stress-test a large part of ro-bind stuff *and* umount()-related code. Could you do C equivalent of the above (just the same syscalls in loop, nothing fancier) and do time-stamped strace?

Quoted 16 lines · Al Viro

Quoted 9 lines · Andrew Morton

Quoted 9 lines

Thread 1:
for ((; ;))

  {
      mount -t cgroup -o cpuset xxx /mnt > /dev/null 2>&1
      mkdir /mnt/0 > /dev/null 2>&1
      rmdir /mnt/0 > /dev/null 2>&1
      umount /mnt > /dev/null 2>&1

}

Quoted 5 lines

Quoted 5 lines

Thread 2:
{
mount -t cpuset xxx /mnt > /dev/null 2>&1
umount /mnt > /dev/null 2>&1
}

Quoted 8 lines

How cute... Same mountpoint in both, so these mount(2) will sometimes fail (cgroup picks the same sb on the same options, AFAICS) and fail silently due to these redirects...

That's a lovely way to stress-test a large part of ro-bind stuff *and* umount()-related code. Could you do C equivalent of the above (just the same syscalls in loop, nothing fancier) and do time-stamped strace?

Could you also add a printk of what ->__mnt_writers was at the time of the WARN_ON()? That will hopefully at least tell us whether we're looking at a real leak or just a single missed mnt_want/drop_write(). Also hopefully in which direction the thing is biased. With the mount not being around long I'm not horribly hopeful, but it can't hurt.