Messages in this thread | | | Date | Wed, 3 Sep 2008 18:17:56 +0400 | From | Alexey Dobriyan <> | Subject | Re: [PATCH] proc: fix return value of proc_reg_open() in "too late" case |
| |
On Tue, Sep 02, 2008 at 04:26:24PM -0700, Andrew Morton wrote: > On Sat, 30 Aug 2008 09:34:12 +0400 > Alexey Dobriyan <adobriyan@gmail.com> wrote: > > > If ->open() wasn't called, returning 0 is misleading and, theoretically, > > oopsable: > > 1. remove_proc_entry clears ->proc_fops, drops lock, > > 2. ->open "succeeds", > > 3. ->release oopses, because it assumes ->open was called (single_release()). > > > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> > > --- > > > > fs/proc/inode.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > --- a/fs/proc/inode.c > > +++ b/fs/proc/inode.c > > @@ -350,7 +350,7 @@ static int proc_reg_open(struct inode *inode, struct file *file) > > if (!pde->proc_fops) { > > spin_unlock(&pde->pde_unload_lock); > > kfree(pdeo); > > - return rv; > > + return -EINVAL; > > } > > pde->pde_users++; > > open = pde->proc_fops->open; > > Can this code path ever actually be executed? afacit if ->proc_fops is > ever NULL, the caller (proc_get_inode) would have already oopsed: > > #ifdef CONFIG_COMPAT > if (!de->proc_fops->compat_ioctl) > inode->i_fop = > &proc_reg_file_ops_no_compat; > else > #endif > inode->i_fop = &proc_reg_file_ops;
Yes, it can.
remove_proc_entry() clears ->proc_fops to indicate that removal of PDE started. But somebody could have already open(2) and is currently at the beginning of proc_reg_open().
Well, that's how bug was noticed in the first place.
| |