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

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

[Dave Jones]

On Thu, Nov 28, 2013 at 05:12:23AM +0000, Linux Kernel wrote:
 > Gitweb:     http://git.kernel.org/linus/;a=commit;h=027a485d12e089314360d459b8d847104dd28702
 > Commit:     027a485d12e089314360d459b8d847104dd28702
 > Parent:     54d71145a4548330313ca664a4a009772fe8b7dd
 > Author:     Tejun Heo <tj@kernel.org>
 > AuthorDate: Sun Nov 17 11:17:36 2013 +0900
 > Committer:  Greg Kroah-Hartman <gregkh@linuxfoundation.org>
 > CommitDate: Sat Nov 23 10:52:13 2013 -0800
 > 
 >     sysfs: use a separate locking class for open files depending on mmap
 >     
 >     The following two commits implemented mmap support in the regular file
 >     path and merged bin file support into the regular path.
 > ...
 > -	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);
 > +
 
Somehow I just triggered this trace again, even with this commit applied.
The trace is pretty much identical to the old one.

	Dave

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

[Tejun Heo]

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.

Thanks.

-- 
tejun

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

[Dave Jones]

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

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

[Tejun Heo]

Hey,

On Tue, Dec 03, 2013 at 04:15:15PM -0500, Dave Jones wrote:
>  > > 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.

Argh, found the culprit.  The unified mmap path grabs of->mutex to
check whether the file implements mmap, which means that if you try to
mmap a file which doesn't implement mmap, you still add the locking
dependency.  I'm working on the fix.

Thanks.

-- 
tejun

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

[Tejun Heo]

Hello,

Can you please test whether this patch makes the lockdep warning go
away?

Thanks a lot!

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index b94f936..fccb645 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -470,6 +470,9 @@ static int sysfs_bin_mmap(struct file *file, struct vm_area_struct *vma)
 	struct kobject *kobj = of->sd->s_parent->s_dir.kobj;
 	int rc;
 
+	if (!(of->sd->s_flags & SYSFS_FLAG_HAS_MMAP))
+		return -ENODEV;
+
 	mutex_lock(&of->mutex);
 
 	/* need of->sd for battr, its parent for kobj */
@@ -477,9 +480,6 @@ static int sysfs_bin_mmap(struct file *file, struct vm_area_struct *vma)
 	if (!sysfs_get_active(of->sd))
 		goto out_unlock;
 
-	if (!battr->mmap)
-		goto out_put;
-
 	rc = battr->mmap(file, kobj, battr, vma);
 	if (rc)
 		goto out_put;
@@ -851,6 +851,14 @@ int sysfs_add_file_mode_ns(struct sysfs_dirent *dir_sd,
 	sd->s_attr.attr = (void *)attr;
 	sysfs_dirent_init_lockdep(sd);
 
+	if (type == SYSFS_KOBJ_BIN_ATTR) {
+		const struct bin_attribute *battr =
+			container_of(attr, struct bin_attribute, attr);
+
+		if (battr->mmap)
+			sd->s_flags |= SYSFS_FLAG_HAS_MMAP;
+	}
+
 	sysfs_addrm_start(&acxt);
 	rc = sysfs_add_one(&acxt, sd, dir_sd);
 	sysfs_addrm_finish(&acxt);
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index 0af09fb..27c1f7e 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -96,6 +96,7 @@ struct sysfs_dirent {
 
 #define SYSFS_FLAG_MASK			~(SYSFS_NS_TYPE_MASK|SYSFS_TYPE_MASK)
 #define SYSFS_FLAG_REMOVED		0x02000
+#define SYSFS_FLAG_HAS_MMAP		0x04000
 
 static inline unsigned int sysfs_type(struct sysfs_dirent *sd)
 {

-- 
tejun

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

[Dave Jones]

On Tue, Dec 03, 2013 at 05:15:43PM -0500, Tejun Heo wrote:
 > Hello,
 > 
 > Can you please test whether this patch makes the lockdep warning go
 > away?
 > 
 > Thanks a lot!
 
been running a few hours now, looks good.

Tested-by: Dave Jones <davej@fedoraproject.org>

	Dave

[PATCH driver-core-linus] sysfs: bail early from sysfs_bin_mmap() to avoid spurious lockdep warning

[Tejun Heo]

027a485d12e0 ("sysfs: use a separate locking class for open files
depending on mmap") assigned different lockdep key to
sysfs_open_file->mutex depending on whether the file implements mmap
or not in an attempt to avoid spurious lockdep warning caused by
merging of regular and bin file paths.

While this restored some of the original behavior of using different
locks (at least lockdep is concerned) for the different clases of
files.  The restoration wasn't full because now the lockdep key
assignment depends on whether the file has mmap or not instead of
whether it's a regular file or not.

This means that bin files which don't implement mmap will get assigned
the same lockdep class as regular files.  This is problematic because
file_operations for bin files still implements the mmap file operation
and checking whether the sysfs file actually implements mmap happens
in the file operation after grabbing @sysfs_open_file->mutex.  We
still end up adding locking dependency from mmap locking to
sysfs_open_file->mutex to the regular file mutex which triggers
spurious circular locking warning.

This can be fixed by either giving sysfs_open_file->mutex different
lockdep keys depending on whether the file is regular or bin instead
of whether mmap exists or not, or avoiding grabbing
sysfs_open_file->mutex from sysfs_bin_mmap() if mmap is not actually
implemented.  While the former is simpler for driver-core-linus,
driver-core-next already has SYSFS_FLAG_HAS_MMAP in place to implement
the latter and doesn't have inherent distinction between regular and
bin files.  This patch implements the latter so that the fix is more
conducive to driver-core-next.

Because anything beyond sysfs_open_file->sd can't be dereferenced
without locking the open file, cache whether mmap is implemented or
not in sysfs_open_file->sd->s_flags and update sysfs_bin_mmap() test
the flag and bail without grabbing the mutex if not implemented.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Dave Jones <davej@redhat.com>
Tested-by: Dave Jones <davej@redhat.com>
Link: http://lkml.kernel.org/g/20131203184324.GA11320@redhat.com
---
 fs/sysfs/file.c  |   21 ++++++++++++++++++---
 fs/sysfs/sysfs.h |    1 +
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index b94f936..90e9e5d 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -470,6 +470,16 @@ static int sysfs_bin_mmap(struct file *file, struct vm_area_struct *vma)
 	struct kobject *kobj = of->sd->s_parent->s_dir.kobj;
 	int rc;
 
+	/*
+	 * mmap path and of->mutex are prone to triggering spurious lockdep
+	 * warnings and we don't want to add spurious locking dependency
+	 * between the two.  Check whether mmap is actually implemented
+	 * without grabbing @of->mutex by testing HAS_MMAP flag.  See the
+	 * comment in sysfs_open_file() for more details.
+	 */
+	if (!(of->sd->s_flags & SYSFS_FLAG_HAS_MMAP))
+		return -ENODEV;
+
 	mutex_lock(&of->mutex);
 
 	/* need of->sd for battr, its parent for kobj */
@@ -477,9 +487,6 @@ static int sysfs_bin_mmap(struct file *file, struct vm_area_struct *vma)
 	if (!sysfs_get_active(of->sd))
 		goto out_unlock;
 
-	if (!battr->mmap)
-		goto out_put;
-
 	rc = battr->mmap(file, kobj, battr, vma);
 	if (rc)
 		goto out_put;
@@ -851,6 +858,14 @@ int sysfs_add_file_mode_ns(struct sysfs_dirent *dir_sd,
 	sd->s_attr.attr = (void *)attr;
 	sysfs_dirent_init_lockdep(sd);
 
+	if (type == SYSFS_KOBJ_BIN_ATTR) {
+		const struct bin_attribute *battr =
+			container_of(attr, struct bin_attribute, attr);
+
+		if (battr->mmap)
+			sd->s_flags |= SYSFS_FLAG_HAS_MMAP;
+	}
+
 	sysfs_addrm_start(&acxt);
 	rc = sysfs_add_one(&acxt, sd, dir_sd);
 	sysfs_addrm_finish(&acxt);
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index 0af09fb..27c1f7e 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -96,6 +96,7 @@ struct sysfs_dirent {
 
 #define SYSFS_FLAG_MASK			~(SYSFS_NS_TYPE_MASK|SYSFS_TYPE_MASK)
 #define SYSFS_FLAG_REMOVED		0x02000
+#define SYSFS_FLAG_HAS_MMAP		0x04000
 
 static inline unsigned int sysfs_type(struct sysfs_dirent *sd)
 {

Re: [PATCH driver-core-linus] sysfs: bail early from sysfs_bin_mmap() to avoid spurious lockdep warning

[Tejun Heo]

On Wed, Dec 04, 2013 at 09:06:39AM -0500, Tejun Heo wrote:
> 027a485d12e0 ("sysfs: use a separate locking class for open files
> depending on mmap") assigned different lockdep key to
> sysfs_open_file->mutex depending on whether the file implements mmap
> or not in an attempt to avoid spurious lockdep warning caused by
> merging of regular and bin file paths.
> 
> While this restored some of the original behavior of using different
> locks (at least lockdep is concerned) for the different clases of
> files.  The restoration wasn't full because now the lockdep key
> assignment depends on whether the file has mmap or not instead of
> whether it's a regular file or not.
> 
> This means that bin files which don't implement mmap will get assigned
> the same lockdep class as regular files.  This is problematic because
> file_operations for bin files still implements the mmap file operation
> and checking whether the sysfs file actually implements mmap happens
> in the file operation after grabbing @sysfs_open_file->mutex.  We
> still end up adding locking dependency from mmap locking to
> sysfs_open_file->mutex to the regular file mutex which triggers
> spurious circular locking warning.
> 
> This can be fixed by either giving sysfs_open_file->mutex different
> lockdep keys depending on whether the file is regular or bin instead
> of whether mmap exists or not, or avoiding grabbing
> sysfs_open_file->mutex from sysfs_bin_mmap() if mmap is not actually
> implemented.  While the former is simpler for driver-core-linus,
> driver-core-next already has SYSFS_FLAG_HAS_MMAP in place to implement
> the latter and doesn't have inherent distinction between regular and
> bin files.  This patch implements the latter so that the fix is more
> conducive to driver-core-next.
> 
> Because anything beyond sysfs_open_file->sd can't be dereferenced
> without locking the open file, cache whether mmap is implemented or
> not in sysfs_open_file->sd->s_flags and update sysfs_bin_mmap() test
> the flag and bail without grabbing the mutex if not implemented.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Dave Jones <davej@redhat.com>
> Tested-by: Dave Jones <davej@redhat.com>
> Link: http://lkml.kernel.org/g/20131203184324.GA11320@redhat.com

Please forget about this.  This is actually a lot harder to translate
to kernfs.  I'll prepare another patch.  Sorry about the noise.

Thanks.

-- 
tejun

[PATCH driver-core-linus] sysfs: give different locking key to regular and bin files

[Tejun Heo]

Dave, can you please test this one too?  Greg, once this turns out to
be okay, I'll send you a merged branch which pulls in
driver-core-linus + this patch into driver-core-next which will surely
generate conflict.

Thanks.
----- 8< ------
027a485d12e0 ("sysfs: use a separate locking class for open files
depending on mmap") assigned different lockdep key to
sysfs_open_file->mutex depending on whether the file implements mmap
or not in an attempt to avoid spurious lockdep warning caused by
merging of regular and bin file paths.

While this restored some of the original behavior of using different
locks (at least lockdep is concerned) for the different clases of
files.  The restoration wasn't full because now the lockdep key
assignment depends on whether the file has mmap or not instead of
whether it's a regular file or not.

This means that bin files which don't implement mmap will get assigned
the same lockdep class as regular files.  This is problematic because
file_operations for bin files still implements the mmap file operation
and checking whether the sysfs file actually implements mmap happens
in the file operation after grabbing @sysfs_open_file->mutex.  We
still end up adding locking dependency from mmap locking to
sysfs_open_file->mutex to the regular file mutex which triggers
spurious circular locking warning.

Fix it by restoring the original behavior fully by differentiating
lockdep key by whether the file is regular or bin, instead of the
existence of mmap.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Dave Jones <davej@redhat.com>
Link: http://lkml.kernel.org/g/20131203184324.GA11320@redhat.com
---
 fs/sysfs/file.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index b94f936..35e7d08 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, has_mmap;
+	bool has_read, has_write;
 	int error = -EACCES;
 
 	/* need attr_sd for attr and ops, its parent for kobj */
@@ -621,7 +621,6 @@ 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);
 
@@ -633,7 +632,6 @@ 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 */
@@ -661,9 +659,9 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
 	 * 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.
+	 * whether the file is bin or not for now.
 	 */
-	if (has_mmap)
+	if (sysfs_is_bin(attr_sd))
 		mutex_init(&of->mutex);
 	else
 		mutex_init(&of->mutex);

Re: [PATCH driver-core-linus] sysfs: give different locking key to regular and bin files

[Greg KH]

On Wed, Dec 04, 2013 at 09:20:40AM -0500, Tejun Heo wrote:
> Dave, can you please test this one too?  Greg, once this turns out to
> be okay, I'll send you a merged branch which pulls in
> driver-core-linus + this patch into driver-core-next which will surely
> generate conflict.

Dave, did this patch fix the issue?

thanks,

greg k-h

> ----- 8< ------
> 027a485d12e0 ("sysfs: use a separate locking class for open files
> depending on mmap") assigned different lockdep key to
> sysfs_open_file->mutex depending on whether the file implements mmap
> or not in an attempt to avoid spurious lockdep warning caused by
> merging of regular and bin file paths.
> 
> While this restored some of the original behavior of using different
> locks (at least lockdep is concerned) for the different clases of
> files.  The restoration wasn't full because now the lockdep key
> assignment depends on whether the file has mmap or not instead of
> whether it's a regular file or not.
> 
> This means that bin files which don't implement mmap will get assigned
> the same lockdep class as regular files.  This is problematic because
> file_operations for bin files still implements the mmap file operation
> and checking whether the sysfs file actually implements mmap happens
> in the file operation after grabbing @sysfs_open_file->mutex.  We
> still end up adding locking dependency from mmap locking to
> sysfs_open_file->mutex to the regular file mutex which triggers
> spurious circular locking warning.
> 
> Fix it by restoring the original behavior fully by differentiating
> lockdep key by whether the file is regular or bin, instead of the
> existence of mmap.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Dave Jones <davej@redhat.com>
> Link: http://lkml.kernel.org/g/20131203184324.GA11320@redhat.com
> ---
>  fs/sysfs/file.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index b94f936..35e7d08 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, has_mmap;
> +	bool has_read, has_write;
>  	int error = -EACCES;
>  
>  	/* need attr_sd for attr and ops, its parent for kobj */
> @@ -621,7 +621,6 @@ 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);
>  
> @@ -633,7 +632,6 @@ 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 */
> @@ -661,9 +659,9 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
>  	 * 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.
> +	 * whether the file is bin or not for now.
>  	 */
> -	if (has_mmap)
> +	if (sysfs_is_bin(attr_sd))
>  		mutex_init(&of->mutex);
>  	else
>  		mutex_init(&of->mutex);

Re: [PATCH driver-core-linus] sysfs: give different locking key to regular and bin files

[Dave Jones]

On Thu, Dec 05, 2013 at 11:44:40AM -0800, Greg KH wrote:
 > On Wed, Dec 04, 2013 at 09:20:40AM -0500, Tejun Heo wrote:
 > > Dave, can you please test this one too?  Greg, once this turns out to
 > > be okay, I'll send you a merged branch which pulls in
 > > driver-core-linus + this patch into driver-core-next which will surely
 > > generate conflict.
 > 
 > Dave, did this patch fix the issue?
 
I hit some unrelated issues instead, so I'd say 'probably'.
Given I could quickly hit it before, I'd say it works.

	Dave