linux-kernel - [PATCH driver-core-linus] sysfs: bail early from sysfs_bin

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131204140639.GI3158@htj.dyndns.org>
Date:	Wed, 4 Dec 2013 09:06:39 -0500
From:	Tejun Heo <tj@...nel.org>
To:	Dave Jones <davej@...hat.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	gregkh@...uxfoundation.org
Subject: [PATCH driver-core-linus] sysfs: bail early from sysfs_bin_mmap() to
 avoid spurious lockdep warning

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@...nel.org>
Reported-by: Dave Jones <davej@...hat.com>
Tested-by: Dave Jones <davej@...hat.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)
 {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/