diff mbox

SELinux + ubifs: possible circular locking dependency

Message ID 1360826119.12703.155.camel@sauron.fi.intel.com
State New, archived
Headers show

Commit Message

Artem Bityutskiy Feb. 14, 2013, 7:15 a.m. UTC
Mark, how about this one? I compiled it and ran on my fedora 16 with
SElinux enabled, no obvious issues.

From a19350097200570571aa522afebb96b34db534f4 Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Date: Thu, 14 Feb 2013 09:07:36 +0200
Subject: [PATCH] selinux: do not confuse lockdep

Selinux has per-inode mutexes called 'isec->lock', and they are initialized in
the same place, which makes lockdep treat all of the them as if they were
identical. However, locking rules may be a little bit different depending on
the file-system, so we should put these locks to separate classes, just like we
do for 'i_mutex'. Namely, we should put them to per-FS type classes, which is
exactly what this patch does.

The problem this patch intends to fix is a strange lockdep warning, which I,
frankly speaking, do not really understand, but I believe the root-cause should
be fixed by this patch.

Look at the stacktrace #4: here we have 'debugfs_create_dir()'

[    5.390312] ======================================================
[    5.396500] [ INFO: possible circular locking dependency detected ]
[    5.402781] 3.8.0-rc6-00005-g4f7e39d #49 Not tainted
[    5.407750] -------------------------------------------------------
[    5.414031] systemd/1 is trying to acquire lock:
[    5.418656]  (&c->tnc_mutex){+.+...}, at: [<c01552d0>] ubifs_tnc_locate+0x30/0x198
[    5.426343]
[    5.426343] but task is already holding lock:
[    5.432218]  (&isec->lock){+.+.+.}, at: [<c018e298>] inode_doinit_with_dentry+0x8c/0x55c
[    5.440375]
[    5.440375] which lock already depends on the new lock.
[    5.440375]
[    5.448593]
[    5.448593] the existing dependency chain (in reverse order) is:
[    5.456093]
-> #4 (&isec->lock){+.+.+.}:
[    5.460250]        [<c004e8e8>] lock_acquire+0x64/0x78
[    5.465437]        [<c0355d5c>] mutex_lock_nested+0x5c/0x2ec
[    5.471125]        [<c018e298>] inode_doinit_with_dentry+0x8c/0x55c
[    5.477437]        [<c0189bd8>] security_d_instantiate+0x1c/0x34
[    5.483500]        [<c017a614>] debugfs_mknod.part.15.constprop.18+0x94/0x128
[    5.490656]        [<c017a858>] __create_file+0x1b0/0x25c
[    5.496093]        [<c017a98c>] debugfs_create_dir+0x1c/0x28
[    5.501781]        [<c0491ce8>] pinctrl_init+0x1c/0xd0
[    5.506968]        [<c0008900>] do_one_initcall+0x108/0x17c
[    5.512593]        [<c0483858>] kernel_init_freeable+0xec/0x1b4
[    5.518562]        [<c034f398>] kernel_init+0x8/0xe4
[    5.523562]        [<c0009448>] ret_from_fork+0x14/0x2c
[    5.528812]
-> #3 (&sb->s_type->i_mutex_key#2){+.+.+.}:
[    5.534312]        [<c004e8e8>] lock_acquire+0x64/0x78
[    5.539468]        [<c0355d5c>] mutex_lock_nested+0x5c/0x2ec
[    5.545187]        [<c017a6f8>] __create_file+0x50/0x25c
[    5.550531]        [<c017a98c>] debugfs_create_dir+0x1c/0x28
[    5.556218]        [<c026cfc4>] clk_debug_create_subtree+0x1c/0x108
[    5.562500]        [<c0495bfc>] clk_debug_init+0x68/0xc0
[    5.567875]        [<c0008900>] do_one_initcall+0x108/0x17c
[    5.573468]        [<c0483858>] kernel_init_freeable+0xec/0x1b4
[    5.579437]        [<c034f398>] kernel_init+0x8/0xe4
[    5.584437]        [<c0009448>] ret_from_fork+0x14/0x2c
[    5.589687]
-> #2 (prepare_lock){+.+.+.}:
[    5.593937]        [<c004e8e8>] lock_acquire+0x64/0x78
[    5.599125]        [<c0355d5c>] mutex_lock_nested+0x5c/0x2ec
[    5.604812]        [<c026d400>] clk_prepare+0x18/0x38
[    5.609906]        [<c023a2f8>] __gpmi_enable_clk+0x30/0xb0
[    5.615531]        [<c023a7bc>] gpmi_begin+0x18/0x530
[    5.620625]        [<c0239244>] gpmi_select_chip+0x3c/0x54
[    5.626156]        [<c02341ac>] nand_do_read_ops+0x7c/0x3e4
[    5.631750]        [<c023483c>] nand_read+0x50/0x74
[    5.636656]        [<c02198e4>] part_read+0x5c/0xa4
[    5.641593]        [<c0217404>] mtd_read+0x84/0xb8
[    5.646406]        [<c02450cc>] ubi_io_read+0xa0/0x2c0
[    5.651593]        [<c0242640>] ubi_eba_read_leb+0x190/0x424
[    5.657281]        [<c0241b18>] ubi_leb_read+0xac/0x120
[    5.662562]        [<c01503e4>] ubifs_leb_read+0x28/0x8c
[    5.667906]        [<c0151f74>] ubifs_read_node+0x98/0x2a0
[    5.673437]        [<c014e780>] ubifs_read_sb_node+0x54/0x78
[    5.679125]        [<c014f444>] ubifs_read_superblock+0xc60/0x163c
[    5.685343]        [<c014d64c>] ubifs_mount+0x800/0x171c
[    5.690687]        [<c00b5740>] mount_fs+0x44/0x184
[    5.695593]        [<c00cd7ec>] vfs_kern_mount+0x4c/0xc0
[    5.700968]        [<c00cf394>] do_mount+0x18c/0x8d0
[    5.705968]        [<c00cfb5c>] sys_mount+0x84/0xb8
[    5.710875]        [<c0483bc8>] mount_block_root+0x118/0x258
[    5.716562]        [<c0483eac>] prepare_namespace+0x8c/0x17c
[    5.722281]        [<c034f398>] kernel_init+0x8/0xe4
[    5.727281]        [<c0009448>] ret_from_fork+0x14/0x2c
[    5.732531]
-> #1 (&le->mutex){++++..}:
[    5.736625]        [<c004e8e8>] lock_acquire+0x64/0x78
[    5.741812]        [<c0356468>] down_read+0x40/0x54
[    5.746718]        [<c02424e4>] ubi_eba_read_leb+0x34/0x424
[    5.752312]        [<c0241b18>] ubi_leb_read+0xac/0x120
[    5.757562]        [<c01503e4>] ubifs_leb_read+0x28/0x8c
[    5.762937]        [<c0151f74>] ubifs_read_node+0x98/0x2a0
[    5.768437]        [<c016edc0>] ubifs_load_znode+0x88/0x560
[    5.774062]        [<c0155254>] ubifs_lookup_level0+0x190/0x1dc
[    5.780031]        [<c01552e4>] ubifs_tnc_locate+0x44/0x198
[    5.785656]        [<c014c614>] ubifs_iget+0x6c/0x8a4
[    5.790718]        [<c014da64>] ubifs_mount+0xc18/0x171c
[    5.796093]        [<c00b5740>] mount_fs+0x44/0x184
[    5.801000]        [<c00cd7ec>] vfs_kern_mount+0x4c/0xc0
[    5.806343]        [<c00cf394>] do_mount+0x18c/0x8d0
[    5.811343]        [<c00cfb5c>] sys_mount+0x84/0xb8
[    5.816250]        [<c0483bc8>] mount_block_root+0x118/0x258
[    5.821968]        [<c0483eac>] prepare_namespace+0x8c/0x17c
[    5.827656]        [<c034f398>] kernel_init+0x8/0xe4
[    5.832656]        [<c0009448>] ret_from_fork+0x14/0x2c
[    5.837906]
-> #0 (&c->tnc_mutex){+.+...}:
[    5.842250]        [<c004de48>] __lock_acquire+0x14ec/0x1b08
[    5.847968]        [<c004e8e8>] lock_acquire+0x64/0x78
[    5.853125]        [<c0355d5c>] mutex_lock_nested+0x5c/0x2ec
[    5.858812]        [<c01552d0>] ubifs_tnc_locate+0x30/0x198
[    5.864437]        [<c0155a48>] ubifs_tnc_lookup_nm+0x28/0x150
[    5.870312]        [<c016fc18>] ubifs_getxattr+0xc0/0x254
[    5.875750]        [<c018e468>] inode_doinit_with_dentry+0x25c/0x55c
[    5.882125]        [<c018f1a8>] sb_finish_set_opts+0xa4/0x21c
[    5.887906]        [<c019055c>] selinux_set_mnt_opts+0x2ec/0x4b8
[    5.893968]        [<c01907dc>] superblock_doinit+0xb4/0xc0
[    5.899562]        [<c00b51f4>] iterate_supers+0xb4/0xdc
[    5.904906]        [<c01a02e8>] security_load_policy+0x248/0x3c4
[    5.910968]        [<c0194994>] sel_write_load+0x9c/0x6a0
[    5.916406]        [<c00b2148>] vfs_write+0xa0/0x17c
[    5.921406]        [<c00b2450>] sys_write+0x3c/0x70
[    5.926343]        [<c00093a0>] ret_fast_syscall+0x0/0x38
[    5.931781]
[    5.931781] other info that might help us debug this:
[    5.931781]
[    5.939781] Chain exists of:
  &c->tnc_mutex --> &sb->s_type->i_mutex_key#2 --> &isec->lock
[    5.948500]  Possible unsafe locking scenario:
[    5.948500]
[    5.954437]        CPU0                    CPU1
[    5.958968]        ----                    ----
[    5.963500]   lock(&isec->lock);
[    5.966781]                                lock(&sb->s_type->i_mutex_key#2);
[    5.973843]                                lock(&isec->lock);
[    5.979625]   lock(&c->tnc_mutex);
[    5.983062]
[    5.983062]  *** DEADLOCK ***
[    5.983062]
[    5.989000] 4 locks held by systemd/1:
[    5.992781]  #0:  (sel_mutex){+.+.+.}, at: [<c0194918>] sel_write_load+0x20/0x6a0
[    6.000343]  #1:  (&type->s_umount_key#26){.+.+..}, at: [<c00b51d0>] iterate_supers+0x90/0xdc
[    6.008968]  #2:  (&sbsec->lock){+.+.+.}, at: [<c01902d4>] selinux_set_mnt_opts+0x64/0x4b8
[    6.017343]  #3:  (&isec->lock){+.+.+.}, at: [<c018e298>] inode_doinit_with_dentry+0x8c/0x55c
[    6.025937]
[    6.025937] stack backtrace:
[    6.030375] [<c000d0f0>] (unwind_backtrace+0x0/0xf0) from [<c0351a2c>] (print_circular_bug+0x25c/0x2a8)
[    6.039843] [<c0351a2c>] (print_circular_bug+0x25c/0x2a8) from [<c004de48>] (__lock_acquire+0x14ec/0x1b08)
[    6.049531] [<c004de48>] (__lock_acquire+0x14ec/0x1b08) from [<c004e8e8>] (lock_acquire+0x64/0x78)
[    6.058531] [<c004e8e8>] (lock_acquire+0x64/0x78) from [<c0355d5c>] (mutex_lock_nested+0x5c/0x2ec)
[    6.067531] [<c0355d5c>] (mutex_lock_nested+0x5c/0x2ec) from [<c01552d0>] (ubifs_tnc_locate+0x30/0x198)
[    6.076968] [<c01552d0>] (ubifs_tnc_locate+0x30/0x198) from [<c0155a48>] (ubifs_tnc_lookup_nm+0x28/0x150)
[    6.086593] [<c0155a48>] (ubifs_tnc_lookup_nm+0x28/0x150) from [<c016fc18>] (ubifs_getxattr+0xc0/0x254)
[    6.096031] [<c016fc18>] (ubifs_getxattr+0xc0/0x254) from [<c018e468>] (inode_doinit_with_dentry+0x25c/0x55c)
[    6.105968] [<c018e468>] (inode_doinit_with_dentry+0x25c/0x55c) from [<c018f1a8>] (sb_finish_set_opts+0xa4/0x21c)
[    6.116281] [<c018f1a8>] (sb_finish_set_opts+0xa4/0x21c) from [<c019055c>] (selinux_set_mnt_opts+0x2ec/0x4b8)
[    6.126218] [<c019055c>] (selinux_set_mnt_opts+0x2ec/0x4b8) from [<c01907dc>] (superblock_doinit+0xb4/0xc0)
[    6.136000] [<c01907dc>] (superblock_doinit+0xb4/0xc0) from [<c00b51f4>] (iterate_supers+0xb4/0xdc)
[    6.145093] [<c00b51f4>] (iterate_supers+0xb4/0xdc) from [<c01a02e8>] (security_load_policy+0x248/0x3c4)
[    6.154625] [<c01a02e8>] (security_load_policy+0x248/0x3c4) from [<c0194994>] (sel_write_load+0x9c/0x6a0)
[    6.164218] [<c0194994>] (sel_write_load+0x9c/0x6a0) from [<c00b2148>] (vfs_write+0xa0/0x17c)
[    6.172781] [<c00b2148>] (vfs_write+0xa0/0x17c) from [<c00b2450>] (sys_write+0x3c/0x70)
[    6.180843] [<c00b2450>] (sys_write+0x3c/0x70) from [<c00093a0>] (ret_fast_syscall+0x0/0x38)

Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 include/linux/fs.h       |    1 +
 security/selinux/hooks.c |    2 ++
 2 files changed, 3 insertions(+), 0 deletions(-)

Comments

Marc Kleine-Budde Feb. 14, 2013, 11:56 a.m. UTC | #1
On 02/14/2013 08:15 AM, Artem Bityutskiy wrote:
> Mark, how about this one? I compiled it and ran on my fedora 16 with
> SElinux enabled, no obvious issues.
> 
> From a19350097200570571aa522afebb96b34db534f4 Mon Sep 17 00:00:00 2001
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> Date: Thu, 14 Feb 2013 09:07:36 +0200
> Subject: [PATCH] selinux: do not confuse lockdep
> 
> Selinux has per-inode mutexes called 'isec->lock', and they are initialized in
> the same place, which makes lockdep treat all of the them as if they were
> identical. However, locking rules may be a little bit different depending on
> the file-system, so we should put these locks to separate classes, just like we
> do for 'i_mutex'. Namely, we should put them to per-FS type classes, which is
> exactly what this patch does.
> 
> The problem this patch intends to fix is a strange lockdep warning, which I,
> frankly speaking, do not really understand, but I believe the root-cause should
> be fixed by this patch.

Thanks, this works with mainline, but not with my xattr patch series
applied.

I get this warnings:

> [   13.659593] ======================================================
> [   13.665781] [ INFO: possible circular locking dependency detected ]
> [   13.672062] 3.8.0-rc7-00011-gd50987e #113 Not tainted
> [   13.677125] -------------------------------------------------------
> [   13.683406] touch/81 is trying to acquire lock:
> [   13.687968]  (&sb->s_type->i_mutex_key#10){+.+...}, at: [<c0178330>] ubifs_init_security+0x24/0x5c
> [   13.697031]
> [   13.697031] but task is already holding lock:
> [   13.702906]  (&ui->ui_mutex){+.+...}, at: [<c0152a98>] ubifs_create+0xb4/0x1ec
> [   13.710218]
> [   13.710218] which lock already depends on the new lock.
> [   13.710218]
> [   13.718406]
> [   13.718406] the existing dependency chain (in reverse order) is:
> [   13.725906]
> -> #1 (&ui->ui_mutex){+.+...}:
> [   13.730250]        [<c00548f0>] lock_acquire+0x64/0x78
> [   13.735437]        [<c035ea74>] mutex_lock_nested+0x5c/0x2ec
> [   13.741156]        [<c014f510>] ubifs_write_begin+0x314/0x500
> [   13.746937]        [<c0084998>] generic_file_buffered_write+0x1b4/0x288
> [   13.753625]        [<c0086704>] __generic_file_aio_write+0x1bc/0x434
> [   13.760000]        [<c00869e4>] generic_file_aio_write+0x68/0xd8
> [   13.766031]        [<c014eaec>] ubifs_aio_write+0xf8/0x194
> [   13.771562]        [<c00b8994>] do_sync_write+0x94/0xc8
> [   13.776843]        [<c00b9148>] vfs_write+0xa0/0x17c
> [   13.781843]        [<c00b9450>] sys_write+0x3c/0x70
> [   13.786750]        [<c000e400>] ret_fast_syscall+0x0/0x38
> [   13.792187]
> -> #0 (&sb->s_type->i_mutex_key#10){+.+...}:
> [   13.797781]        [<c0053e50>] __lock_acquire+0x14ec/0x1b08
> [   13.803468]        [<c00548f0>] lock_acquire+0x64/0x78
> [   13.808625]        [<c035ea74>] mutex_lock_nested+0x5c/0x2ec
> [   13.814343]        [<c0178330>] ubifs_init_security+0x24/0x5c
> [   13.820125]        [<c0152b10>] ubifs_create+0x12c/0x1ec
> [   13.825468]        [<c00c40a0>] vfs_create+0xa8/0x118
> [   13.830562]        [<c00c6594>] do_last+0x930/0xd4c
> [   13.835468]        [<c00c6a58>] path_openat+0xa8/0x4b8
> [   13.840625]        [<c00c7168>] do_filp_open+0x2c/0x80
> [   13.845812]        [<c00b86e4>] do_sys_open+0xe4/0x170
> [   13.850968]        [<c000e400>] ret_fast_syscall+0x0/0x38
> [   13.856406]
> [   13.856406] other info that might help us debug this:
> [   13.856406]
> [   13.864437]  Possible unsafe locking scenario:
> [   13.864437]
> [   13.870375]        CPU0                    CPU1
> [   13.874906]        ----                    ----
> [   13.879468]   lock(&ui->ui_mutex);
> [   13.882906]                                lock(&sb->s_type->i_mutex_key#10);
> [   13.890093]                                lock(&ui->ui_mutex);
> [   13.896031]   lock(&sb->s_type->i_mutex_key#10);
> [   13.900718]
> [   13.900718]  *** DEADLOCK ***
> [   13.900718]
> [   13.906656] 3 locks held by touch/81:
> [   13.910312]  #0:  (sb_writers#3){.+.+.+}, at: [<c00d4e68>] mnt_want_write+0x18/0x3c
> [   13.918093]  #1:  (&type->i_mutex_dir_key){+.+.+.}, at: [<c00c5fcc>] do_last+0x368/0xd4c
> [   13.926281]  #2:  (&ui->ui_mutex){+.+...}, at: [<c0152a98>] ubifs_create+0xb4/0x1ec
> [   13.934031]
> [   13.934031] stack backtrace:
> [   13.938468] [<c00124f0>] (unwind_backtrace+0x0/0xf0) from [<c035a670>] (print_circular_bug+0x25c/0x2a8)
> [   13.947906] [<c035a670>] (print_circular_bug+0x25c/0x2a8) from [<c0053e50>] (__lock_acquire+0x14ec/0x1b08)
> [   13.957593] [<c0053e50>] (__lock_acquire+0x14ec/0x1b08) from [<c00548f0>] (lock_acquire+0x64/0x78)
> [   13.966593] [<c00548f0>] (lock_acquire+0x64/0x78) from [<c035ea74>] (mutex_lock_nested+0x5c/0x2ec)
> [   13.975593] [<c035ea74>] (mutex_lock_nested+0x5c/0x2ec) from [<c0178330>] (ubifs_init_security+0x24/0x5c)
> [   13.985187] [<c0178330>] (ubifs_init_security+0x24/0x5c) from [<c0152b10>] (ubifs_create+0x12c/0x1ec)
> [   13.994437] [<c0152b10>] (ubifs_create+0x12c/0x1ec) from [<c00c40a0>] (vfs_create+0xa8/0x118)
> [   14.003000] [<c00c40a0>] (vfs_create+0xa8/0x118) from [<c00c6594>] (do_last+0x930/0xd4c)
> [   14.011125] [<c00c6594>] (do_last+0x930/0xd4c) from [<c00c6a58>] (path_openat+0xa8/0x4b8)
> [   14.019343] [<c00c6a58>] (path_openat+0xa8/0x4b8) from [<c00c7168>] (do_filp_open+0x2c/0x80)
> [   14.027812] [<c00c7168>] (do_filp_open+0x2c/0x80) from [<c00b86e4>] (do_sys_open+0xe4/0x170)
> [   14.036281] [<c00b86e4>] (do_sys_open+0xe4/0x170) from [<c000e400>] (ret_fast_syscall+0x0/0x38)

or this:

> [   54.994687]
> [   54.996218] ======================================================
> [   55.002437] [ INFO: possible circular locking dependency detected ]
> [   55.008718] 3.8.0-rc7-00011-gd50987e #113 Not tainted
> [   55.013781] -------------------------------------------------------
> [   55.020062] semodule/427 is trying to acquire lock:
> [   55.024937]  (&sb->s_type->i_mutex_key#12){+.+.+.}, at: [<c0178330>] ubifs_init_security+0x24/0x5c
> [   55.034031]
> [   55.034031] but task is already holding lock:
> [   55.039875]  (&ui->ui_mutex){+.+...}, at: [<c0152614>] ubifs_mkdir+0x98/0x204
> [   55.047125]
> [   55.047125] which lock already depends on the new lock.
> [   55.047125]
> [   55.055312]
> [   55.055312] the existing dependency chain (in reverse order) is:
> [   55.062812]
> -> #1 (&ui->ui_mutex){+.+...}:
> [   55.067156]        [<c00548f0>] lock_acquire+0x64/0x78
> [   55.072343]        [<c035ea74>] mutex_lock_nested+0x5c/0x2ec
> [   55.078031]        [<c014ffb8>] ubifs_setattr+0x2c8/0x3f8
> [   55.083500]        [<c00d1b14>] notify_change+0x1dc/0x330
> [   55.088937]        [<c00b7838>] do_truncate+0x78/0x9c
> [   55.094031]        [<c00c6318>] do_last+0x6b4/0xd4c
> [   55.098937]        [<c00c6a58>] path_openat+0xa8/0x4b8
> [   55.104125]        [<c00c7168>] do_filp_open+0x2c/0x80
> [   55.109281]        [<c00b86e4>] do_sys_open+0xe4/0x170
> [   55.114468]        [<c000e400>] ret_fast_syscall+0x0/0x38
> [   55.119906]
> -> #0 (&sb->s_type->i_mutex_key#12){+.+.+.}:
> [   55.125468]        [<c0053e50>] __lock_acquire+0x14ec/0x1b08
> [   55.131187]        [<c00548f0>] lock_acquire+0x64/0x78
> [   55.136343]        [<c035ea74>] mutex_lock_nested+0x5c/0x2ec
> [   55.142062]        [<c0178330>] ubifs_init_security+0x24/0x5c
> [   55.147843]        [<c01526b4>] ubifs_mkdir+0x138/0x204
> [   55.153093]        [<c00c3e50>] vfs_mkdir+0xb8/0x138
> [   55.158093]        [<c00c74bc>] sys_mkdirat+0x5c/0xb0
> [   55.163187]        [<c000e400>] ret_fast_syscall+0x0/0x38
> [   55.168625]
> [   55.168625] other info that might help us debug this:
> [   55.168625]
> [   55.176625]  Possible unsafe locking scenario:
> [   55.176625]
> [   55.182562]        CPU0                    CPU1
> [   55.187125]        ----                    ----
> [   55.191656]   lock(&ui->ui_mutex);
> [   55.195093]                                lock(&sb->s_type->i_mutex_key#12);
> [   55.202281]                                lock(&ui->ui_mutex);
> [   55.208218]   lock(&sb->s_type->i_mutex_key#12);
> [   55.212906]
> [   55.212906]  *** DEADLOCK ***
> [   55.212906]
> [   55.218843] 3 locks held by semodule/427:
> [   55.222875]  #0:  (sb_writers#3){.+.+.+}, at: [<c00d4e68>] mnt_want_write+0x18/0x3c
> [   55.230656]  #1:  (&type->i_mutex_dir_key/1){+.+.+.}, at: [<c00c4ed0>] kern_path_create+0x6c/0x11c
> [   55.239718]  #2:  (&ui->ui_mutex){+.+...}, at: [<c0152614>] ubifs_mkdir+0x98/0x204
> [   55.247375]
> [   55.247375] stack backtrace:
> [   55.251812] [<c00124f0>] (unwind_backtrace+0x0/0xf0) from [<c035a670>] (print_circular_bug+0x25c/0x2a8)
> [   55.261250] [<c035a670>] (print_circular_bug+0x25c/0x2a8) from [<c0053e50>] (__lock_acquire+0x14ec/0x1b08)
> [   55.270937] [<c0053e50>] (__lock_acquire+0x14ec/0x1b08) from [<c00548f0>] (lock_acquire+0x64/0x78)
> [   55.279937] [<c00548f0>] (lock_acquire+0x64/0x78) from [<c035ea74>] (mutex_lock_nested+0x5c/0x2ec)
> [   55.288937] [<c035ea74>] (mutex_lock_nested+0x5c/0x2ec) from [<c0178330>] (ubifs_init_security+0x24/0x5c)
> [   55.298531] [<c0178330>] (ubifs_init_security+0x24/0x5c) from [<c01526b4>] (ubifs_mkdir+0x138/0x204)
> [   55.307687] [<c01526b4>] (ubifs_mkdir+0x138/0x204) from [<c00c3e50>] (vfs_mkdir+0xb8/0x138)
> [   55.316062] [<c00c3e50>] (vfs_mkdir+0xb8/0x138) from [<c00c74bc>] (sys_mkdirat+0x5c/0xb0)
> [   55.324281] [<c00c74bc>] (sys_mkdirat+0x5c/0xb0) from [<c000e400>] (ret_fast_syscall+0x0/0x38)


Marc
Artem Bityutskiy Feb. 14, 2013, 12:08 p.m. UTC | #2
On Thu, 2013-02-14 at 12:56 +0100, Marc Kleine-Budde wrote:
> On 02/14/2013 08:15 AM, Artem Bityutskiy wrote:
> > Mark, how about this one? I compiled it and ran on my fedora 16 with
> > SElinux enabled, no obvious issues.
> > 
> > From a19350097200570571aa522afebb96b34db534f4 Mon Sep 17 00:00:00 2001
> > From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> > Date: Thu, 14 Feb 2013 09:07:36 +0200
> > Subject: [PATCH] selinux: do not confuse lockdep
> > 
> > Selinux has per-inode mutexes called 'isec->lock', and they are initialized in
> > the same place, which makes lockdep treat all of the them as if they were
> > identical. However, locking rules may be a little bit different depending on
> > the file-system, so we should put these locks to separate classes, just like we
> > do for 'i_mutex'. Namely, we should put them to per-FS type classes, which is
> > exactly what this patch does.
> > 
> > The problem this patch intends to fix is a strange lockdep warning, which I,
> > frankly speaking, do not really understand, but I believe the root-cause should
> > be fixed by this patch.
> 
> Thanks, this works with mainline, but not with my xattr patch series
> applied.

Hmm, probably I have to annotate the ui mutex. Let's drop security
mailing lists from the loop so far, I'll come up with a patch tomorrow.
Marc Kleine-Budde Feb. 20, 2013, 11:19 a.m. UTC | #3
On 02/14/2013 01:08 PM, Artem Bityutskiy wrote:
> On Thu, 2013-02-14 at 12:56 +0100, Marc Kleine-Budde wrote:
>> On 02/14/2013 08:15 AM, Artem Bityutskiy wrote:
>>> Mark, how about this one? I compiled it and ran on my fedora 16 with
>>> SElinux enabled, no obvious issues.
>>>
>>> From a19350097200570571aa522afebb96b34db534f4 Mon Sep 17 00:00:00 2001
>>> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>>> Date: Thu, 14 Feb 2013 09:07:36 +0200
>>> Subject: [PATCH] selinux: do not confuse lockdep
>>>
>>> Selinux has per-inode mutexes called 'isec->lock', and they are initialized in
>>> the same place, which makes lockdep treat all of the them as if they were
>>> identical. However, locking rules may be a little bit different depending on
>>> the file-system, so we should put these locks to separate classes, just like we
>>> do for 'i_mutex'. Namely, we should put them to per-FS type classes, which is
>>> exactly what this patch does.
>>>
>>> The problem this patch intends to fix is a strange lockdep warning, which I,
>>> frankly speaking, do not really understand, but I believe the root-cause should
>>> be fixed by this patch.
>>
>> Thanks, this works with mainline, but not with my xattr patch series
>> applied.
> 
> Hmm, probably I have to annotate the ui mutex. Let's drop security
> mailing lists from the loop so far, I'll come up with a patch tomorrow.

Do you have a patch I can test?

Marc
Artem Bityutskiy Feb. 20, 2013, 11:24 a.m. UTC | #4
On Wed, 2013-02-20 at 12:19 +0100, Marc Kleine-Budde wrote:
> > Hmm, probably I have to annotate the ui mutex. Let's drop security
> > mailing lists from the loop so far, I'll come up with a patch tomorrow.
> 
> Do you have a patch I can test?

Mark, I apologise for not keeping my promise, I got distracted. I will
_try_ to look at this again this week. Sorry!
Marc Kleine-Budde Feb. 20, 2013, 11:39 a.m. UTC | #5
On 02/20/2013 12:24 PM, Artem Bityutskiy wrote:
> On Wed, 2013-02-20 at 12:19 +0100, Marc Kleine-Budde wrote:
>>> Hmm, probably I have to annotate the ui mutex. Let's drop security
>>> mailing lists from the loop so far, I'll come up with a patch tomorrow.
>>
>> Do you have a patch I can test?
> 
> Mark, I apologise for not keeping my promise, I got distracted. I will
> _try_ to look at this again this week. Sorry!

I know what you're talking about ;), no problem.

regards,
Marc
diff mbox

Patch

From a19350097200570571aa522afebb96b34db534f4 Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Date: Thu, 14 Feb 2013 09:07:36 +0200
Subject: [PATCH] selinux: do not confuse lockdep

Selinux has per-inode mutexes called 'isec->lock', and they are initialized in
the same place, which makes lockdep treat all of the them as if they were
identical. However, locking rules may be a little bit different depending on
the file-system, so we should put these locks to separate classes, just like we
do for 'i_mutex'. Namely, we should put them to per-FS type classes, which is
exactly what this patch does.

The problem this patch intends to fix is a strange lockdep warning, which I,
frankly speaking, do not really understand, but I believe the root-cause should
be fixed by this patch.

Look at the stacktrace #4: here we have 'debugfs_create_dir()'

[    5.390312] ======================================================
[    5.396500] [ INFO: possible circular locking dependency detected ]
[    5.402781] 3.8.0-rc6-00005-g4f7e39d #49 Not tainted
[    5.407750] -------------------------------------------------------
[    5.414031] systemd/1 is trying to acquire lock:
[    5.418656]  (&c->tnc_mutex){+.+...}, at: [<c01552d0>] ubifs_tnc_locate+0x30/0x198
[    5.426343]
[    5.426343] but task is already holding lock:
[    5.432218]  (&isec->lock){+.+.+.}, at: [<c018e298>] inode_doinit_with_dentry+0x8c/0x55c
[    5.440375]
[    5.440375] which lock already depends on the new lock.
[    5.440375]
[    5.448593]
[    5.448593] the existing dependency chain (in reverse order) is:
[    5.456093]
-> #4 (&isec->lock){+.+.+.}:
[    5.460250]        [<c004e8e8>] lock_acquire+0x64/0x78
[    5.465437]        [<c0355d5c>] mutex_lock_nested+0x5c/0x2ec
[    5.471125]        [<c018e298>] inode_doinit_with_dentry+0x8c/0x55c
[    5.477437]        [<c0189bd8>] security_d_instantiate+0x1c/0x34
[    5.483500]        [<c017a614>] debugfs_mknod.part.15.constprop.18+0x94/0x128
[    5.490656]        [<c017a858>] __create_file+0x1b0/0x25c
[    5.496093]        [<c017a98c>] debugfs_create_dir+0x1c/0x28
[    5.501781]        [<c0491ce8>] pinctrl_init+0x1c/0xd0
[    5.506968]        [<c0008900>] do_one_initcall+0x108/0x17c
[    5.512593]        [<c0483858>] kernel_init_freeable+0xec/0x1b4
[    5.518562]        [<c034f398>] kernel_init+0x8/0xe4
[    5.523562]        [<c0009448>] ret_from_fork+0x14/0x2c
[    5.528812]
-> #3 (&sb->s_type->i_mutex_key#2){+.+.+.}:
[    5.534312]        [<c004e8e8>] lock_acquire+0x64/0x78
[    5.539468]        [<c0355d5c>] mutex_lock_nested+0x5c/0x2ec
[    5.545187]        [<c017a6f8>] __create_file+0x50/0x25c
[    5.550531]        [<c017a98c>] debugfs_create_dir+0x1c/0x28
[    5.556218]        [<c026cfc4>] clk_debug_create_subtree+0x1c/0x108
[    5.562500]        [<c0495bfc>] clk_debug_init+0x68/0xc0
[    5.567875]        [<c0008900>] do_one_initcall+0x108/0x17c
[    5.573468]        [<c0483858>] kernel_init_freeable+0xec/0x1b4
[    5.579437]        [<c034f398>] kernel_init+0x8/0xe4
[    5.584437]        [<c0009448>] ret_from_fork+0x14/0x2c
[    5.589687]
-> #2 (prepare_lock){+.+.+.}:
[    5.593937]        [<c004e8e8>] lock_acquire+0x64/0x78
[    5.599125]        [<c0355d5c>] mutex_lock_nested+0x5c/0x2ec
[    5.604812]        [<c026d400>] clk_prepare+0x18/0x38
[    5.609906]        [<c023a2f8>] __gpmi_enable_clk+0x30/0xb0
[    5.615531]        [<c023a7bc>] gpmi_begin+0x18/0x530
[    5.620625]        [<c0239244>] gpmi_select_chip+0x3c/0x54
[    5.626156]        [<c02341ac>] nand_do_read_ops+0x7c/0x3e4
[    5.631750]        [<c023483c>] nand_read+0x50/0x74
[    5.636656]        [<c02198e4>] part_read+0x5c/0xa4
[    5.641593]        [<c0217404>] mtd_read+0x84/0xb8
[    5.646406]        [<c02450cc>] ubi_io_read+0xa0/0x2c0
[    5.651593]        [<c0242640>] ubi_eba_read_leb+0x190/0x424
[    5.657281]        [<c0241b18>] ubi_leb_read+0xac/0x120
[    5.662562]        [<c01503e4>] ubifs_leb_read+0x28/0x8c
[    5.667906]        [<c0151f74>] ubifs_read_node+0x98/0x2a0
[    5.673437]        [<c014e780>] ubifs_read_sb_node+0x54/0x78
[    5.679125]        [<c014f444>] ubifs_read_superblock+0xc60/0x163c
[    5.685343]        [<c014d64c>] ubifs_mount+0x800/0x171c
[    5.690687]        [<c00b5740>] mount_fs+0x44/0x184
[    5.695593]        [<c00cd7ec>] vfs_kern_mount+0x4c/0xc0
[    5.700968]        [<c00cf394>] do_mount+0x18c/0x8d0
[    5.705968]        [<c00cfb5c>] sys_mount+0x84/0xb8
[    5.710875]        [<c0483bc8>] mount_block_root+0x118/0x258
[    5.716562]        [<c0483eac>] prepare_namespace+0x8c/0x17c
[    5.722281]        [<c034f398>] kernel_init+0x8/0xe4
[    5.727281]        [<c0009448>] ret_from_fork+0x14/0x2c
[    5.732531]
-> #1 (&le->mutex){++++..}:
[    5.736625]        [<c004e8e8>] lock_acquire+0x64/0x78
[    5.741812]        [<c0356468>] down_read+0x40/0x54
[    5.746718]        [<c02424e4>] ubi_eba_read_leb+0x34/0x424
[    5.752312]        [<c0241b18>] ubi_leb_read+0xac/0x120
[    5.757562]        [<c01503e4>] ubifs_leb_read+0x28/0x8c
[    5.762937]        [<c0151f74>] ubifs_read_node+0x98/0x2a0
[    5.768437]        [<c016edc0>] ubifs_load_znode+0x88/0x560
[    5.774062]        [<c0155254>] ubifs_lookup_level0+0x190/0x1dc
[    5.780031]        [<c01552e4>] ubifs_tnc_locate+0x44/0x198
[    5.785656]        [<c014c614>] ubifs_iget+0x6c/0x8a4
[    5.790718]        [<c014da64>] ubifs_mount+0xc18/0x171c
[    5.796093]        [<c00b5740>] mount_fs+0x44/0x184
[    5.801000]        [<c00cd7ec>] vfs_kern_mount+0x4c/0xc0
[    5.806343]        [<c00cf394>] do_mount+0x18c/0x8d0
[    5.811343]        [<c00cfb5c>] sys_mount+0x84/0xb8
[    5.816250]        [<c0483bc8>] mount_block_root+0x118/0x258
[    5.821968]        [<c0483eac>] prepare_namespace+0x8c/0x17c
[    5.827656]        [<c034f398>] kernel_init+0x8/0xe4
[    5.832656]        [<c0009448>] ret_from_fork+0x14/0x2c
[    5.837906]
-> #0 (&c->tnc_mutex){+.+...}:
[    5.842250]        [<c004de48>] __lock_acquire+0x14ec/0x1b08
[    5.847968]        [<c004e8e8>] lock_acquire+0x64/0x78
[    5.853125]        [<c0355d5c>] mutex_lock_nested+0x5c/0x2ec
[    5.858812]        [<c01552d0>] ubifs_tnc_locate+0x30/0x198
[    5.864437]        [<c0155a48>] ubifs_tnc_lookup_nm+0x28/0x150
[    5.870312]        [<c016fc18>] ubifs_getxattr+0xc0/0x254
[    5.875750]        [<c018e468>] inode_doinit_with_dentry+0x25c/0x55c
[    5.882125]        [<c018f1a8>] sb_finish_set_opts+0xa4/0x21c
[    5.887906]        [<c019055c>] selinux_set_mnt_opts+0x2ec/0x4b8
[    5.893968]        [<c01907dc>] superblock_doinit+0xb4/0xc0
[    5.899562]        [<c00b51f4>] iterate_supers+0xb4/0xdc
[    5.904906]        [<c01a02e8>] security_load_policy+0x248/0x3c4
[    5.910968]        [<c0194994>] sel_write_load+0x9c/0x6a0
[    5.916406]        [<c00b2148>] vfs_write+0xa0/0x17c
[    5.921406]        [<c00b2450>] sys_write+0x3c/0x70
[    5.926343]        [<c00093a0>] ret_fast_syscall+0x0/0x38
[    5.931781]
[    5.931781] other info that might help us debug this:
[    5.931781]
[    5.939781] Chain exists of:
  &c->tnc_mutex --> &sb->s_type->i_mutex_key#2 --> &isec->lock
[    5.948500]  Possible unsafe locking scenario:
[    5.948500]
[    5.954437]        CPU0                    CPU1
[    5.958968]        ----                    ----
[    5.963500]   lock(&isec->lock);
[    5.966781]                                lock(&sb->s_type->i_mutex_key#2);
[    5.973843]                                lock(&isec->lock);
[    5.979625]   lock(&c->tnc_mutex);
[    5.983062]
[    5.983062]  *** DEADLOCK ***
[    5.983062]
[    5.989000] 4 locks held by systemd/1:
[    5.992781]  #0:  (sel_mutex){+.+.+.}, at: [<c0194918>] sel_write_load+0x20/0x6a0
[    6.000343]  #1:  (&type->s_umount_key#26){.+.+..}, at: [<c00b51d0>] iterate_supers+0x90/0xdc
[    6.008968]  #2:  (&sbsec->lock){+.+.+.}, at: [<c01902d4>] selinux_set_mnt_opts+0x64/0x4b8
[    6.017343]  #3:  (&isec->lock){+.+.+.}, at: [<c018e298>] inode_doinit_with_dentry+0x8c/0x55c
[    6.025937]
[    6.025937] stack backtrace:
[    6.030375] [<c000d0f0>] (unwind_backtrace+0x0/0xf0) from [<c0351a2c>] (print_circular_bug+0x25c/0x2a8)
[    6.039843] [<c0351a2c>] (print_circular_bug+0x25c/0x2a8) from [<c004de48>] (__lock_acquire+0x14ec/0x1b08)
[    6.049531] [<c004de48>] (__lock_acquire+0x14ec/0x1b08) from [<c004e8e8>] (lock_acquire+0x64/0x78)
[    6.058531] [<c004e8e8>] (lock_acquire+0x64/0x78) from [<c0355d5c>] (mutex_lock_nested+0x5c/0x2ec)
[    6.067531] [<c0355d5c>] (mutex_lock_nested+0x5c/0x2ec) from [<c01552d0>] (ubifs_tnc_locate+0x30/0x198)
[    6.076968] [<c01552d0>] (ubifs_tnc_locate+0x30/0x198) from [<c0155a48>] (ubifs_tnc_lookup_nm+0x28/0x150)
[    6.086593] [<c0155a48>] (ubifs_tnc_lookup_nm+0x28/0x150) from [<c016fc18>] (ubifs_getxattr+0xc0/0x254)
[    6.096031] [<c016fc18>] (ubifs_getxattr+0xc0/0x254) from [<c018e468>] (inode_doinit_with_dentry+0x25c/0x55c)
[    6.105968] [<c018e468>] (inode_doinit_with_dentry+0x25c/0x55c) from [<c018f1a8>] (sb_finish_set_opts+0xa4/0x21c)
[    6.116281] [<c018f1a8>] (sb_finish_set_opts+0xa4/0x21c) from [<c019055c>] (selinux_set_mnt_opts+0x2ec/0x4b8)
[    6.126218] [<c019055c>] (selinux_set_mnt_opts+0x2ec/0x4b8) from [<c01907dc>] (superblock_doinit+0xb4/0xc0)
[    6.136000] [<c01907dc>] (superblock_doinit+0xb4/0xc0) from [<c00b51f4>] (iterate_supers+0xb4/0xdc)
[    6.145093] [<c00b51f4>] (iterate_supers+0xb4/0xdc) from [<c01a02e8>] (security_load_policy+0x248/0x3c4)
[    6.154625] [<c01a02e8>] (security_load_policy+0x248/0x3c4) from [<c0194994>] (sel_write_load+0x9c/0x6a0)
[    6.164218] [<c0194994>] (sel_write_load+0x9c/0x6a0) from [<c00b2148>] (vfs_write+0xa0/0x17c)
[    6.172781] [<c00b2148>] (vfs_write+0xa0/0x17c) from [<c00b2450>] (sys_write+0x3c/0x70)
[    6.180843] [<c00b2450>] (sys_write+0x3c/0x70) from [<c00093a0>] (ret_fast_syscall+0x0/0x38)

Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 include/linux/fs.h       |    1 +
 security/selinux/hooks.c |    2 ++
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7617ee0..ccd49e1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1824,6 +1824,7 @@  struct file_system_type {
 	struct lock_class_key i_lock_key;
 	struct lock_class_key i_mutex_key;
 	struct lock_class_key i_mutex_dir_key;
+	struct lock_class_key i_security_lock_key;
 };
 
 extern struct dentry *mount_ns(struct file_system_type *fs_type, int flags,
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ef26e96..13ccbb0 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -207,6 +207,8 @@  static int inode_alloc_security(struct inode *inode)
 		return -ENOMEM;
 
 	mutex_init(&isec->lock);
+	lockdep_set_class(&isec->lock,
+			  &inode->i_sb->s_type->i_security_lock_key);
 	INIT_LIST_HEAD(&isec->list);
 	isec->inode = inode;
 	isec->sid = SECINITSID_UNLABELED;
-- 
1.7.7.6