Messages in this thread | | | Date | Wed, 21 Sep 2005 04:54:55 +0100 | From | Al Viro <> | Subject | Re: [PATCH 12/12] HPPFS: fix nameidata handling |
| |
On Sun, Sep 18, 2005 at 04:10:09PM +0200, Paolo 'Blaisorblade' Giarrusso wrote: > From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it> > > In follow_link, we call the underlying method with the same nameidata we got - > it will then call path_release() and then dput()/mntput() on hppfs dentries / > vfsmount rather than his own, which could be problematic (I'm not really sure, > however). > > This issue exists potentially also for other methods getting nameidata. Fix > this. > > However, I couldn't make a lot of sense of the reference counting used in > namei.c. So I'm uncertain whether this patch makes sense. Al, please have a > critical eye toward this one. Especially, proc_pid_follow_link calls > path_release itself. Which makes me wonder a lot.
> + sav_mnt = nd->mnt; > + > + nd->dentry = dget(proc_dentry); > + nd->mnt = mntget(proc_submnt); > + ret = (*d_revalidate)(proc_dentry, nd); > + path_release(nd);
Don't bother with that, procfs doesn't and will not care anyway. It _is_ legal to pass NULL as nd, so you've actually introduced breakage here. Just pass NULL and be done with that.
> + sav_dentry = nd->dentry; > + sav_mnt = nd->mnt; > + > + nd->dentry = dget(proc_dentry); > + nd->mnt = mntget(proc_submnt); > new = (*parent->d_inode->i_op->lookup)(parent->d_inode, > - proc_dentry, NULL); > + proc_dentry, nd); > + path_release(nd); > + > + nd->dentry = sav_dentry; > + nd->mnt = sav_mnt; > if(new){ > dput(proc_dentry); > proc_dentry = new;
Ditto.
> @@ -213,11 +240,20 @@ static struct dentry *hppfs_lookup(struc > } else { > up(&parent->d_inode->i_sem); > if (proc_dentry->d_op && proc_dentry->d_op->d_revalidate) { > - if (!proc_dentry->d_op->d_revalidate(proc_dentry, NULL) && > + sav_dentry = nd->dentry; > + sav_mnt = nd->mnt; > + > + nd->dentry = dget(proc_dentry); > + nd->mnt = mntget(proc_submnt); > + if (!proc_dentry->d_op->d_revalidate(proc_dentry, nd) && > !d_invalidate(proc_dentry)) { > dput(proc_dentry); > proc_dentry = ERR_PTR(-ENOENT); > } > + path_release(nd); > + > + nd->dentry = sav_dentry; > + nd->mnt = sav_mnt; > } > }
Shouldn't be there at all (use lookup_one_len() instead of open-coding it)
> @@ -248,16 +284,32 @@ static struct dentry *hppfs_lookup(struc > > static int hppfs_permission(struct inode *inode, int mask, struct nameidata *nd) > { > + struct dentry *sav_dentry; > + struct vfsmount *sav_mnt; > + > struct inode *proc_inode; > + struct dentry *proc_dentry; > int (*permission) (struct inode *, int, struct nameidata *); > + int ret; > > - proc_inode = HPPFS_I(inode)->proc_dentry->d_inode; > + proc_dentry = HPPFS_I(inode)->proc_dentry; > + proc_inode = proc_dentry->d_inode; > permission = proc_inode->i_op->permission; > > if (permission == NULL) > return generic_permission(inode, mask, NULL); > > - return (*permission)(proc_inode, mask, nd); > + sav_dentry = nd->dentry; > + sav_mnt = nd->mnt; > + > + nd->dentry = dget(proc_dentry); > + nd->mnt = mntget(proc_submnt); > + ret = (*permission)(proc_inode, mask, nd); > + path_release(nd); > + > + nd->dentry = sav_dentry; > + nd->mnt = sav_mnt; > + return ret; > }
Same note about NULL.
> static struct inode_operations hppfs_file_iops = { > @@ -794,6 +846,9 @@ static int hppfs_readlink(struct dentry > > static void* hppfs_follow_link(struct dentry *dentry, struct nameidata *nd) > { > + struct dentry *sav_dentry; > + struct vfsmount *sav_mnt; > + > struct dentry *proc_dentry; > void * (*follow_link)(struct dentry *, struct nameidata *); > void *ret; > @@ -808,7 +863,18 @@ static void* hppfs_follow_link(struct de > if (follow_link == NULL) > return ERR_PTR(-EOPNOTSUPP); > > + /* We have a reference on this already - so it won't go.*/ > + sav_dentry = nd->dentry; > + sav_mnt = nd->mnt; > + > + nd->dentry = dget(proc_dentry); > + nd->mnt = mntget(proc_submnt); > ret = follow_link(proc_dentry, nd); > + /* XXX: would this be done normally when calling follow_link or not? */ > + path_release(nd); > + > + nd->dentry = sav_dentry; > + nd->mnt = sav_mnt;
And this is absolutely bogus. The whole point of ->follow_link() is to move where your nameidata points to. So no, you do _not_ want to flip nameidata, you do not want to drop it and you certainly do not want to flip it back. Just call the underlying one. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |