Re: [autofs] [PATCH 04/38] autofs4: Save autofs trigger's vfsmountin super block info

[Miklos Szeredi]

On Mon, 21 Jun 2010, Ian Kent wrote:
> On Wed, 2010-06-16 at 12:04 +0800, Ian Kent wrote:
> > On Tue, 2010-06-15 at 11:39 -0700, Valerie Aurora wrote:
> > > From: Jan Blunck <jblunck@xxxxxxx>
> > > 
> > > XXX - This is broken and included just to make union mounts work.  See
> > > discussion at:
> > > 
> > > http://kerneltrap.org/mailarchive/linux-fsdevel/2010/1/15/6708053/thread
> > 
> > Instead of saving the vfsmount we could save a pointer to the dentry of
> > the mount point in the autofs super block info struct. I think that's
> > the bit I don't have so it would be sufficient for a lookup_mnt() for
> > the needed vfsmount in ->follow_mount().
> > 
> > Objections?
> > Suggestions?
> 
> No comments so far.
> 
> Before I dive into testing if this actually does what I need, can I get
> an "in principal" ack or nack for the patch so union mounts can move on
> please?
> 
> Note that this patch hasn't even been compile tested so the point is to
> decide whether it is worth going ahead with it.

mnt_mountpoint is NULL at the point you try to save it, so this is not
going to work.

> 
> 
> autofs4 - save autofs trigger mountpoint in super block info
> 
> From: Ian Kent <raven@xxxxxxxxxx>
> 
> Adapted from the original patch from Jan Blunck <jblunck@xxxxxxx>.
> 
> Original commit message:
> 
> This is a bugfix/replacement for commit
> 051d381259eb57d6074d02a6ba6e90e744f1a29f:
> 
>     During a path walk if an autofs trigger is mounted on a dentry,
>     when the follow_link method is called, the nameidata struct
>     contains the vfsmount and mountpoint dentry of the parent mount
>     while the dentry that is passed in is the root of the autofs
>     trigger mount.  I believe it is impossible to get the vfsmount of
>     the trigger mount, within the follow_link method, when only the
>     parent vfsmount and the root dentry of the trigger mount are
>     known.
> 
> The solution in this commit was to replace the path embedded in the
> parent's nameidata with the path of the link itself in
> __do_follow_link().  This is a relatively harmless misuse of the
> field, but union mounts ran into a bug during follow_link() caused by
> the nameidata containing the wrong path (we count on it being what it
> is all other places - the path of the parent).
> 
> A cleaner and easier to understand solution is to save the necessary
> mountpoint dentry in the autofs superblock info when it is mounted.
> Then we can cwlookup the needed vfsmount in autofs4_follow_link().
> 
> Signed-off-by: Ian Kent <raven@xxxxxxxxxx>
> Cc: Jan Blunck <jblunck@xxxxxxx>
> Cc: Valerie Aurora <vaurora@xxxxxxxxxx>
> Cc: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: autofs@xxxxxxxxxxxxxxxx
> ---
> 
>  fs/autofs4/autofs_i.h |    1 +
>  fs/autofs4/init.c     |   11 ++++++++++-
>  fs/autofs4/root.c     |   13 +++++++++++++
>  fs/namei.c            |    7 ++-----
>  fs/namespace.c        |    1 +
>  5 files changed, 27 insertions(+), 6 deletions(-)
> 
> 
> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> index 3d283ab..9fc6d69 100644
> --- a/fs/autofs4/autofs_i.h
> +++ b/fs/autofs4/autofs_i.h
> @@ -133,6 +133,7 @@ struct autofs_sb_info {
>  	int reghost_enabled;
>  	int needs_reghost;
>  	struct super_block *sb;
> +	struct dentry *mountpoint;
>  	struct mutex wq_mutex;
>  	spinlock_t fs_lock;
>  	struct autofs_wait_queue *queues; /* Wait queue pointer */
> diff --git a/fs/autofs4/init.c b/fs/autofs4/init.c
> index 9722e4b..f305b7d 100644
> --- a/fs/autofs4/init.c
> +++ b/fs/autofs4/init.c
> @@ -17,7 +17,16 @@
>  static int autofs_get_sb(struct file_system_type *fs_type,
>  	int flags, const char *dev_name, void *data, struct vfsmount *mnt)
>  {
> -	return get_sb_nodev(fs_type, flags, data, autofs4_fill_super, mnt);
> +	struct autofs_sb_info *sbi;
> +	int ret;
> +
> +	ret = get_sb_nodev(fs_type, flags, data, autofs4_fill_super, mnt);
> +	if (ret)
> +		return ret;
> +
> +	sbi = autofs4_sbi(mnt->mnt_sb);
> +	sbi->mountpoint = mnt->mnt_mountpoint;
> +	return 0;
>  }
>  
>  static struct file_system_type autofs_fs_type = {
> diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
> index db4117e..be89fd2 100644
> --- a/fs/autofs4/root.c
> +++ b/fs/autofs4/root.c
> @@ -215,11 +215,24 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
>  	struct autofs_info *ino = autofs4_dentry_ino(dentry);
>  	int oz_mode = autofs4_oz_mode(sbi);
>  	unsigned int lookup_type;
> +	struct vfsmount *mnt;
>  	int status;
>  
>  	DPRINTK("dentry=%p %.*s oz_mode=%d nd->flags=%d",
>  		dentry, dentry->d_name.len, dentry->d_name.name, oz_mode,
>  		nd->flags);
> +
> +	mnt = lookup_mount(nd->path.mnt, sbi->mountpoint);
> +	if (!mnt) {
> +		status = -ENOENT;
> +		goto out_error;
> +	}
> +
> +	dput(nd->path.dentry);
> +	mntput(nd->path.mnt);
> +	nd->path.mnt = mnt;
> +	nd->path.dentry = dget(dentry);
> +
>  	/*
>  	 * For an expire of a covered direct or offset mount we need
>  	 * to break out of follow_down() at the autofs mount trigger
> diff --git a/fs/namei.c b/fs/namei.c
> index 868d0cb..69a78ee 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -539,11 +539,8 @@ __do_follow_link(struct path *path, struct nameidata *nd, void **p)
>  	touch_atime(path->mnt, dentry);
>  	nd_set_link(nd, NULL);
>  
> -	if (path->mnt != nd->path.mnt) {
> -		path_to_nameidata(path, nd);
> -		dget(dentry);
> -	}
> -	mntget(path->mnt);
> +	if (path->mnt == nd->path.mnt)
> +		mntget(nd->path.mnt);
>  	nd->last_type = LAST_BIND;
>  	*p = dentry->d_inode->i_op->follow_link(dentry, nd);
>  	error = PTR_ERR(*p);
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 88058de..19b7fc9 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -445,6 +445,7 @@ struct vfsmount *lookup_mnt(struct path *path)
>  	spin_unlock(&vfsmount_lock);
>  	return child_mnt;
>  }
> +EXPORT_SYMBOL_GPL(lookup_mnt);
>  
>  static inline int check_mnt(struct vfsmount *mnt)
>  {
> 
> 
> 
--
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/