From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Kirill A. Shutemov" Subject: [PATCH, RESEND] procfs: silence lockdep warning about read vs. exec seq_file Date: Sat, 2 Aug 2014 23:10:27 +0300 Message-ID: <1407010227-2269-1-git-send-email-kirill@shutemov.name> Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, David Howells , Peter Zijlstra , Sasha Levin , Cyrill Gorcunov , Oleg Nesterov , "David S. Miller" , "Kirill A. Shutemov" To: Alexander Viro Return-path: Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org From: "Kirill A. Shutemov" Testcase: cat /proc/self/maps >/dev/null chmod +x /proc/self/net/packet exec /proc/self/net/packet It triggers lockdep warning: [ INFO: possible circular locking dependency detected ] 3.16.0-rc7-00064-g26bcd8b72563 #8 Not tainted ------------------------------------------------------- sh/157 is trying to acquire lock: (&p->lock){+.+.+.}, at: [] seq_read+0x38/0x3e0 but task is already holding lock: (&sig->cred_guard_mutex){+.+.+.}, at: [] prepare_bprm_creds+0x28/0x90 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (&sig->cred_guard_mutex){+.+.+.}: [] __lock_acquire+0x531/0xde0 [] lock_acquire+0x79/0xd0 [] mutex_lock_killable_nested+0x68/0x460 [] lock_trace+0x1f/0x60 [] proc_pid_personality+0x17/0x60 [] proc_single_show+0x4b/0x90 [] seq_read+0xe0/0x3e0 [] vfs_read+0x8e/0x170 [] SyS_read+0x48/0xc0 [] system_call_fastpath+0x16/0x1b -> #0 (&p->lock){+.+.+.}: [] validate_chain.isra.37+0xfe7/0x13b0 [] __lock_acquire+0x531/0xde0 [] lock_acquire+0x79/0xd0 [] mutex_lock_nested+0x6a/0x3d0 [] seq_read+0x38/0x3e0 [] proc_reg_read+0x43/0x70 [] vfs_read+0x8e/0x170 [] kernel_read+0x43/0x60 [] prepare_binprm+0xd5/0x170 [] do_execve_common.isra.32+0x548/0x800 [] do_execve+0x13/0x20 [] SyS_execve+0x20/0x30 [] stub_execve+0x69/0xa0 other info that might help us debug this: Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(&sig->cred_guard_mutex); lock(&p->lock); lock(&sig->cred_guard_mutex); lock(&p->lock); *** DEADLOCK *** 1 lock held by sh/157: #0: (&sig->cred_guard_mutex){+.+.+.}, at: [] prepare_bprm_creds+0x28/0x90 It's a false positive: seq files which take cred_guard_mutex are never executable. Let's use separate lock class for them. I don't know why we allow "chmod +x" on some proc files, notably net-related. Is it a bug? Also I suspect eb94cd96e05d fixes non-existing bug, like this one. Signed-off-by: Kirill A. Shutemov --- fs/proc/base.c | 24 +++++++++++++++++++++++- fs/proc/task_mmu.c | 14 ++++++++++++++ fs/proc/task_nommu.c | 4 ++++ 3 files changed, 41 insertions(+), 1 deletion(-) diff --git a/fs/proc/base.c b/fs/proc/base.c index 2d696b0c93bf..c05b4a227acb 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -655,9 +655,31 @@ static int proc_single_show(struct seq_file *m, void *v) return ret; } +/* + * proc_pid_personality() and proc_pid_stack() take cred_guard_mutex via + * lock_trace() with seq_file->lock held. + * execve(2) calls vfs_read() with cred_guard_mutex held. + * + * So if you will try to execute a seq_file, lockdep will report a possible + * circular locking dependency. It's false-positive, since ONE() files are + * never executable. + * + * Let's set separate lock class for seq_file->lock of ONE() files. + */ +static struct lock_class_key proc_single_open_lock_class; + static int proc_single_open(struct inode *inode, struct file *filp) { - return single_open(filp, proc_single_show, inode); + struct seq_file *m; + int ret; + + ret = single_open(filp, proc_single_show, inode); + if (ret) + return ret; + + m = filp->private_data; + lockdep_set_class(&m->lock, &proc_single_open_lock_class); + return 0; } static const struct file_operations proc_single_file_operations = { diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index cfa63ee92c96..536b9f9a9ff5 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -19,6 +19,18 @@ #include #include "internal.h" +/* + * m_start() takes cred_guard_mutex via mm_access() with seq_file->lock held. + * execve(2) calls vfs_read() with cred_guard_mutex held. + * + * So if you will try to execute a seq_file, lockdep will report a possible + * circular locking dependency. It's false positive, since m_start() users are + * never executable. + * + * Let's set separate class lock for seq_file->lock of m_start() users. + */ +static struct lock_class_key pid_maps_seq_file_lock; + void task_mem(struct seq_file *m, struct mm_struct *mm) { unsigned long data, text, lib, swap; @@ -242,6 +254,7 @@ static int do_maps_open(struct inode *inode, struct file *file, ret = seq_open(file, ops); if (!ret) { struct seq_file *m = file->private_data; + lockdep_set_class(&m->lock, &pid_maps_seq_file_lock); m->private = priv; } else { kfree(priv); @@ -1512,6 +1525,7 @@ static int numa_maps_open(struct inode *inode, struct file *file, ret = seq_open(file, ops); if (!ret) { struct seq_file *m = file->private_data; + lockdep_set_class(&m->lock, &pid_maps_seq_file_lock); m->private = priv; } else { kfree(priv); diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c index 678455d2d683..35a799443990 100644 --- a/fs/proc/task_nommu.c +++ b/fs/proc/task_nommu.c @@ -9,6 +9,9 @@ #include #include "internal.h" +/* See comment in task_mmu.c */ +static struct lock_class_key pid_maps_seq_file_lock; + /* * Logic: we've got two memory sums for each process, "shared", and * "non-shared". Shared memory may get counted more than once, for @@ -277,6 +280,7 @@ static int maps_open(struct inode *inode, struct file *file, ret = seq_open(file, ops); if (!ret) { struct seq_file *m = file->private_data; + lockdep_set_class(&m->lock, &pid_maps_seq_file_lock); m->private = priv; } else { kfree(priv); -- 2.0.3