[PATCH 12/12] HPPFS: fix nameidata handling

[PATCH 12/12] HPPFS: fix nameidata handling

[Paolo 'Blaisorblade' Giarrusso]

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.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 fs/hppfs/hppfs_kern.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 71 insertions(+), 5 deletions(-)

diff --git a/fs/hppfs/hppfs_kern.c b/fs/hppfs/hppfs_kern.c
--- a/fs/hppfs/hppfs_kern.c
+++ b/fs/hppfs/hppfs_kern.c
@@ -15,6 +15,7 @@
 #include <linux/dcache.h>
 #include <linux/statfs.h>
 #include <linux/mount.h>
+#include <linux/namei.h>
 #include <asm/uaccess.h>
 #include <asm/fcntl.h>
 #include "os.h"
@@ -106,8 +107,12 @@ static char *dentry_name(struct dentry *
 
 static int hppfs_d_revalidate(struct dentry * dentry, struct nameidata * nd)
 {
+	struct dentry *sav_dentry;
+	struct vfsmount *sav_mnt;
+
 	int (*d_revalidate)(struct dentry *, struct nameidata *);
 	struct dentry *proc_dentry;
+	int ret;
 
 	proc_dentry = HPPFS_I(dentry->d_inode)->proc_dentry;
 	if (proc_dentry->d_op && proc_dentry->d_op->d_revalidate)
@@ -115,7 +120,17 @@ static int hppfs_d_revalidate(struct den
 	else
 		return 1; /* "Still valid" code */
 
-	return (*d_revalidate)(proc_dentry, nd);
+	sav_dentry = nd->dentry;
+	sav_mnt = nd->mnt;
+
+	nd->dentry = dget(proc_dentry);
+	nd->mnt = mntget(proc_submnt);
+	ret = (*d_revalidate)(proc_dentry, nd);
+	path_release(nd);
+
+	nd->dentry = sav_dentry;
+	nd->mnt = sav_mnt;
+	return ret;
 }
 
 static struct dentry_operations hppfs_dentry_ops = {
@@ -175,6 +190,9 @@ static void hppfs_read_inode(struct inod
 static struct dentry *hppfs_lookup(struct inode *parent_ino, struct dentry *dentry,
                                   struct nameidata *nd)
 {
+	struct dentry *sav_dentry;
+	struct vfsmount *sav_mnt;
+
 	struct dentry *proc_dentry, *new, *parent;
 	struct inode *inode;
 	int err, deleted;
@@ -203,8 +221,17 @@ static struct dentry *hppfs_lookup(struc
 			up(&parent->d_inode->i_sem);
 			goto out;
 		}
+		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;
@@ -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;
 		}
 	}
 
@@ -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;
 }
 
 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;
 
 	return ret;
 }

Re: [PATCH 12/12] HPPFS: fix nameidata handling

[Al Viro]

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.

Re: [uml-devel] Re: [PATCH 12/12] HPPFS: fix nameidata handling

[Blaisorblade]

On Wednesday 21 September 2005 05:54, Al Viro wrote:
> 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).

> Don't bother with that, procfs doesn't and will not care anyway.  It _is_
> legal to pass NULL as nd,
Where? At least not in proc_follow_link, I don't know for the rest. 

I'm now seeing lookup_one_len doing exactly that. But still, could these 
conventions be documented, as they're surely untrivial to guess?

> so you've actually introduced breakage here. 
> Just pass NULL and be done with that.

> > @@ -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)


> >  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;
> > +
[...]
> 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.
But wouldn't the callee expect that nd->dentry is the same thing which I pass?

Yes, nameidata has other fields too, but is ->dentry meant to be unused here?
Not surely.

And especially, how would proc_pid_follow_link()'s path_release() call handle 
that?

I've been suspicious that, indeed, recursive lookup holds a ref on each 
pathname component dentry, and that proc, in that case, knows it can do 
without.

-- 
Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894)
http://www.user-mode-linux.org/~blaisorblade

	

	
		
___________________________________ 
Yahoo! Mail: gratis 1GB per i messaggi e allegati da 10MB 
http://mail.yahoo.it

Re: [uml-devel] Re: [PATCH 12/12] HPPFS: fix nameidata handling

[Al Viro]

On Wed, Sep 21, 2005 at 06:34:11PM +0200, Blaisorblade wrote:
> > 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.

> But wouldn't the callee expect that nd->dentry is the same thing which I pass?

No.

> Yes, nameidata has other fields too, but is ->dentry meant to be unused here?
> Not surely.

Yes, it is used.  As in "here's where we stand in the tree, move as
specified by your symlink (or leave the pathname to be traversed by caller)".

> And especially, how would proc_pid_follow_link()'s path_release() call handle 
> that?

It would drop the reference to previous location and slap relevant
vsmount and dentry instead - which is exactly what we want from
e.g. following /proc/<pid>/root.

->follow_link() gets your current location (in nameidata) and its job
is to shift it according to your symlink.  Which will end up with
original references being released, ones to new location grabbed and
left in nameidata.

Which is exactly what proc_pid_follow_link() is doing.  The whole point
of ->follow_link() is its effect on nd->{mnt,dentry}.

One optimization comes from the fact that
	* 99% of symlinks end up with "get a pathname out of filesystem,
traverse that pathname"
	* we are in the middle of mutual recursion and sensitive to call
chain depth.

So while ->follow_link() _can_ do it itself (call vfs_follow_link(nd, string),
which will move nameidata according to pathname we pass to it), it can just
leave the pathname to caller.  If your ->follow_link() had done
	nd_set_link(nd, string)
the caller will do the rest - call vfs_follow_link() *after* we'd returned
from ->follow_link().  Then it will call your ->put_link() (if present)
to undo whatever you've done to pin the string down.

See __do_follow_link() in fs/namei.c - that's what calls these suckers.
The reason for allowing to leave pathname traversal to caller is simple -
then our ->follow_link() is not in the mutual recursion anymore and we
win stack space.

Examples:
1) /proc/self:
static void *proc_self_follow_link(struct dentry *dentry, struct nameidata *nd)
{
        char tmp[30];
        sprintf(tmp, "%d", current->tgid);
        return ERR_PTR(vfs_follow_link(nd,tmp));
}
calls vfs_follow_link() itself, nothing left to do in caller.

2) ext2 fast symlinks:
static void *ext2_follow_link(struct dentry *dentry, struct nameidata *nd)
{
        struct ext2_inode_info *ei = EXT2_I(dentry->d_inode);
        nd_set_link(nd, (char *)ei->i_data);
        return NULL;
}
symlink body is embedded into in-core inode, just tell the caller to
traverse it, no cleanup needed since inode will stay pinned down
all that.

3) ext2 normal symlinks (and the same thing is used by a lot of other
filesystems; that's the ones that have symlink look like a file with
->readpage() bringing its contents into page cache):
void *page_follow_link_light(struct dentry *dentry, struct nameidata *nd)
{
        struct page *page = NULL;
        nd_set_link(nd, page_getlink(dentry, &page));
        return page;
}
symlink body is in page cache; read it, pin it down and tell the caller to
traverse it.  Cleanup is done by
void page_put_link(struct dentry *dentry, struct nameidata *nd, void *cookie)
{
        struct page *page = cookie;
        if (page) {
                kunmap(page);
                page_cache_release(page);
        }
}

4) normal symlinks in /proc (such as e.g. /proc/ide/hda):
static void *proc_follow_link(struct dentry *dentry, struct nameidata *nd)
{
        nd_set_link(nd, PDE(dentry->d_inode)->data);
        return NULL;
}
same story as with ext2 fast symlinks, no cleanup needed.

5) /proc/<pid>/cwd and its ilk:
static void *proc_pid_follow_link(struct dentry *dentry, struct nameidata *nd)
{
        struct inode *inode = dentry->d_inode;
        int error = -EACCES;

        /* We don't need a base pointer in the /proc filesystem */
        path_release(nd);

        if (current->fsuid != inode->i_uid && !capable(CAP_DAC_OVERRIDE))
                goto out;
        error = proc_check_root(inode);
        if (error)
                goto out;

        error = PROC_I(inode)->op.proc_get_link(inode, &nd->dentry, &nd->mnt);
        nd->last_type = LAST_BIND;
out:
        return ERR_PTR(error);
}
this is not a traversal of some pathname, so we do everything ourselves -
release the original location, find where to go and slap new dentry/vfsmount
into nd->dentry/nd->mnt.


What HPPFS should be doing:
	1) have your ->follow_link() call ->follow_link() of underlying
procfs inode and just return whatever it had returned.
	2) have ->put_link() that would call ->put_link() of underlying
procfs inode, passing it underlying dentry, nameidata it had been given
and pointer it got as the last argument.  If underlying inode has no
->put_link(), do nothing.
	3) if you want to override a symlink, refer to examples above.
At a guess, it will be along the lines of (2) and (4).