[CRAZY-RFF] debugfs: track open files and release on remove

[CRAZY-RFF] debugfs: track open files and release on remove

[Johannes Berg]

[RFF = Request For Flaming]

THIS IS PROBABLY COMPLETELY CRAZY.

Currently, if a module is unloaded while debugfs files are being
kept open, things crash since the ->release() method is called
only at the actual release, despite the proxy_fops, and then the
code is no longer around.

The correct way to fix this is to annotate all the debugfs fops
with .owner= THIS_MODULE, but as a learning exercise and to see
how much hate I can possibly receive, I figured I'd try to work
around this in debugfs itself.

First, if the fops have a release method and no owner, we track
all the open files - currently using a very simple array data
structure for it linked into struct debugfs_fsdata. This required
changing the API of debugfs_file_get() and debugfs_file_put() to
pass the struct file * to it.

Then, once we know which files are still open at remove time, we
can call all their release functions, and avoid calling them from
the proxy_fops. Of course still call them from the proxy_fops if
the file is still around, and clean up, so the release isn't all
deferred to the end, but done as soon as possible.

This introduces a potential locking issue, we could have a fops
struct that takes a lock in its release that the same module is
also taking around debugfs_remove(). To flag these issues using
lockdep, take inode_lock_shared() in full_proxy_release(), see
the comment there. If this triggers for some fops, add the owner
as it should have been in the first place.

Over adding the owner everywhere this has two small advantages:
 1) you can unload the module while a debugfs file is open;
 2) no need to change hundreds of places in the kernel :-)

(No, I won't sign off on this unless somebody _really_ wants it.)
---
 drivers/base/regmap/regmap-debugfs.c |   8 +-
 drivers/infiniband/hw/hfi1/debugfs.c |  10 +-
 drivers/infiniband/hw/hfi1/fault.c   |   8 +-
 fs/debugfs/file.c                    | 168 +++++++++++++++++++++------
 fs/debugfs/inode.c                   |  11 ++
 fs/debugfs/internal.h                |   7 ++
 include/linux/debugfs.h              |   4 +-
 7 files changed, 165 insertions(+), 51 deletions(-)

diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index b6d63ef16b44..1415fe9ba4c9 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -470,7 +470,7 @@ static ssize_t regmap_cache_only_write_file(struct file *file,
 	if (err)
 		return count;
 
-	err = debugfs_file_get(file->f_path.dentry);
+	err = debugfs_file_get(file);
 	if (err)
 		return err;
 
@@ -486,7 +486,7 @@ static ssize_t regmap_cache_only_write_file(struct file *file,
 	map->cache_only = new_val;
 
 	map->unlock(map->lock_arg);
-	debugfs_file_put(file->f_path.dentry);
+	debugfs_file_put(file);
 
 	if (require_sync) {
 		err = regcache_sync(map);
@@ -517,7 +517,7 @@ static ssize_t regmap_cache_bypass_write_file(struct file *file,
 	if (err)
 		return count;
 
-	err = debugfs_file_get(file->f_path.dentry);
+	err = debugfs_file_get(file);
 	if (err)
 		return err;
 
@@ -532,7 +532,7 @@ static ssize_t regmap_cache_bypass_write_file(struct file *file,
 	map->cache_bypass = new_val;
 
 	map->unlock(map->lock_arg);
-	debugfs_file_put(file->f_path.dentry);
+	debugfs_file_put(file);
 
 	return count;
 }
diff --git a/drivers/infiniband/hw/hfi1/debugfs.c b/drivers/infiniband/hw/hfi1/debugfs.c
index 2ced236e1553..81f38da1dee0 100644
--- a/drivers/infiniband/hw/hfi1/debugfs.c
+++ b/drivers/infiniband/hw/hfi1/debugfs.c
@@ -68,27 +68,25 @@ static struct dentry *hfi1_dbg_root;
 ssize_t hfi1_seq_read(struct file *file, char __user *buf, size_t size,
 		      loff_t *ppos)
 {
-	struct dentry *d = file->f_path.dentry;
 	ssize_t r;
 
-	r = debugfs_file_get(d);
+	r = debugfs_file_get(file);
 	if (unlikely(r))
 		return r;
 	r = seq_read(file, buf, size, ppos);
-	debugfs_file_put(d);
+	debugfs_file_put(file);
 	return r;
 }
 
 loff_t hfi1_seq_lseek(struct file *file, loff_t offset, int whence)
 {
-	struct dentry *d = file->f_path.dentry;
 	loff_t r;
 
-	r = debugfs_file_get(d);
+	r = debugfs_file_get(file);
 	if (unlikely(r))
 		return r;
 	r = seq_lseek(file, offset, whence);
-	debugfs_file_put(d);
+	debugfs_file_put(file);
 	return r;
 }
 
diff --git a/drivers/infiniband/hw/hfi1/fault.c b/drivers/infiniband/hw/hfi1/fault.c
index 0dfbcfb048ca..19b6e41458b6 100644
--- a/drivers/infiniband/hw/hfi1/fault.c
+++ b/drivers/infiniband/hw/hfi1/fault.c
@@ -146,7 +146,7 @@ static ssize_t fault_opcodes_write(struct file *file, const char __user *buf,
 		goto free_data;
 	}
 
-	ret = debugfs_file_get(file->f_path.dentry);
+	ret = debugfs_file_get(file);
 	if (unlikely(ret))
 		goto free_data;
 	ptr = data;
@@ -196,7 +196,7 @@ static ssize_t fault_opcodes_write(struct file *file, const char __user *buf,
 	}
 	ret = len;
 
-	debugfs_file_put(file->f_path.dentry);
+	debugfs_file_put(file);
 free_data:
 	kfree(data);
 	return ret;
@@ -215,7 +215,7 @@ static ssize_t fault_opcodes_read(struct file *file, char __user *buf,
 	data = kcalloc(datalen, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
-	ret = debugfs_file_get(file->f_path.dentry);
+	ret = debugfs_file_get(file);
 	if (unlikely(ret))
 		goto free_data;
 	bit = find_first_bit(fault->opcodes, bitsize);
@@ -231,7 +231,7 @@ static ssize_t fault_opcodes_read(struct file *file, char __user *buf,
 					 bit);
 		bit = find_next_bit(fault->opcodes, bitsize, zero);
 	}
-	debugfs_file_put(file->f_path.dentry);
+	debugfs_file_put(file);
 	data[size - 1] = '\n';
 	data[size] = '\0';
 	ret = simple_read_from_buffer(buf, len, pos, data, size);
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index a768a09430c3..fa758e774568 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -66,7 +66,7 @@ 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.
+ * @file: the file 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
@@ -79,8 +79,9 @@ EXPORT_SYMBOL_GPL(debugfs_real_fops);
  * 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)
+int debugfs_file_get(struct file *file)
 {
+	struct dentry *dentry = F_DENTRY(file);
 	struct debugfs_fsdata *fsd;
 	void *d_fsd;
 
@@ -94,9 +95,25 @@ int debugfs_file_get(struct dentry *dentry)
 
 		fsd->real_fops = (void *)((unsigned long)d_fsd &
 					~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
+
+		if (fsd->real_fops->release && !fsd->real_fops->owner) {
+			fsd->files = kzalloc(struct_size(fsd->files, files, 4),
+					     GFP_KERNEL);
+			if (!fsd->files) {
+				kfree(fsd);
+				return -ENOMEM;
+			}
+			fsd->files->n_alloc = 4;
+			fsd->files->n_used = 0;
+			mutex_init(&fsd->files_lock);
+		} else {
+			fsd->files = NULL;
+		}
+
 		refcount_set(&fsd->active_users, 1);
 		init_completion(&fsd->active_users_drained);
 		if (cmpxchg(&dentry->d_fsdata, d_fsd, fsd) != d_fsd) {
+			kfree(fsd->files);
 			kfree(fsd);
 			fsd = READ_ONCE(dentry->d_fsdata);
 		}
@@ -116,21 +133,60 @@ int debugfs_file_get(struct dentry *dentry)
 	if (!refcount_inc_not_zero(&fsd->active_users))
 		return -EIO;
 
+	if (fsd->files) {
+		struct debugfs_files_release *files;
+		bool found = false;
+		unsigned int i;
+
+		mutex_lock(&fsd->files_lock);
+		/* re-read under mutex */
+		files = READ_ONCE(fsd->files);
+		for (i = 0; i < files->n_used; i++) {
+			if (files->files[i] == file) {
+				found = true;
+				break;
+			}
+		}
+
+		if (!found) {
+			if (files->n_used >= files->n_alloc) {
+				struct debugfs_files_release *new;
+
+				files->n_alloc += 4;
+				new = krealloc(files,
+					       struct_size(new, files,
+							   files->n_alloc),
+					       GFP_KERNEL);
+				if (!new) {
+					files->n_alloc -= 4;
+					mutex_unlock(&fsd->files_lock);
+					refcount_dec(&fsd->active_users);
+					return -ENOMEM;
+				}
+				files = new;
+				fsd->files = files;
+			}
+			files->files[files->n_used++] = file;
+		}
+		mutex_unlock(&fsd->files_lock);
+	}
+
 	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().
+ * @file: the file 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)
+void debugfs_file_put(struct file *file)
 {
+	struct dentry *dentry = F_DENTRY(file);
 	struct debugfs_fsdata *fsd = READ_ONCE(dentry->d_fsdata);
 
 	if (refcount_dec_and_test(&fsd->active_users))
@@ -166,7 +222,7 @@ static int open_proxy_open(struct inode *inode, struct file *filp)
 	const struct file_operations *real_fops = NULL;
 	int r;
 
-	r = debugfs_file_get(dentry);
+	r = debugfs_file_get(filp);
 	if (r)
 		return r == -EIO ? -ENOENT : r;
 
@@ -195,7 +251,7 @@ static int open_proxy_open(struct inode *inode, struct file *filp)
 		r = real_fops->open(inode, filp);
 
 out:
-	debugfs_file_put(dentry);
+	debugfs_file_put(filp);
 	return r;
 }
 
@@ -209,16 +265,15 @@ 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)				\
 {									\
-	struct dentry *dentry = F_DENTRY(filp);			\
 	const struct file_operations *real_fops;			\
 	ret_type r;							\
 									\
-	r = debugfs_file_get(dentry);					\
+	r = debugfs_file_get(filp);					\
 	if (unlikely(r))						\
 		return r;						\
 	real_fops = debugfs_real_fops(filp);				\
 	r = real_fops->name(args);					\
-	debugfs_file_put(dentry);					\
+	debugfs_file_put(filp);						\
 	return r;							\
 }
 
@@ -243,16 +298,15 @@ FULL_PROXY_FUNC(unlocked_ioctl, long, filp,
 static __poll_t full_proxy_poll(struct file *filp,
 				struct poll_table_struct *wait)
 {
-	struct dentry *dentry = F_DENTRY(filp);
 	__poll_t r = 0;
 	const struct file_operations *real_fops;
 
-	if (debugfs_file_get(dentry))
+	if (debugfs_file_get(filp))
 		return EPOLLHUP;
 
 	real_fops = debugfs_real_fops(filp);
 	r = real_fops->poll(filp, wait);
-	debugfs_file_put(dentry);
+	debugfs_file_put(filp);
 	return r;
 }
 
@@ -261,16 +315,65 @@ 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;
+	struct debugfs_fsdata *fsd = dentry->d_fsdata;
 	int r = 0;
 
 	/*
-	 * We must not protect this against removal races here: the
-	 * original releaser should be called unconditionally in order
-	 * not to leak any resources. Releasers must not assume that
-	 * ->i_private is still being meaningful here.
+	 * If debugfs_file_get() fails here, the original releaser
+	 * was already called at debugfs_remove() time, so don't do
+	 * it again. This allows even removing a module while files
+	 * are open.
 	 */
-	if (real_fops->release)
+	if (!fsd->files) {
+		if (real_fops->release)
+			r = real_fops->release(inode, filp);
+	} else if (debugfs_file_get(filp) == 0) {
+		struct debugfs_files_release *files;
+
+		unsigned int i, old_n_used;
+
+		/*
+		 * XXX This is a hack - is this even allowed? XXX
+		 *
+		 * Take the inode lock here because we also hold it
+		 * during the other possible call to ->release during
+		 * debugfs_remove() over in __debugfs_file_removed().
+		 *
+		 * Taking it here also means that lockdep will *always*
+		 * record a chain from the inode lock to any locks the
+		 * debugfs user might take during release, and then if
+		 * they also hold the same locks during debugfs_remove()
+		 * it will complain.
+		 *
+		 * When it complains, we can then fix it by adding
+		 *
+		 *	.owner = THIS_MODULE
+		 *
+		 * to the relevant fops, in which case we won't get to
+		 * this path here, but to the above code block that's
+		 * only calling it directly.
+		 */
+		inode_lock_shared(inode);
 		r = real_fops->release(inode, filp);
+		inode_unlock_shared(inode);
+
+		mutex_lock(&fsd->files_lock);
+		/* re-read under mutex */
+		files = READ_ONCE(fsd->files);
+		old_n_used = files->n_used;
+		for (i = 0; i < files->n_used; i++) {
+			if (files->files[i] == filp) {
+				files->n_used--;
+				files->files[i] = files->files[files->n_used];
+				files->files[files->n_used] = NULL;
+				break;
+			}
+		}
+		WARN_ON(i >= old_n_used);
+		mutex_unlock(&fsd->files_lock);
+
+		debugfs_file_put(filp);
+	}
 
 	replace_fops(filp, d_inode(dentry)->i_fop);
 	kfree(proxy_fops);
@@ -301,7 +404,7 @@ static int full_proxy_open(struct inode *inode, struct file *filp)
 	struct file_operations *proxy_fops = NULL;
 	int r;
 
-	r = debugfs_file_get(dentry);
+	r = debugfs_file_get(filp);
 	if (r)
 		return r == -EIO ? -ENOENT : r;
 
@@ -351,7 +454,7 @@ static int full_proxy_open(struct inode *inode, struct file *filp)
 	kfree(proxy_fops);
 	fops_put(real_fops);
 out:
-	debugfs_file_put(dentry);
+	debugfs_file_put(filp);
 	return r;
 }
 
@@ -362,14 +465,13 @@ 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;
 
-	ret = debugfs_file_get(dentry);
+	ret = debugfs_file_get(file);
 	if (unlikely(ret))
 		return ret;
 	ret = simple_attr_read(file, buf, len, ppos);
-	debugfs_file_put(dentry);
+	debugfs_file_put(file);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(debugfs_attr_read);
@@ -377,14 +479,13 @@ 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;
 
-	ret = debugfs_file_get(dentry);
+	ret = debugfs_file_get(file);
 	if (unlikely(ret))
 		return ret;
 	ret = simple_attr_write(file, buf, len, ppos);
-	debugfs_file_put(dentry);
+	debugfs_file_put(file);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(debugfs_attr_write);
@@ -776,13 +877,12 @@ ssize_t debugfs_read_file_bool(struct file *file, char __user *user_buf,
 	char buf[3];
 	bool val;
 	int r;
-	struct dentry *dentry = F_DENTRY(file);
 
-	r = debugfs_file_get(dentry);
+	r = debugfs_file_get(file);
 	if (unlikely(r))
 		return r;
 	val = *(bool *)file->private_data;
-	debugfs_file_put(dentry);
+	debugfs_file_put(file);
 
 	if (val)
 		buf[0] = 'Y';
@@ -800,15 +900,14 @@ ssize_t debugfs_write_file_bool(struct file *file, const char __user *user_buf,
 	bool bv;
 	int r;
 	bool *val = file->private_data;
-	struct dentry *dentry = F_DENTRY(file);
 
 	r = kstrtobool_from_user(user_buf, count, &bv);
 	if (!r) {
-		r = debugfs_file_get(dentry);
+		r = debugfs_file_get(file);
 		if (unlikely(r))
 			return r;
 		*val = bv;
-		debugfs_file_put(dentry);
+		debugfs_file_put(file);
 	}
 
 	return count;
@@ -869,15 +968,14 @@ 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;
 
-	r = debugfs_file_get(dentry);
+	r = debugfs_file_get(file);
 	if (unlikely(r))
 		return r;
 	r = simple_read_from_buffer(user_buf, count, ppos, blob->data,
 				blob->size);
-	debugfs_file_put(dentry);
+	debugfs_file_put(file);
 	return r;
 }
 
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 2fcf66473436..f34202c1d7ee 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -694,6 +694,17 @@ static void __debugfs_file_removed(struct dentry *dentry)
 		return;
 	if (!refcount_dec_and_test(&fsd->active_users))
 		wait_for_completion(&fsd->active_users_drained);
+
+	if (fsd->files) {
+		unsigned int i;
+
+		for (i = 0; i < fsd->files->n_used; i++)
+			fsd->real_fops->release(d_inode(dentry),
+						fsd->files->files[i]);
+
+		kfree(fsd->files);
+		fsd->files = NULL;
+	}
 }
 
 static void remove_one(struct dentry *victim)
diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h
index 92af8ae31313..d898ef8108e6 100644
--- a/fs/debugfs/internal.h
+++ b/fs/debugfs/internal.h
@@ -15,10 +15,17 @@ 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_files_release {
+	unsigned int n_used, n_alloc;
+	struct file *files[];
+};
+
 struct debugfs_fsdata {
 	const struct file_operations *real_fops;
 	refcount_t active_users;
 	struct completion active_users_drained;
+	struct debugfs_files_release *files;
+	struct mutex files_lock;
 };
 
 /*
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 851dd1f9a8a5..ef32f7be19bc 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -93,8 +93,8 @@ void debugfs_remove(struct dentry *dentry);
 
 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);
+int debugfs_file_get(struct file *file);
+void debugfs_file_put(struct file *file);
 
 ssize_t debugfs_attr_read(struct file *file, char __user *buf,
 			size_t len, loff_t *ppos);
-- 
2.26.2

Re: [CRAZY-RFF] debugfs: track open files and release on remove

[Greg KH]

On Fri, Oct 09, 2020 at 09:53:06AM +0200, Johannes Berg wrote:
> [RFF = Request For Flaming]
> 
> THIS IS PROBABLY COMPLETELY CRAZY.
> 
> Currently, if a module is unloaded while debugfs files are being
> kept open, things crash since the ->release() method is called
> only at the actual release, despite the proxy_fops, and then the
> code is no longer around.
> 
> The correct way to fix this is to annotate all the debugfs fops
> with .owner= THIS_MODULE, but as a learning exercise and to see
> how much hate I can possibly receive, I figured I'd try to work
> around this in debugfs itself.

For lots of debugfs files, .owner should already be set, if you use the
DEFINE_SIMPLE_ATTRIBUTE() or DEFINE_DEBUGFS_ATTRIBUTE() macros.

But yes, not all.

> First, if the fops have a release method and no owner, we track
> all the open files - currently using a very simple array data
> structure for it linked into struct debugfs_fsdata. This required
> changing the API of debugfs_file_get() and debugfs_file_put() to
> pass the struct file * to it.
> 
> Then, once we know which files are still open at remove time, we
> can call all their release functions, and avoid calling them from
> the proxy_fops. Of course still call them from the proxy_fops if
> the file is still around, and clean up, so the release isn't all
> deferred to the end, but done as soon as possible.
> 
> This introduces a potential locking issue, we could have a fops
> struct that takes a lock in its release that the same module is
> also taking around debugfs_remove(). To flag these issues using
> lockdep, take inode_lock_shared() in full_proxy_release(), see
> the comment there. If this triggers for some fops, add the owner
> as it should have been in the first place.
> 
> Over adding the owner everywhere this has two small advantages:
>  1) you can unload the module while a debugfs file is open;
>  2) no need to change hundreds of places in the kernel :-)

I thought the proxy-ops stuff was supposed to fix this issue already.
Why isn't it, what is broken in them that causes this to still crash?

And of course, removing kernel modules is never a guaranteed operation,
nor is it anything that ever happens automatically, so is this really an
issue?  :)

thanks,

greg k-h

Re: [CRAZY-RFF] debugfs: track open files and release on remove

[Johannes Berg]

On Fri, 2020-10-09 at 10:03 +0200, Greg KH wrote:

> For lots of debugfs files, .owner should already be set, if you use the
> DEFINE_SIMPLE_ATTRIBUTE() or DEFINE_DEBUGFS_ATTRIBUTE() macros.
> 
> But yes, not all.

Right.

You didn't see the original thread:

https://lore.kernel.org/netdev/20201008155048.17679-1-ap420073@gmail.com/

> I thought the proxy-ops stuff was supposed to fix this issue already.
> Why isn't it, what is broken in them that causes this to still crash?

Well exactly what I described - the proxy_fops *release* doesn't get
proxied, since we don't have any knowledge of the open files (without
this patch) when the proxy_fops are redirected to nothing when a file is
removed.

Nicolai also discussed it a bit here:

https://lore.kernel.org/netdev/87v9fkgf4i.fsf@suse.de/

> And of course, removing kernel modules is never a guaranteed operation,
> nor is it anything that ever happens automatically, so is this really an
> issue?  :)

:)

We used to say the proxy_fops weren't needed and it wasn't an issue, and
then still implemented it. Dunno. I'm not really too concerned about it
myself, only root can hold the files open and remove modules ...

johannes

Re: [CRAZY-RFF] debugfs: track open files and release on remove

[Greg KH]

On Fri, Oct 09, 2020 at 10:06:14AM +0200, Johannes Berg wrote:
> We used to say the proxy_fops weren't needed and it wasn't an issue, and
> then still implemented it. Dunno. I'm not really too concerned about it
> myself, only root can hold the files open and remove modules ...

proxy_fops were needed because devices can be removed from the system at
any time, causing their debugfs files to want to also be removed.  It
wasn't because of unloading kernel code.

thanks,

greg k-h

Re: [CRAZY-RFF] debugfs: track open files and release on remove

[Johannes Berg]

On Fri, 2020-10-09 at 10:16 +0200, Greg KH wrote:
> On Fri, Oct 09, 2020 at 10:06:14AM +0200, Johannes Berg wrote:
> > We used to say the proxy_fops weren't needed and it wasn't an issue, and
> > then still implemented it. Dunno. I'm not really too concerned about it
> > myself, only root can hold the files open and remove modules ...
> 
> proxy_fops were needed because devices can be removed from the system at
> any time, causing their debugfs files to want to also be removed.  It
> wasn't because of unloading kernel code.

Indeed, that's true. Still, we lived with it for years.

Anyway, like I said, I really just did this more to see that it _could_
be done, not to suggest that it _should_ :-)

I think adding the .owner everywhere would be good, and perhaps we can
somehow put a check somewhere like

	WARN_ON(is_module_address((unsigned long)fops) && !fops->owner);

to prevent the issue in the future?

johannes

RE: [CRAZY-RFF] debugfs: track open files and release on remove

[David Laight]

From: Johannes Berg
> Sent: 09 October 2020 09:19
> 
> On Fri, 2020-10-09 at 10:16 +0200, Greg KH wrote:
> > On Fri, Oct 09, 2020 at 10:06:14AM +0200, Johannes Berg wrote:
> > > We used to say the proxy_fops weren't needed and it wasn't an issue, and
> > > then still implemented it. Dunno. I'm not really too concerned about it
> > > myself, only root can hold the files open and remove modules ...
> >
> > proxy_fops were needed because devices can be removed from the system at
> > any time, causing their debugfs files to want to also be removed.  It
> > wasn't because of unloading kernel code.
> 
> Indeed, that's true. Still, we lived with it for years.
> 
> Anyway, like I said, I really just did this more to see that it _could_
> be done, not to suggest that it _should_ :-)
> 
> I think adding the .owner everywhere would be good, and perhaps we can
> somehow put a check somewhere like
> 
> 	WARN_ON(is_module_address((unsigned long)fops) && !fops->owner);
> 
> to prevent the issue in the future?

Does it ever make any sense to set .owner to anything other than
THIS_MODULE?

If not the code that saves the 'struct file_operations' address
ought to be able to save the associated module.

I was also wondering if this affects normal opens?
They should hold a reference on the module to stop it being unloaded.
Does that rely on .owner being set?

For debugfs surely it is possible to determine and save THIS_MODULE
when he nodes are registers and do a try_module_get() in the open?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [CRAZY-RFF] debugfs: track open files and release on remove

[Johannes Berg]

On Fri, 2020-10-09 at 08:34 +0000, David Laight wrote:
> 
> > I think adding the .owner everywhere would be good, and perhaps we can
> > somehow put a check somewhere like
> > 
> > 	WARN_ON(is_module_address((unsigned long)fops) && !fops->owner);
> > 
> > to prevent the issue in the future?
> 
> Does it ever make any sense to set .owner to anything other than
> THIS_MODULE?

No. But I believe THIS_MODULE is NULL for built-in code, so we can't
just WARN_ON(!fops->owner).

> If not the code that saves the 'struct file_operations' address
> ought to be able to save the associated module.

No, it's const.

> I was also wondering if this affects normal opens?
> They should hold a reference on the module to stop it being unloaded.
> Does that rely on .owner being set?

Yes.

> For debugfs surely it is possible to determine and save THIS_MODULE
> when he nodes are registers and do a try_module_get() in the open?

I don't really see where to save it?

johannes

RE: [CRAZY-RFF] debugfs: track open files and release on remove

[David Laight]

From: Johannes Berg
> Sent: 09 October 2020 09:45
> 
> On Fri, 2020-10-09 at 08:34 +0000, David Laight wrote:
> >
...
> > Does it ever make any sense to set .owner to anything other than
> > THIS_MODULE?
> 
> No. But I believe THIS_MODULE is NULL for built-in code, so we can't
> just WARN_ON(!fops->owner).
...
> > I was also wondering if this affects normal opens?
> > They should hold a reference on the module to stop it being unloaded.
> > Does that rely on .owner being set?
> 
> Yes.

Sound like the module load code should be verifying it then.

Looking at one of my modules (which does set .owner).
Perhaps cdev_init() could be a #define that picks up THIS_MODULE.
This could then be checked against the one in fops or saved
in the 'struct cdev'.

I presume debugfs (which I've not used) has some similar calls.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [CRAZY-RFF] debugfs: track open files and release on remove

[Greg KH]

On Fri, Oct 09, 2020 at 10:19:02AM +0200, Johannes Berg wrote:
> On Fri, 2020-10-09 at 10:16 +0200, Greg KH wrote:
> > On Fri, Oct 09, 2020 at 10:06:14AM +0200, Johannes Berg wrote:
> > > We used to say the proxy_fops weren't needed and it wasn't an issue, and
> > > then still implemented it. Dunno. I'm not really too concerned about it
> > > myself, only root can hold the files open and remove modules ...
> > 
> > proxy_fops were needed because devices can be removed from the system at
> > any time, causing their debugfs files to want to also be removed.  It
> > wasn't because of unloading kernel code.
> 
> Indeed, that's true. Still, we lived with it for years.

Because no one wanted to fix the code, not because it was correct :)

> Anyway, like I said, I really just did this more to see that it _could_
> be done, not to suggest that it _should_ :-)

Agreed.

> I think adding the .owner everywhere would be good, and perhaps we can
> somehow put a check somewhere like
> 
> 	WARN_ON(is_module_address((unsigned long)fops) && !fops->owner);
> 
> to prevent the issue in the future?

That will fail for all of the debugfs_create_* operations, as there is
only one set of file operations for all of the different files created
with these calls.

Which, now that I remember it, is why we went down the proxy "solution"
in the first place :(

thanks,

greg k-h

Re: [CRAZY-RFF] debugfs: track open files and release on remove

[Johannes Berg]

On Fri, 2020-10-09 at 10:47 +0200, Greg KH wrote:

> > I think adding the .owner everywhere would be good, and perhaps we can
> > somehow put a check somewhere like
> > 
> > 	WARN_ON(is_module_address((unsigned long)fops) && !fops->owner);
> > 
> > to prevent the issue in the future?
> 
> That will fail for all of the debugfs_create_* operations, as there is
> only one set of file operations for all of the different files created
> with these calls.

Why would it fail? Those have their fops in the core debugfs code, which
might have a .owner assigned but is probably built-in anyway?

> Which, now that I remember it, is why we went down the proxy "solution"
> in the first place :(

Not sure I understand. That was related more to (arbitrary) files having
to be disappeared rather than anything else?

johannes

Re: [CRAZY-RFF] debugfs: track open files and release on remove

[Greg KH]

On Fri, Oct 09, 2020 at 10:48:09AM +0200, Johannes Berg wrote:
> On Fri, 2020-10-09 at 10:47 +0200, Greg KH wrote:
> 
> > > I think adding the .owner everywhere would be good, and perhaps we can
> > > somehow put a check somewhere like
> > > 
> > > 	WARN_ON(is_module_address((unsigned long)fops) && !fops->owner);
> > > 
> > > to prevent the issue in the future?
> > 
> > That will fail for all of the debugfs_create_* operations, as there is
> > only one set of file operations for all of the different files created
> > with these calls.
> 
> Why would it fail? Those have their fops in the core debugfs code, which
> might have a .owner assigned but is probably built-in anyway?

Bad choice of terms, it would "fail" in that this type of check would
never actually work because the debugfs code is built into the kernel,
and there is no module owner for it.  But the value it is referencing is
an address in a module.

> > Which, now that I remember it, is why we went down the proxy "solution"
> > in the first place :(
> 
> Not sure I understand. That was related more to (arbitrary) files having
> to be disappeared rather than anything else?

Isn't this the same issue?

thanks,

greg k-h

Re: [CRAZY-RFF] debugfs: track open files and release on remove

[Johannes Berg]

On Sat, 2020-10-10 at 11:38 +0200, Greg KH wrote:
> On Fri, Oct 09, 2020 at 10:48:09AM +0200, Johannes Berg wrote:
> > On Fri, 2020-10-09 at 10:47 +0200, Greg KH wrote:
> > 
> > > > I think adding the .owner everywhere would be good, and perhaps we can
> > > > somehow put a check somewhere like
> > > > 
> > > > 	WARN_ON(is_module_address((unsigned long)fops) && !fops->owner);
> > > > 
> > > > to prevent the issue in the future?
> > > 
> > > That will fail for all of the debugfs_create_* operations, as there is
> > > only one set of file operations for all of the different files created
> > > with these calls.
> > 
> > Why would it fail? Those have their fops in the core debugfs code, which
> > might have a .owner assigned but is probably built-in anyway?
> 
> Bad choice of terms, it would "fail" in that this type of check would
> never actually work because the debugfs code is built into the kernel,
> and there is no module owner for it.  But the value it is referencing is
> an address in a module.

Ahh.

Yes and no. I mean, yes, the check wouldn't really work.

But OTOH, this is exactly what the proxy_fops protects against.

The _only_ thing that proxy_fops *doesn't* proxy is the ->release()
method.

If you have a debugfs file that's say debugfs_create_u32(), then the
code is all built into the kernel, and - if ->release() even exists, I
didn't check now - it would surely not dereference the pointer you gave
to debugfs_create_u32(). So as long as the file is debugfs_remove()d
before the pointer becomes invalid, there's no issue.

The check I'm proposing (and actually wrote in my separate RFC patch
that didn't seem quite as crazy) would basically protect the ->release()
method only, if needed. Everything else is handled by proxy_fops.

> > > Which, now that I remember it, is why we went down the proxy "solution"
> > > in the first place :(
> > 
> > Not sure I understand. That was related more to (arbitrary) files having
> > to be disappeared rather than anything else?
> 
> Isn't this the same issue?

Well, not exactly? The difference is that proxy_fops basically protects
the *value*, read/write/etc., but not ->release(). So it protects more
against bus unbind or the like, where the *device* disappears, rather
than the *code* disappearing.

Now, you still need to be careful that ->release() doesn't actually
access anything related to the device, of course. As long as we don't
have a general revoke() at least.

I guess in that sense this crazy patch actually makes things *better*
than the RFC patch because it *does* call the ->release() during
debugfs_remove() and therefore allows even ->release() to access data of
the device or other data structures that are being removed; whereas the
RFC patch I also sent doesn't protect that, it just protects the code
itself.

johannes