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@xxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/