vfs: INFO: possible circular locking dependency detected

Hi all,

I've started seeing the following warning while fuzzing inside a KVM guest with the latest -next:

[   91.892449] ======================================================
[   91.894167] [ INFO: possible circular locking dependency detected ]
[   91.894167] 3.4.0-rc6-next-20120508-sasha-00003-g9f34dd7-dirty #120 Tainted: G        W   
[   91.894167] -------------------------------------------------------
[   91.894167] trinity/21513 is trying to acquire lock:
[   91.894167]  (&p->lock){+.+.+.}, at: [<ffffffff811fe53a>] seq_read+0x3a/0x420
[   91.894167] 
[   91.894167] but task is already holding lock:
[   91.894167]  (&sig->cred_guard_mutex){+.+.+.}, at: [<ffffffff811e2cb2>] prepare_bprm_creds+0x32/0x80
[   91.894167] 
[   91.894167] which lock already depends on the new lock.
[   91.894167] 
[   91.894167] 
[   91.894167] the existing dependency chain (in reverse order) is:
[   91.894167] 
[   91.894167] -> #1 (&sig->cred_guard_mutex){+.+.+.}:
[   91.894167]        [<ffffffff81115e3e>] validate_chain.clone.26+0x70e/0x8c0
[   91.894167]        [<ffffffff8111910d>] __lock_acquire+0xa3d/0xb00
[   91.894167]        [<ffffffff81119881>] lock_acquire+0xd1/0x110
[   91.894167]        [<ffffffff82d89515>] __mutex_lock_common+0x65/0x570
[   91.894167]        [<ffffffff82d89b00>] mutex_lock_killable_nested+0x40/0x50
[   91.894167]        [<ffffffff810b1a7f>] mm_access+0x2f/0xb0
[   91.894167]        [<ffffffff8123f0c4>] m_start+0x74/0x1c0
[   91.894167]        [<ffffffff811fe687>] seq_read+0x187/0x420
[   91.894167]        [<ffffffff811dac8d>] vfs_read+0xbd/0x190
[   91.894167]        [<ffffffff811db04f>] sys_read+0x4f/0x90
[   91.894167]        [<ffffffff82d8dbbd>] system_call_fastpath+0x1a/0x1f
[   91.894167] 
[   91.894167] -> #0 (&p->lock){+.+.+.}:
[   91.894167]        [<ffffffff8111523e>] check_prev_add+0x11e/0x610
[   91.894167]        [<ffffffff81115e3e>] validate_chain.clone.26+0x70e/0x8c0
[   91.894167]        [<ffffffff8111910d>] __lock_acquire+0xa3d/0xb00
[   91.894167]        [<ffffffff81119881>] lock_acquire+0xd1/0x110
[   91.894167]        [<ffffffff82d89515>] __mutex_lock_common+0x65/0x570
[   91.894167]        [<ffffffff82d89a60>] mutex_lock_nested+0x40/0x50
[   91.894167]        [<ffffffff811fe53a>] seq_read+0x3a/0x420
[   91.894167]        [<ffffffff81241769>] proc_reg_read+0xa9/0xe0
[   91.894167]        [<ffffffff811dac8d>] vfs_read+0xbd/0x190
[   91.894167]        [<ffffffff811e2a51>] kernel_read+0x41/0x60
[   91.894167]        [<ffffffff811e2ed3>] prepare_binprm+0x123/0x140
[   91.894167]        [<ffffffff811e3487>] do_execve_common.clone.32+0x197/0x320
[   91.894167]        [<ffffffff811e3626>] do_execve+0x16/0x20
[   91.894167]        [<ffffffff81056571>] sys_execve+0x51/0x80
[   91.894167]        [<ffffffff82d8e08c>] stub_execve+0x6c/0xc0
[   91.894167] 
[   91.894167] other info that might help us debug this:
[   91.894167] 
[   91.894167]  Possible unsafe locking scenario:
[   91.894167] 
[   91.894167]        CPU0                    CPU1
[   91.894167]        ----                    ----
[   91.894167]   lock(&sig->cred_guard_mutex);
[   91.894167]                                lock(&p->lock);
[   91.894167]                                lock(&sig->cred_guard_mutex);
[   91.894167]   lock(&p->lock);
[   91.894167] 
[   91.894167]  *** DEADLOCK ***
[   91.894167] 
[   91.894167] 1 lock held by trinity/21513:
[   91.894167]  #0:  (&sig->cred_guard_mutex){+.+.+.}, at: [<ffffffff811e2cb2>] prepare_bprm_creds+0x32/0x80
[   91.894167] 
[   91.894167] stack backtrace:
[   91.894167] Pid: 21513, comm: trinity Tainted: G        W    3.4.0-rc6-next-20120508-sasha-00003-g9f34dd7-dirty #120
[   91.894167] Call Trace:
[   91.894167]  [<ffffffff811144d7>] print_circular_bug+0x107/0x130
[   91.894167]  [<ffffffff8111523e>] check_prev_add+0x11e/0x610
[   91.894167]  [<ffffffff81115e3e>] validate_chain.clone.26+0x70e/0x8c0
[   91.894167]  [<ffffffff8111910d>] __lock_acquire+0xa3d/0xb00
[   91.894167]  [<ffffffff81116c06>] ? mark_held_locks+0xe6/0x120
[   91.894167]  [<ffffffff81119881>] lock_acquire+0xd1/0x110
[   91.894167]  [<ffffffff811fe53a>] ? seq_read+0x3a/0x420
[   91.894167]  [<ffffffff82d8d426>] ? retint_kernel+0x26/0x30
[   91.894167]  [<ffffffff82d89515>] __mutex_lock_common+0x65/0x570
[   91.894167]  [<ffffffff811fe53a>] ? seq_read+0x3a/0x420
[   91.894167]  [<ffffffff810b4879>] ? vprintk+0x3f9/0x480
[   91.894167]  [<ffffffff811fe53a>] ? seq_read+0x3a/0x420
[   91.894167]  [<ffffffff811fe500>] ? seq_open+0xb0/0xb0
[   91.894167]  [<ffffffff82d89a60>] mutex_lock_nested+0x40/0x50
[   91.894167]  [<ffffffff811fe53a>] seq_read+0x3a/0x420
[   91.894167]  [<ffffffff810e891e>] ? sub_preempt_count+0xae/0xe0
[   91.894167]  [<ffffffff811fe500>] ? seq_open+0xb0/0xb0
[   91.894167]  [<ffffffff81241769>] proc_reg_read+0xa9/0xe0
[   91.894167]  [<ffffffff811dac8d>] vfs_read+0xbd/0x190
[   91.894167]  [<ffffffff811e2a51>] kernel_read+0x41/0x60
[   91.894167]  [<ffffffff811e2ed3>] prepare_binprm+0x123/0x140
[   91.894167]  [<ffffffff811e3487>] do_execve_common.clone.32+0x197/0x320
[   91.894167]  [<ffffffff811e3626>] do_execve+0x16/0x20
[   91.894167]  [<ffffffff81056571>] sys_execve+0x51/0x80
[   91.894167]  [<ffffffff82d8e08c>] stub_execve+0x6c/0xc0

[->read() may grab ->cred_guard_mutex, but it may itself be called by prepare_binprm() after having ->cred_guard_mutex grabbed]

Nasty, that... What's more, it's not just prepare_binprm() itself - ->load_binary() might end up calling read(); it doesn't have to limit itself to mmap(), so essentially anything that can be grabbed by ->read() of a regular file might nest under ->cred_guard_mutex.

	AFAICS, /proc/*/stack, /proc/*/syscall, /proc/*/personality,

/proc/*/io_accounting, /proc/*/auxv, /proc/*/environ, /proc/*/*maps and /proc/*/pagemap have ->cred_guard_mutex grabbed on read. seq_file is a red herring here - io_accounting has the same issue and it does things directly, without seq_read().

It's not a realistic attack, fortunately, since you need root
to get past open_exec() on any of those... Wait. How _did_ you get past open_exec(), anyway? MAY_EXEC is not supposed to be granted on anything that has no exec bits at all and AFAICS none of those files have them.

You could chmod +x and run them, no?

Can't. proc_setattr() will give you -EPERM and refuse to do anything if you call it with ATTR_MODE in ->ia_valid.

If we look at /proc/irq/*/smp_affinity, which uses seq file ops, we can do this:

sh-4.2# ls -al /proc/irq/5/smp_affinity
-rw------- 1 root 0 0 May 9 16:35 /proc/irq/5/smp_affinity sh-4.2# chmod +x /proc/irq/5/smp_affinity
sh-4.2# ls -al /proc/irq/5/smp_affinity
-rwx--x--x 1 root 0 0 May 9 16:35 /proc/irq/5/smp_affinity sh-4.2# /proc/irq/5/smp_affinity /proc/irq/5/smp_affinity: line 1: 1f: command not found

There are quite a lot of files under /proc that let me do that.

OTOH, you probably can do that on unrelated seq_file outside of per-process part of procfs. So, yes, one could get a warning like that if they, as root, would do e.g.
chmod +x /proc/swaps
attempt to execve() /proc/swaps
cat /proc/self/environ
and enjoy the hard-earned false positive (it's a different seq_file, so we have no deadlock). If that's _all_ that happened, I'm not particulary concerned; it's not pretty, but saying "thou shalt not grab ->cred_guard_mutex anywhere in ->read() on anything that has exec bits or might get one" is not too terrible. If that's something else, though, we might have a real problem...

That's probably the case. The proc file got chmodded and exec'ed.

Can we do something to eliminate this false positive though? I can't think of anything nice...

btw,
I've never seen this issue before, even though I run same tests for a while now. What could have triggered it now?

Assuming you're running trinity du-jour, I only added fuzzing of execve yesterday.

Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

FWIW, that's _probably_ a false positive, but I really wonder what has triggered it. It would take seq_file-based file somewhere with _some_ exec bits set (otherwise it shouldn't have been seen by prepare_binprm()). The file itself isn't one of those that grab ->cred_guard_mutex anywhere in their ->read(), but since lockdep can't tell one seq_file from another, we get the warning.

The interesting part is who the hell had managed to do executable
seq_file-based anything - false positive or not, it's almost certainly a bug...