Re: [PATCH v4] Fix rmmod/read/write races in /proc entries

[Alexey Dobriyan <adobriyan@openvz.org>]

On Mon, Feb 12, 2007 at 10:35:52PM -0800, Andrew Morton wrote:
> On Sun, 11 Feb 2007 23:23:30 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > [PATCH v4] Fix rmmod/read/write races in /proc entries
> 
> This:
> 
> static ssize_t
> proc_file_write(struct file *file, const char __user *buffer,
> 		size_t count, loff_t *ppos)
> {
> 	struct inode *inode = file->f_path.dentry->d_inode;
> 	struct proc_dir_entry * dp;
> 	ssize_t rv = -EIO;
> 	
> 	dp = PDE(inode);
> 
> 	if (!dp->write_proc)
> 		goto out;
> 
> 	spin_lock(&dp->pde_unload_lock);
> 	if (!dp->proc_fops)
> 		/*
> 		 * remove_proc_entry() marked PDE as "going away".
> 		 * No new writers allowed.
> 		 */
> 		goto out_unlock;
> 
> versus
> 
> 		spin_lock(&de->pde_unload_lock);
> 		/*
> 		 * Stop accepting new readers/writers. If you're dynamically
> 		 * allocating ->proc_fops, save a pointer somewhere.
> 		 */
> 		de->proc_fops = NULL;
> 		/* Wait until all existing readers/writers are done. */
> 		if (de->pde_users > 0) {
> 			struct completion c;
>
> 			init_completion(&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);
>   <here>
> 	...
> 	<free de>
>
> What prevents proc_file_write() from looking up and playing with this de in
> <here>?

If I understood your two-column diagram correctly, scenario below can't
happen because of PDE's own refcount (->count) and existence of
->deleted (0/1)

remove_proc_entry() sees positive ->count and doesn't immediately free
PDE. remove_proc_entry() will at most a) lock b) access to check
->proc_fops which is NULL now, and c) unlock which is fine because
memory is in place.

->count is bumped in proc_get_inode after checking PDEs lists, but our
PDE was already removed from it.