Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1624232 > unrolled thread

[RFC PATCH 0/9] debugfs: per-file removal protection

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC PATCH 0/9] debugfs: per-file removal protection Nicolai Stange <nicstange@gmail.com> - 2017-04-16 12:00 +0200
    [RFC PATCH 4/9] debugfs: convert to debugfs_file_get() and -put() Nicolai Stange <nicstange@gmail.com> - 2017-04-16 12:00 +0200
    [RFC PATCH 2/9] debugfs: implement per-file removal protection Nicolai Stange <nicstange@gmail.com> - 2017-04-16 12:00 +0200
    [RFC PATCH 5/9] IB/hfi1: convert to debugfs_file_get() and -put() Nicolai Stange <nicstange@gmail.com> - 2017-04-16 12:00 +0200
    [RFC PATCH 8/9] debugfs: defer debugfs_fsdata allocation to first usage Nicolai Stange <nicstange@gmail.com> - 2017-04-16 12:00 +0200
      Re: [RFC PATCH 8/9] debugfs: defer debugfs_fsdata allocation to  first usage Johannes Berg <johannes@sipsolutions.net> - 2017-04-18 11:40 +0200
    [RFC PATCH 6/9] debugfs: purge obsolete SRCU based removal protection Nicolai Stange <nicstange@gmail.com> - 2017-04-16 12:00 +0200
    [RFC PATCH 9/9] debugfs: free debugfs_fsdata instances Nicolai Stange <nicstange@gmail.com> - 2017-04-16 12:00 +0200
      Re: [RFC PATCH 9/9] debugfs: free debugfs_fsdata instances "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-04-17 18:10 +0200
        Re: [RFC PATCH 9/9] debugfs: free debugfs_fsdata instances Johannes Berg <johannes@sipsolutions.net> - 2017-04-18 11:50 +0200
          Re: [RFC PATCH 9/9] debugfs: free debugfs_fsdata instances "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-04-18 15:40 +0200
            Re: [RFC PATCH 9/9] debugfs: free debugfs_fsdata instances Johannes Berg <johannes@sipsolutions.net> - 2017-04-18 15:50 +0200
              Re: [RFC PATCH 9/9] debugfs: free debugfs_fsdata instances "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-04-18 17:20 +0200
                Re: [RFC PATCH 9/9] debugfs: free debugfs_fsdata instances Johannes Berg <johannes@sipsolutions.net> - 2017-04-18 17:30 +0200
                  Re: [RFC PATCH 9/9] debugfs: free debugfs_fsdata instances "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-04-18 19:20 +0200
    [RFC PATCH 1/9] debugfs: add support for more elaborate ->d_fsdata Nicolai Stange <nicstange@gmail.com> - 2017-04-16 12:00 +0200
    [RFC PATCH 3/9] debugfs: debugfs_real_fops(): drop __must_hold sparse annotation Nicolai Stange <nicstange@gmail.com> - 2017-04-16 12:00 +0200
    [RFC PATCH 7/9] debugfs: call debugfs_real_fops() only after debugfs_file_get() Nicolai Stange <nicstange@gmail.com> - 2017-04-16 12:00 +0200

#1624232 — [RFC PATCH 0/9] debugfs: per-file removal protection

Hello Greg,

this series implements the debugfs removal protection at file granularity,
meant to solve the livelock issue reported by Johannes [1]:

  Task 1               Task 2
  mutex_lock(&m);
                       debugfs_use_file_start(&d2, ...);
  debugfs_remove(d1);  mutex_lock(&m);

with d1 != d2.

In order to be able to store the additionally required per-dentry state,
a small container struct, debugfs_fsdata, will be allocated and installed
at ->d_fsdata.

The remaining question is if and how these debugfs_fsdata instances should
be freed. For one possible solution, please see
[9/9] ("debugfs: free debugfs_fsdata instances"). As stated in that patch's
description, I'm not convinced that it's a particularly good one -- it's
included only to show how a lock-free/RCU based scheme would look like.
In order to avoid frequent allocations and deallocations, I'd personally
not free the debugfs_fsdata instances at all: there would be exactly one
per debugfs file ever opened, which probably isn't too much. If you don't
agree, I can try and see how a simpler solution based on a global spinlock
would look like... Any advice on how to proceed with this welcome!


Thanks,

Nicolai

[1] http://lkml.kernel.org/r/1490280886.2766.4.camel@sipsolutions.net


Nicolai Stange (9):
  debugfs: add support for more elaborate ->d_fsdata
  debugfs: implement per-file removal protection
  debugfs: debugfs_real_fops(): drop __must_hold sparse annotation
  debugfs: convert to debugfs_file_get() and -put()
  IB/hfi1: convert to debugfs_file_get() and -put()
  debugfs: purge obsolete SRCU based removal protection
  debugfs: call debugfs_real_fops() only after debugfs_file_get()
  debugfs: defer debugfs_fsdata allocation to first usage
  debugfs: free debugfs_fsdata instances

 drivers/infiniband/hw/hfi1/debugfs.c |  20 +--
 fs/debugfs/file.c                    | 272 +++++++++++++++++++++++------------
 fs/debugfs/inode.c                   |  55 +++++--
 fs/debugfs/internal.h                |  15 ++
 include/linux/debugfs.h              |  33 +----
 lib/Kconfig.debug                    |   1 -
 6 files changed, 256 insertions(+), 140 deletions(-)

-- 
2.12.2

[toc] | [next] | [standalone]

#1624233 — [RFC PATCH 4/9] debugfs: convert to debugfs_file_get() and -put()

Convert all calls to the now obsolete debugfs_use_file_start() and
debugfs_use_file_finish() from the debugfs core itself to the new
debugfs_file_get() and debugfs_file_put() API.

Fixes: 49d200deaa68 ("debugfs: prevent access to removed files' private
                      data")
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 fs/debugfs/file.c | 106 ++++++++++++++++++++++++++----------------------------
 1 file changed, 50 insertions(+), 56 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 3bc3e2e69f80..ebf5b2faf91c 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -155,15 +155,12 @@ EXPORT_SYMBOL_GPL(debugfs_file_put);
 
 static int open_proxy_open(struct inode *inode, struct file *filp)
 {
-	const struct dentry *dentry = F_DENTRY(filp);
+	struct dentry *dentry = F_DENTRY(filp);
 	const struct file_operations *real_fops = NULL;
-	int srcu_idx, r;
+	int r = 0;
 
-	r = debugfs_use_file_start(dentry, &srcu_idx);
-	if (r) {
-		r = -ENOENT;
-		goto out;
-	}
+	if (debugfs_file_get(dentry))
+		return -ENOENT;
 
 	real_fops = debugfs_real_fops(filp);
 	real_fops = fops_get(real_fops);
@@ -180,7 +177,7 @@ static int open_proxy_open(struct inode *inode, struct file *filp)
 		r = real_fops->open(inode, filp);
 
 out:
-	debugfs_use_file_finish(srcu_idx);
+	debugfs_file_put(dentry);
 	return r;
 }
 
@@ -194,16 +191,16 @@ const struct file_operations debugfs_open_proxy_file_operations = {
 #define FULL_PROXY_FUNC(name, ret_type, filp, proto, args)		\
 static ret_type full_proxy_ ## name(proto)				\
 {									\
-	const struct dentry *dentry = F_DENTRY(filp);			\
+	struct dentry *dentry = F_DENTRY(filp);			\
 	const struct file_operations *real_fops =			\
 		debugfs_real_fops(filp);				\
-	int srcu_idx;							\
 	ret_type r;							\
 									\
-	r = debugfs_use_file_start(dentry, &srcu_idx);			\
-	if (likely(!r))						\
-		r = real_fops->name(args);				\
-	debugfs_use_file_finish(srcu_idx);				\
+	r = debugfs_file_get(dentry);					\
+	if (unlikely(r))						\
+		return r;						\
+	r = real_fops->name(args);					\
+	debugfs_file_put(dentry);					\
 	return r;							\
 }
 
@@ -228,18 +225,15 @@ FULL_PROXY_FUNC(unlocked_ioctl, long, filp,
 static unsigned int full_proxy_poll(struct file *filp,
 				struct poll_table_struct *wait)
 {
-	const struct dentry *dentry = F_DENTRY(filp);
 	const struct file_operations *real_fops = debugfs_real_fops(filp);
-	int srcu_idx;
+	struct dentry *dentry = F_DENTRY(filp);
 	unsigned int r = 0;
 
-	if (debugfs_use_file_start(dentry, &srcu_idx)) {
-		debugfs_use_file_finish(srcu_idx);
+	if (debugfs_file_get(dentry))
 		return POLLHUP;
-	}
 
 	r = real_fops->poll(filp, wait);
-	debugfs_use_file_finish(srcu_idx);
+	debugfs_file_put(dentry);
 	return r;
 }
 
@@ -283,16 +277,13 @@ static void __full_proxy_fops_init(struct file_operations *proxy_fops,
 
 static int full_proxy_open(struct inode *inode, struct file *filp)
 {
-	const struct dentry *dentry = F_DENTRY(filp);
+	struct dentry *dentry = F_DENTRY(filp);
 	const struct file_operations *real_fops = NULL;
 	struct file_operations *proxy_fops = NULL;
-	int srcu_idx, r;
+	int r = 0;
 
-	r = debugfs_use_file_start(dentry, &srcu_idx);
-	if (r) {
-		r = -ENOENT;
-		goto out;
-	}
+	if (debugfs_file_get(dentry))
+		return -ENOENT;
 
 	real_fops = debugfs_real_fops(filp);
 	real_fops = fops_get(real_fops);
@@ -330,7 +321,7 @@ static int full_proxy_open(struct inode *inode, struct file *filp)
 	kfree(proxy_fops);
 	fops_put(real_fops);
 out:
-	debugfs_use_file_finish(srcu_idx);
+	debugfs_file_put(dentry);
 	return r;
 }
 
@@ -341,13 +332,14 @@ const struct file_operations debugfs_full_proxy_file_operations = {
 ssize_t debugfs_attr_read(struct file *file, char __user *buf,
 			size_t len, loff_t *ppos)
 {
+	struct dentry *dentry = F_DENTRY(file);
 	ssize_t ret;
-	int srcu_idx;
 
-	ret = debugfs_use_file_start(F_DENTRY(file), &srcu_idx);
-	if (likely(!ret))
-		ret = simple_attr_read(file, buf, len, ppos);
-	debugfs_use_file_finish(srcu_idx);
+	ret = debugfs_file_get(dentry);
+	if (unlikely(ret))
+		return ret;
+	ret = simple_attr_read(file, buf, len, ppos);
+	debugfs_file_put(dentry);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(debugfs_attr_read);
@@ -355,13 +347,14 @@ EXPORT_SYMBOL_GPL(debugfs_attr_read);
 ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
 			 size_t len, loff_t *ppos)
 {
+	struct dentry *dentry = F_DENTRY(file);
 	ssize_t ret;
-	int srcu_idx;
 
-	ret = debugfs_use_file_start(F_DENTRY(file), &srcu_idx);
-	if (likely(!ret))
-		ret = simple_attr_write(file, buf, len, ppos);
-	debugfs_use_file_finish(srcu_idx);
+	ret = debugfs_file_get(dentry);
+	if (unlikely(ret))
+		return ret;
+	ret = simple_attr_write(file, buf, len, ppos);
+	debugfs_file_put(dentry);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(debugfs_attr_write);
@@ -795,14 +788,14 @@ ssize_t debugfs_read_file_bool(struct file *file, char __user *user_buf,
 {
 	char buf[3];
 	bool val;
-	int r, srcu_idx;
+	int r;
+	struct dentry *dentry = F_DENTRY(file);
 
-	r = debugfs_use_file_start(F_DENTRY(file), &srcu_idx);
-	if (likely(!r))
-		val = *(bool *)file->private_data;
-	debugfs_use_file_finish(srcu_idx);
-	if (r)
+	r = debugfs_file_get(dentry);
+	if (unlikely(r))
 		return r;
+	val = *(bool *)file->private_data;
+	debugfs_file_put(dentry);
 
 	if (val)
 		buf[0] = 'Y';
@@ -820,8 +813,9 @@ ssize_t debugfs_write_file_bool(struct file *file, const char __user *user_buf,
 	char buf[32];
 	size_t buf_size;
 	bool bv;
-	int r, srcu_idx;
+	int r;
 	bool *val = file->private_data;
+	struct dentry *dentry = F_DENTRY(file);
 
 	buf_size = min(count, (sizeof(buf)-1));
 	if (copy_from_user(buf, user_buf, buf_size))
@@ -829,12 +823,11 @@ ssize_t debugfs_write_file_bool(struct file *file, const char __user *user_buf,
 
 	buf[buf_size] = '\0';
 	if (strtobool(buf, &bv) == 0) {
-		r = debugfs_use_file_start(F_DENTRY(file), &srcu_idx);
-		if (likely(!r))
-			*val = bv;
-		debugfs_use_file_finish(srcu_idx);
-		if (r)
+		r = debugfs_file_get(dentry);
+		if (unlikely(r))
 			return r;
+		*val = bv;
+		debugfs_file_put(dentry);
 	}
 
 	return count;
@@ -896,14 +889,15 @@ static ssize_t read_file_blob(struct file *file, char __user *user_buf,
 			      size_t count, loff_t *ppos)
 {
 	struct debugfs_blob_wrapper *blob = file->private_data;
+	struct dentry *dentry = F_DENTRY(file);
 	ssize_t r;
-	int srcu_idx;
 
-	r = debugfs_use_file_start(F_DENTRY(file), &srcu_idx);
-	if (likely(!r))
-		r = simple_read_from_buffer(user_buf, count, ppos, blob->data,
-					blob->size);
-	debugfs_use_file_finish(srcu_idx);
+	r = debugfs_file_get(dentry);
+	if (unlikely(r))
+		return r;
+	r = simple_read_from_buffer(user_buf, count, ppos, blob->data,
+				blob->size);
+	debugfs_file_put(dentry);
 	return r;
 }
 
-- 
2.12.2

[toc] | [prev] | [next] | [standalone]

#1624234 — [RFC PATCH 2/9] debugfs: implement per-file removal protection

Since commit 49d200deaa68 ("debugfs: prevent access to removed files'
private data"), accesses to a file's private data are protected from
concurrent removal by covering all file_operations with a SRCU read section
and sychronizing with those before returning from debugfs_remove() by means
of synchronize_srcu().

As pointed out by Johannes Berg, there are debugfs files with forever
blocking file_operations. Their corresponding SRCU read side sections would
block any debugfs_remove() forever as well, even unrelated ones. This
results in a livelock. Because a remover can't cancel any indefinite
blocking within foreign files, this is a problem.

Resolve this by introducing support for more granular protection on a
per-file basis.

This is implemented by introducing an  'active_users' refcount_t to the
per-file struct debugfs_fsdata state. At file creation time, it is set to
one and a debugfs_remove() will drop that initial reference. The new
debugfs_file_get() and debugfs_file_put(), intended to be used in place of
former debugfs_use_file_start() and debugfs_use_file_finish(), increment
and decrement it respectively. Once the count drops to zero,
debugfs_file_put() will signal a completion which is possibly being waited
for from debugfs_remove().
Thus, as long as there is a debugfs_file_get() not yet matched by a
corresponding debugfs_file_put() around, debugfs_remove() will block.

Actual users of debugfs_use_file_start() and -finish() will get converted
to the new debugfs_file_get() and debugfs_file_put() by followup patches.

Fixes: 49d200deaa68 ("debugfs: prevent access to removed files' private
                      data")
Reported-by: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 fs/debugfs/file.c       | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 fs/debugfs/inode.c      | 24 +++++++++++++++++++-----
 fs/debugfs/internal.h   |  2 ++
 include/linux/debugfs.h | 11 +++++++++++
 4 files changed, 80 insertions(+), 5 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 15655a1a0704..081d74d390a6 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -109,6 +109,54 @@ const struct file_operations *debugfs_real_fops(const struct file *filp)
 }
 EXPORT_SYMBOL_GPL(debugfs_real_fops);
 
+/**
+ * debugfs_file_get - mark the beginning of file data access
+ * @dentry: the dentry object whose data is being accessed.
+ *
+ * Up to a matching call to debugfs_file_put(), 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_get() 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.
+ */
+int debugfs_file_get(struct dentry *dentry)
+{
+	struct debugfs_fsdata *fsd = dentry->d_fsdata;
+
+	/* Avoid starvation of removers. */
+	if (d_unlinked(dentry))
+		return -EIO;
+
+	if (!refcount_inc_not_zero(&fsd->active_users))
+		return -EIO;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(debugfs_file_get);
+
+/**
+ * debugfs_file_put - mark the end of file data access
+ * @dentry: the dentry object formerly passed to
+ *          debugfs_file_get().
+ *
+ * Allow any ongoing concurrent call into debugfs_remove() or
+ * debugfs_remove_recursive() blocked by a former call to
+ * debugfs_file_get() to proceed and return to its caller.
+ */
+void debugfs_file_put(struct dentry *dentry)
+{
+	struct debugfs_fsdata *fsd = dentry->d_fsdata;
+
+	if (refcount_dec_and_test(&fsd->active_users))
+		complete(&fsd->active_users_drained);
+}
+EXPORT_SYMBOL_GPL(debugfs_file_put);
+
 static int open_proxy_open(struct inode *inode, struct file *filp)
 {
 	const struct dentry *dentry = F_DENTRY(filp);
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 1ae133064ca1..88d51e666c38 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -376,6 +376,7 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
 
 	inode->i_fop = proxy_fops;
 	fsd->real_fops = real_fops;
+	refcount_set(&fsd->active_users, 1);
 	dentry->d_fsdata = fsd;
 
 	d_instantiate(dentry, inode);
@@ -633,18 +634,31 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
 }
 EXPORT_SYMBOL_GPL(debugfs_create_symlink);
 
+static void __debugfs_remove_file(struct dentry *dentry, struct dentry *parent)
+{
+	struct debugfs_fsdata *fsd;
+
+	simple_unlink(d_inode(parent), dentry);
+	d_delete(dentry);
+	fsd = dentry->d_fsdata;
+	init_completion(&fsd->active_users_drained);
+	if (!refcount_dec_and_test(&fsd->active_users))
+		wait_for_completion(&fsd->active_users_drained);
+}
+
 static int __debugfs_remove(struct dentry *dentry, struct dentry *parent)
 {
 	int ret = 0;
 
 	if (simple_positive(dentry)) {
 		dget(dentry);
-		if (d_is_dir(dentry))
+		if (d_is_dir(dentry)) {
 			ret = simple_rmdir(d_inode(parent), dentry);
-		else
-			simple_unlink(d_inode(parent), dentry);
-		if (!ret)
-			d_delete(dentry);
+			if (!ret)
+				d_delete(dentry);
+		} else {
+			__debugfs_remove_file(dentry, parent);
+		}
 		dput(dentry);
 	}
 	return ret;
diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h
index 512601eed3ce..0eea99432840 100644
--- a/fs/debugfs/internal.h
+++ b/fs/debugfs/internal.h
@@ -21,6 +21,8 @@ extern const struct file_operations debugfs_full_proxy_file_operations;
 
 struct debugfs_fsdata {
 	const struct file_operations *real_fops;
+	refcount_t active_users;
+	struct completion active_users_drained;
 };
 
 #endif /* _DEBUGFS_INTERNAL_H_ */
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index d614be21412a..d1f1104c41ee 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -98,6 +98,9 @@ void debugfs_use_file_finish(int srcu_idx) __releases(&debugfs_srcu);
 const struct file_operations *debugfs_real_fops(const struct file *filp)
 	__must_hold(&debugfs_srcu);
 
+int debugfs_file_get(struct dentry *dentry);
+void debugfs_file_put(struct dentry *dentry);
+
 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,
@@ -228,6 +231,14 @@ static inline void debugfs_use_file_finish(int srcu_idx)
 	__releases(&debugfs_srcu)
 { }
 
+static inline int debugfs_file_get(struct dentry *dentry)
+{
+	return 0;
+}
+
+static inline void debugfs_file_put(struct dentry *dentry)
+{ }
+
 static inline ssize_t debugfs_attr_read(struct file *file, char __user *buf,
 					size_t len, loff_t *ppos)
 {
-- 
2.12.2

[toc] | [prev] | [next] | [standalone]

#1624235 — [RFC PATCH 5/9] IB/hfi1: convert to debugfs_file_get() and -put()

Convert all calls to the now obsolete debugfs_use_file_start() and
debugfs_use_file_finish() to the new debugfs_file_get() and
debugfs_file_put() API.

Fixes: 49d200deaa68 ("debugfs: prevent access to removed files' private
                      data")
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 drivers/infiniband/hw/hfi1/debugfs.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/debugfs.c b/drivers/infiniband/hw/hfi1/debugfs.c
index dc2c1c993f04..ec78fe678179 100644
--- a/drivers/infiniband/hw/hfi1/debugfs.c
+++ b/drivers/infiniband/hw/hfi1/debugfs.c
@@ -72,13 +72,13 @@ static ssize_t hfi1_seq_read(
 	loff_t *ppos)
 {
 	struct dentry *d = file->f_path.dentry;
-	int srcu_idx;
 	ssize_t r;
 
-	r = debugfs_use_file_start(d, &srcu_idx);
-	if (likely(!r))
-		r = seq_read(file, buf, size, ppos);
-	debugfs_use_file_finish(srcu_idx);
+	r = debugfs_file_get(d);
+	if (unlikely(r))
+		return r;
+	r = seq_read(file, buf, size, ppos);
+	debugfs_file_put(d);
 	return r;
 }
 
@@ -88,13 +88,13 @@ static loff_t hfi1_seq_lseek(
 	int whence)
 {
 	struct dentry *d = file->f_path.dentry;
-	int srcu_idx;
 	loff_t r;
 
-	r = debugfs_use_file_start(d, &srcu_idx);
-	if (likely(!r))
-		r = seq_lseek(file, offset, whence);
-	debugfs_use_file_finish(srcu_idx);
+	r = debugfs_file_get(d);
+	if (unlikely(r))
+		return r;
+	r = seq_lseek(file, offset, whence);
+	debugfs_file_put(d);
 	return r;
 }
 
-- 
2.12.2

[toc] | [prev] | [next] | [standalone]

#1624236 — [RFC PATCH 8/9] debugfs: defer debugfs_fsdata allocation to first usage

Currently, __debugfs_create_file allocates one struct debugfs_fsdata
instance for every file created. However, there are potentially many
debugfs file around, most of which are never touched by userspace.

Thus, defer the allocations to the first usage, i.e. to the first
debugfs_file_get().

A dentry's ->d_fsdata starts out to point to the "real", user provided
fops. After a debugfs_fsdata instance has been allocated (and the real
fops pointer has been moved over into its ->real_fops member),
->d_fsdata is changed to point to it from then on. The two cases are
distinguished by setting BIT(0) for the real fops case.

struct debugfs_fsdata's foremost purpose is to track active users and to
make debugfs_remove() block until they are done. Since no debugfs_fsdata
instance means no active users, make debugfs_remove() return immediately
in this case.

Take care of possible races between debugfs_file_get() and
debugfs_remove(): either debugfs_remove() must see a debugfs_fsdata
instance and thus wait for possible active users or debugfs_file_get() must
see a dead dentry and return immediately.

Make a dentry's ->d_release(), i.e. debugfs_release_dentry(), check whether
->d_fsdata is actually a debugfs_fsdata instance before kfree()ing it.

Finally, the set of possible error codes returned from debugfs_file_get()
has grown from -EIO to -EIO and -ENOMEM. Make open_proxy_open() and
full_proxy_open() pass the -ENOMEM onwards to their callers.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 fs/debugfs/file.c     | 47 ++++++++++++++++++++++++++++++++++++++---------
 fs/debugfs/inode.c    | 36 +++++++++++++++++++-----------------
 fs/debugfs/internal.h |  8 ++++++++
 3 files changed, 65 insertions(+), 26 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index d92038c5f131..f4dfd7d0d625 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -53,6 +53,7 @@ const struct file_operations *debugfs_real_fops(const struct file *filp)
 {
 	struct debugfs_fsdata *fsd = F_DENTRY(filp)->d_fsdata;
 
+	WARN_ON((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
 	return fsd->real_fops;
 }
 EXPORT_SYMBOL_GPL(debugfs_real_fops);
@@ -74,9 +75,35 @@ EXPORT_SYMBOL_GPL(debugfs_real_fops);
  */
 int debugfs_file_get(struct dentry *dentry)
 {
-	struct debugfs_fsdata *fsd = dentry->d_fsdata;
+	struct debugfs_fsdata *fsd;
+	void *d_fsd;
+
+	d_fsd = READ_ONCE(dentry->d_fsdata);
+	if (!((unsigned long)d_fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) {
+		fsd = d_fsd;
+	} else {
+		fsd = kmalloc(sizeof(*fsd), GFP_KERNEL);
+		if (!fsd)
+			return -ENOMEM;
+
+		fsd->real_fops = (void *)((unsigned long)d_fsd &
+					~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
+		refcount_set(&fsd->active_users, 1);
+		init_completion(&fsd->active_users_drained);
+		if (cmpxchg(&dentry->d_fsdata, d_fsd, fsd) != d_fsd) {
+			kfree(fsd);
+			fsd = READ_ONCE(dentry->d_fsdata);
+		}
+	}
 
-	/* Avoid starvation of removers. */
+	/*
+	 * In case of a successful cmpxchg() above, this check is
+	 * strictly necessary and must follow it, see the comment in
+	 * __debugfs_remove_file().
+	 * OTOH, if the cmpxchg() hasn't been executed or wasn't
+	 * successful, this serves the purpose of not starving
+	 * removers.
+	 */
 	if (d_unlinked(dentry))
 		return -EIO;
 
@@ -98,7 +125,7 @@ EXPORT_SYMBOL_GPL(debugfs_file_get);
  */
 void debugfs_file_put(struct dentry *dentry)
 {
-	struct debugfs_fsdata *fsd = dentry->d_fsdata;
+	struct debugfs_fsdata *fsd = READ_ONCE(dentry->d_fsdata);
 
 	if (refcount_dec_and_test(&fsd->active_users))
 		complete(&fsd->active_users_drained);
@@ -109,10 +136,11 @@ static int open_proxy_open(struct inode *inode, struct file *filp)
 {
 	struct dentry *dentry = F_DENTRY(filp);
 	const struct file_operations *real_fops = NULL;
-	int r = 0;
+	int r;
 
-	if (debugfs_file_get(dentry))
-		return -ENOENT;
+	r = debugfs_file_get(dentry);
+	if (r)
+		return r == -EIO ? -ENOENT : r;
 
 	real_fops = debugfs_real_fops(filp);
 	real_fops = fops_get(real_fops);
@@ -233,10 +261,11 @@ static int full_proxy_open(struct inode *inode, struct file *filp)
 	struct dentry *dentry = F_DENTRY(filp);
 	const struct file_operations *real_fops = NULL;
 	struct file_operations *proxy_fops = NULL;
-	int r = 0;
+	int r;
 
-	if (debugfs_file_get(dentry))
-		return -ENOENT;
+	r = debugfs_file_get(dentry);
+	if (r)
+		return r == -EIO ? -ENOENT : r;
 
 	real_fops = debugfs_real_fops(filp);
 	real_fops = fops_get(real_fops);
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 5550f11d60bd..2360c17ec00a 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -184,7 +184,10 @@ static const struct super_operations debugfs_super_operations = {
 
 static void debugfs_release_dentry(struct dentry *dentry)
 {
-	kfree(dentry->d_fsdata);
+	void *fsd = dentry->d_fsdata;
+
+	if (!((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT))
+		kfree(dentry->d_fsdata);
 }
 
 static struct vfsmount *debugfs_automount(struct path *path)
@@ -346,35 +349,25 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
 {
 	struct dentry *dentry;
 	struct inode *inode;
-	struct debugfs_fsdata *fsd;
-
-	fsd = kmalloc(sizeof(*fsd), GFP_KERNEL);
-	if (!fsd)
-		return NULL;
 
 	if (!(mode & S_IFMT))
 		mode |= S_IFREG;
 	BUG_ON(!S_ISREG(mode));
 	dentry = start_creating(name, parent);
 
-	if (IS_ERR(dentry)) {
-		kfree(fsd);
+	if (IS_ERR(dentry))
 		return NULL;
-	}
 
 	inode = debugfs_get_inode(dentry->d_sb);
-	if (unlikely(!inode)) {
-		kfree(fsd);
+	if (unlikely(!inode))
 		return failed_creating(dentry);
-	}
 
 	inode->i_mode = mode;
 	inode->i_private = data;
 
 	inode->i_fop = proxy_fops;
-	fsd->real_fops = real_fops;
-	refcount_set(&fsd->active_users, 1);
-	dentry->d_fsdata = fsd;
+	dentry->d_fsdata = (void *)((unsigned long)real_fops |
+				DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
 
 	d_instantiate(dentry, inode);
 	fsnotify_create(d_inode(dentry->d_parent), dentry);
@@ -637,8 +630,17 @@ static void __debugfs_remove_file(struct dentry *dentry, struct dentry *parent)
 
 	simple_unlink(d_inode(parent), dentry);
 	d_delete(dentry);
-	fsd = dentry->d_fsdata;
-	init_completion(&fsd->active_users_drained);
+
+	/*
+	 * Paired with the closing smp_mb() implied by a successful
+	 * cmpxchg() in debugfs_file_get(): either
+	 * debugfs_file_get() must see a dead dentry or we must see a
+	 * debugfs_fsdata instance at ->d_fsdata here (or both).
+	 */
+	smp_mb();
+	fsd = READ_ONCE(dentry->d_fsdata);
+	if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)
+		return;
 	if (!refcount_dec_and_test(&fsd->active_users))
 		wait_for_completion(&fsd->active_users_drained);
 }
diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h
index 0eea99432840..cb1e8139c398 100644
--- a/fs/debugfs/internal.h
+++ b/fs/debugfs/internal.h
@@ -25,4 +25,12 @@ struct debugfs_fsdata {
 	struct completion active_users_drained;
 };
 
+/*
+ * A dentry's ->d_fsdata either points to the real fops or to a
+ * dynamically allocated debugfs_fsdata instance.
+ * In order to distinguish between these two cases, a real fops
+ * pointer gets its lowest bit set.
+ */
+#define DEBUGFS_FSDATA_IS_REAL_FOPS_BIT BIT(0)
+
 #endif /* _DEBUGFS_INTERNAL_H_ */
-- 
2.12.2

[toc] | [prev] | [next] | [standalone]

#1625179 — Re: [RFC PATCH 8/9] debugfs: defer debugfs_fsdata allocation to first usage

On Sun, 2017-04-16 at 11:51 +0200, Nicolai Stange wrote:
> 
> +++ b/fs/debugfs/file.c
> @@ -53,6 +53,7 @@ const struct file_operations
> *debugfs_real_fops(const struct file *filp)
>  {
>  	struct debugfs_fsdata *fsd = F_DENTRY(filp)->d_fsdata;
>  
> +	WARN_ON((unsigned long)fsd &
> DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
>  	return fsd->real_fops;

I'm not a fan of BUG_ON(), but in this case, if you have a completely
bogus pointer here, and then you return fsd->real_fops which will be
even more bogus, and *then* you call a function from within it... that
seems like a recipe for disaster.

So either you could return some valid ops (perhaps
debugfs_noop_file_operations although those don't have .name or .poll,
so it doesn't cover everything), or you can just BUG_ON() here
directly, saving the incomprehensible crash later.

johannes

>  EXPORT_SYMBOL_GPL(debugfs_real_fops);
> @@ -74,9 +75,35 @@ EXPORT_SYMBOL_GPL(debugfs_real_fops);
>   */
>  int debugfs_file_get(struct dentry *dentry)
>  {
> -	struct debugfs_fsdata *fsd = dentry->d_fsdata;
> +	struct debugfs_fsdata *fsd;
> +	void *d_fsd;
> +
> +	d_fsd = READ_ONCE(dentry->d_fsdata);
> +	if (!((unsigned long)d_fsd &
> DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) {
> +		fsd = d_fsd;
> +	} else {
> +		fsd = kmalloc(sizeof(*fsd), GFP_KERNEL);
> +		if (!fsd)
> +			return -ENOMEM;
> +
> +		fsd->real_fops = (void *)((unsigned long)d_fsd &
> +					~DEBUGFS_FSDATA_IS_REAL_FOPS
> _BIT);
> +		refcount_set(&fsd->active_users, 1);
> +		init_completion(&fsd->active_users_drained);
> +		if (cmpxchg(&dentry->d_fsdata, d_fsd, fsd) != d_fsd)
> {
> +			kfree(fsd);
> +			fsd = READ_ONCE(dentry->d_fsdata);
> +		}
> +	}
>  
> -	/* Avoid starvation of removers. */
> +	/*
> +	 * In case of a successful cmpxchg() above, this check is
> +	 * strictly necessary and must follow it, see the comment in
> +	 * __debugfs_remove_file().
> +	 * OTOH, if the cmpxchg() hasn't been executed or wasn't
> +	 * successful, this serves the purpose of not starving
> +	 * removers.
> +	 */
>  	if (d_unlinked(dentry))
>  		return -EIO;
>  
> @@ -98,7 +125,7 @@ EXPORT_SYMBOL_GPL(debugfs_file_get);
>   */
>  void debugfs_file_put(struct dentry *dentry)
>  {
> -	struct debugfs_fsdata *fsd = dentry->d_fsdata;
> +	struct debugfs_fsdata *fsd = READ_ONCE(dentry->d_fsdata);
>  
>  	if (refcount_dec_and_test(&fsd->active_users))
>  		complete(&fsd->active_users_drained);
> @@ -109,10 +136,11 @@ static int open_proxy_open(struct inode *inode,
> struct file *filp)
>  {
>  	struct dentry *dentry = F_DENTRY(filp);
>  	const struct file_operations *real_fops = NULL;
> -	int r = 0;
> +	int r;
>  
> -	if (debugfs_file_get(dentry))
> -		return -ENOENT;
> +	r = debugfs_file_get(dentry);
> +	if (r)
> +		return r == -EIO ? -ENOENT : r;
>  
>  	real_fops = debugfs_real_fops(filp);
>  	real_fops = fops_get(real_fops);
> @@ -233,10 +261,11 @@ static int full_proxy_open(struct inode *inode,
> struct file *filp)
>  	struct dentry *dentry = F_DENTRY(filp);
>  	const struct file_operations *real_fops = NULL;
>  	struct file_operations *proxy_fops = NULL;
> -	int r = 0;
> +	int r;
>  
> -	if (debugfs_file_get(dentry))
> -		return -ENOENT;
> +	r = debugfs_file_get(dentry);
> +	if (r)
> +		return r == -EIO ? -ENOENT : r;
>  
>  	real_fops = debugfs_real_fops(filp);
>  	real_fops = fops_get(real_fops);
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 5550f11d60bd..2360c17ec00a 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -184,7 +184,10 @@ static const struct super_operations
> debugfs_super_operations = {
>  
>  static void debugfs_release_dentry(struct dentry *dentry)
>  {
> -	kfree(dentry->d_fsdata);
> +	void *fsd = dentry->d_fsdata;
> +
> +	if (!((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT))
> +		kfree(dentry->d_fsdata);
>  }
>  
>  static struct vfsmount *debugfs_automount(struct path *path)
> @@ -346,35 +349,25 @@ static struct dentry
> *__debugfs_create_file(const char *name, umode_t mode,
>  {
>  	struct dentry *dentry;
>  	struct inode *inode;
> -	struct debugfs_fsdata *fsd;
> -
> -	fsd = kmalloc(sizeof(*fsd), GFP_KERNEL);
> -	if (!fsd)
> -		return NULL;
>  
>  	if (!(mode & S_IFMT))
>  		mode |= S_IFREG;
>  	BUG_ON(!S_ISREG(mode));
>  	dentry = start_creating(name, parent);
>  
> -	if (IS_ERR(dentry)) {
> -		kfree(fsd);
> +	if (IS_ERR(dentry))
>  		return NULL;
> -	}
>  
>  	inode = debugfs_get_inode(dentry->d_sb);
> -	if (unlikely(!inode)) {
> -		kfree(fsd);
> +	if (unlikely(!inode))
>  		return failed_creating(dentry);
> -	}
>  
>  	inode->i_mode = mode;
>  	inode->i_private = data;
>  
>  	inode->i_fop = proxy_fops;
> -	fsd->real_fops = real_fops;
> -	refcount_set(&fsd->active_users, 1);
> -	dentry->d_fsdata = fsd;
> +	dentry->d_fsdata = (void *)((unsigned long)real_fops |
> +				DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
>  
>  	d_instantiate(dentry, inode);
>  	fsnotify_create(d_inode(dentry->d_parent), dentry);
> @@ -637,8 +630,17 @@ static void __debugfs_remove_file(struct dentry
> *dentry, struct dentry *parent)
>  
>  	simple_unlink(d_inode(parent), dentry);
>  	d_delete(dentry);
> -	fsd = dentry->d_fsdata;
> -	init_completion(&fsd->active_users_drained);
> +
> +	/*
> +	 * Paired with the closing smp_mb() implied by a successful
> +	 * cmpxchg() in debugfs_file_get(): either
> +	 * debugfs_file_get() must see a dead dentry or we must see
> a
> +	 * debugfs_fsdata instance at ->d_fsdata here (or both).
> +	 */
> +	smp_mb();
> +	fsd = READ_ONCE(dentry->d_fsdata);
> +	if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)
> +		return;
>  	if (!refcount_dec_and_test(&fsd->active_users))
>  		wait_for_completion(&fsd->active_users_drained);
>  }
> diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h
> index 0eea99432840..cb1e8139c398 100644
> --- a/fs/debugfs/internal.h
> +++ b/fs/debugfs/internal.h
> @@ -25,4 +25,12 @@ struct debugfs_fsdata {
>  	struct completion active_users_drained;
>  };
>  
> +/*
> + * A dentry's ->d_fsdata either points to the real fops or to a
> + * dynamically allocated debugfs_fsdata instance.
> + * In order to distinguish between these two cases, a real fops
> + * pointer gets its lowest bit set.
> + */
> +#define DEBUGFS_FSDATA_IS_REAL_FOPS_BIT BIT(0)
> +
>  #endif /* _DEBUGFS_INTERNAL_H_ */

[toc] | [prev] | [next] | [standalone]

#1624237 — [RFC PATCH 6/9] debugfs: purge obsolete SRCU based removal protection

Purge the SRCU based file removal race protection in favour of the new,
refcount based debugfs_file_get()/debugfs_file_put() API.

Fixes: 49d200deaa68 ("debugfs: prevent access to removed files' private
                      data")
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 fs/debugfs/file.c       | 48 ------------------------------------------------
 fs/debugfs/inode.c      |  7 -------
 include/linux/debugfs.h | 19 -------------------
 lib/Kconfig.debug       |  1 -
 4 files changed, 75 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index ebf5b2faf91c..7de733ccdf6c 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -22,7 +22,6 @@
 #include <linux/slab.h>
 #include <linux/atomic.h>
 #include <linux/device.h>
-#include <linux/srcu.h>
 #include <asm/poll.h>
 
 #include "internal.h"
@@ -48,53 +47,6 @@ const struct file_operations debugfs_noop_file_operations = {
 	.llseek =	noop_llseek,
 };
 
-/**
- * debugfs_use_file_start - mark the beginning of file data access
- * @dentry: the dentry 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_use_file_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_use_file_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_use_file_start() must be followed by a matching call
- * to debugfs_use_file_finish().
- */
-int debugfs_use_file_start(const struct dentry *dentry, int *srcu_idx)
-	__acquires(&debugfs_srcu)
-{
-	*srcu_idx = srcu_read_lock(&debugfs_srcu);
-	barrier();
-	if (d_unlinked(dentry))
-		return -EIO;
-	return 0;
-}
-EXPORT_SYMBOL_GPL(debugfs_use_file_start);
-
-/**
- * debugfs_use_file_finish - mark the end of file data access
- * @srcu_idx: the SRCU index "created" by a former call to
- *            debugfs_use_file_start().
- *
- * Allow any ongoing concurrent call into debugfs_remove() or
- * debugfs_remove_recursive() blocked by a former call to
- * debugfs_use_file_start() to proceed and return to its caller.
- */
-void debugfs_use_file_finish(int srcu_idx) __releases(&debugfs_srcu)
-{
-	srcu_read_unlock(&debugfs_srcu, srcu_idx);
-}
-EXPORT_SYMBOL_GPL(debugfs_use_file_finish);
-
 #define F_DENTRY(filp) ((filp)->f_path.dentry)
 
 const struct file_operations *debugfs_real_fops(const struct file *filp)
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 88d51e666c38..5550f11d60bd 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -27,14 +27,11 @@
 #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;
@@ -692,8 +689,6 @@ 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);
 
@@ -767,8 +762,6 @@ 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/include/linux/debugfs.h b/include/linux/debugfs.h
index c65ff61b498c..df4d6eff2aac 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -23,7 +23,6 @@
 
 struct device;
 struct file_operations;
-struct srcu_struct;
 
 struct debugfs_blob_wrapper {
 	void *data;
@@ -43,8 +42,6 @@ struct debugfs_regset32 {
 
 extern struct dentry *arch_debugfs_dir;
 
-extern struct srcu_struct debugfs_srcu;
-
 #define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)		\
 static int __fops ## _open(struct inode *inode, struct file *file)	\
 {									\
@@ -90,11 +87,6 @@ struct dentry *debugfs_create_automount(const char *name,
 void debugfs_remove(struct dentry *dentry);
 void debugfs_remove_recursive(struct dentry *dentry);
 
-int debugfs_use_file_start(const struct dentry *dentry, int *srcu_idx)
-	__acquires(&debugfs_srcu);
-
-void debugfs_use_file_finish(int srcu_idx) __releases(&debugfs_srcu);
-
 const struct file_operations *debugfs_real_fops(const struct file *filp);
 
 int debugfs_file_get(struct dentry *dentry);
@@ -219,17 +211,6 @@ static inline void debugfs_remove(struct dentry *dentry)
 static inline void debugfs_remove_recursive(struct dentry *dentry)
 { }
 
-static inline int debugfs_use_file_start(const struct dentry *dentry,
-					int *srcu_idx)
-	__acquires(&debugfs_srcu)
-{
-	return 0;
-}
-
-static inline void debugfs_use_file_finish(int srcu_idx)
-	__releases(&debugfs_srcu)
-{ }
-
 static inline int debugfs_file_get(struct dentry *dentry)
 {
 	return 0;
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 77fadface4f9..92759731854c 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -279,7 +279,6 @@ 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.12.2

[toc] | [prev] | [next] | [standalone]

#1624238 — [RFC PATCH 9/9] debugfs: free debugfs_fsdata instances

Currently, a dentry's debugfs_fsdata instance is allocated from
debugfs_file_get() at first usage, i.e. at first file opening.

It won't ever get freed though.

Ideally, these instances would get freed after the last open file handle
gets closed again, that is from the fops' ->release().

Unfortunately, we can't hook easily into those ->release()ers of files
created through debugfs_create_file_unsafe(), i.e. of those proxied through
debugfs_open_proxy_file_operations rather than through the "full"
debugfs_full_proxy_file_operations proxy.

Hence, free unreferenced debugfs_fsdata instances from debugfs_file_put(),
with the drawback of a potential allocation + deallocation per
debugfs_file_get() + debugfs_file_put() pair, that is per fops invocation.

In addition to its former role of tracking pending fops, use the
->active_users for reference counting on the debugfs_fsdata instance
itself. In particular, don't keep a dummy reference to be dropped from
__debugfs_remove_file(): a d_delete()ed dentry and thus, request for
completion notification, is now signaled by the d_unlinked() dentry itself.

Once ->active_users drops to zero (and the dentry is still intact), free
the debugfs_fsdata instance from debugfs_file_put(). RCU protects any
concurrent debugfs_file_get() attempts to get a hold of the instance here.
Likewise for full_proxy_release() which lacks a call to debugfs_file_get().

Note that due to non-atomic updates to the d_unlinked() + ->d_fsdata pair,
care must be taken in order to avoid races between debugfs_file_put() and
debugfs_file_get() as well as __debugfs_remove_file(). Rather than
introducing a global lock, exploit the fact that there will ever be only a
single !d_unlinked() -> d_unlinked() transition and add memory barriers
where needed. Given the lack of proper benchmarking, that debugfs fops
aren't performance critical and that we've already got a potential
allocation/deallocation pair anyway, the added code complexity might be
highly questionable though.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 fs/debugfs/file.c     | 102 ++++++++++++++++++++++++++++++++++++++++----------
 fs/debugfs/inode.c    |   8 +++-
 fs/debugfs/internal.h |   1 +
 3 files changed, 90 insertions(+), 21 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index f4dfd7d0d625..b2cc25d44a39 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -22,6 +22,7 @@
 #include <linux/slab.h>
 #include <linux/atomic.h>
 #include <linux/device.h>
+#include <linux/rcupdate.h>
 #include <asm/poll.h>
 
 #include "internal.h"
@@ -78,10 +79,39 @@ int debugfs_file_get(struct dentry *dentry)
 	struct debugfs_fsdata *fsd;
 	void *d_fsd;
 
-	d_fsd = READ_ONCE(dentry->d_fsdata);
+	rcu_read_lock();
+retry:
+	d_fsd = rcu_dereference(dentry->d_fsdata);
 	if (!((unsigned long)d_fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) {
+		/*
+		 * Paired with the control dependency in
+		 * debugfs_file_put(): if we saw the debugfs_fsdata
+		 * instance "restored" there but not the dead dentry,
+		 * we'd erroneously instantiate a fresh debugfs_fsdata
+		 * instance below.
+		 */
+		smp_rmb();
+		if (d_unlinked(dentry)) {
+			rcu_read_unlock();
+			return -EIO;
+		}
+
 		fsd = d_fsd;
+		if (!refcount_inc_not_zero(&fsd->active_users)) {
+			/*
+			 * A concurrent debugfs_file_put() dropped the
+			 * count to zero and is about to free the
+			 * debugfs_fsdata. Help out resetting the
+			 * ->d_fsdata and retry.
+			 */
+			d_fsd = (void *)((unsigned long)fsd->real_fops |
+					DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
+			RCU_INIT_POINTER(dentry->d_fsdata, d_fsd);
+			goto retry;
+		}
+		rcu_read_unlock();
 	} else {
+		rcu_read_unlock();
 		fsd = kmalloc(sizeof(*fsd), GFP_KERNEL);
 		if (!fsd)
 			return -ENOMEM;
@@ -91,25 +121,28 @@ int debugfs_file_get(struct dentry *dentry)
 		refcount_set(&fsd->active_users, 1);
 		init_completion(&fsd->active_users_drained);
 		if (cmpxchg(&dentry->d_fsdata, d_fsd, fsd) != d_fsd) {
+			/*
+			 * Another debugfs_file_get() has installed a
+			 * debugfs_fsdata instance concurrently.
+			 * Free ours and retry to grab a reference on
+			 * the installed one.
+			 */
 			kfree(fsd);
-			fsd = READ_ONCE(dentry->d_fsdata);
+			rcu_read_lock();
+			goto retry;
+		}
+		/*
+		 * In case of a successful cmpxchg() above, this check is
+		 * strictly necessary and must follow it, see the comment in
+		 * __debugfs_remove_file().
+		 */
+		if (d_unlinked(dentry)) {
+			if (refcount_dec_and_test(&fsd->active_users))
+				complete(&fsd->active_users_drained);
+			return -EIO;
 		}
 	}
 
-	/*
-	 * In case of a successful cmpxchg() above, this check is
-	 * strictly necessary and must follow it, see the comment in
-	 * __debugfs_remove_file().
-	 * OTOH, if the cmpxchg() hasn't been executed or wasn't
-	 * successful, this serves the purpose of not starving
-	 * removers.
-	 */
-	if (d_unlinked(dentry))
-		return -EIO;
-
-	if (!refcount_inc_not_zero(&fsd->active_users))
-		return -EIO;
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(debugfs_file_get);
@@ -126,9 +159,29 @@ EXPORT_SYMBOL_GPL(debugfs_file_get);
 void debugfs_file_put(struct dentry *dentry)
 {
 	struct debugfs_fsdata *fsd = READ_ONCE(dentry->d_fsdata);
+	void *d_fsd;
 
-	if (refcount_dec_and_test(&fsd->active_users))
-		complete(&fsd->active_users_drained);
+	if (refcount_dec_and_test(&fsd->active_users)) {
+		d_fsd = (void *)((unsigned long)fsd->real_fops |
+				DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
+		RCU_INIT_POINTER(dentry->d_fsdata, d_fsd);
+		/* Paired with smp_mb() in __debugfs_remove_file(). */
+		smp_mb();
+		if (d_unlinked(dentry)) {
+			/*
+			 * We have a control dependency paired with the
+			 * smp_rmb() in debugfs_file_get() here.
+			 *
+			 * Restore the debugfs_fsdata instance into
+			 * ->d_fsdata s.t. ->d_release() can free
+			 * it.
+			 */
+			WRITE_ONCE(dentry->d_fsdata, fsd);
+			complete(&fsd->active_users_drained);
+		} else {
+			kfree_rcu(fsd, rcu_head);
+		}
+	}
 }
 EXPORT_SYMBOL_GPL(debugfs_file_put);
 
@@ -221,9 +274,20 @@ static unsigned int full_proxy_poll(struct file *filp,
 static int full_proxy_release(struct inode *inode, struct file *filp)
 {
 	const struct dentry *dentry = F_DENTRY(filp);
-	const struct file_operations *real_fops = debugfs_real_fops(filp);
 	const struct file_operations *proxy_fops = filp->f_op;
 	int r = 0;
+	void *d_fsd;
+	const struct file_operations *real_fops;
+
+	rcu_read_lock();
+	d_fsd = rcu_dereference(F_DENTRY(filp)->d_fsdata);
+	if ((unsigned long)d_fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT) {
+		real_fops = (void *)((unsigned long)d_fsd &
+				~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
+	} else {
+		real_fops = ((struct debugfs_fsdata *)d_fsd)->real_fops;
+	}
+	rcu_read_unlock();
 
 	/*
 	 * We must not protect this against removal races here: the
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 2360c17ec00a..bacb4d6bf178 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -27,6 +27,7 @@
 #include <linux/parser.h>
 #include <linux/magic.h>
 #include <linux/slab.h>
+#include <linux/rcupdate.h>
 
 #include "internal.h"
 
@@ -636,13 +637,16 @@ static void __debugfs_remove_file(struct dentry *dentry, struct dentry *parent)
 	 * cmpxchg() in debugfs_file_get(): either
 	 * debugfs_file_get() must see a dead dentry or we must see a
 	 * debugfs_fsdata instance at ->d_fsdata here (or both).
+	 *
+	 * Also paired with the smp_mb() in debugfs_file_put(): if we
+	 * see a debugfs_fsdata instance here, then debugfs_file_put()
+	 * must see a dead dentry.
 	 */
 	smp_mb();
 	fsd = READ_ONCE(dentry->d_fsdata);
 	if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)
 		return;
-	if (!refcount_dec_and_test(&fsd->active_users))
-		wait_for_completion(&fsd->active_users_drained);
+	wait_for_completion(&fsd->active_users_drained);
 }
 
 static int __debugfs_remove(struct dentry *dentry, struct dentry *parent)
diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h
index cb1e8139c398..0445bd7d11f2 100644
--- a/fs/debugfs/internal.h
+++ b/fs/debugfs/internal.h
@@ -23,6 +23,7 @@ struct debugfs_fsdata {
 	const struct file_operations *real_fops;
 	refcount_t active_users;
 	struct completion active_users_drained;
+	struct rcu_head rcu_head;
 };
 
 /*
-- 
2.12.2

[toc] | [prev] | [next] | [standalone]

#1624729 — Re: [RFC PATCH 9/9] debugfs: free debugfs_fsdata instances

On Sun, Apr 16, 2017 at 11:51:37AM +0200, Nicolai Stange wrote:
> Currently, a dentry's debugfs_fsdata instance is allocated from
> debugfs_file_get() at first usage, i.e. at first file opening.
> 
> It won't ever get freed though.
> 
> Ideally, these instances would get freed after the last open file handle
> gets closed again, that is from the fops' ->release().
> 
> Unfortunately, we can't hook easily into those ->release()ers of files
> created through debugfs_create_file_unsafe(), i.e. of those proxied through
> debugfs_open_proxy_file_operations rather than through the "full"
> debugfs_full_proxy_file_operations proxy.
> 
> Hence, free unreferenced debugfs_fsdata instances from debugfs_file_put(),
> with the drawback of a potential allocation + deallocation per
> debugfs_file_get() + debugfs_file_put() pair, that is per fops invocation.
> 
> In addition to its former role of tracking pending fops, use the
> ->active_users for reference counting on the debugfs_fsdata instance
> itself. In particular, don't keep a dummy reference to be dropped from
> __debugfs_remove_file(): a d_delete()ed dentry and thus, request for
> completion notification, is now signaled by the d_unlinked() dentry itself.
> 
> Once ->active_users drops to zero (and the dentry is still intact), free
> the debugfs_fsdata instance from debugfs_file_put(). RCU protects any
> concurrent debugfs_file_get() attempts to get a hold of the instance here.
> Likewise for full_proxy_release() which lacks a call to debugfs_file_get().
> 
> Note that due to non-atomic updates to the d_unlinked() + ->d_fsdata pair,
> care must be taken in order to avoid races between debugfs_file_put() and
> debugfs_file_get() as well as __debugfs_remove_file(). Rather than
> introducing a global lock, exploit the fact that there will ever be only a
> single !d_unlinked() -> d_unlinked() transition and add memory barriers
> where needed. Given the lack of proper benchmarking, that debugfs fops
> aren't performance critical and that we've already got a potential
> allocation/deallocation pair anyway, the added code complexity might be
> highly questionable though.
> 
> Signed-off-by: Nicolai Stange <nicstange@gmail.com>

If you have not already done so, please run this with debug enabled,
especially CONFIG_PROVE_LOCKING=y (which implies CONFIG_PROVE_RCU=y).
This is important because there are configurations for which the deadlocks
you saw with SRCU turn into silent failure, including memory corruption.
CONFIG_PROVE_RCU=y will catch many of those situations.

(And yes, kfree_rcu() doesn't have that problem, but...)

Another issue called out inline.

							Thanx, Paul

> ---
>  fs/debugfs/file.c     | 102 ++++++++++++++++++++++++++++++++++++++++----------
>  fs/debugfs/inode.c    |   8 +++-
>  fs/debugfs/internal.h |   1 +
>  3 files changed, 90 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index f4dfd7d0d625..b2cc25d44a39 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -22,6 +22,7 @@
>  #include <linux/slab.h>
>  #include <linux/atomic.h>
>  #include <linux/device.h>
> +#include <linux/rcupdate.h>
>  #include <asm/poll.h>
> 
>  #include "internal.h"
> @@ -78,10 +79,39 @@ int debugfs_file_get(struct dentry *dentry)
>  	struct debugfs_fsdata *fsd;
>  	void *d_fsd;
> 
> -	d_fsd = READ_ONCE(dentry->d_fsdata);
> +	rcu_read_lock();
> +retry:
> +	d_fsd = rcu_dereference(dentry->d_fsdata);
>  	if (!((unsigned long)d_fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) {
> +		/*
> +		 * Paired with the control dependency in
> +		 * debugfs_file_put(): if we saw the debugfs_fsdata
> +		 * instance "restored" there but not the dead dentry,
> +		 * we'd erroneously instantiate a fresh debugfs_fsdata
> +		 * instance below.
> +		 */
> +		smp_rmb();
> +		if (d_unlinked(dentry)) {
> +			rcu_read_unlock();
> +			return -EIO;
> +		}
> +
>  		fsd = d_fsd;
> +		if (!refcount_inc_not_zero(&fsd->active_users)) {
> +			/*
> +			 * A concurrent debugfs_file_put() dropped the
> +			 * count to zero and is about to free the
> +			 * debugfs_fsdata. Help out resetting the
> +			 * ->d_fsdata and retry.
> +			 */
> +			d_fsd = (void *)((unsigned long)fsd->real_fops |
> +					DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
> +			RCU_INIT_POINTER(dentry->d_fsdata, d_fsd);

This is an infrequent race, I hope?  If on the other hand there is
a possibility of this branch being taken a huge number of times in
one call, it would be good to exit the RCU read-side critical section
before retrying.

> +			goto retry;
> +		}
> +		rcu_read_unlock();
>  	} else {
> +		rcu_read_unlock();
>  		fsd = kmalloc(sizeof(*fsd), GFP_KERNEL);
>  		if (!fsd)
>  			return -ENOMEM;
> @@ -91,25 +121,28 @@ int debugfs_file_get(struct dentry *dentry)
>  		refcount_set(&fsd->active_users, 1);
>  		init_completion(&fsd->active_users_drained);
>  		if (cmpxchg(&dentry->d_fsdata, d_fsd, fsd) != d_fsd) {
> +			/*
> +			 * Another debugfs_file_get() has installed a
> +			 * debugfs_fsdata instance concurrently.
> +			 * Free ours and retry to grab a reference on
> +			 * the installed one.
> +			 */
>  			kfree(fsd);
> -			fsd = READ_ONCE(dentry->d_fsdata);
> +			rcu_read_lock();
> +			goto retry;

And given this code path, why not put the retry: label before the
initial rcu_read_lock()?  Same number of lines of code, rcu_read_lock()
and rcu_read_unlock() are very lightweight, the extra executions should
be rare, and you might be avoiding a grace-period-starvation problem.

> +		}
> +		/*
> +		 * In case of a successful cmpxchg() above, this check is
> +		 * strictly necessary and must follow it, see the comment in
> +		 * __debugfs_remove_file().
> +		 */
> +		if (d_unlinked(dentry)) {
> +			if (refcount_dec_and_test(&fsd->active_users))
> +				complete(&fsd->active_users_drained);
> +			return -EIO;
>  		}
>  	}
> 
> -	/*
> -	 * In case of a successful cmpxchg() above, this check is
> -	 * strictly necessary and must follow it, see the comment in
> -	 * __debugfs_remove_file().
> -	 * OTOH, if the cmpxchg() hasn't been executed or wasn't
> -	 * successful, this serves the purpose of not starving
> -	 * removers.
> -	 */
> -	if (d_unlinked(dentry))
> -		return -EIO;
> -
> -	if (!refcount_inc_not_zero(&fsd->active_users))
> -		return -EIO;
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(debugfs_file_get);
> @@ -126,9 +159,29 @@ EXPORT_SYMBOL_GPL(debugfs_file_get);
>  void debugfs_file_put(struct dentry *dentry)
>  {
>  	struct debugfs_fsdata *fsd = READ_ONCE(dentry->d_fsdata);
> +	void *d_fsd;
> 
> -	if (refcount_dec_and_test(&fsd->active_users))
> -		complete(&fsd->active_users_drained);
> +	if (refcount_dec_and_test(&fsd->active_users)) {
> +		d_fsd = (void *)((unsigned long)fsd->real_fops |
> +				DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
> +		RCU_INIT_POINTER(dentry->d_fsdata, d_fsd);
> +		/* Paired with smp_mb() in __debugfs_remove_file(). */
> +		smp_mb();
> +		if (d_unlinked(dentry)) {
> +			/*
> +			 * We have a control dependency paired with the
> +			 * smp_rmb() in debugfs_file_get() here.
> +			 *
> +			 * Restore the debugfs_fsdata instance into
> +			 * ->d_fsdata s.t. ->d_release() can free
> +			 * it.
> +			 */
> +			WRITE_ONCE(dentry->d_fsdata, fsd);
> +			complete(&fsd->active_users_drained);
> +		} else {
> +			kfree_rcu(fsd, rcu_head);
> +		}
> +	}
>  }
>  EXPORT_SYMBOL_GPL(debugfs_file_put);
> 
> @@ -221,9 +274,20 @@ static unsigned int full_proxy_poll(struct file *filp,
>  static int full_proxy_release(struct inode *inode, struct file *filp)
>  {
>  	const struct dentry *dentry = F_DENTRY(filp);
> -	const struct file_operations *real_fops = debugfs_real_fops(filp);
>  	const struct file_operations *proxy_fops = filp->f_op;
>  	int r = 0;
> +	void *d_fsd;
> +	const struct file_operations *real_fops;
> +
> +	rcu_read_lock();
> +	d_fsd = rcu_dereference(F_DENTRY(filp)->d_fsdata);
> +	if ((unsigned long)d_fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT) {
> +		real_fops = (void *)((unsigned long)d_fsd &
> +				~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
> +	} else {
> +		real_fops = ((struct debugfs_fsdata *)d_fsd)->real_fops;
> +	}
> +	rcu_read_unlock();
> 
>  	/*
>  	 * We must not protect this against removal races here: the
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 2360c17ec00a..bacb4d6bf178 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -27,6 +27,7 @@
>  #include <linux/parser.h>
>  #include <linux/magic.h>
>  #include <linux/slab.h>
> +#include <linux/rcupdate.h>
> 
>  #include "internal.h"
> 
> @@ -636,13 +637,16 @@ static void __debugfs_remove_file(struct dentry *dentry, struct dentry *parent)
>  	 * cmpxchg() in debugfs_file_get(): either
>  	 * debugfs_file_get() must see a dead dentry or we must see a
>  	 * debugfs_fsdata instance at ->d_fsdata here (or both).
> +	 *
> +	 * Also paired with the smp_mb() in debugfs_file_put(): if we
> +	 * see a debugfs_fsdata instance here, then debugfs_file_put()
> +	 * must see a dead dentry.
>  	 */
>  	smp_mb();
>  	fsd = READ_ONCE(dentry->d_fsdata);
>  	if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)
>  		return;
> -	if (!refcount_dec_and_test(&fsd->active_users))
> -		wait_for_completion(&fsd->active_users_drained);
> +	wait_for_completion(&fsd->active_users_drained);
>  }
> 
>  static int __debugfs_remove(struct dentry *dentry, struct dentry *parent)
> diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h
> index cb1e8139c398..0445bd7d11f2 100644
> --- a/fs/debugfs/internal.h
> +++ b/fs/debugfs/internal.h
> @@ -23,6 +23,7 @@ struct debugfs_fsdata {
>  	const struct file_operations *real_fops;
>  	refcount_t active_users;
>  	struct completion active_users_drained;
> +	struct rcu_head rcu_head;
>  };
> 
>  /*
> -- 
> 2.12.2
> 

[toc] | [prev] | [next] | [standalone]

#1625189 — Re: [RFC PATCH 9/9] debugfs: free debugfs_fsdata instances

On Mon, 2017-04-17 at 09:01 -0700, Paul E. McKenney wrote:

> If you have not already done so, please run this with debug enabled,
> especially CONFIG_PROVE_LOCKING=y (which implies CONFIG_PROVE_RCU=y).
> This is important because there are configurations for which the
> deadlocks you saw with SRCU turn into silent failure, including
> memory corruption.
> CONFIG_PROVE_RCU=y will catch many of those situations.

Can you elaborate on that? I think we may have had CONFIG_PROVE_RCU
enabled in the builds where we saw the problem, but I'm not sure.

Can you say which configurations you're thinking of? And perhaps what
kind of corruption you're thinking of also? I'm having a hard time
imagining any corruption that should happen?

Nicolai probably never even ran into this problem, though it should be
easy to reproduce.

johannes

[toc] | [prev] | [next] | [standalone]

#1625328 — Re: [RFC PATCH 9/9] debugfs: free debugfs_fsdata instances

On Tue, Apr 18, 2017 at 11:39:27AM +0200, Johannes Berg wrote:
> On Mon, 2017-04-17 at 09:01 -0700, Paul E. McKenney wrote:
> 
> > If you have not already done so, please run this with debug enabled,
> > especially CONFIG_PROVE_LOCKING=y (which implies CONFIG_PROVE_RCU=y).
> > This is important because there are configurations for which the
> > deadlocks you saw with SRCU turn into silent failure, including
> > memory corruption.
> > CONFIG_PROVE_RCU=y will catch many of those situations.
> 
> Can you elaborate on that? I think we may have had CONFIG_PROVE_RCU
> enabled in the builds where we saw the problem, but I'm not sure.

CONFIG_PROVE_RCU=y will reliably catch things like this:

1.	rcu_read_lock();
	synchronize_rcu();
	rcu_read_unlock();

	With CONFIG_PROVE_RCU=n and CONFIG_PREEMPT=n, this will result in
	too-short grace periods, which can free things out from under the
	read-side critical section, which in turn can result in arbitrary
	memory corruption.  You might not even get a "scheduling while
	atomic", though CONFIG_PREEMPT_COUNT=y will produce this message.

	With CONFIG_PREEMPT=y, on the other hand, this should
	deadlock in a manner similar to the earlier SRCU deadlocks
	seen in debugfs.

2.	rcu_read_lock();
	schedule_timeout_interruptible(HZ);
	rcu_read_unlock();

	With CONFIG_PROVE_RCU=y and CONFIG_PREEMPT=y, this will just
	work, more or less.  Until someone runs with CONFIG_PREEMPT=n,
	which will produce "scheduling while atomic".  (I have a
	fix for this queued for 4.13, FWIW, so that in the future
	CONFIG_PROVE_RCU=y and CONFIG_PREEMPT=y will complain about
	this.  But for now, silent bug.)

There are more, but this should get you the flavor of the types
of bugs CONFIG_PROVE_RCU=y can locate for you.

> Can you say which configurations you're thinking of? And perhaps what
> kind of corruption you're thinking of also? I'm having a hard time
> imagining any corruption that should happen?

#1 is the silent corruption case given CONFIG_PROVE_RCU=n,
CONFIG_PREEMPT=n, and CONFIG_PREEMPT_COUNT=n.

> Nicolai probably never even ran into this problem, though it should be
> easy to reproduce.

I am just worried that the situation resulting in the earlier SRCU
deadlocks might be hiding behind CONFIG_PROVE_RCU=n, CONFIG_PREEMPT=n,
and CONFIG_PREEMPT_COUNT=n.  Or some other bug hiding behind some
other set of Kconfig options.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1625331 — Re: [RFC PATCH 9/9] debugfs: free debugfs_fsdata instances

On Tue, 2017-04-18 at 06:31 -0700, Paul E. McKenney wrote:
> On Tue, Apr 18, 2017 at 11:39:27AM +0200, Johannes Berg wrote:
> > On Mon, 2017-04-17 at 09:01 -0700, Paul E. McKenney wrote:
> > 
> > > If you have not already done so, please run this with debug
> > > enabled,
> > > especially CONFIG_PROVE_LOCKING=y (which implies
> > > CONFIG_PROVE_RCU=y).
> > > This is important because there are configurations for which the
> > > deadlocks you saw with SRCU turn into silent failure, including
> > > memory corruption.
> > > CONFIG_PROVE_RCU=y will catch many of those situations.
> > 
> > Can you elaborate on that? I think we may have had CONFIG_PROVE_RCU
> > enabled in the builds where we saw the problem, but I'm not sure.
> 
> CONFIG_PROVE_RCU=y will reliably catch things like this:
> 
> 1.	rcu_read_lock();
> 	synchronize_rcu();
> 	rcu_read_unlock();

Ok, that's not something that happens here either.

> 2.	rcu_read_lock();
> 	schedule_timeout_interruptible(HZ);
> 	rcu_read_unlock();

Neither is this happening.

> There are more, but this should get you the flavor of the types
> of bugs CONFIG_PROVE_RCU=y can locate for you.

Makes sense. However, the issue at hand is what we (you and I)
discussed earlier wrt. lockdep -- from SRCU's point of view everything
is actually OK, except that the one thread is waiting for something and
we can never finish the grace period, and thus synchronize_srcu() will
never return. But there's no real SRCU bug here.

> > Nicolai probably never even ran into this problem, though it should
> > be easy to reproduce.
> 
> I am just worried that the situation resulting in the earlier SRCU
> deadlocks might be hiding behind CONFIG_PROVE_RCU=n,
> CONFIG_PREEMPT=n, and CONFIG_PREEMPT_COUNT=n.  Or some other bug
> hiding behind some other set of Kconfig options.

There's no SRCU deadlock though. I know exactly why it happens, in my
case, which is the following:

Thread 1
userspace: read(debugfs_file_1)
srcu_read_lock(&debugfs_srcu); // in debugfs bowels
wait_event_interruptible(...); // in my driver's debugfs read method

Thread 2:
debugfs_remove(debugfs_file_2);
srcu_synchronize(&debugfs_srcu); // in debugfs bowels


This is the live-lock. The deadlock is something I posited but never
ran into:

CPU 1				CPU 2
srcu_read_lock(&debugfs_srcu);
				rtnl_lock();
rtnl_lock();
				srcu_synchronize(&debugfs_srcu);

Again, no (S)RCU abuse here, just an ABBA deadlock.

johannes

[toc] | [prev] | [next] | [standalone]

#1625393 — Re: [RFC PATCH 9/9] debugfs: free debugfs_fsdata instances

On Tue, Apr 18, 2017 at 03:40:32PM +0200, Johannes Berg wrote:
> On Tue, 2017-04-18 at 06:31 -0700, Paul E. McKenney wrote:
> > On Tue, Apr 18, 2017 at 11:39:27AM +0200, Johannes Berg wrote:
> > > On Mon, 2017-04-17 at 09:01 -0700, Paul E. McKenney wrote:
> > > 
> > > > If you have not already done so, please run this with debug
> > > > enabled,
> > > > especially CONFIG_PROVE_LOCKING=y (which implies
> > > > CONFIG_PROVE_RCU=y).
> > > > This is important because there are configurations for which the
> > > > deadlocks you saw with SRCU turn into silent failure, including
> > > > memory corruption.
> > > > CONFIG_PROVE_RCU=y will catch many of those situations.
> > > 
> > > Can you elaborate on that? I think we may have had CONFIG_PROVE_RCU
> > > enabled in the builds where we saw the problem, but I'm not sure.
> > 
> > CONFIG_PROVE_RCU=y will reliably catch things like this:
> > 
> > 1.	rcu_read_lock();
> > 	synchronize_rcu();
> > 	rcu_read_unlock();
> 
> Ok, that's not something that happens here either.
> 
> > 2.	rcu_read_lock();
> > 	schedule_timeout_interruptible(HZ);
> > 	rcu_read_unlock();
> 
> Neither is this happening.
> 
> > There are more, but this should get you the flavor of the types
> > of bugs CONFIG_PROVE_RCU=y can locate for you.
> 
> Makes sense. However, the issue at hand is what we (you and I)
> discussed earlier wrt. lockdep -- from SRCU's point of view everything
> is actually OK, except that the one thread is waiting for something and
> we can never finish the grace period, and thus synchronize_srcu() will
> never return. But there's no real SRCU bug here.
> 
> > > Nicolai probably never even ran into this problem, though it should
> > > be easy to reproduce.
> > 
> > I am just worried that the situation resulting in the earlier SRCU
> > deadlocks might be hiding behind CONFIG_PROVE_RCU=n,
> > CONFIG_PREEMPT=n, and CONFIG_PREEMPT_COUNT=n.  Or some other bug
> > hiding behind some other set of Kconfig options.
> 
> There's no SRCU deadlock though. I know exactly why it happens, in my
> case, which is the following:
> 
> Thread 1
> userspace: read(debugfs_file_1)
> srcu_read_lock(&debugfs_srcu); // in debugfs bowels
> wait_event_interruptible(...); // in my driver's debugfs read method
> 
> Thread 2:
> debugfs_remove(debugfs_file_2);
> srcu_synchronize(&debugfs_srcu); // in debugfs bowels
> 
> 
> This is the live-lock. The deadlock is something I posited but never
> ran into:
> 
> CPU 1				CPU 2
> srcu_read_lock(&debugfs_srcu);
> 				rtnl_lock();
> rtnl_lock();
> 				srcu_synchronize(&debugfs_srcu);
> 
> Again, no (S)RCU abuse here, just an ABBA deadlock.

OK, please accept my apologies for failing to follow the thread.

I nevertheless reiterate my advice to run at least some tests with
CONFIG_PROVE_RCU=y.  And yes, it would be good to upgrade lockdep
to find the above theoretical deadlock.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1625398 — Re: [RFC PATCH 9/9] debugfs: free debugfs_fsdata instances

On Tue, 2017-04-18 at 08:17 -0700, Paul E. McKenney wrote:

> > Again, no (S)RCU abuse here, just an ABBA deadlock.
> 
> OK, please accept my apologies for failing to follow the thread.

No worries - just wanted to clarify this in case I was missing
something.

> I nevertheless reiterate my advice to run at least some tests with
> CONFIG_PROVE_RCU=y.  And yes, it would be good to upgrade lockdep
> to find the above theoretical deadlock.

Right. It won't matter for debugfs after this patchset, but it would
indeed be nice - I failed in my attempt though :)

johannes

[toc] | [prev] | [next] | [standalone]

#1625484 — Re: [RFC PATCH 9/9] debugfs: free debugfs_fsdata instances

On Tue, Apr 18, 2017 at 05:20:38PM +0200, Johannes Berg wrote:
> On Tue, 2017-04-18 at 08:17 -0700, Paul E. McKenney wrote:
> 
> > > Again, no (S)RCU abuse here, just an ABBA deadlock.
> > 
> > OK, please accept my apologies for failing to follow the thread.
> 
> No worries - just wanted to clarify this in case I was missing
> something.
> 
> > I nevertheless reiterate my advice to run at least some tests with
> > CONFIG_PROVE_RCU=y.  And yes, it would be good to upgrade lockdep
> > to find the above theoretical deadlock.
> 
> Right. It won't matter for debugfs after this patchset, but it would
> indeed be nice - I failed in my attempt though :)

Hey, I was hoping!  ;-)

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1624239 — [RFC PATCH 1/9] debugfs: add support for more elaborate ->d_fsdata

Currently, the user provided fops, "real_fops", are stored directly into
->d_fsdata.

In order to be able to store more per-file state and thus prepare for more
granular file removal protection, wrap the real_fops into a dynamically
allocated container struct, debugfs_fsdata.

A struct debugfs_fsdata gets allocated at file creation and freed from the
newly intoduced ->d_release().

Finally, move the implementation of debugfs_real_fops() out of the public
debugfs header such that struct debugfs_fsdata's declaration can be kept
private.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 fs/debugfs/file.c       | 12 ++++++++++++
 fs/debugfs/inode.c      | 22 +++++++++++++++++++---
 fs/debugfs/internal.h   |  4 ++++
 include/linux/debugfs.h | 20 +++-----------------
 4 files changed, 38 insertions(+), 20 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 354e2ab62031..15655a1a0704 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -97,6 +97,18 @@ EXPORT_SYMBOL_GPL(debugfs_use_file_finish);
 
 #define F_DENTRY(filp) ((filp)->f_path.dentry)
 
+const struct file_operations *debugfs_real_fops(const struct file *filp)
+	__must_hold(&debugfs_srcu)
+{
+	struct debugfs_fsdata *fsd = F_DENTRY(filp)->d_fsdata;
+	/*
+	 * Neither the pointer to the struct file_operations, nor its
+	 * contents ever change -- srcu_dereference() is not needed here.
+	 */
+	return fsd->real_fops;
+}
+EXPORT_SYMBOL_GPL(debugfs_real_fops);
+
 static int open_proxy_open(struct inode *inode, struct file *filp)
 {
 	const struct dentry *dentry = F_DENTRY(filp);
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 7fd4ec4bb214..1ae133064ca1 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -185,6 +185,11 @@ static const struct super_operations debugfs_super_operations = {
 	.evict_inode	= debugfs_evict_inode,
 };
 
+static void debugfs_release_dentry(struct dentry *dentry)
+{
+	kfree(dentry->d_fsdata);
+}
+
 static struct vfsmount *debugfs_automount(struct path *path)
 {
 	debugfs_automount_t f;
@@ -194,6 +199,7 @@ static struct vfsmount *debugfs_automount(struct path *path)
 
 static const struct dentry_operations debugfs_dops = {
 	.d_delete = always_delete_dentry,
+	.d_release = debugfs_release_dentry,
 	.d_automount = debugfs_automount,
 };
 
@@ -343,24 +349,34 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
 {
 	struct dentry *dentry;
 	struct inode *inode;
+	struct debugfs_fsdata *fsd;
+
+	fsd = kmalloc(sizeof(*fsd), GFP_KERNEL);
+	if (!fsd)
+		return NULL;
 
 	if (!(mode & S_IFMT))
 		mode |= S_IFREG;
 	BUG_ON(!S_ISREG(mode));
 	dentry = start_creating(name, parent);
 
-	if (IS_ERR(dentry))
+	if (IS_ERR(dentry)) {
+		kfree(fsd);
 		return NULL;
+	}
 
 	inode = debugfs_get_inode(dentry->d_sb);
-	if (unlikely(!inode))
+	if (unlikely(!inode)) {
+		kfree(fsd);
 		return failed_creating(dentry);
+	}
 
 	inode->i_mode = mode;
 	inode->i_private = data;
 
 	inode->i_fop = proxy_fops;
-	dentry->d_fsdata = (void *)real_fops;
+	fsd->real_fops = real_fops;
+	dentry->d_fsdata = fsd;
 
 	d_instantiate(dentry, inode);
 	fsnotify_create(d_inode(dentry->d_parent), dentry);
diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h
index b3e8443a1f47..512601eed3ce 100644
--- a/fs/debugfs/internal.h
+++ b/fs/debugfs/internal.h
@@ -19,4 +19,8 @@ extern const struct file_operations debugfs_noop_file_operations;
 extern const struct file_operations debugfs_open_proxy_file_operations;
 extern const struct file_operations debugfs_full_proxy_file_operations;
 
+struct debugfs_fsdata {
+	const struct file_operations *real_fops;
+};
+
 #endif /* _DEBUGFS_INTERNAL_H_ */
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 9174b0d28582..d614be21412a 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -45,23 +45,6 @@ extern struct dentry *arch_debugfs_dir;
 
 extern struct srcu_struct debugfs_srcu;
 
-/**
- * debugfs_real_fops - getter for the real file operation
- * @filp: a pointer to a struct file
- *
- * Must only be called under the protection established by
- * debugfs_use_file_start().
- */
-static inline const struct file_operations *debugfs_real_fops(const struct file *filp)
-	__must_hold(&debugfs_srcu)
-{
-	/*
-	 * Neither the pointer to the struct file_operations, nor its
-	 * contents ever change -- srcu_dereference() is not needed here.
-	 */
-	return filp->f_path.dentry->d_fsdata;
-}
-
 #define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)		\
 static int __fops ## _open(struct inode *inode, struct file *file)	\
 {									\
@@ -112,6 +95,9 @@ int debugfs_use_file_start(const struct dentry *dentry, int *srcu_idx)
 
 void debugfs_use_file_finish(int srcu_idx) __releases(&debugfs_srcu);
 
+const struct file_operations *debugfs_real_fops(const struct file *filp)
+	__must_hold(&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,
-- 
2.12.2

[toc] | [prev] | [next] | [standalone]

#1624240 — [RFC PATCH 3/9] debugfs: debugfs_real_fops(): drop __must_hold sparse annotation

Currently, debugfs_real_fops() is annotated with a
__must_hold(&debugfs_srcu) sparse annotation.

With the conversion of the SRCU based protection of users against
concurrent file removals to a per-file refcount based scheme, this becomes
wrong.

Drop this annotation.

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

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 081d74d390a6..3bc3e2e69f80 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -98,13 +98,9 @@ EXPORT_SYMBOL_GPL(debugfs_use_file_finish);
 #define F_DENTRY(filp) ((filp)->f_path.dentry)
 
 const struct file_operations *debugfs_real_fops(const struct file *filp)
-	__must_hold(&debugfs_srcu)
 {
 	struct debugfs_fsdata *fsd = F_DENTRY(filp)->d_fsdata;
-	/*
-	 * Neither the pointer to the struct file_operations, nor its
-	 * contents ever change -- srcu_dereference() is not needed here.
-	 */
+
 	return fsd->real_fops;
 }
 EXPORT_SYMBOL_GPL(debugfs_real_fops);
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index d1f1104c41ee..c65ff61b498c 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -95,8 +95,7 @@ int debugfs_use_file_start(const struct dentry *dentry, int *srcu_idx)
 
 void debugfs_use_file_finish(int srcu_idx) __releases(&debugfs_srcu);
 
-const struct file_operations *debugfs_real_fops(const struct file *filp)
-	__must_hold(&debugfs_srcu);
+const struct file_operations *debugfs_real_fops(const struct file *filp);
 
 int debugfs_file_get(struct dentry *dentry);
 void debugfs_file_put(struct dentry *dentry);
-- 
2.12.2

[toc] | [prev] | [next] | [standalone]

#1624241 — [RFC PATCH 7/9] debugfs: call debugfs_real_fops() only after debugfs_file_get()

The current implementation of debugfs_real_fops() relies on a
debugfs_fsdata instance to be installed at ->d_fsdata.

With future patches introducing lazy allocation of these, this requirement
will be guaranteed to be fullfilled only inbetween a
debugfs_file_get()/debugfs_file_put() pair.

The full proxies' fops implemented by debugfs happen to be the only
offenders. Fix them up by moving their debugfs_real_fops() calls past those
to debugfs_file_get().

full_proxy_release() is special as it doesn't invoke debugfs_file_get() at
all. Leave it alone for now.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 fs/debugfs/file.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 7de733ccdf6c..d92038c5f131 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -144,13 +144,13 @@ const struct file_operations debugfs_open_proxy_file_operations = {
 static ret_type full_proxy_ ## name(proto)				\
 {									\
 	struct dentry *dentry = F_DENTRY(filp);			\
-	const struct file_operations *real_fops =			\
-		debugfs_real_fops(filp);				\
+	const struct file_operations *real_fops;			\
 	ret_type r;							\
 									\
 	r = debugfs_file_get(dentry);					\
 	if (unlikely(r))						\
 		return r;						\
+	real_fops = debugfs_real_fops(filp);				\
 	r = real_fops->name(args);					\
 	debugfs_file_put(dentry);					\
 	return r;							\
@@ -177,13 +177,14 @@ FULL_PROXY_FUNC(unlocked_ioctl, long, filp,
 static unsigned int full_proxy_poll(struct file *filp,
 				struct poll_table_struct *wait)
 {
-	const struct file_operations *real_fops = debugfs_real_fops(filp);
 	struct dentry *dentry = F_DENTRY(filp);
 	unsigned int r = 0;
+	const struct file_operations *real_fops;
 
 	if (debugfs_file_get(dentry))
 		return POLLHUP;
 
+	real_fops = debugfs_real_fops(filp);
 	r = real_fops->poll(filp, wait);
 	debugfs_file_put(dentry);
 	return r;
-- 
2.12.2

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web