* sysfs_bin_mmap lockdep trace.
@ 2013-11-13 18:45 Dave Jones
2013-11-13 20:10 ` Al Viro
0 siblings, 1 reply; 9+ messages in thread
From: Dave Jones @ 2013-11-13 18:45 UTC (permalink / raw)
To: Al Viro; +Cc: Linux Kernel, Linus Torvalds
Al, is this one also known ? Also seen on v3.12-7033-g42a2d923cc34
======================================================
[ INFO: possible circular locking dependency detected ]
3.12.0+ #2 Not tainted
-------------------------------------------------------
trinity-child0/9004 is trying to acquire lock:
(&of->mutex){+.+.+.}, at: [<ffffffff8123c0cf>] sysfs_bin_mmap+0x4f/0x120
eady holding lock:
(&mm->mmap_sem){++++++}, at: [<ffffffff8116b5ff>] vm_mmap_pgoff+0x6f/0xc0
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #3 (&mm->mmap_sem){++++++}:
[<ffffffff810d7a23>] lock_acquire+0x93/0x1c0
[<ffffffff81175a5c>] might_fault+0x8c/0xb0
[<ffffffff812fa105>] scsi_cmd_ioctl+0x295/0x470
[<ffffffff812fa322>] scsi_cmd_blk_ioctl+0x42/0x50
[<ffffffff81502a61>] cdrom_ioctl+0x41/0x1050
[<ffffffff814d5baf>] sr_block_ioctl+0x6f/0xd0
[<ffffffff812f5fe4>] blkdev_ioctl+0x234/0x840
[<ffffffff811f6b47>] block_ioctl+0x47/0x50
[<ffffffff811cb4f0>] do_vfs_ioctl+0x300/0x520
[<ffffffff811cb791>] SyS_ioctl+0x81/0xa0
[<ffffffff8172e064>] tracesys+0xdd/0xe2
-> #2 (sr_mutex){+.+.+.}:
[<ffffffff810d7a23>] lock_acquire+0x93/0x1c0
[<ffffffff8171f277>] mutex_lock_nested+0x77/0x400
[<ffffffff814d6244>] sr_block_open+0x24/0x130
[<ffffffff811f7911>] __blkdev_get+0xd1/0x4c0
[<ffffffff811f7ee5>] blkdev_get+0x1e5/0x380
[<ffffffff811f813a>] blkdev_open+0x6a/0x90
[<ffffffff811b45f7>] do_dentry_open+0x1e7/0x340
[<ffffffff811b4860>] finish_open+0x40/0x50
[<ffffffff811c7274>] do_last+0xa34/0x1170
[<ffffffff811c7a6e>] path_openat+0xbe/0x6a0
[<ffffffff811c87ca>] do_filp_open+0x3a/0x90
[<ffffffff811b627e>] do_sys_open+0x12e/0x210
[<ffffffff811b637e>] SyS_open+0x1e/0x20
[<ffffffff8172e064>] tracesys+0xdd/0xe2
-> #1 (&bdev->bd_mutex){+.+.+.}:
[<ffffffff810d7a23>] lock_acquire+0x93/0x1c0
[<ffffffff8171f277>] mutex_lock_nested+0x77/0x400
[<ffffffff8112e03f>] sysfs_blk_trace_attr_show+0x5f/0x1f0
[<ffffffff814a86c0>] dev_attr_show+0x20/0x60
[<ffffffff8123c968>] sysfs_seq_show+0xc8/0x160
[<ffffffff811df6dc>] seq_read+0x16c/0x450
[<ffffffff811b6583>] do_loop_readv_writev+0x63/0x90
[<ffffffff811b7e9d>] do_readv_writev+0x20d/0x220
[<ffffffff811b7ee0>] vfs_readv+0x30/0x60
[<ffffffff811b7fc0>] SyS_readv+0x50/0xd0
[<ffffffff8172e064>] tracesys+0xdd/0xe2
-> #0 (&of->mutex){+.+.+.}:
[<ffffffff810d7002>] __lock_acquire+0x1782/0x19f0
[<ffffffff810d7a23>] lock_acquire+0x93/0x1c0
[<ffffffff8171f277>] mutex_lock_nested+0x77/0x400
[<ffffffff8123c0cf>] sysfs_bin_mmap+0x4f/0x120
[<ffffffff81180695>] mmap_region+0x3e5/0x5d0
[<ffffffff81180bd7>] do_mmap_pgoff+0x357/0x3e0
[<ffffffff8116b620>] vm_mmap_pgoff+0x90/0xc0
[<ffffffff8117f125>] SyS_mmap_pgoff+0x1d5/0x270
[<ffffffff81007eb2>] SyS_mmap+0x22/0x30
[<ffffffff8172e064>] tracesys+0xdd/0xe2
other info that might help us debug this:
Chain exists of:
&of->mutex --> sr_mutex --> &mm->mmap_sem
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&mm->mmap_sem);
lock(sr_mutex);
lock(&mm->mmap_sem);
lock(&of->mutex);
*** DEADLOCK ***
1 lock held by trinity-child0/9004:
#0: (&mm->mmap_sem){++++++}, at: [<ffffffff8116b5ff>] vm_mmap_pgoff+0x6f/0xc0
stack backtrace:
CPU: 0 PID: 9004 Comm: trinity-child0 Not tainted 3.12.0+ #2
ffffffff82501d30 ffff880093e3bbc0 ffffffff8171b3dc ffffffff825294d0
ffff880093e3bc00 ffffffff81717c8f ffff880093e3bc50 ffff88009ce00700
ffff88009ce00000 0000000000000001 0000000000000001 ffff88009ce00700
Call Trace:
[<ffffffff8171b3dc>] dump_stack+0x4e/0x7a
[<ffffffff81717c8f>] print_circular_bug+0x200/0x20f
[<ffffffff810d7002>] __lock_acquire+0x1782/0x19f0
[<ffffffff810d7a23>] lock_acquire+0x93/0x1c0
[<ffffffff8123c0cf>] ? sysfs_bin_mmap+0x4f/0x120
[<ffffffff8123c0cf>] ? sysfs_bin_mmap+0x4f/0x120
[<ffffffff8171f277>] mutex_lock_nested+0x77/0x400
[<ffffffff8123c0cf>] ? sysfs_bin_mmap+0x4f/0x120
[<ffffffff8123c0cf>] ? sysfs_bin_mmap+0x4f/0x120
[<ffffffff8123c0cf>] sysfs_bin_mmap+0x4f/0x120
[<ffffffff81180695>] mmap_region+0x3e5/0x5d0
[<ffffffff81180bd7>] do_mmap_pgoff+0x357/0x3e0
[<ffffffff8116b620>] vm_mmap_pgoff+0x90/0xc0
[<ffffffff8117f125>] SyS_mmap_pgoff+0x1d5/0x270
[<ffffffff810109f5>] ? syscall_trace_enter+0x145/0x2a0
[<ffffffff81007eb2>] SyS_mmap+0x22/0x30
[<ffffffff8172e064>] tracesys+0xdd/0xe2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: sysfs_bin_mmap lockdep trace.
2013-11-13 18:45 sysfs_bin_mmap lockdep trace Dave Jones
@ 2013-11-13 20:10 ` Al Viro
2013-11-14 5:41 ` Tejun Heo
0 siblings, 1 reply; 9+ messages in thread
From: Al Viro @ 2013-11-13 20:10 UTC (permalink / raw)
To: Dave Jones, Linux Kernel, Linus Torvalds; +Cc: Tejun Heo
On Wed, Nov 13, 2013 at 01:45:38PM -0500, Dave Jones wrote:
> Al, is this one also known ? Also seen on v3.12-7033-g42a2d923cc34
Umm... I've seen something like that reported after sysfs merge went in
(right after 3.12), but I hadn't looked into details.
> -> #3 (&mm->mmap_sem){++++++}:
[sr_block_ioctl() grabs sr_mutex and does copy_from_user() under it]
> -> #2 (sr_mutex){+.+.+.}:
[sr_block_open() grabs sr_mutex under ->bd_mutex]
> -> #1 (&bdev->bd_mutex){+.+.+.}:
[sysfs_blk_trace_attr_show() grabs ->bd_mutex and is called under
sysfs_open_file ->mutex]
> -> #0 (&of->mutex){+.+.+.}:
[sysfs_open_file ->mutex is grabbed by ->mmap()]
Cute... AFAICS, it came from "sysfs: copy bin mmap support from fs/sysfs/bin.c
to fs/sysfs/file.c". The first impression is that sysfs_bin_mmap() is
checking for battr->mmap too late, but I'm not sure whether we need of->mutex
to stabilize it... Tejun, any comments?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: sysfs_bin_mmap lockdep trace.
2013-11-13 20:10 ` Al Viro
@ 2013-11-14 5:41 ` Tejun Heo
2013-11-15 1:19 ` Dave Jones
0 siblings, 1 reply; 9+ messages in thread
From: Tejun Heo @ 2013-11-14 5:41 UTC (permalink / raw)
To: Al Viro; +Cc: Dave Jones, Linux Kernel, Linus Torvalds
hey,
On Wed, Nov 13, 2013 at 08:10:43PM +0000, Al Viro wrote:
> On Wed, Nov 13, 2013 at 01:45:38PM -0500, Dave Jones wrote:
> > Al, is this one also known ? Also seen on v3.12-7033-g42a2d923cc34
>
> Umm... I've seen something like that reported after sysfs merge went in
> (right after 3.12), but I hadn't looked into details.
>
> > -> #3 (&mm->mmap_sem){++++++}:
>
> [sr_block_ioctl() grabs sr_mutex and does copy_from_user() under it]
>
> > -> #2 (sr_mutex){+.+.+.}:
> [sr_block_open() grabs sr_mutex under ->bd_mutex]
>
> > -> #1 (&bdev->bd_mutex){+.+.+.}:
> [sysfs_blk_trace_attr_show() grabs ->bd_mutex and is called under
> sysfs_open_file ->mutex]
>
> > -> #0 (&of->mutex){+.+.+.}:
> [sysfs_open_file ->mutex is grabbed by ->mmap()]
>
> Cute... AFAICS, it came from "sysfs: copy bin mmap support from fs/sysfs/bin.c
> to fs/sysfs/file.c". The first impression is that sysfs_bin_mmap() is
> checking for battr->mmap too late, but I'm not sure whether we need of->mutex
> to stabilize it... Tejun, any comments?
Hmmm... so this is a false positive from regular and bin file paths
being merged. There was a sysfs regular file which grabbed sr_mutex
while holding sysfs mutex and only bin files supported mmap which of
course nest under mmap_sem. As the two paths were separate and using
separate locks, this deadlock scenario didn't trigger. Now that the
two paths are merged, lockdep considers the two paths to be using the
same mutex (they're per-file so still actually separate) and generates
this warning. The easiest way out would be giving different lock
subclasses to files w/ and w/o mmap method. I'll think more about it.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: sysfs_bin_mmap lockdep trace.
2013-11-14 5:41 ` Tejun Heo
@ 2013-11-15 1:19 ` Dave Jones
2013-11-17 2:17 ` [PATCH] sysfs: use a separate locking class for open files depending on mmap Tejun Heo
0 siblings, 1 reply; 9+ messages in thread
From: Dave Jones @ 2013-11-15 1:19 UTC (permalink / raw)
To: Tejun Heo; +Cc: Al Viro, Linux Kernel, Linus Torvalds
On Thu, Nov 14, 2013 at 02:41:16PM +0900, Tejun Heo wrote:
> hey,
>
> On Wed, Nov 13, 2013 at 08:10:43PM +0000, Al Viro wrote:
> > On Wed, Nov 13, 2013 at 01:45:38PM -0500, Dave Jones wrote:
> > > Al, is this one also known ? Also seen on v3.12-7033-g42a2d923cc34
> >
> > Umm... I've seen something like that reported after sysfs merge went in
> > (right after 3.12), but I hadn't looked into details.
> >
> > > -> #3 (&mm->mmap_sem){++++++}:
> >
> > [sr_block_ioctl() grabs sr_mutex and does copy_from_user() under it]
> >
> > > -> #2 (sr_mutex){+.+.+.}:
> > [sr_block_open() grabs sr_mutex under ->bd_mutex]
> >
> > > -> #1 (&bdev->bd_mutex){+.+.+.}:
> > [sysfs_blk_trace_attr_show() grabs ->bd_mutex and is called under
> > sysfs_open_file ->mutex]
> >
> > > -> #0 (&of->mutex){+.+.+.}:
> > [sysfs_open_file ->mutex is grabbed by ->mmap()]
> >
> > Cute... AFAICS, it came from "sysfs: copy bin mmap support from fs/sysfs/bin.c
> > to fs/sysfs/file.c". The first impression is that sysfs_bin_mmap() is
> > checking for battr->mmap too late, but I'm not sure whether we need of->mutex
> > to stabilize it... Tejun, any comments?
>
> Hmmm... so this is a false positive from regular and bin file paths
> being merged. There was a sysfs regular file which grabbed sr_mutex
> while holding sysfs mutex and only bin files supported mmap which of
> course nest under mmap_sem. As the two paths were separate and using
> separate locks, this deadlock scenario didn't trigger. Now that the
> two paths are merged, lockdep considers the two paths to be using the
> same mutex (they're per-file so still actually separate) and generates
> this warning. The easiest way out would be giving different lock
> subclasses to files w/ and w/o mmap method. I'll think more about it.
Tejun,
Is this another variant of the above, or something different ?
Dave
[ 218.248982] ======================================================
[ 218.249006] [ INFO: possible circular locking dependency detected ]
[ 218.249031] 3.12.0+ #8 Not tainted
[ 218.249607] -------------------------------------------------------
[ 218.250194] trinity-child1/2100 is trying to acquire lock:
[ 218.250784] (&bdev->bd_mutex){+.+.+.}, at: [<ffffffff8112f89f>] sysfs_blk_trace_attr_show+0x5f/0x1f0
[ 218.251408]
but task is already holding lock:
[ 218.252615] (&of->mutex){+.+.+.}, at: [<ffffffff8124083f>] sysfs_seq_show+0x7f/0x160
[ 218.253249]
which lock already depends on the new lock.
[ 218.255178]
the existing dependency chain (in reverse order) is:
[ 218.256480]
-> #3 (&of->mutex){+.+.+.}:
[ 218.257853] [<ffffffff810af623>] lock_acquire+0x93/0x1c0
[ 218.258557] [<ffffffff81725f67>] mutex_lock_nested+0x77/0x400
[ 218.259263] [<ffffffff8123ffef>] sysfs_bin_mmap+0x4f/0x120
[ 218.259900] [<ffffffff81182695>] mmap_region+0x3e5/0x5d0
[ 218.260526] [<ffffffff81182bd7>] do_mmap_pgoff+0x357/0x3e0
[ 218.261149] [<ffffffff8116d150>] vm_mmap_pgoff+0x90/0xc0
[ 218.261765] [<ffffffff81181125>] SyS_mmap_pgoff+0x1d5/0x270
[ 218.262374] [<ffffffff81007ec2>] SyS_mmap+0x22/0x30
[ 218.262983] [<ffffffff81732ce4>] tracesys+0xdd/0xe2
[ 218.263581]
-> #2 (&mm->mmap_sem){++++++}:
[ 218.264853] [<ffffffff810af623>] lock_acquire+0x93/0x1c0
[ 218.265464] [<ffffffff8117762c>] might_fault+0x8c/0xb0
[ 218.266114] [<ffffffff81302ed5>] scsi_cmd_ioctl+0x295/0x470
[ 218.266760] [<ffffffff813030f2>] scsi_cmd_blk_ioctl+0x42/0x50
[ 218.267405] [<ffffffff8150b5e1>] cdrom_ioctl+0x41/0x1050
[ 218.268056] [<ffffffff814de69f>] sr_block_ioctl+0x6f/0xd0
[ 218.268724] [<ffffffff812fede4>] blkdev_ioctl+0x234/0x840
[ 218.269373] [<ffffffff811faa27>] block_ioctl+0x47/0x50
[ 218.270011] [<ffffffff811ce4f0>] do_vfs_ioctl+0x300/0x520
[ 218.270651] [<ffffffff811ce791>] SyS_ioctl+0x81/0xa0
[ 218.271282] [<ffffffff81732ce4>] tracesys+0xdd/0xe2
[ 218.271913]
-> #1 (sr_mutex){+.+.+.}:
[ 218.273153] [<ffffffff810af623>] lock_acquire+0x93/0x1c0
[ 218.273795] [<ffffffff81725f67>] mutex_lock_nested+0x77/0x400
[ 218.274443] [<ffffffff814ded34>] sr_block_open+0x24/0x130
[ 218.275084] [<ffffffff811fb7f1>] __blkdev_get+0xd1/0x4c0
[ 218.275707] [<ffffffff811fbdc5>] blkdev_get+0x1e5/0x380
[ 218.276314] [<ffffffff811fc01a>] blkdev_open+0x6a/0x90
[ 218.276904] [<ffffffff811b6f97>] do_dentry_open+0x1e7/0x340
[ 218.277490] [<ffffffff811b7200>] finish_open+0x40/0x50
[ 218.278070] [<ffffffff811ca207>] do_last+0xbc7/0x1370
[ 218.278637] [<ffffffff811caa6e>] path_openat+0xbe/0x6a0
[ 218.279206] [<ffffffff811cb7ca>] do_filp_open+0x3a/0x90
[ 218.279718] [<ffffffff811b8c1e>] do_sys_open+0x12e/0x210
[ 218.280221] [<ffffffff811b8d1e>] SyS_open+0x1e/0x20
[ 218.280708] [<ffffffff81732ce4>] tracesys+0xdd/0xe2
[ 218.281184]
-> #0 (&bdev->bd_mutex){+.+.+.}:
[ 218.282109] [<ffffffff810aec02>] __lock_acquire+0x1782/0x19f0
[ 218.282622] [<ffffffff810af623>] lock_acquire+0x93/0x1c0
[ 218.283108] [<ffffffff81725f67>] mutex_lock_nested+0x77/0x400
[ 218.283582] [<ffffffff8112f89f>] sysfs_blk_trace_attr_show+0x5f/0x1f0
[ 218.284060] [<ffffffff814b09f0>] dev_attr_show+0x20/0x60
[ 218.284531] [<ffffffff81240888>] sysfs_seq_show+0xc8/0x160
[ 218.284997] [<ffffffff811e2da2>] traverse.isra.6+0xf2/0x260
[ 218.285468] [<ffffffff811e3531>] seq_read+0x3e1/0x450
[ 218.285929] [<ffffffff811b9768>] vfs_read+0x98/0x170
[ 218.286386] [<ffffffff811ba412>] SyS_pread64+0x72/0xb0
[ 218.286844] [<ffffffff81732ce4>] tracesys+0xdd/0xe2
[ 218.287299]
other info that might help us debug this:
[ 218.288691] Chain exists of:
&bdev->bd_mutex --> &mm->mmap_sem --> &of->mutex
[ 218.290060] Possible unsafe locking scenario:
[ 218.291013] CPU0 CPU1
[ 218.291512] ---- ----
[ 218.292003] lock(&of->mutex);
[ 218.292447] lock(&mm->mmap_sem);
[ 218.292900] lock(&of->mutex);
[ 218.293348] lock(&bdev->bd_mutex);
[ 218.293793]
*** DEADLOCK ***
[ 218.295130] 3 locks held by trinity-child1/2100:
[ 218.295615] #0: (&p->lock){+.+.+.}, at: [<ffffffff811e318d>] seq_read+0x3d/0x450
[ 218.296068] #1: (&of->mutex){+.+.+.}, at: [<ffffffff8124083f>] sysfs_seq_show+0x7f/0x160
[ 218.296555] #2: (s_active#113){.+.+.+}, at: [<ffffffff81240848>] sysfs_seq_show+0x88/0x160
[ 218.297036]
stack backtrace:
[ 218.297952] CPU: 1 PID: 2100 Comm: trinity-child1 Not tainted 3.12.0+ #8
[ 218.299470] ffffffff824ccc60 ffff8801bccabbd0 ffffffff8172017e ffffffff824c5c00
[ 218.300013] ffff8801bccabc10 ffffffff8171c51d ffff8801bccabc60 ffff8802411a0778
[ 218.300571] ffff8802411a0000 0000000000000002 0000000000000003 ffff8802411a07b0
[ 218.301124] Call Trace:
[ 218.301670] [<ffffffff8172017e>] dump_stack+0x4e/0x7a
[ 218.302227] [<ffffffff8171c51d>] print_circular_bug+0x200/0x20f
[ 218.302788] [<ffffffff810aec02>] __lock_acquire+0x1782/0x19f0
[ 218.303351] [<ffffffff810af623>] lock_acquire+0x93/0x1c0
[ 218.303916] [<ffffffff8112f89f>] ? sysfs_blk_trace_attr_show+0x5f/0x1f0
[ 218.304488] [<ffffffff8112f89f>] ? sysfs_blk_trace_attr_show+0x5f/0x1f0
[ 218.305051] [<ffffffff81725f67>] mutex_lock_nested+0x77/0x400
[ 218.305606] [<ffffffff8112f89f>] ? sysfs_blk_trace_attr_show+0x5f/0x1f0
[ 218.306165] [<ffffffff8112f89f>] ? sysfs_blk_trace_attr_show+0x5f/0x1f0
[ 218.306719] [<ffffffff8112f89f>] sysfs_blk_trace_attr_show+0x5f/0x1f0
[ 218.307268] [<ffffffff814b09f0>] dev_attr_show+0x20/0x60
[ 218.307837] [<ffffffff8124048d>] ? sysfs_file_ops+0x5d/0x80
[ 218.308389] [<ffffffff81240888>] sysfs_seq_show+0xc8/0x160
[ 218.308944] [<ffffffff811e2da2>] traverse.isra.6+0xf2/0x260
[ 218.309496] [<ffffffff811e3531>] seq_read+0x3e1/0x450
[ 218.310048] [<ffffffff811b9768>] vfs_read+0x98/0x170
[ 218.310601] [<ffffffff811ba412>] SyS_pread64+0x72/0xb0
[ 218.311157] [<ffffffff81732ce4>] tracesys+0xdd/0xe2
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] sysfs: use a separate locking class for open files depending on mmap
2013-11-15 1:19 ` Dave Jones
@ 2013-11-17 2:17 ` Tejun Heo
2013-11-17 3:21 ` Dave Jones
0 siblings, 1 reply; 9+ messages in thread
From: Tejun Heo @ 2013-11-17 2:17 UTC (permalink / raw)
To: Dave Jones, Al Viro, Linus Torvalds, Linux Kernel, Greg Kroah-Hartman
The following two commits implemented mmap support in the regular file
path and merged bin file support into the regular path.
73d9714627ad ("sysfs: copy bin mmap support from fs/sysfs/bin.c to fs/sysfs/file.c")
3124eb1679b2 ("sysfs: merge regular and bin file handling")
After the merge, the following commands trigger a spurious lockdep
warning. "test-mmap-read" simply mmaps the file and dumps the
content.
$ cat /sys/block/sda/trace/act_mask
$ test-mmap-read /sys/devices/pci0000\:00/0000\:00\:03.0/resource0 4096
======================================================
[ INFO: possible circular locking dependency detected ]
3.12.0-work+ #378 Not tainted
-------------------------------------------------------
test-mmap-read/567 is trying to acquire lock:
(&of->mutex){+.+.+.}, at: [<ffffffff8120a8df>] sysfs_bin_mmap+0x4f/0x120
but task is already holding lock:
(&mm->mmap_sem){++++++}, at: [<ffffffff8114b399>] vm_mmap_pgoff+0x49/0xa0
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #3 (&mm->mmap_sem){++++++}:
...
-> #2 (sr_mutex){+.+.+.}:
...
-> #1 (&bdev->bd_mutex){+.+.+.}:
...
-> #0 (&of->mutex){+.+.+.}:
...
other info that might help us debug this:
Chain exists of:
&of->mutex --> sr_mutex --> &mm->mmap_sem
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&mm->mmap_sem);
lock(sr_mutex);
lock(&mm->mmap_sem);
lock(&of->mutex);
*** DEADLOCK ***
1 lock held by test-mmap-read/567:
#0: (&mm->mmap_sem){++++++}, at: [<ffffffff8114b399>] vm_mmap_pgoff+0x49/0xa0
stack backtrace:
CPU: 3 PID: 567 Comm: test-mmap-read Not tainted 3.12.0-work+ #378
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
ffffffff81ed41a0 ffff880009441bc8 ffffffff81611ad2 ffffffff81eccb80
ffff880009441c08 ffffffff8160f215 ffff880009441c60 ffff880009c75208
0000000000000000 ffff880009c751e0 ffff880009c75208 ffff880009c74ac0
Call Trace:
[<ffffffff81611ad2>] dump_stack+0x4e/0x7a
[<ffffffff8160f215>] print_circular_bug+0x2b0/0x2bf
[<ffffffff8109ca0a>] __lock_acquire+0x1a3a/0x1e60
[<ffffffff8109d6ba>] lock_acquire+0x9a/0x1d0
[<ffffffff81615547>] mutex_lock_nested+0x67/0x3f0
[<ffffffff8120a8df>] sysfs_bin_mmap+0x4f/0x120
[<ffffffff8115d363>] mmap_region+0x3b3/0x5b0
[<ffffffff8115d8ae>] do_mmap_pgoff+0x34e/0x3d0
[<ffffffff8114b3ba>] vm_mmap_pgoff+0x6a/0xa0
[<ffffffff8115be3e>] SyS_mmap_pgoff+0xbe/0x250
[<ffffffff81008282>] SyS_mmap+0x22/0x30
[<ffffffff8161a4d2>] system_call_fastpath+0x16/0x1b
This happens because one file nests sr_mutex, which nests mm->mmap_sem
under it, under of->mutex while mmap implementation naturally nests
of->mutex under mm->mmap_sem. The warning is false positive as
of->mutex is per open-file and the two paths belong to two different
files. This warning didn't trigger before regular and bin file
supports were merged because only bin file supported mmap and the
other side of locking happened only on regular files which used
equivalent but separate locking.
It'd be best if we give separate locking classes per file but we can't
easily do that. Let's differentiate on ->mmap() for now. Later we'll
add explicit file operations struct and can add per-ops lockdep key
there.
Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Dave Jones <davej@redhat.com>
---
fs/sysfs/file.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 79b5da2..b94f936 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -609,7 +609,7 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
struct kobject *kobj = attr_sd->s_parent->s_dir.kobj;
struct sysfs_open_file *of;
- bool has_read, has_write;
+ bool has_read, has_write, has_mmap;
int error = -EACCES;
/* need attr_sd for attr and ops, its parent for kobj */
@@ -621,6 +621,7 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
has_read = battr->read || battr->mmap;
has_write = battr->write || battr->mmap;
+ has_mmap = battr->mmap;
} else {
const struct sysfs_ops *ops = sysfs_file_ops(attr_sd);
@@ -632,6 +633,7 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
has_read = ops->show;
has_write = ops->store;
+ has_mmap = false;
}
/* check perms and supported operations */
@@ -649,7 +651,23 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
if (!of)
goto err_out;
- mutex_init(&of->mutex);
+ /*
+ * 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);
+
of->sd = attr_sd;
of->file = file;
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] sysfs: use a separate locking class for open files depending on mmap
2013-11-17 2:17 ` [PATCH] sysfs: use a separate locking class for open files depending on mmap Tejun Heo
@ 2013-11-17 3:21 ` Dave Jones
2013-11-17 3:29 ` Tejun Heo
0 siblings, 1 reply; 9+ messages in thread
From: Dave Jones @ 2013-11-17 3:21 UTC (permalink / raw)
To: Tejun Heo; +Cc: Al Viro, Linus Torvalds, Linux Kernel, Greg Kroah-Hartman
On Sun, Nov 17, 2013 at 11:17:36AM +0900, Tejun Heo wrote:
> + if (has_mmap)
> + mutex_init(&of->mutex);
> + else
> + mutex_init(&of->mutex);
ummm...
Dave
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sysfs: use a separate locking class for open files depending on mmap
2013-11-17 3:21 ` Dave Jones
@ 2013-11-17 3:29 ` Tejun Heo
2013-11-18 4:45 ` Greg Kroah-Hartman
0 siblings, 1 reply; 9+ messages in thread
From: Tejun Heo @ 2013-11-17 3:29 UTC (permalink / raw)
To: Dave Jones; +Cc: Al Viro, Linus Torvalds, Linux Kernel, Greg Kroah-Hartman
On Sat, Nov 16, 2013 at 10:21:19PM -0500, Dave Jones wrote:
> On Sun, Nov 17, 2013 at 11:17:36AM +0900, Tejun Heo wrote:
>
>
> > + if (has_mmap)
> > + mutex_init(&of->mutex);
> > + else
> > + mutex_init(&of->mutex);
>
> ummm...
Supposed to look that way. It'll give two separate static lock class
keys to of->mutex. Yeah, looks weird. Any better ideas?
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sysfs: use a separate locking class for open files depending on mmap
2013-11-17 3:29 ` Tejun Heo
@ 2013-11-18 4:45 ` Greg Kroah-Hartman
2013-11-20 6:30 ` Tejun Heo
0 siblings, 1 reply; 9+ messages in thread
From: Greg Kroah-Hartman @ 2013-11-18 4:45 UTC (permalink / raw)
To: Tejun Heo; +Cc: Dave Jones, Al Viro, Linus Torvalds, Linux Kernel
On Sun, Nov 17, 2013 at 12:29:37PM +0900, Tejun Heo wrote:
> On Sat, Nov 16, 2013 at 10:21:19PM -0500, Dave Jones wrote:
> > On Sun, Nov 17, 2013 at 11:17:36AM +0900, Tejun Heo wrote:
> >
> >
> > > + if (has_mmap)
> > > + mutex_init(&of->mutex);
> > > + else
> > > + mutex_init(&of->mutex);
> >
> > ummm...
>
> Supposed to look that way. It'll give two separate static lock class
> keys to of->mutex. Yeah, looks weird. Any better ideas?
Doesn't gcc optimize that away to just one lock class anyway?
greg k-h
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sysfs: use a separate locking class for open files depending on mmap
2013-11-18 4:45 ` Greg Kroah-Hartman
@ 2013-11-20 6:30 ` Tejun Heo
0 siblings, 0 replies; 9+ messages in thread
From: Tejun Heo @ 2013-11-20 6:30 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Dave Jones, Al Viro, Linus Torvalds, Linux Kernel
On Sun, Nov 17, 2013 at 08:45:23PM -0800, Greg Kroah-Hartman wrote:
> On Sun, Nov 17, 2013 at 12:29:37PM +0900, Tejun Heo wrote:
> > On Sat, Nov 16, 2013 at 10:21:19PM -0500, Dave Jones wrote:
> > > On Sun, Nov 17, 2013 at 11:17:36AM +0900, Tejun Heo wrote:
> > >
> > >
> > > > + if (has_mmap)
> > > > + mutex_init(&of->mutex);
> > > > + else
> > > > + mutex_init(&of->mutex);
> > >
> > > ummm...
> >
> > Supposed to look that way. It'll give two separate static lock class
> > keys to of->mutex. Yeah, looks weird. Any better ideas?
>
> Doesn't gcc optimize that away to just one lock class anyway?
Well, it basically becomes
if (has_mmap) {
static struct lock_class_key key;
__mutex_init(blah, &key);
} else {
static struct lock_class_key key;
__mutex_init(blah, &key);
}
So, the compiler isn't allowed to merge the two keys and we actually
use constructs like the above from a few places. Yeah, it is weird
but does work.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-11-20 6:30 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-13 18:45 sysfs_bin_mmap lockdep trace Dave Jones
2013-11-13 20:10 ` Al Viro
2013-11-14 5:41 ` Tejun Heo
2013-11-15 1:19 ` Dave Jones
2013-11-17 2:17 ` [PATCH] sysfs: use a separate locking class for open files depending on mmap Tejun Heo
2013-11-17 3:21 ` Dave Jones
2013-11-17 3:29 ` Tejun Heo
2013-11-18 4:45 ` Greg Kroah-Hartman
2013-11-20 6:30 ` Tejun Heo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).