[PATCH v2 0/2] fix debugfs file removal races

[PATCH v2 0/2] fix debugfs file removal races

[Nicolai Stange]

Original v1 thread is here:

  http://lkml.kernel.org/r/8737vnvzt0.fsf@gmail.com

Paul E. McKenney has judged on v1's SRCU usage already and I did
not change anything in the related logic.

Regarding Greg K-H's comment to v1 about moving as much stuff as
possible from debugfs.h into a separate internal.h: in my opinion, new
users of debugfs should really use DEBUGFS_DEFINE_ATTRIBUTE() or
manage lifetime from their custom readers and writers via
debugfs_file_use_data_*() from now on and thus, I kept the declaration
of these within the public debugfs.h. The rest has been moved either into
internal.h or in the case of definitions, into file.c though.

Changes v1 -> v2:
  [1/2] ("debugfs: prevent access to possibly dead file_operations at file open")
   - Resolve trivial diff conflict in debugfs_remove_recursive():
     in the meanwhile, an unrelated 'mutex_unlock(...)' had been rewritten to
     'inode_unlock(...)' which broke the diff's context.
   - Introduce the fs/debugfs/internal.h header and move the declarations of
     debugfs_noop_file_operations, debugfs_proxy_file_operations and
     debugfs_rcu from include/linux/debugfs.h thereinto. Include this header
     from file.c and inode.c.
   - Add a word about the new internal header to the commit message.
   - Move the inclusion of linux/srcu.h from include/linux/debugfs.h
     into file.c and inode.c respectively.

  [2/2] ("debugfs: prevent access to removed files' private data")
   - Move the definitions of debugfs_file_use_data_start() and
     debugfs_file_use_data_finish() from include/linux/debugfs.h to
     file.c. Export them and keep their declarations in debugfs.h,
   - In order to be able to attach proper __acquires() and __releases() tags
     to the decalarations of debugfs_file_use_data_*() in debugfs.h,
     move the debugfs_srcu declaration from internal.h into debugfs.h.
   - Since the definitions as well as the docstrings of
     debugfs_file_use_data_*() have been moved into file.c,
     there is no need to run DocBook on debugfs.h: do not modify
     Documentation/DocBook/filesystems.tmpl anymore.
   - In the commit message, encourage new users of debugfs to prefer
     DEFINE_DEBUGFS_ATTRIBUTE() and friends over DEFINE_SIMPLE_ATTRIBUTE().
     
Nicolai Stange (2):
  debugfs: prevent access to possibly dead file_operations at file open
  debugfs: prevent access to removed files' private data

 fs/debugfs/file.c       | 252 +++++++++++++++++++++++++++++++++++++++---------
 fs/debugfs/inode.c      |  18 +++-
 fs/debugfs/internal.h   |  23 +++++
 include/linux/debugfs.h |  29 +++++-
 lib/Kconfig.debug       |   1 +
 5 files changed, 273 insertions(+), 50 deletions(-)
 create mode 100644 fs/debugfs/internal.h

-- 
2.7.0

[PATCH v2 1/2] debugfs: prevent access to possibly dead file_operations at file open

[Nicolai Stange]

Nothing prevents a dentry found by path lookup before a return of
__debugfs_remove() to actually get opened after that return. Now, after
the return of __debugfs_remove(), there are no guarantees whatsoever
regarding the memory the corresponding inode's file_operations object
had been kept in.

Since __debugfs_remove() is seldomly invoked, usually from module exit
handlers only, the race is hard to trigger and the impact is very low.

A discussion of the problem outlined above as well as a suggested
solution can be found in the (sub-)thread rooted at

  http://lkml.kernel.org/g/20130401203445.GA20862@ZenIV.linux.org.uk
  ("Yet another pipe related oops.")

Basically, Greg KH suggests to introduce an intermediate fops and
Al Viro points out that a pointer to the original ones may be stored in
->d_fsdata.

Follow this line of reasoning:
- Add SRCU as a reverse dependency of DEBUG_FS.
- Introduce a srcu_struct object for the debugfs subsystem.
- In debugfs_create_file(), store a pointer to the original
  file_operations object in ->d_fsdata.
- In __debugfs_remove(), clear out that dentry->d_fsdata member again.
  debugfs_remove() and debugfs_remove_recursive() wait for a SRCU grace
  period before returning to their caller.
- Introduce an intermediate file_operations object named
  "debugfs_proxy_file_operations". It's ->open() functions checks,
  under the protection of a SRCU read lock, whether the "original"
  file_operations pointed to by ->d_fsdata is still valid and if so,
  tries to acquire a reference on the owning module. On success, it sets
  the file object's ->f_op to the original file_operations and forwards
  the ongoing open() call to the original ->open().
- For clarity, rename the former debugfs_file_operations to
  debugfs_noop_file_operations -- they are in no way canonical.

The choice of SRCU over "normal" RCU is justified by the fact, that the
former may also be used to protect ->i_private data from going away
during the execution of a file's readers and writers which may (and do)
sleep.

Finally, introduce the fs/debugfs/internal.h header containing some
declarations internal to the debugfs implementation.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 fs/debugfs/file.c       | 32 +++++++++++++++++++++++++++++++-
 fs/debugfs/inode.c      | 19 ++++++++++++++++---
 fs/debugfs/internal.h   | 24 ++++++++++++++++++++++++
 include/linux/debugfs.h |  3 ---
 lib/Kconfig.debug       |  1 +
 5 files changed, 72 insertions(+), 7 deletions(-)
 create mode 100644 fs/debugfs/internal.h

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index d2ba12e..35ceae7 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -22,6 +22,9 @@
 #include <linux/slab.h>
 #include <linux/atomic.h>
 #include <linux/device.h>
+#include <linux/srcu.h>
+
+#include "internal.h"
 
 static ssize_t default_read_file(struct file *file, char __user *buf,
 				 size_t count, loff_t *ppos)
@@ -35,13 +38,40 @@ static ssize_t default_write_file(struct file *file, const char __user *buf,
 	return count;
 }
 
-const struct file_operations debugfs_file_operations = {
+const struct file_operations debugfs_noop_file_operations = {
 	.read =		default_read_file,
 	.write =	default_write_file,
 	.open =		simple_open,
 	.llseek =	noop_llseek,
 };
 
+static int proxy_open(struct inode *inode, struct file *filp)
+{
+	struct dentry * const dentry = filp->f_path.dentry;
+	const struct file_operations __rcu *rcu_real_fops;
+	const struct file_operations *real_fops;
+	int srcu_idx;
+
+	srcu_idx = srcu_read_lock(&debugfs_srcu);
+	rcu_real_fops = (const struct file_operations __rcu *)dentry->d_fsdata;
+	real_fops = srcu_dereference(rcu_real_fops, &debugfs_srcu);
+	real_fops = fops_get(real_fops);
+	srcu_read_unlock(&debugfs_srcu, srcu_idx);
+
+	if (!real_fops)
+		return -ENOENT;
+
+	replace_fops(filp, real_fops);
+	if (filp->f_op->open)
+		return filp->f_op->open(inode, filp);
+
+	return 0;
+}
+
+const struct file_operations debugfs_proxy_file_operations = {
+	.open = proxy_open,
+};
+
 static struct dentry *debugfs_create_mode(const char *name, umode_t mode,
 					  struct dentry *parent, void *value,
 				          const struct file_operations *fops,
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index bece948..fbf86cf 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -27,9 +27,14 @@
 #include <linux/parser.h>
 #include <linux/magic.h>
 #include <linux/slab.h>
+#include <linux/srcu.h>
+
+#include "internal.h"
 
 #define DEBUGFS_DEFAULT_MODE	0700
 
+DEFINE_SRCU(debugfs_srcu);
+
 static struct vfsmount *debugfs_mount;
 static int debugfs_mount_count;
 static bool debugfs_registered;
@@ -340,8 +345,12 @@ struct dentry *debugfs_create_file(const char *name, umode_t mode,
 		return failed_creating(dentry);
 
 	inode->i_mode = mode;
-	inode->i_fop = fops ? fops : &debugfs_file_operations;
 	inode->i_private = data;
+
+	inode->i_fop = fops ? &debugfs_proxy_file_operations
+		: &debugfs_noop_file_operations;
+	rcu_assign_pointer(dentry->d_fsdata, (void *)fops);
+
 	d_instantiate(dentry, inode);
 	fsnotify_create(d_inode(dentry->d_parent), dentry);
 	return end_creating(dentry);
@@ -523,10 +532,12 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent)
 
 	if (simple_positive(dentry)) {
 		dget(dentry);
-		if (d_is_dir(dentry))
+		if (d_is_dir(dentry)) {
 			ret = simple_rmdir(d_inode(parent), dentry);
-		else
+		} else {
 			simple_unlink(d_inode(parent), dentry);
+			rcu_assign_pointer(dentry->d_fsdata, NULL);
+		}
 		if (!ret)
 			d_delete(dentry);
 		dput(dentry);
@@ -565,6 +576,7 @@ void debugfs_remove(struct dentry *dentry)
 	inode_unlock(d_inode(parent));
 	if (!ret)
 		simple_release_fs(&debugfs_mount, &debugfs_mount_count);
+	synchronize_srcu(&debugfs_srcu);
 }
 EXPORT_SYMBOL_GPL(debugfs_remove);
 
@@ -642,6 +654,7 @@ void debugfs_remove_recursive(struct dentry *dentry)
 	if (!__debugfs_remove(child, parent))
 		simple_release_fs(&debugfs_mount, &debugfs_mount_count);
 	inode_unlock(d_inode(parent));
+	synchronize_srcu(&debugfs_srcu);
 }
 EXPORT_SYMBOL_GPL(debugfs_remove_recursive);
 
diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h
new file mode 100644
index 0000000..1f87f3c
--- /dev/null
+++ b/fs/debugfs/internal.h
@@ -0,0 +1,24 @@
+/*
+ *  internal.h - declarations internal to debugfs
+ *
+ *  Copyright (C) 2016 Nicolai Stange <nicstange@gmail.com>
+ *
+ *	This program is free software; you can redistribute it and/or
+ *	modify it under the terms of the GNU General Public License version
+ *	2 as published by the Free Software Foundation.
+ *
+ */
+
+#ifndef _DEBUGFS_INTERNAL_H_
+#define _DEBUGFS_INTERNAL_H_
+
+struct file_operations;
+struct srcu_struct;
+
+/* declared over in file.c */
+extern const struct file_operations debugfs_noop_file_operations;
+extern const struct file_operations debugfs_proxy_file_operations;
+
+extern struct srcu_struct debugfs_srcu;
+
+#endif /* _DEBUGFS_INTERNAL_H_ */
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 19c066d..8b44093 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -43,9 +43,6 @@ extern struct dentry *arch_debugfs_dir;
 
 #if defined(CONFIG_DEBUG_FS)
 
-/* declared over in file.c */
-extern const struct file_operations debugfs_file_operations;
-
 struct dentry *debugfs_create_file(const char *name, umode_t mode,
 				   struct dentry *parent, void *data,
 				   const struct file_operations *fops);
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index ecb9e75..080d243 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -257,6 +257,7 @@ config PAGE_OWNER
 
 config DEBUG_FS
 	bool "Debug Filesystem"
+	select SRCU
 	help
 	  debugfs is a virtual file system that kernel developers use to put
 	  debugging files into.  Enable this option to be able to read and
-- 
2.7.0

[PATCH v2 2/2] debugfs: prevent access to removed files' private data

[Nicolai Stange]

Upon return of debugfs_remove()/debugfs_remove_recursive(), it might
still be attempted to access associated private file data through
previously opened struct file objects. If that data has been freed by
the caller of debugfs_remove*() in the meanwhile, the reading/writing
process would either encounter a fault or, if the memory address in
question has been reassigned again, unrelated data structures could get
overwritten.

However, since debugfs files are seldomly removed, usually from module
exit handlers only, the impact is very low.

Since debugfs_remove() and debugfs_remove_recursive() are already
waiting for a SRCU grace period before returning to their callers,
enclosing the access to private file data from ->read() and ->write()
within a SRCU read-side critical section does the trick:
- Introduce the debugfs_file_use_data_start() and
  debugfs_file_use_data_finish() helpers which just enter and leave
  a SRCU read-side critical section. The former also reports whether the
  file is still alive, that is if d_delete() has _not_ been called on
  the corresponding dentry.
- Introduce the DEFINE_DEBUGFS_ATTRIBUTE() macro which is completely
  equivalent to the DEFINE_SIMPLE_ATTRIBUTE() macro except that
  ->read() and ->write are set to SRCU protecting wrappers around the
  original simple_read() and simple_write() helpers.
- Use that DEFINE_DEBUGFS_ATTRIBUTE() macro for all debugfs_create_*()
  attribute creation variants where appropriate.
- Manually introduce SRCU protection to the debugfs-predefined readers
  and writers not covered by the above DEFINE_SIMPLE_ATTRIBUTE()->
  DEFINE_DEBUGFS_ATTRIBUTE() replacement.

Finally, it should be worth to note that in the vast majority of cases
where debugfs users are handing in a "custom" struct file_operations
object to debugfs_create_file(), an attribute's associated data's
lifetime is bound to the one of the containing module and thus,
taking a reference on ->owner during file opening acts as a proxy here.
There is no need to do a mass replace of DEFINE_SIMPLE_ATTRIBUTE() to
DEFINE_DEBUGFS_ATTRIBUTE() outside of debugfs.

OTOH, new users of debugfs are encouraged to prefer the
DEFINE_DEBUGFS_ATTRIBUTE() macro over DEFINE_SIMPLE_ATTRIBUTE() and it,
as well as the needed read/write wrappers are made available globally.
For new users implementing their own readers and writers, the lifetime
management helpers debugfs_file_use_data_start() and
debugfs_file_use_data_finish() are exported.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 fs/debugfs/file.c       | 221 ++++++++++++++++++++++++++++++++++++++----------
 fs/debugfs/internal.h   |   3 -
 include/linux/debugfs.h |  28 ++++++
 3 files changed, 205 insertions(+), 47 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 35ceae7..e6591a8 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -72,6 +72,81 @@ const struct file_operations debugfs_proxy_file_operations = {
 	.open = proxy_open,
 };
 
+/**
+ * debugfs_file_use_data_start - mark the beginning of file data access
+ * @file: the file object whose data is being accessed.
+ * @srcu_idx: a pointer to some memory to store a SRCU index in.
+ *
+ * Up to a matching call to debugfs_file_use_data_finish(), any
+ * successive call into the file removing functions debugfs_remove()
+ * and debugfs_remove_recursive() will block. Since associated private
+ * file data may only get freed after a successful return of any of
+ * the removal functions, you may safely access it after a successful
+ * call to debugfs_file_use_data_start() without worrying about
+ * lifetime issues.
+ *
+ * If -%EIO is returned, the file has already been removed and thus,
+ * it is not safe to access any of its data. If, on the other hand,
+ * it is allowed to access the file data, zero is returned.
+ *
+ * Regardless of the return code, any call to
+ * debugfs_file_use_data_start() must be followed by a matching call
+ * to debugfs_file_use_data_finish().
+ */
+int debugfs_file_use_data_start(struct file *file, int *srcu_idx)
+	__acquires(&debugfs_srcu)
+{
+	*srcu_idx = srcu_read_lock(&debugfs_srcu);
+	if (d_unlinked(file->f_path.dentry))
+		return -EIO;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(debugfs_file_use_data_start);
+
+/**
+ * debugfs_file_use_data_finish - mark the end of file data access
+ * @srcu_idx: the SRCU index "created" by a former call to
+ *            debugfs_file_use_data_start().
+ *
+ * Allow any ongoing concurrent call into debugfs_remove() or
+ * debugfs_remove_recursive() blocked by a former call to
+ * debugfs_file_use_data_start() to proceed and return to its caller.
+ */
+void debugfs_file_use_data_finish(int srcu_idx) __releases(&debugfs_srcu)
+{
+	srcu_read_unlock(&debugfs_srcu, srcu_idx);
+}
+EXPORT_SYMBOL_GPL(debugfs_file_use_data_finish);
+
+ssize_t debugfs_attr_read(struct file *file, char __user *buf,
+			 size_t len, loff_t *ppos)
+{
+	ssize_t ret;
+	int srcu_idx;
+
+	ret = debugfs_file_use_data_start(file, &srcu_idx);
+	if (!ret)
+		ret = simple_attr_read(file, buf, len, ppos);
+	debugfs_file_use_data_finish(srcu_idx);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(debugfs_attr_read);
+
+ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
+			  size_t len, loff_t *ppos)
+{
+	ssize_t ret;
+	int srcu_idx;
+
+	ret = debugfs_file_use_data_start(file, &srcu_idx);
+	if (!ret)
+		ret = simple_attr_write(file, buf, len, ppos);
+	debugfs_file_use_data_finish(srcu_idx);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(debugfs_attr_write);
+
 static struct dentry *debugfs_create_mode(const char *name, umode_t mode,
 					  struct dentry *parent, void *value,
 				          const struct file_operations *fops,
@@ -98,9 +173,9 @@ static int debugfs_u8_get(void *data, u64 *val)
 	*val = *(u8 *)data;
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(fops_u8, debugfs_u8_get, debugfs_u8_set, "%llu\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_u8_ro, debugfs_u8_get, NULL, "%llu\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_u8_wo, NULL, debugfs_u8_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u8, debugfs_u8_get, debugfs_u8_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u8_ro, debugfs_u8_get, NULL, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u8_wo, NULL, debugfs_u8_set, "%llu\n");
 
 /**
  * debugfs_create_u8 - create a debugfs file that is used to read and write an unsigned 8-bit value
@@ -144,9 +219,9 @@ static int debugfs_u16_get(void *data, u64 *val)
 	*val = *(u16 *)data;
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(fops_u16, debugfs_u16_get, debugfs_u16_set, "%llu\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_u16_ro, debugfs_u16_get, NULL, "%llu\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_u16_wo, NULL, debugfs_u16_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u16, debugfs_u16_get, debugfs_u16_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u16_ro, debugfs_u16_get, NULL, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u16_wo, NULL, debugfs_u16_set, "%llu\n");
 
 /**
  * debugfs_create_u16 - create a debugfs file that is used to read and write an unsigned 16-bit value
@@ -190,9 +265,9 @@ static int debugfs_u32_get(void *data, u64 *val)
 	*val = *(u32 *)data;
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(fops_u32, debugfs_u32_get, debugfs_u32_set, "%llu\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_u32_ro, debugfs_u32_get, NULL, "%llu\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_u32_wo, NULL, debugfs_u32_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u32, debugfs_u32_get, debugfs_u32_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u32_ro, debugfs_u32_get, NULL, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u32_wo, NULL, debugfs_u32_set, "%llu\n");
 
 /**
  * debugfs_create_u32 - create a debugfs file that is used to read and write an unsigned 32-bit value
@@ -237,9 +312,9 @@ static int debugfs_u64_get(void *data, u64 *val)
 	*val = *(u64 *)data;
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(fops_u64, debugfs_u64_get, debugfs_u64_set, "%llu\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_u64_ro, debugfs_u64_get, NULL, "%llu\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_u64_wo, NULL, debugfs_u64_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u64, debugfs_u64_get, debugfs_u64_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u64_ro, debugfs_u64_get, NULL, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u64_wo, NULL, debugfs_u64_set, "%llu\n");
 
 /**
  * debugfs_create_u64 - create a debugfs file that is used to read and write an unsigned 64-bit value
@@ -284,9 +359,10 @@ static int debugfs_ulong_get(void *data, u64 *val)
 	*val = *(unsigned long *)data;
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(fops_ulong, debugfs_ulong_get, debugfs_ulong_set, "%llu\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_ulong_ro, debugfs_ulong_get, NULL, "%llu\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_ulong_wo, NULL, debugfs_ulong_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_ulong, debugfs_ulong_get, debugfs_ulong_set,
+			"%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_ulong_ro, debugfs_ulong_get, NULL, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_ulong_wo, NULL, debugfs_ulong_set, "%llu\n");
 
 /**
  * debugfs_create_ulong - create a debugfs file that is used to read and write
@@ -321,21 +397,24 @@ struct dentry *debugfs_create_ulong(const char *name, umode_t mode,
 }
 EXPORT_SYMBOL_GPL(debugfs_create_ulong);
 
-DEFINE_SIMPLE_ATTRIBUTE(fops_x8, debugfs_u8_get, debugfs_u8_set, "0x%02llx\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_x8_ro, debugfs_u8_get, NULL, "0x%02llx\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_x8_wo, NULL, debugfs_u8_set, "0x%02llx\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_x8, debugfs_u8_get, debugfs_u8_set, "0x%02llx\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_x8_ro, debugfs_u8_get, NULL, "0x%02llx\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_x8_wo, NULL, debugfs_u8_set, "0x%02llx\n");
 
-DEFINE_SIMPLE_ATTRIBUTE(fops_x16, debugfs_u16_get, debugfs_u16_set, "0x%04llx\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_x16_ro, debugfs_u16_get, NULL, "0x%04llx\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_x16_wo, NULL, debugfs_u16_set, "0x%04llx\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_x16, debugfs_u16_get, debugfs_u16_set,
+			"0x%04llx\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_x16_ro, debugfs_u16_get, NULL, "0x%04llx\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_x16_wo, NULL, debugfs_u16_set, "0x%04llx\n");
 
-DEFINE_SIMPLE_ATTRIBUTE(fops_x32, debugfs_u32_get, debugfs_u32_set, "0x%08llx\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_x32_ro, debugfs_u32_get, NULL, "0x%08llx\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_x32_wo, NULL, debugfs_u32_set, "0x%08llx\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_x32, debugfs_u32_get, debugfs_u32_set,
+			"0x%08llx\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_x32_ro, debugfs_u32_get, NULL, "0x%08llx\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_x32_wo, NULL, debugfs_u32_set, "0x%08llx\n");
 
-DEFINE_SIMPLE_ATTRIBUTE(fops_x64, debugfs_u64_get, debugfs_u64_set, "0x%016llx\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_x64_ro, debugfs_u64_get, NULL, "0x%016llx\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_x64_wo, NULL, debugfs_u64_set, "0x%016llx\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_x64, debugfs_u64_get, debugfs_u64_set,
+			"0x%016llx\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_x64_ro, debugfs_u64_get, NULL, "0x%016llx\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_x64_wo, NULL, debugfs_u64_set, "0x%016llx\n");
 
 /*
  * debugfs_create_x{8,16,32,64} - create a debugfs file that is used to read and write an unsigned {8,16,32,64}-bit value
@@ -428,10 +507,10 @@ static int debugfs_size_t_get(void *data, u64 *val)
 	*val = *(size_t *)data;
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(fops_size_t, debugfs_size_t_get, debugfs_size_t_set,
+DEFINE_DEBUGFS_ATTRIBUTE(fops_size_t, debugfs_size_t_get, debugfs_size_t_set,
 			"%llu\n");	/* %llu and %zu are more or less the same */
-DEFINE_SIMPLE_ATTRIBUTE(fops_size_t_ro, debugfs_size_t_get, NULL, "%llu\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_size_t_wo, NULL, debugfs_size_t_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_size_t_ro, debugfs_size_t_get, NULL, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_size_t_wo, NULL, debugfs_size_t_set, "%llu\n");
 
 /**
  * debugfs_create_size_t - create a debugfs file that is used to read and write an size_t value
@@ -461,10 +540,12 @@ static int debugfs_atomic_t_get(void *data, u64 *val)
 	*val = atomic_read((atomic_t *)data);
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(fops_atomic_t, debugfs_atomic_t_get,
+DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic_t, debugfs_atomic_t_get,
 			debugfs_atomic_t_set, "%lld\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_atomic_t_ro, debugfs_atomic_t_get, NULL, "%lld\n");
-DEFINE_SIMPLE_ATTRIBUTE(fops_atomic_t_wo, NULL, debugfs_atomic_t_set, "%lld\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic_t_ro, debugfs_atomic_t_get, NULL,
+			"%lld\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic_t_wo, NULL, debugfs_atomic_t_set,
+			"%lld\n");
 
 /**
  * debugfs_create_atomic_t - create a debugfs file that is used to read and
@@ -490,11 +571,18 @@ ssize_t debugfs_read_file_bool(struct file *file, char __user *user_buf,
 {
 	char buf[3];
 	bool *val = file->private_data;
+	int ret, srcu_idx;
 
+	ret = debugfs_file_use_data_start(file, &srcu_idx);
+	if (ret) {
+		debugfs_file_use_data_finish(srcu_idx);
+		return ret;
+	}
 	if (*val)
 		buf[0] = 'Y';
 	else
 		buf[0] = 'N';
+	debugfs_file_use_data_finish(srcu_idx);
 	buf[1] = '\n';
 	buf[2] = 0x00;
 	return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
@@ -508,16 +596,26 @@ ssize_t debugfs_write_file_bool(struct file *file, const char __user *user_buf,
 	size_t buf_size;
 	bool bv;
 	bool *val = file->private_data;
+	ssize_t ret;
+	int srcu_idx;
 
 	buf_size = min(count, (sizeof(buf)-1));
 	if (copy_from_user(buf, user_buf, buf_size))
 		return -EFAULT;
 
 	buf[buf_size] = '\0';
-	if (strtobool(buf, &bv) == 0)
-		*val = bv;
+	if (strtobool(buf, &bv) == 0) {
+		ret = debugfs_file_use_data_start(file, &srcu_idx);
+		if (!ret) {
+			*val = bv;
+			ret = count;
+		}
+		debugfs_file_use_data_finish(srcu_idx);
+	} else {
+		return -EINVAL;
+	}
 
-	return count;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(debugfs_write_file_bool);
 
@@ -575,9 +673,16 @@ EXPORT_SYMBOL_GPL(debugfs_create_bool);
 static ssize_t read_file_blob(struct file *file, char __user *user_buf,
 			      size_t count, loff_t *ppos)
 {
+	ssize_t ret;
+	int srcu_idx;
 	struct debugfs_blob_wrapper *blob = file->private_data;
-	return simple_read_from_buffer(user_buf, count, ppos, blob->data,
-			blob->size);
+
+	ret = debugfs_file_use_data_start(file, &srcu_idx);
+	if (!ret)
+		ret = simple_read_from_buffer(user_buf, count, ppos, blob->data,
+					blob->size);
+	debugfs_file_use_data_finish(srcu_idx);
+	return ret;
 }
 
 static const struct file_operations fops_blob = {
@@ -646,6 +751,7 @@ static int u32_array_open(struct inode *inode, struct file *file)
 	struct array_data *data = inode->i_private;
 	int size, elements = data->elements;
 	char *buf;
+	int ret, srcu_idx;
 
 	/*
 	 * Max size:
@@ -659,9 +765,17 @@ static int u32_array_open(struct inode *inode, struct file *file)
 	buf[size] = 0;
 
 	file->private_data = buf;
+	ret = debugfs_file_use_data_start(file, &srcu_idx);
+	if (ret) {
+		kfree(file->private_data);
+		goto out;
+	}
 	u32_format_array(buf, size, data->array, data->elements);
+	nonseekable_open(inode, file);
 
-	return nonseekable_open(inode, file);
+out:
+	debugfs_file_use_data_finish(srcu_idx);
+	return ret;
 }
 
 static ssize_t u32_array_read(struct file *file, char __user *buf, size_t len,
@@ -764,15 +878,21 @@ EXPORT_SYMBOL_GPL(debugfs_print_regs32);
 
 static int debugfs_show_regset32(struct seq_file *s, void *data)
 {
-	struct debugfs_regset32 *regset = s->private;
+	struct file *f = s->private;
+	struct debugfs_regset32 *regset = file_inode(f)->i_private;
+	int ret, srcu_idx;
 
-	debugfs_print_regs32(s, regset->regs, regset->nregs, regset->base, "");
-	return 0;
+	ret = debugfs_file_use_data_start(f, &srcu_idx);
+	if (!ret)
+		debugfs_print_regs32(s, regset->regs, regset->nregs,
+				regset->base, "");
+	debugfs_file_use_data_finish(srcu_idx);
+	return ret;
 }
 
 static int debugfs_open_regset32(struct inode *inode, struct file *file)
 {
-	return single_open(file, debugfs_show_regset32, inode->i_private);
+	return single_open(file, debugfs_show_regset32, file);
 }
 
 static const struct file_operations fops_regset32 = {
@@ -829,11 +949,24 @@ static int debugfs_devm_entry_open(struct inode *inode, struct file *f)
 	return single_open(f, entry->read, entry->dev);
 }
 
+static ssize_t debugfs_devm_entry_read(struct file *file, char __user *buf,
+				size_t size, loff_t *ppos)
+{
+	ssize_t ret;
+	int srcu_idx;
+
+	ret = debugfs_file_use_data_start(file, &srcu_idx);
+	if (!ret)
+		ret = seq_read(file, buf, size, ppos);
+	debugfs_file_use_data_finish(srcu_idx);
+	return ret;
+}
+
 static const struct file_operations debugfs_devm_entry_ops = {
 	.owner = THIS_MODULE,
 	.open = debugfs_devm_entry_open,
 	.release = single_release,
-	.read = seq_read,
+	.read = debugfs_devm_entry_read,
 	.llseek = seq_lseek
 };
 
diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h
index 1f87f3c..51798ed 100644
--- a/fs/debugfs/internal.h
+++ b/fs/debugfs/internal.h
@@ -13,12 +13,9 @@
 #define _DEBUGFS_INTERNAL_H_
 
 struct file_operations;
-struct srcu_struct;
 
 /* declared over in file.c */
 extern const struct file_operations debugfs_noop_file_operations;
 extern const struct file_operations debugfs_proxy_file_operations;
 
-extern struct srcu_struct debugfs_srcu;
-
 #endif /* _DEBUGFS_INTERNAL_H_ */
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 8b44093..8acd369 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -22,6 +22,7 @@
 
 struct device;
 struct file_operations;
+struct srcu_struct;
 
 struct debugfs_blob_wrapper {
 	void *data;
@@ -43,6 +44,8 @@ extern struct dentry *arch_debugfs_dir;
 
 #if defined(CONFIG_DEBUG_FS)
 
+extern struct srcu_struct debugfs_srcu;
+
 struct dentry *debugfs_create_file(const char *name, umode_t mode,
 				   struct dentry *parent, void *data,
 				   const struct file_operations *fops);
@@ -121,6 +124,31 @@ ssize_t debugfs_read_file_bool(struct file *file, char __user *user_buf,
 ssize_t debugfs_write_file_bool(struct file *file, const char __user *user_buf,
 				size_t count, loff_t *ppos);
 
+int debugfs_file_use_data_start(struct file *file, int *srcu_idx)
+	__acquires(&debugfs_srcu);
+
+void debugfs_file_use_data_finish(int srcu_idx) __releases(&debugfs_srcu);
+
+ssize_t debugfs_attr_read(struct file *file, char __user *buf,
+			 size_t len, loff_t *ppos);
+ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
+			  size_t len, loff_t *ppos);
+
+#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)		\
+static int __fops ## _open(struct inode *inode, struct file *file)	\
+{									\
+	__simple_attr_check_format(__fmt, 0ull);			\
+	return simple_attr_open(inode, file, __get, __set, __fmt);	\
+}									\
+static const struct file_operations __fops = {				\
+	.owner	 = THIS_MODULE,					\
+	.open	 = __fops ## _open,					\
+	.release = simple_attr_release,				\
+	.read	 = debugfs_attr_read,					\
+	.write	 = debugfs_attr_write,					\
+	.llseek	 = generic_file_llseek,			\
+}
+
 #else
 
 #include <linux/err.h>
-- 
2.7.0

Re: [PATCH v2 2/2] debugfs: prevent access to removed files' private data

[Greg Kroah-Hartman]

On Mon, Feb 08, 2016 at 04:03:27PM +0100, Nicolai Stange wrote:
> Upon return of debugfs_remove()/debugfs_remove_recursive(), it might
> still be attempted to access associated private file data through
> previously opened struct file objects. If that data has been freed by
> the caller of debugfs_remove*() in the meanwhile, the reading/writing
> process would either encounter a fault or, if the memory address in
> question has been reassigned again, unrelated data structures could get
> overwritten.
> 
> However, since debugfs files are seldomly removed, usually from module
> exit handlers only, the impact is very low.
> 
> Since debugfs_remove() and debugfs_remove_recursive() are already
> waiting for a SRCU grace period before returning to their callers,
> enclosing the access to private file data from ->read() and ->write()
> within a SRCU read-side critical section does the trick:
> - Introduce the debugfs_file_use_data_start() and
>   debugfs_file_use_data_finish() helpers which just enter and leave
>   a SRCU read-side critical section. The former also reports whether the
>   file is still alive, that is if d_delete() has _not_ been called on
>   the corresponding dentry.
> - Introduce the DEFINE_DEBUGFS_ATTRIBUTE() macro which is completely
>   equivalent to the DEFINE_SIMPLE_ATTRIBUTE() macro except that
>   ->read() and ->write are set to SRCU protecting wrappers around the
>   original simple_read() and simple_write() helpers.
> - Use that DEFINE_DEBUGFS_ATTRIBUTE() macro for all debugfs_create_*()
>   attribute creation variants where appropriate.
> - Manually introduce SRCU protection to the debugfs-predefined readers
>   and writers not covered by the above DEFINE_SIMPLE_ATTRIBUTE()->
>   DEFINE_DEBUGFS_ATTRIBUTE() replacement.
> 
> Finally, it should be worth to note that in the vast majority of cases
> where debugfs users are handing in a "custom" struct file_operations
> object to debugfs_create_file(), an attribute's associated data's
> lifetime is bound to the one of the containing module and thus,
> taking a reference on ->owner during file opening acts as a proxy here.
> There is no need to do a mass replace of DEFINE_SIMPLE_ATTRIBUTE() to
> DEFINE_DEBUGFS_ATTRIBUTE() outside of debugfs.
> 
> OTOH, new users of debugfs are encouraged to prefer the
> DEFINE_DEBUGFS_ATTRIBUTE() macro over DEFINE_SIMPLE_ATTRIBUTE() and it,
> as well as the needed read/write wrappers are made available globally.
> For new users implementing their own readers and writers, the lifetime
> management helpers debugfs_file_use_data_start() and
> debugfs_file_use_data_finish() are exported.

Nice job.  One more request... :)

Can you show how you would convert a subsystem to use these new
macros/calls to give a solid example of it in use outside of the debugfs
core?

thanks,

greg k-h

Re: [PATCH v2 2/2] debugfs: prevent access to removed files' private data

[Nicolai Stange]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Mon, Feb 08, 2016 at 04:03:27PM +0100, Nicolai Stange wrote:
>> Upon return of debugfs_remove()/debugfs_remove_recursive(), it might
>> still be attempted to access associated private file data through
>> previously opened struct file objects. If that data has been freed by
>> the caller of debugfs_remove*() in the meanwhile, the reading/writing
>> process would either encounter a fault or, if the memory address in
>> question has been reassigned again, unrelated data structures could get
>> overwritten.
>> 
>> However, since debugfs files are seldomly removed, usually from module
>> exit handlers only, the impact is very low.
>> 
>> Since debugfs_remove() and debugfs_remove_recursive() are already
>> waiting for a SRCU grace period before returning to their callers,
>> enclosing the access to private file data from ->read() and ->write()
>> within a SRCU read-side critical section does the trick:
>> - Introduce the debugfs_file_use_data_start() and
>>   debugfs_file_use_data_finish() helpers which just enter and leave
>>   a SRCU read-side critical section. The former also reports whether the
>>   file is still alive, that is if d_delete() has _not_ been called on
>>   the corresponding dentry.
>> - Introduce the DEFINE_DEBUGFS_ATTRIBUTE() macro which is completely
>>   equivalent to the DEFINE_SIMPLE_ATTRIBUTE() macro except that
>>   ->read() and ->write are set to SRCU protecting wrappers around the
>>   original simple_read() and simple_write() helpers.
>> - Use that DEFINE_DEBUGFS_ATTRIBUTE() macro for all debugfs_create_*()
>>   attribute creation variants where appropriate.
>> - Manually introduce SRCU protection to the debugfs-predefined readers
>>   and writers not covered by the above DEFINE_SIMPLE_ATTRIBUTE()->
>>   DEFINE_DEBUGFS_ATTRIBUTE() replacement.
>> 
>> Finally, it should be worth to note that in the vast majority of cases
>> where debugfs users are handing in a "custom" struct file_operations
>> object to debugfs_create_file(), an attribute's associated data's
>> lifetime is bound to the one of the containing module and thus,
>> taking a reference on ->owner during file opening acts as a proxy here.
>> There is no need to do a mass replace of DEFINE_SIMPLE_ATTRIBUTE() to
>> DEFINE_DEBUGFS_ATTRIBUTE() outside of debugfs.
>> 
>> OTOH, new users of debugfs are encouraged to prefer the
>> DEFINE_DEBUGFS_ATTRIBUTE() macro over DEFINE_SIMPLE_ATTRIBUTE() and it,
>> as well as the needed read/write wrappers are made available globally.
>> For new users implementing their own readers and writers, the lifetime
>> management helpers debugfs_file_use_data_start() and
>> debugfs_file_use_data_finish() are exported.
>
> Nice job.  One more request... :)
>
> Can you show how you would convert a subsystem to use these new
> macros/calls to give a solid example of it in use outside of the debugfs
> core?

You mean in the form of a patch [3/3] for an arbitrary subsystem other
than debugfs? Or in the form of an update of
Documentation/filesystems/debugfs.txt?

In case you want to have a patch: for the DEFINE_DEBUGFS_ATTRIBUTE, I
could simply abuse
  drivers/staging/android/ion/ion.c
as it has got a DEFINE_SIMPLE_ATTRIBUTE debug_shrink_fops passed to
debugfs. In this particular case, it even looks like that this debugfs
file can be removed through ion_client_destroy() without any module
removal. Fixing this would be as easy as
s/DEFINE_SIMPLE_ATTRIBUTE/DEFINE_DEBUGFS_ATTRIBUTE/.

Regarding a use case with custom made file_operations whose
reader and writer are protected by the debugfs_file_use_data_*()
helpers, I'm a little bit at a loss with: ion.c has got its custom
'debug_heap_fops', but in this case, it would probably be more
appropriate to create a general debugfs_create_seqfile() centrally in
debugfs.

Thank you!

Nicolai

Re: [PATCH v2 2/2] debugfs: prevent access to removed files' private data

[Greg Kroah-Hartman]

On Mon, Feb 08, 2016 at 06:14:58PM +0100, Nicolai Stange wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Mon, Feb 08, 2016 at 04:03:27PM +0100, Nicolai Stange wrote:
> >> Upon return of debugfs_remove()/debugfs_remove_recursive(), it might
> >> still be attempted to access associated private file data through
> >> previously opened struct file objects. If that data has been freed by
> >> the caller of debugfs_remove*() in the meanwhile, the reading/writing
> >> process would either encounter a fault or, if the memory address in
> >> question has been reassigned again, unrelated data structures could get
> >> overwritten.
> >> 
> >> However, since debugfs files are seldomly removed, usually from module
> >> exit handlers only, the impact is very low.
> >> 
> >> Since debugfs_remove() and debugfs_remove_recursive() are already
> >> waiting for a SRCU grace period before returning to their callers,
> >> enclosing the access to private file data from ->read() and ->write()
> >> within a SRCU read-side critical section does the trick:
> >> - Introduce the debugfs_file_use_data_start() and
> >>   debugfs_file_use_data_finish() helpers which just enter and leave
> >>   a SRCU read-side critical section. The former also reports whether the
> >>   file is still alive, that is if d_delete() has _not_ been called on
> >>   the corresponding dentry.
> >> - Introduce the DEFINE_DEBUGFS_ATTRIBUTE() macro which is completely
> >>   equivalent to the DEFINE_SIMPLE_ATTRIBUTE() macro except that
> >>   ->read() and ->write are set to SRCU protecting wrappers around the
> >>   original simple_read() and simple_write() helpers.
> >> - Use that DEFINE_DEBUGFS_ATTRIBUTE() macro for all debugfs_create_*()
> >>   attribute creation variants where appropriate.
> >> - Manually introduce SRCU protection to the debugfs-predefined readers
> >>   and writers not covered by the above DEFINE_SIMPLE_ATTRIBUTE()->
> >>   DEFINE_DEBUGFS_ATTRIBUTE() replacement.
> >> 
> >> Finally, it should be worth to note that in the vast majority of cases
> >> where debugfs users are handing in a "custom" struct file_operations
> >> object to debugfs_create_file(), an attribute's associated data's
> >> lifetime is bound to the one of the containing module and thus,
> >> taking a reference on ->owner during file opening acts as a proxy here.
> >> There is no need to do a mass replace of DEFINE_SIMPLE_ATTRIBUTE() to
> >> DEFINE_DEBUGFS_ATTRIBUTE() outside of debugfs.
> >> 
> >> OTOH, new users of debugfs are encouraged to prefer the
> >> DEFINE_DEBUGFS_ATTRIBUTE() macro over DEFINE_SIMPLE_ATTRIBUTE() and it,
> >> as well as the needed read/write wrappers are made available globally.
> >> For new users implementing their own readers and writers, the lifetime
> >> management helpers debugfs_file_use_data_start() and
> >> debugfs_file_use_data_finish() are exported.
> >
> > Nice job.  One more request... :)
> >
> > Can you show how you would convert a subsystem to use these new
> > macros/calls to give a solid example of it in use outside of the debugfs
> > core?
> 
> You mean in the form of a patch [3/3] for an arbitrary subsystem other
> than debugfs? Or in the form of an update of
> Documentation/filesystems/debugfs.txt?

For an arbritary subsystem would be great.  Showing how this should be
used / converted tree-wide.

> In case you want to have a patch: for the DEFINE_DEBUGFS_ATTRIBUTE, I
> could simply abuse
>   drivers/staging/android/ion/ion.c
> as it has got a DEFINE_SIMPLE_ATTRIBUTE debug_shrink_fops passed to
> debugfs. In this particular case, it even looks like that this debugfs
> file can be removed through ion_client_destroy() without any module
> removal. Fixing this would be as easy as
> s/DEFINE_SIMPLE_ATTRIBUTE/DEFINE_DEBUGFS_ATTRIBUTE/.

Great, why wouldn't we do that for all users of debugfs that have this
type of interaction with it?

> Regarding a use case with custom made file_operations whose
> reader and writer are protected by the debugfs_file_use_data_*()
> helpers, I'm a little bit at a loss with: ion.c has got its custom
> 'debug_heap_fops', but in this case, it would probably be more
> appropriate to create a general debugfs_create_seqfile() centrally in
> debugfs.

ion is 'rough', but if enough people use seqfile in debugfs, yes, we
should provide a generic interface for it to make it easier to use so
they don't have to roll their own, and so they get the fixes you did
here for their code as well.

thanks,

greg k-h

Re: [PATCH v2 2/2] debugfs: prevent access to removed files' private data

[Nicolai Stange]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Mon, Feb 08, 2016 at 06:14:58PM +0100, Nicolai Stange wrote:
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> 
>> > On Mon, Feb 08, 2016 at 04:03:27PM +0100, Nicolai Stange wrote:
>> >> Upon return of debugfs_remove()/debugfs_remove_recursive(), it might
>> >> still be attempted to access associated private file data through
>> >> previously opened struct file objects. If that data has been freed by
>> >> the caller of debugfs_remove*() in the meanwhile, the reading/writing
>> >> process would either encounter a fault or, if the memory address in
>> >> question has been reassigned again, unrelated data structures could get
>> >> overwritten.
>> >> 
>> >> However, since debugfs files are seldomly removed, usually from module
>> >> exit handlers only, the impact is very low.
>> >> 
>> >> Since debugfs_remove() and debugfs_remove_recursive() are already
>> >> waiting for a SRCU grace period before returning to their callers,
>> >> enclosing the access to private file data from ->read() and ->write()
>> >> within a SRCU read-side critical section does the trick:
>> >> - Introduce the debugfs_file_use_data_start() and
>> >>   debugfs_file_use_data_finish() helpers which just enter and leave
>> >>   a SRCU read-side critical section. The former also reports whether the
>> >>   file is still alive, that is if d_delete() has _not_ been called on
>> >>   the corresponding dentry.
>> >> - Introduce the DEFINE_DEBUGFS_ATTRIBUTE() macro which is completely
>> >>   equivalent to the DEFINE_SIMPLE_ATTRIBUTE() macro except that
>> >>   ->read() and ->write are set to SRCU protecting wrappers around the
>> >>   original simple_read() and simple_write() helpers.
>> >> - Use that DEFINE_DEBUGFS_ATTRIBUTE() macro for all debugfs_create_*()
>> >>   attribute creation variants where appropriate.
>> >> - Manually introduce SRCU protection to the debugfs-predefined readers
>> >>   and writers not covered by the above DEFINE_SIMPLE_ATTRIBUTE()->
>> >>   DEFINE_DEBUGFS_ATTRIBUTE() replacement.
>> >> 
>> >> Finally, it should be worth to note that in the vast majority of cases
>> >> where debugfs users are handing in a "custom" struct file_operations
>> >> object to debugfs_create_file(), an attribute's associated data's
>> >> lifetime is bound to the one of the containing module and thus,
>> >> taking a reference on ->owner during file opening acts as a proxy here.
>> >> There is no need to do a mass replace of DEFINE_SIMPLE_ATTRIBUTE() to
>> >> DEFINE_DEBUGFS_ATTRIBUTE() outside of debugfs.
>> >> 
>> >> OTOH, new users of debugfs are encouraged to prefer the
>> >> DEFINE_DEBUGFS_ATTRIBUTE() macro over DEFINE_SIMPLE_ATTRIBUTE() and it,
>> >> as well as the needed read/write wrappers are made available globally.
>> >> For new users implementing their own readers and writers, the lifetime
>> >> management helpers debugfs_file_use_data_start() and
>> >> debugfs_file_use_data_finish() are exported.
>> >
>> > Nice job.  One more request... :)
>> >
>> > Can you show how you would convert a subsystem to use these new
>> > macros/calls to give a solid example of it in use outside of the debugfs
>> > core?
>> 
>> You mean in the form of a patch [3/3] for an arbitrary subsystem other
>> than debugfs? Or in the form of an update of
>> Documentation/filesystems/debugfs.txt?
>
> For an arbritary subsystem would be great.  Showing how this should be
> used / converted tree-wide.
>
>> In case you want to have a patch: for the DEFINE_DEBUGFS_ATTRIBUTE, I
>> could simply abuse
>>   drivers/staging/android/ion/ion.c
>> as it has got a DEFINE_SIMPLE_ATTRIBUTE debug_shrink_fops passed to
>> debugfs. In this particular case, it even looks like that this debugfs
>> file can be removed through ion_client_destroy() without any module
>> removal. Fixing this would be as easy as
>> s/DEFINE_SIMPLE_ATTRIBUTE/DEFINE_DEBUGFS_ATTRIBUTE/.
>
> Great, why wouldn't we do that for all users of debugfs that have this
> type of interaction with it?

So this is a "yes", I should include these kind of fixes within this
series as [3/X], [4/X], ..., [X/X]?

Last time I checked the tree (Nov.), there weren't any users of this
kind (debugfs file removal w/o module unload).
Obviously I missed ion though... I will recheck.

>
>> Regarding a use case with custom made file_operations whose
>> reader and writer are protected by the debugfs_file_use_data_*()
>> helpers, I'm a little bit at a loss with: ion.c has got its custom
>> 'debug_heap_fops', but in this case, it would probably be more
>> appropriate to create a general debugfs_create_seqfile() centrally in
>> debugfs.
>
> ion is 'rough', but if enough people use seqfile in debugfs, yes, we
> should provide a generic interface for it to make it easier to use so
> they don't have to roll their own, and so they get the fixes you did
> here for their code as well.

A quick check revealed that there are *many* such seqfile users.

Since these would all get touched, I think it is better to postpone the
introduction of a debugfs_create_seqfile() to another series dedicated
to that?

Thank you,

Nicolai

Re: [PATCH v2 2/2] debugfs: prevent access to removed files' private data

[Greg Kroah-Hartman]

On Mon, Feb 08, 2016 at 09:00:05PM +0100, Nicolai Stange wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Mon, Feb 08, 2016 at 06:14:58PM +0100, Nicolai Stange wrote:
> >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> >> 
> >> > On Mon, Feb 08, 2016 at 04:03:27PM +0100, Nicolai Stange wrote:
> >> >> Upon return of debugfs_remove()/debugfs_remove_recursive(), it might
> >> >> still be attempted to access associated private file data through
> >> >> previously opened struct file objects. If that data has been freed by
> >> >> the caller of debugfs_remove*() in the meanwhile, the reading/writing
> >> >> process would either encounter a fault or, if the memory address in
> >> >> question has been reassigned again, unrelated data structures could get
> >> >> overwritten.
> >> >> 
> >> >> However, since debugfs files are seldomly removed, usually from module
> >> >> exit handlers only, the impact is very low.
> >> >> 
> >> >> Since debugfs_remove() and debugfs_remove_recursive() are already
> >> >> waiting for a SRCU grace period before returning to their callers,
> >> >> enclosing the access to private file data from ->read() and ->write()
> >> >> within a SRCU read-side critical section does the trick:
> >> >> - Introduce the debugfs_file_use_data_start() and
> >> >>   debugfs_file_use_data_finish() helpers which just enter and leave
> >> >>   a SRCU read-side critical section. The former also reports whether the
> >> >>   file is still alive, that is if d_delete() has _not_ been called on
> >> >>   the corresponding dentry.
> >> >> - Introduce the DEFINE_DEBUGFS_ATTRIBUTE() macro which is completely
> >> >>   equivalent to the DEFINE_SIMPLE_ATTRIBUTE() macro except that
> >> >>   ->read() and ->write are set to SRCU protecting wrappers around the
> >> >>   original simple_read() and simple_write() helpers.
> >> >> - Use that DEFINE_DEBUGFS_ATTRIBUTE() macro for all debugfs_create_*()
> >> >>   attribute creation variants where appropriate.
> >> >> - Manually introduce SRCU protection to the debugfs-predefined readers
> >> >>   and writers not covered by the above DEFINE_SIMPLE_ATTRIBUTE()->
> >> >>   DEFINE_DEBUGFS_ATTRIBUTE() replacement.
> >> >> 
> >> >> Finally, it should be worth to note that in the vast majority of cases
> >> >> where debugfs users are handing in a "custom" struct file_operations
> >> >> object to debugfs_create_file(), an attribute's associated data's
> >> >> lifetime is bound to the one of the containing module and thus,
> >> >> taking a reference on ->owner during file opening acts as a proxy here.
> >> >> There is no need to do a mass replace of DEFINE_SIMPLE_ATTRIBUTE() to
> >> >> DEFINE_DEBUGFS_ATTRIBUTE() outside of debugfs.
> >> >> 
> >> >> OTOH, new users of debugfs are encouraged to prefer the
> >> >> DEFINE_DEBUGFS_ATTRIBUTE() macro over DEFINE_SIMPLE_ATTRIBUTE() and it,
> >> >> as well as the needed read/write wrappers are made available globally.
> >> >> For new users implementing their own readers and writers, the lifetime
> >> >> management helpers debugfs_file_use_data_start() and
> >> >> debugfs_file_use_data_finish() are exported.
> >> >
> >> > Nice job.  One more request... :)
> >> >
> >> > Can you show how you would convert a subsystem to use these new
> >> > macros/calls to give a solid example of it in use outside of the debugfs
> >> > core?
> >> 
> >> You mean in the form of a patch [3/3] for an arbitrary subsystem other
> >> than debugfs? Or in the form of an update of
> >> Documentation/filesystems/debugfs.txt?
> >
> > For an arbritary subsystem would be great.  Showing how this should be
> > used / converted tree-wide.
> >
> >> In case you want to have a patch: for the DEFINE_DEBUGFS_ATTRIBUTE, I
> >> could simply abuse
> >>   drivers/staging/android/ion/ion.c
> >> as it has got a DEFINE_SIMPLE_ATTRIBUTE debug_shrink_fops passed to
> >> debugfs. In this particular case, it even looks like that this debugfs
> >> file can be removed through ion_client_destroy() without any module
> >> removal. Fixing this would be as easy as
> >> s/DEFINE_SIMPLE_ATTRIBUTE/DEFINE_DEBUGFS_ATTRIBUTE/.
> >
> > Great, why wouldn't we do that for all users of debugfs that have this
> > type of interaction with it?
> 
> So this is a "yes", I should include these kind of fixes within this
> series as [3/X], [4/X], ..., [X/X]?

Yes please.

> Last time I checked the tree (Nov.), there weren't any users of this
> kind (debugfs file removal w/o module unload).
> Obviously I missed ion though... I will recheck.
> 
> >
> >> Regarding a use case with custom made file_operations whose
> >> reader and writer are protected by the debugfs_file_use_data_*()
> >> helpers, I'm a little bit at a loss with: ion.c has got its custom
> >> 'debug_heap_fops', but in this case, it would probably be more
> >> appropriate to create a general debugfs_create_seqfile() centrally in
> >> debugfs.
> >
> > ion is 'rough', but if enough people use seqfile in debugfs, yes, we
> > should provide a generic interface for it to make it easier to use so
> > they don't have to roll their own, and so they get the fixes you did
> > here for their code as well.
> 
> A quick check revealed that there are *many* such seqfile users.
> 
> Since these would all get touched, I think it is better to postpone the
> introduction of a debugfs_create_seqfile() to another series dedicated
> to that?

Yes that would be a good idea.

thanks,

greg k-h

Re: [PATCH v2 2/2] debugfs: prevent access to removed files' private data

[Nicolai Stange]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Mon, Feb 08, 2016 at 09:00:05PM +0100, Nicolai Stange wrote:
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> 
>> > On Mon, Feb 08, 2016 at 06:14:58PM +0100, Nicolai Stange wrote:
>> >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> >> 
>> >> > On Mon, Feb 08, 2016 at 04:03:27PM +0100, Nicolai Stange wrote:
>> >> >> Upon return of debugfs_remove()/debugfs_remove_recursive(), it might
>> >> >> still be attempted to access associated private file data through
>> >> >> previously opened struct file objects. If that data has been freed by
>> >> >> the caller of debugfs_remove*() in the meanwhile, the reading/writing
>> >> >> process would either encounter a fault or, if the memory address in
>> >> >> question has been reassigned again, unrelated data structures could get
>> >> >> overwritten.
>> >> >> 
>> >> >> However, since debugfs files are seldomly removed, usually from module
>> >> >> exit handlers only, the impact is very low.
>> >> >> 
>> >> >> Since debugfs_remove() and debugfs_remove_recursive() are already
>> >> >> waiting for a SRCU grace period before returning to their callers,
>> >> >> enclosing the access to private file data from ->read() and ->write()
>> >> >> within a SRCU read-side critical section does the trick:
>> >> >> - Introduce the debugfs_file_use_data_start() and
>> >> >>   debugfs_file_use_data_finish() helpers which just enter and leave
>> >> >>   a SRCU read-side critical section. The former also reports whether the
>> >> >>   file is still alive, that is if d_delete() has _not_ been called on
>> >> >>   the corresponding dentry.
>> >> >> - Introduce the DEFINE_DEBUGFS_ATTRIBUTE() macro which is completely
>> >> >>   equivalent to the DEFINE_SIMPLE_ATTRIBUTE() macro except that
>> >> >>   ->read() and ->write are set to SRCU protecting wrappers around the
>> >> >>   original simple_read() and simple_write() helpers.
>> >> >> - Use that DEFINE_DEBUGFS_ATTRIBUTE() macro for all debugfs_create_*()
>> >> >>   attribute creation variants where appropriate.
>> >> >> - Manually introduce SRCU protection to the debugfs-predefined readers
>> >> >>   and writers not covered by the above DEFINE_SIMPLE_ATTRIBUTE()->
>> >> >>   DEFINE_DEBUGFS_ATTRIBUTE() replacement.
>> >> >> 
>> >> >> Finally, it should be worth to note that in the vast majority of cases
>> >> >> where debugfs users are handing in a "custom" struct file_operations
>> >> >> object to debugfs_create_file(), an attribute's associated data's
>> >> >> lifetime is bound to the one of the containing module and thus,
>> >> >> taking a reference on ->owner during file opening acts as a proxy here.
>> >> >> There is no need to do a mass replace of DEFINE_SIMPLE_ATTRIBUTE() to
>> >> >> DEFINE_DEBUGFS_ATTRIBUTE() outside of debugfs.
>> >> >> 
>> >> >> OTOH, new users of debugfs are encouraged to prefer the
>> >> >> DEFINE_DEBUGFS_ATTRIBUTE() macro over DEFINE_SIMPLE_ATTRIBUTE() and it,
>> >> >> as well as the needed read/write wrappers are made available globally.
>> >> >> For new users implementing their own readers and writers, the lifetime
>> >> >> management helpers debugfs_file_use_data_start() and
>> >> >> debugfs_file_use_data_finish() are exported.
>> >> >
>> >> > Nice job.  One more request... :)
>> >> >
>> >> > Can you show how you would convert a subsystem to use these new
>> >> > macros/calls to give a solid example of it in use outside of the debugfs
>> >> > core?
>> >> 
>> >> You mean in the form of a patch [3/3] for an arbitrary subsystem other
>> >> than debugfs? Or in the form of an update of
>> >> Documentation/filesystems/debugfs.txt?
>> >
>> > For an arbritary subsystem would be great.  Showing how this should be
>> > used / converted tree-wide.
>> >
>> >> In case you want to have a patch: for the DEFINE_DEBUGFS_ATTRIBUTE, I
>> >> could simply abuse
>> >>   drivers/staging/android/ion/ion.c
>> >> as it has got a DEFINE_SIMPLE_ATTRIBUTE debug_shrink_fops passed to
>> >> debugfs. In this particular case, it even looks like that this debugfs
>> >> file can be removed through ion_client_destroy() without any module
>> >> removal. Fixing this would be as easy as
>> >> s/DEFINE_SIMPLE_ATTRIBUTE/DEFINE_DEBUGFS_ATTRIBUTE/.
>> >
>> > Great, why wouldn't we do that for all users of debugfs that have this
>> > type of interaction with it?
>> 
>> So this is a "yes", I should include these kind of fixes within this
>> series as [3/X], [4/X], ..., [X/X]?
>
> Yes please.
>
>> Last time I checked the tree (Nov.), there weren't any users of this
>> kind (debugfs file removal w/o module unload).
>> Obviously I missed ion though... I will recheck.

Uhm, I must have been drinking back then.

The real statistics are:
There are 101 DEFINE_SIMPE_ATTRIBUTE fops handed to debugfs.
>From these, 49 suffer from "dynamic" debugfs file removal races.
These 49 are distributed over the following 15 files:

  drivers/gpu/drm/i915/i915_debugfs.c
  drivers/hwmon/asus_atk0110.c
  drivers/iio/gyro/adis16136.c
  drivers/iio/imu/adis16400_core.c
  drivers/iio/imu/adis16480.c
  drivers/input/touchscreen/edt-ft5x06.c
  drivers/misc/cxl/debugfs.c
  drivers/mmc/core/debugfs.c
  drivers/net/wimax/i2400m/debugfs.c
  drivers/net/wireless/ath/wil6210/debugfs.c
  drivers/net/wireless/mac80211_hwsim.c
  fs/ceph/debugfs.c
  fs/ocfs2/blockcheck.c
  lib/fault-inject.c
  net/bluetooth/hci_debugfs.c

Details of my manual analysis can be found at
http://nicst.de/debugfs_dsa_users.html


Now, a quick grep shows that there are 1002 call sites of
debugfs_create_file() in total. Minus 101 yields 901 users of debugfs
who hand in custom file_operations (assuming a 1:1 relationship between
DEFINE_SIMPLE_ATTRIBUTE() and debugfs_create_file(), which mostly holds).

Another 342 of these custom file_operations hand-ins are of the seqfile
style, i.e. have their ->read set to seq_read().
(A coccinelle patch for finding these 342 is available at
 http://nicst.de/debugfs_seq.cocci
 Run spatch with '-D org' or '-D report')

Migrating those to a debugfs_create_seqfile() might be good in terms of
both, avoiding the removal race and refactorization in general.
If only they were not so many...


However, there are still 901-342=559 debugfs_create_file() invocations I
haven't even looked at yet. No clue how safe these are against removal
races.



Before I proceed with the current approach, let me note that there might
be an alternative that would not require modification of each and every
debugfs user:
We could rather not replace the ->f_op in debugfs' proxy_open() (from
this series) with the original one, but instead keep an "extended" proxy
there forever.
This extended proxy would simply proxy every file_operations method to
the original one, under protection of the SRCU lock. However, the
question is what to do with file_operation methods that are NULL in the
original. Probably, -ENOTSUPP most of the time.


Now it's up to you to decide which way to go: mass patching throughout
the tree or the full proxy?

>> 
>> >
>> >> Regarding a use case with custom made file_operations whose
>> >> reader and writer are protected by the debugfs_file_use_data_*()
>> >> helpers, I'm a little bit at a loss with: ion.c has got its custom
>> >> 'debug_heap_fops', but in this case, it would probably be more
>> >> appropriate to create a general debugfs_create_seqfile() centrally in
>> >> debugfs.
>> >
>> > ion is 'rough', but if enough people use seqfile in debugfs, yes, we
>> > should provide a generic interface for it to make it easier to use so
>> > they don't have to roll their own, and so they get the fixes you did
>> > here for their code as well.
>> 
>> A quick check revealed that there are *many* such seqfile users.
>> 
>> Since these would all get touched, I think it is better to postpone the
>> introduction of a debugfs_create_seqfile() to another series dedicated
>> to that?
>
> Yes that would be a good idea.
>
> thanks,
>
> greg k-h

Re: [PATCH v2 2/2] debugfs: prevent access to removed files' private data

[Greg Kroah-Hartman]

On Tue, Feb 09, 2016 at 11:03:49PM +0100, Nicolai Stange wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> >> Last time I checked the tree (Nov.), there weren't any users of this
> >> kind (debugfs file removal w/o module unload).
> >> Obviously I missed ion though... I will recheck.
> 
> Uhm, I must have been drinking back then.

I too want to drink when dealing with debugfs removal issues :)

> The real statistics are:
> There are 101 DEFINE_SIMPE_ATTRIBUTE fops handed to debugfs.
> From these, 49 suffer from "dynamic" debugfs file removal races.
> These 49 are distributed over the following 15 files:
> 
>   drivers/gpu/drm/i915/i915_debugfs.c
>   drivers/hwmon/asus_atk0110.c
>   drivers/iio/gyro/adis16136.c
>   drivers/iio/imu/adis16400_core.c
>   drivers/iio/imu/adis16480.c
>   drivers/input/touchscreen/edt-ft5x06.c
>   drivers/misc/cxl/debugfs.c
>   drivers/mmc/core/debugfs.c
>   drivers/net/wimax/i2400m/debugfs.c
>   drivers/net/wireless/ath/wil6210/debugfs.c
>   drivers/net/wireless/mac80211_hwsim.c
>   fs/ceph/debugfs.c
>   fs/ocfs2/blockcheck.c
>   lib/fault-inject.c
>   net/bluetooth/hci_debugfs.c
> 
> Details of my manual analysis can be found at
> http://nicst.de/debugfs_dsa_users.html

Nice summary.

> Now, a quick grep shows that there are 1002 call sites of
> debugfs_create_file() in total. Minus 101 yields 901 users of debugfs
> who hand in custom file_operations (assuming a 1:1 relationship between
> DEFINE_SIMPLE_ATTRIBUTE() and debugfs_create_file(), which mostly holds).

Ok, those will be easy to fix.

> Another 342 of these custom file_operations hand-ins are of the seqfile
> style, i.e. have their ->read set to seq_read().
> (A coccinelle patch for finding these 342 is available at
>  http://nicst.de/debugfs_seq.cocci
>  Run spatch with '-D org' or '-D report')
> 
> Migrating those to a debugfs_create_seqfile() might be good in terms of
> both, avoiding the removal race and refactorization in general.
> If only they were not so many...

That's what .cocci scripts are for :)

> However, there are still 901-342=559 debugfs_create_file() invocations I
> haven't even looked at yet. No clue how safe these are against removal
> races.

Ugh, that seems really high.  I found one such example in
sound/soc/soc-dapm.c and it should be simple to convert to use a seqfile
operation instead, so maybe the number of overall problems are smaller
than we think.  Will take a bunch of auditing to ensure it though.

> Before I proceed with the current approach, let me note that there might
> be an alternative that would not require modification of each and every
> debugfs user:
> We could rather not replace the ->f_op in debugfs' proxy_open() (from
> this series) with the original one, but instead keep an "extended" proxy
> there forever.
> This extended proxy would simply proxy every file_operations method to
> the original one, under protection of the SRCU lock.

Oh, that would be nice, and solve the issue for almost everyone right
away.

> However, the question is what to do with file_operation methods that
> are NULL in the original. Probably, -ENOTSUPP most of the time.

Yes, that should be fine.

> Now it's up to you to decide which way to go: mass patching throughout
> the tree or the full proxy?

Try the "full proxy" way and see how it goes, that might make this
simpler than touching every user in the tree, which would be good to not
have to do unless necessary.

thanks again for looking into this,

greg k-h