Fix rmmod/read/write races in /proc entries

Differences from version 4:

	Updated in-code comments. Largely rewritten changelog.
	Lockdep please. --akpm
	->read_proc, ->write_proc aren't special, Extend protection to
	most methods for regular /proc files. Mentioned by viro.

Differences from version 3:
Use completion instead of unlock/schedule/lock Move refcount waiting business after removing PDE from lists, so that *cough* possible concurrent remove_proc_entry() will work.

Fix following races:
===========================================
1. Write via ->write_proc sleeps in copy_from_user(). Module disappears meanwhile. Or, more generically, system call done on /proc file, method supplied by module is called, module dissapeares meanwhile.

   pde = create_proc_entry()
   if (!pde)
	return -ENOMEM;
   pde->write_proc = ...
				open
				write
				copy_from_user
   pde = create_proc_entry();
   if (!pde) {
	remove_proc_entry();
	return -ENOMEM;
	/* module unloaded */
   }

==========================================
2. bogo-revoke aka proc_kill_inodes()

remove_proc_entry vfs_read
proc_kill_inodes [check ->f_op validness]

				[check ->f_op->read validness]
				[verify_area, security permissions checks]
	->f_op = NULL;
				if (file->f_op->read)
					/* ->f_op dereference, boom */

NOTE, NOTE, NOTE: file_operations are proxied for regular files only. Let's see how this scheme behaves, then extend if needed for directories. Directories creators in /proc only set ->owner for them, so proxying for directories may be unneeded.

NOTE, NOTE, NOTE: methods being proxied are ->llseek, ->read, ->write, ->poll, ->unlocked_ioctl, ->ioctl, ->compat_ioctl, ->open, ->release. If your in-tree module uses something else, yell on me. Full audit pending.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 fs/proc/generic.c       |   32 +++++
 fs/proc/inode.c         |  279 +++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/proc_fs.h |   13 ++
 3 files changed, 321 insertions(+), 3 deletions(-)

fs/proc/generic.c +31 -1

@@ -20,6 +20,7 @@ #include <linux/idr.h>
 #include <linux/namei.h>
 #include <linux/bitops.h>
 #include <linux/spinlock.h>
+#include <linux/completion.h>
 #include <asm/uaccess.h>
 
 #include "internal.h"

@@ -613,6 +614,9 @@ static struct proc_dir_entry *proc_creat
 	ent->namelen = len;
 	ent->mode = mode;
 	ent->nlink = nlink;
+	ent->pde_users = 0;
+	spin_lock_init(&ent->pde_unload_lock);
+	ent->pde_unload_completion = NULL;
  out:
 	return ent;
 }

@@ -734,9 +738,35 @@ void remove_proc_entry(const char *name,
 		de = *p;
 		*p = de->next;
 		de->next = NULL;
+
+		spin_lock(&de->pde_unload_lock);
+		/*
+		 * Stop accepting new callers into module. If you're
+		 * dynamically allocating ->proc_fops, save a pointer somewhere.
+		 */
+		de->proc_fops = NULL;
+		/* Wait until all existing callers into module are done. */
+		if (de->pde_users > 0) {
+			DECLARE_COMPLETION_ONSTACK(c);
+
+			if (!de->pde_unload_completion)
+				de->pde_unload_completion = &c;
+
+			spin_unlock(&de->pde_unload_lock);
+			spin_unlock(&proc_subdir_lock);
+
+			wait_for_completion(de->pde_unload_completion);
+
+			spin_lock(&proc_subdir_lock);
+			goto continue_removing;
+		}
+		spin_unlock(&de->pde_unload_lock);
+
+continue_removing:
 		if (S_ISDIR(de->mode))
 			parent->nlink--;
-		proc_kill_inodes(de);
+		if (!S_ISREG(de->mode))
+			proc_kill_inodes(de);
 		de->nlink = 0;
 		WARN_ON(de->subdir);
 		if (!atomic_read(&de->count))

fs/proc/inode.c +24 -0

@@ -142,6 +142,277 @@ static const struct super_operations pro
 	.remount_fs	= proc_remount,
 };
 
+static loff_t proc_reg_llseek(struct file *file, loff_t offset, int whence)
+{
+	struct proc_dir_entry *pde = PDE(file->f_path.dentry->d_inode);
+	loff_t rv = -EINVAL;
+	loff_t (*llseek)(struct file *, loff_t, int);
+
+	spin_lock(&pde->pde_unload_lock);
+	/*
+	 * remove_proc_entry() is going to delete PDE (as part of module
+	 * cleanup sequence). No new callers into module allowed.
+	 */
+	if (!pde->proc_fops)
+		goto out_unlock;
+	/*
+	 * Bump refcount so that remove_proc_entry will wail for ->llseek to
+	 * complete.
+	 */
+	pde->pde_users++;
+	/*
+	 * Save function pointer under lock, to protect against ->proc_fops
+	 * NULL'ifying right after ->pde_unload_lock is dropped.
+	 */
+	llseek = pde->proc_fops->llseek;
+	spin_unlock(&pde->pde_unload_lock);

My, what a lot of code you have here. I note that nobody can be assed even reviewing it. Now why is that?

Quoted 6 lines

Fix following races:
===========================================
1. Write via ->write_proc sleeps in copy_from_user(). Module disappears meanwhile. Or, more generically, system call done on /proc file, method supplied by module is called, module dissapeares meanwhile.

Quoted 13 lines

   pde = create_proc_entry()
   if (!pde)
	return -ENOMEM;
   pde->write_proc = ...
				open
				write
				copy_from_user
   pde = create_proc_entry();
   if (!pde) {
	remove_proc_entry();
	return -ENOMEM;
	/* module unloaded */
   }

We usually fix that race by pinning the module: make whoever registered the proc entries also register their THIS_MODULE, do a try_module_get() on it before we start to play with data structures which the module owns.

Can we do that here?

And is the above race fix related to the below one in any fashion?

Quoted 10 lines

==========================================
2. bogo-revoke aka proc_kill_inodes()

remove_proc_entry vfs_read
proc_kill_inodes [check ->f_op validness]

				[check ->f_op->read validness]
				[verify_area, security permissions checks]
	->f_op = NULL;
				if (file->f_op->read)
					/* ->f_op dereference, boom */

So you fixed this via sort-of-refcounting on pde->pde_users.

hmm.

Quoted 3 lines

fs/proc/generic.c +2 -1

-		proc_kill_inodes(de);
+		if (!S_ISREG(de->mode))
+			proc_kill_inodes(de);

fs/proc/inode.c static const struct super_operations pro · line 178

Quoted 6 lines

fs/proc/inode.c +6 -0

+	spin_lock(&pde->pde_unload_lock);
+	pde->pde_users--;
+	if (pde->pde_unload_completion && pde->pde_users == 0)
+		complete(pde->pde_unload_completion);
+out_unlock:
+	spin_unlock(&pde->pde_unload_lock);

The above six lines happen rather a lot - perhaps it could be placed in a helper funtion?

Quoted 2 lines · Andrew Morton

My, what a lot of code you have here. I note that nobody can be assed even reviewing it. Now why is that?

I hope, Al could find some time again.

Quoted 21 lines · On Sun, 11 Mar 2007 20:04:56 +0300 Alexey Dobriyan <[email protected]> wrote:

Quoted 6 lines

Fix following races:
===========================================
1. Write via ->write_proc sleeps in copy_from_user(). Module disappears meanwhile. Or, more generically, system call done on /proc file, method supplied by module is called, module dissapeares meanwhile.

Quoted 13 lines

   pde = create_proc_entry()
   if (!pde)
	return -ENOMEM;
   pde->write_proc = ...
				open
				write
				copy_from_user
   pde = create_proc_entry();
   if (!pde) {
	remove_proc_entry();
	return -ENOMEM;
	/* module unloaded */
   }

Quoted 5 lines

We usually fix that race by pinning the module: make whoever registered the proc entries also register their THIS_MODULE, do a try_module_get() on it before we start to play with data structures which the module owns.

Can we do that here?

We can, but it will be unreliable:

Typical proc entry creation sequence is

	pde = create_proc_entry(...);
	if (pde)
		pde->owner = THIS_MODULE;

Right after create_proc_entry() ->owner is NULL, so try_module_get() won't do anything, but proc_delete_inode() could put module which was never getted.

This should fixable by always setting ->owner before proc entry is glued to proc entries tree. Something like this:

	#define create_proc_entry(...) __create_proc_entry(..., THIS_MODULE)

However, I think it's not enough: delete_module(2) first waits for refcount becoming zero, only then calls modules's exit function which starts removing proc entries. In between, proc entries are accessible and fully-functional, so try_module_get() can again get module and module_put(pde->owner) can happen AFTER module dissapears. What will it put?

And how can you fix that? The only way I know is to REMOVE ->owner completely, once we agree on this pde_users/pde_unload_lock stuff.

Quoted 41 lines · On Fri, 16 Mar 2007 12:16:13 +0300 Alexey Dobriyan <[email protected]> wrote:

Quoted 2 lines · Andrew Morton

My, what a lot of code you have here. I note that nobody can be assed even reviewing it. Now why is that?

I hope, Al could find some time again.

Quoted 21 lines · On Sun, 11 Mar 2007 20:04:56 +0300 Alexey Dobriyan <[email protected]> wrote:

Quoted 6 lines

Fix following races:
===========================================
1. Write via ->write_proc sleeps in copy_from_user(). Module disappears meanwhile. Or, more generically, system call done on /proc file, method supplied by module is called, module dissapeares meanwhile.

Quoted 13 lines

   pde = create_proc_entry()
   if (!pde)
	return -ENOMEM;
   pde->write_proc = ...
				open
				write
				copy_from_user
   pde = create_proc_entry();
   if (!pde) {
	remove_proc_entry();
	return -ENOMEM;
	/* module unloaded */
   }

Quoted 5 lines

We usually fix that race by pinning the module: make whoever registered the proc entries also register their THIS_MODULE, do a try_module_get() on it before we start to play with data structures which the module owns.

Can we do that here?

We can, but it will be unreliable:

Typical proc entry creation sequence is

	pde = create_proc_entry(...);
	if (pde)
		pde->owner = THIS_MODULE;

Quoted 8 lines

Right after create_proc_entry() ->owner is NULL, so try_module_get() won't do anything, but proc_delete_inode() could put module which was never getted.

This should fixable by always setting ->owner before proc entry is glued to proc entries tree. Something like this:

	#define create_proc_entry(...) __create_proc_entry(..., THIS_MODULE)

Yes, I was thinking of something like that.

Quoted 7 lines

However, I think it's not enough: delete_module(2) first waits for refcount becoming zero, only then calls modules's exit function which starts removing proc entries. In between, proc entries are accessible and fully-functional, so try_module_get() can again get module and module_put(pde->owner) can happen AFTER module dissapears. What will it put?

Quoted 2 lines

And how can you fix that? The only way I know is to REMOVE ->owner completely, once we agree on this pde_users/pde_unload_lock stuff.

I think the rmmod code will take care of that.

Once delete_module() has called try_stop_module(), no following try_module_get() will succeed. And see that wait_for_zero_refcount() call in there which waits for any present users of the module to go away.