Re: sysfs: use a separate locking class for open files depending on mmap

[Dave Jones <davej@redhat.com>]

On Tue, Dec 03, 2013 at 04:10:28PM -0500, Tejun Heo wrote:
 > Hello, Dave.
 > 
 > On Tue, Dec 03, 2013 at 01:43:24PM -0500, Dave Jones wrote:
 > >  > +	/*
 > >  > +	 * The following is done to give a different lockdep key to
 > >  > +	 * @of->mutex for files which implement mmap.  This is a rather
 > >  > +	 * crude way to avoid false positive lockdep warning around
 > >  > +	 * mm->mmap_sem - mmap nests @of->mutex under mm->mmap_sem and
 > >  > +	 * reading /sys/block/sda/trace/act_mask grabs sr_mutex, under
 > >  > +	 * which mm->mmap_sem nests, while holding @of->mutex.  As each
 > >  > +	 * open file has a separate mutex, it's okay as long as those don't
 > >  > +	 * happen on the same file.  At this point, we can't easily give
 > >  > +	 * each file a separate locking class.  Let's differentiate on
 > >  > +	 * whether the file has mmap or not for now.
 > >  > +	 */
 > >  > +	if (has_mmap)
 > >  > +		mutex_init(&of->mutex);
 > >  > +	else
 > >  > +		mutex_init(&of->mutex);
 > >  > +
 > >  
 > > Somehow I just triggered this trace again, even with this commit applied.
 > > The trace is pretty much identical to the old one.
 > 
 > Hah, ain't that weird.  That's the trace you reported on the other
 > mail, right?  I'll follow up on that one.

just so there's no doubt, here's a fresh one.

[ 1396.487831] ======================================================
[ 1396.487852] [ INFO: possible circular locking dependency detected ]
[ 1396.487879] 3.13.0-rc2+ #15 Not tainted
[ 1396.487894] -------------------------------------------------------
[ 1396.487915] trinity-child0/24146 is trying to acquire lock:
[ 1396.487935]  (&of->mutex){+.+.+.}, at: [<ffffffff8124119f>] sysfs_bin_mmap+0x4f/0x120
[ 1396.487971] 
but task is already holding lock:
[ 1396.487991]  (&mm->mmap_sem){++++++}, at: [<ffffffff8116e07f>] vm_mmap_pgoff+0x6f/0xc0
[ 1396.488027] 
which lock already depends on the new lock.

[ 1396.488054] 
the existing dependency chain (in reverse order) is:
[ 1396.488079] 
-> #3 (&mm->mmap_sem){++++++}:
[ 1396.488104]        [<ffffffff810af853>] lock_acquire+0x93/0x1c0
[ 1396.488127]        [<ffffffff8117854c>] might_fault+0x8c/0xb0
[ 1396.488150]        [<ffffffff81304505>] scsi_cmd_ioctl+0x295/0x470
[ 1396.488174]        [<ffffffff81304722>] scsi_cmd_blk_ioctl+0x42/0x50
[ 1396.488198]        [<ffffffff81520961>] cdrom_ioctl+0x41/0x1050
[ 1396.488221]        [<ffffffff814f390f>] sr_block_ioctl+0x6f/0xd0
[ 1396.488245]        [<ffffffff81300414>] blkdev_ioctl+0x234/0x840
[ 1396.488268]        [<ffffffff811fba67>] block_ioctl+0x47/0x50
[ 1396.488292]        [<ffffffff811cf470>] do_vfs_ioctl+0x300/0x520
[ 1396.488315]        [<ffffffff811cf711>] SyS_ioctl+0x81/0xa0
[ 1396.488337]        [<ffffffff8174eb64>] tracesys+0xdd/0xe2
[ 1396.488359] 
-> #2 (sr_mutex){+.+.+.}:
[ 1396.488384]        [<ffffffff810af853>] lock_acquire+0x93/0x1c0
[ 1396.489088]        [<ffffffff81741df7>] mutex_lock_nested+0x77/0x400
[ 1396.489794]        [<ffffffff814f3fa4>] sr_block_open+0x24/0x130
[ 1396.490496]        [<ffffffff811fc831>] __blkdev_get+0xd1/0x4c0
[ 1396.491200]        [<ffffffff811fce05>] blkdev_get+0x1e5/0x380
[ 1396.491892]        [<ffffffff811fd05a>] blkdev_open+0x6a/0x90
[ 1396.492566]        [<ffffffff811b7e77>] do_dentry_open+0x1e7/0x340
[ 1396.493235]        [<ffffffff811b80e0>] finish_open+0x40/0x50
[ 1396.493910]        [<ffffffff811cb0e7>] do_last+0xbc7/0x1370
[ 1396.494579]        [<ffffffff811cb94e>] path_openat+0xbe/0x6a0
[ 1396.495244]        [<ffffffff811cc74a>] do_filp_open+0x3a/0x90
[ 1396.495905]        [<ffffffff811b9afe>] do_sys_open+0x12e/0x210
[ 1396.496564]        [<ffffffff811b9bfe>] SyS_open+0x1e/0x20
[ 1396.497219]        [<ffffffff8174eb64>] tracesys+0xdd/0xe2
[ 1396.497869] 
-> #1 (&bdev->bd_mutex){+.+.+.}:
[ 1396.499153]        [<ffffffff810af853>] lock_acquire+0x93/0x1c0
[ 1396.499808]        [<ffffffff81741df7>] mutex_lock_nested+0x77/0x400
[ 1396.500462]        [<ffffffff8112ff8f>] sysfs_blk_trace_attr_show+0x5f/0x1f0
[ 1396.501118]        [<ffffffff814c5c40>] dev_attr_show+0x20/0x60
[ 1396.501776]        [<ffffffff81241a38>] sysfs_seq_show+0xc8/0x160
[ 1396.502433]        [<ffffffff811e42a4>] seq_read+0x164/0x450
[ 1396.503085]        [<ffffffff811ba648>] vfs_read+0x98/0x170
[ 1396.503735]        [<ffffffff811bb18c>] SyS_read+0x4c/0xa0
[ 1396.504378]        [<ffffffff8174eb64>] tracesys+0xdd/0xe2
[ 1396.505021] 
-> #0 (&of->mutex){+.+.+.}:
[ 1396.506286]        [<ffffffff810aed36>] __lock_acquire+0x1786/0x1af0
[ 1396.506939]        [<ffffffff810af853>] lock_acquire+0x93/0x1c0
[ 1396.507589]        [<ffffffff81741df7>] mutex_lock_nested+0x77/0x400
[ 1396.508236]        [<ffffffff8124119f>] sysfs_bin_mmap+0x4f/0x120
[ 1396.508883]        [<ffffffff811835b5>] mmap_region+0x3e5/0x5d0
[ 1396.509526]        [<ffffffff81183af7>] do_mmap_pgoff+0x357/0x3e0
[ 1396.510158]        [<ffffffff8116e0a0>] vm_mmap_pgoff+0x90/0xc0
[ 1396.510790]        [<ffffffff81182045>] SyS_mmap_pgoff+0x1d5/0x270
[ 1396.511417]        [<ffffffff81007ed2>] SyS_mmap+0x22/0x30
[ 1396.512041]        [<ffffffff8174eb64>] tracesys+0xdd/0xe2
[ 1396.512657] 
other info that might help us debug this:

[ 1396.514444] Chain exists of:
  &of->mutex --> sr_mutex --> &mm->mmap_sem

[ 1396.516191]  Possible unsafe locking scenario:

[ 1396.517326]        CPU0                    CPU1
[ 1396.517894]        ----                    ----
[ 1396.518461]   lock(&mm->mmap_sem);
[ 1396.519024]                                lock(sr_mutex);
[ 1396.519591]                                lock(&mm->mmap_sem);
[ 1396.520158]   lock(&of->mutex);
[ 1396.520711] 
 *** DEADLOCK ***

[ 1396.522339] 1 lock held by trinity-child0/24146:
[ 1396.522887]  #0:  (&mm->mmap_sem){++++++}, at: [<ffffffff8116e07f>] vm_mmap_pgoff+0x6f/0xc0
[ 1396.523463] 
stack backtrace:
[ 1396.524579] CPU: 0 PID: 24146 Comm: trinity-child0 Not tainted 3.13.0-rc2+ #15 
[ 1396.525796]  ffffffff824a7960 ffff8802161d9bc0 ffffffff8173bd22 ffffffff824d19b0
[ 1396.526427]  ffff8802161d9c00 ffffffff817380bd ffff8802161d9c50 ffff8802406a5e80
[ 1396.527061]  ffff8802406a5740 0000000000000001 0000000000000001 ffff8802406a5e80
[ 1396.527702] Call Trace:
[ 1396.528327]  [<ffffffff8173bd22>] dump_stack+0x4e/0x7a
[ 1396.528962]  [<ffffffff817380bd>] print_circular_bug+0x200/0x20f
[ 1396.529594]  [<ffffffff810aed36>] __lock_acquire+0x1786/0x1af0
[ 1396.530228]  [<ffffffff81183510>] ? mmap_region+0x340/0x5d0
[ 1396.530864]  [<ffffffff810ad05b>] ? mark_held_locks+0xbb/0x140
[ 1396.531506]  [<ffffffff810af853>] lock_acquire+0x93/0x1c0
[ 1396.532144]  [<ffffffff8124119f>] ? sysfs_bin_mmap+0x4f/0x120
[ 1396.532787]  [<ffffffff8124119f>] ? sysfs_bin_mmap+0x4f/0x120
[ 1396.533423]  [<ffffffff81741df7>] mutex_lock_nested+0x77/0x400
[ 1396.534057]  [<ffffffff8124119f>] ? sysfs_bin_mmap+0x4f/0x120
[ 1396.534694]  [<ffffffff8124119f>] ? sysfs_bin_mmap+0x4f/0x120
[ 1396.535329]  [<ffffffff8124119f>] sysfs_bin_mmap+0x4f/0x120
[ 1396.535963]  [<ffffffff811835b5>] mmap_region+0x3e5/0x5d0
[ 1396.536599]  [<ffffffff81183af7>] do_mmap_pgoff+0x357/0x3e0
[ 1396.537234]  [<ffffffff8116e0a0>] vm_mmap_pgoff+0x90/0xc0
[ 1396.537868]  [<ffffffff81182045>] SyS_mmap_pgoff+0x1d5/0x270
[ 1396.538497]  [<ffffffff81010a45>] ? syscall_trace_enter+0x145/0x2a0
[ 1396.539131]  [<ffffffff81007ed2>] SyS_mmap+0x22/0x30
[ 1396.539759]  [<ffffffff8174eb64>] tracesys+0xdd/0xe2