debugfs: fixes to "file removal race" series

Hi Greg,

the recent report by Sasha Levin made me review my debugfs "file removal race" series again.

For your reference, the original series in question was posted here:
http://lkml.kernel.org/g/[email protected]
It has been merged through your driver-core tree into mainline.

Unfortunately, I found three issues:

The first one is a minor code style issue.
The second one is more serious: on ->open() failure, references to modules as well as memory are leaked.
The third one is a potential leak of module references with future debugfs users.

The patches don't depend on each other and are unrelated except that they all address issues introduced by the same series.
I tested them with linux-next 20160523.

Apologies and thanks,

Nicolai

Nicolai Stange (3):
debugfs: remove extra debugfs_create_file_unsafe() declaration
debugfs: full_proxy_open(): free proxy on ->open() failure
debugfs: open_proxy_open(): avoid double fops release

 fs/debugfs/file.c     | 7 ++++---
 fs/debugfs/internal.h | 4 ----
 2 files changed, 4 insertions(+), 7 deletions(-)

2.8.2

debugfs_create_file_unsafe() is declared twice in exactly the same manner each: once in fs/debugfs/internal.h and once in include/linux/debugfs.h

All files that include the former also include the latter and thus, the declaration in fs/debugfs/internal.h is superfluous.

Remove it.

---
 fs/debugfs/internal.h | 4 ----
 1 file changed, 4 deletions(-)

index bba5263..b3e8443 100644
@@ -19,8 +19,4 @@ 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 dentry *debugfs_create_file_unsafe(const char *name, umode_t mode,
-					struct dentry *parent, void *data,
-					const struct file_operations *fops);
-
 #endif /* _DEBUGFS_INTERNAL_H_ */

2.8.2

Debugfs' full_proxy_open(), the ->open() installed at all inodes created through debugfs_create_file(),
- grabs a reference to the original struct file_operations instance passed to debugfs_create_file(),
- dynamically allocates a proxy struct file_operations instance wrapping the original
- and installs this at the file's ->f_op.

Afterwards, it calls the original ->open() and passes its return value back to the VFS layer.

Now, if that return value indicates failure, the VFS layer won't ever call ->release() and thus, neither the reference to the original file_operations nor the memory for the proxy file_operations will get released, i.e. both are leaked.

Upon failure of the original fops' ->open(), undo the proxy installation. That is:
- Set the struct file ->f_op to what it had been when full_proxy_open() was entered.
- Drop the reference to the original file_operations.
- Free the memory holding the proxy file_operations.

                      data")

---
 fs/debugfs/file.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index 9c1c9a0..d1ec803 100644
@@ -262,8 +262,10 @@ static int full_proxy_open(struct inode *inode, struct file *filp)
 
 	if (real_fops->open) {
 		r = real_fops->open(inode, filp);
-
-		if (filp->f_op != proxy_fops) {
+		if (r) {
+			replace_fops(filp, d_inode(dentry)->i_fop);
+			goto free_proxy;
+		} else if (filp->f_op != proxy_fops) {
 			/* No protection against file removal anymore. */
 			WARN(1, "debugfs file owner replaced proxy fops: %pd",
 				dentry);

2.8.2

Debugfs' open_proxy_open(), the ->open() installed at all inodes created through debugfs_create_file_unsafe(),
- grabs a reference to the original file_operations instance passed to debugfs_create_file_unsafe() via fops_get(),
- installs it at the file's ->f_op by means of replace_fops()
- and calls fops_put() on it.

Since the semantics of replace_fops() are such that the reference's ownership is transferred, the subsequent fops_put() will result in a double release when the file is eventually closed.

Currently, this is not an issue since fops_put() basically does a module_put() on the file_operations' ->owner only and there don't exist any modules calling debugfs_create_file_unsafe() yet. This is expected to change in the future though, c.f. commit c64688081490 ("debugfs: add support for self-protecting attribute file fops").

Remove the call to fops_put() from open_proxy_open().

file_operations at file open")

---
 fs/debugfs/file.c | 1 -
 1 file changed, 1 deletion(-)

index d1ec803..592059f 100644
@@ -127,7 +127,6 @@ static int open_proxy_open(struct inode *inode, struct file *filp)
 		r = real_fops->open(inode, filp);
 
 out:
-	fops_put(real_fops);
 	debugfs_use_file_finish(srcu_idx);
 	return r;
 }

2.8.2