[PATCH 1/2] vfs - rename user_path_umountat() to user_path_mntpointat()

[PATCH 1/2] vfs - rename user_path_umountat() to user_path_mntpointat()

[Ian Kent]

The function user_path_umountat() not only avoids revalidation but
avoids the managed dentry code as well.

This function has other uses, such as the case where autofs is
reconnecting to a tree of mounts and needs to avoid the managed
dentry call since that would incorrectly cause a callback to the
daemon.

So give the function a more descriptive name.

Signed-off-by: Ian Kent <raven@themaw.net>
Cc: Jeff Layton <jlayton@redhat.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
---
 fs/namei.c            |   34 +++++++++++++++++++---------------
 fs/namespace.c        |    2 +-
 include/linux/namei.h |    2 +-
 3 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index f415c66..24f7562 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2250,7 +2250,7 @@ user_path_parent(int dfd, const char __user *path, struct nameidata *nd,
  *         to the link, and nd->path will *not* be put.
  */
 static int
-umount_lookup_last(struct nameidata *nd, struct path *path)
+mntpoint_lookup_last(struct nameidata *nd, struct path *path)
 {
 	int error = 0;
 	struct dentry *dentry;
@@ -2309,17 +2309,19 @@ error_check:
 }
 
 /**
- * path_umountat - look up a path to be umounted
+ * path_mntpointat - look up a path to a mount point
  * @dfd:	directory file descriptor to start walk from
  * @name:	full pathname to walk
  * @flags:	lookup flags
  * @nd:		pathwalk nameidata
  *
- * Look up the given name, but don't attempt to revalidate the last component.
+ * Look up the given name, but don't attempt to revalidate the last component
+ * or call the managed dentry code.
  * Returns 0 and "path" will be valid on success; Retuns error otherwise.
  */
 static int
-path_umountat(int dfd, const char *name, struct path *path, unsigned int flags)
+path_mntpointat(int dfd, const char *name,
+		struct path *path, unsigned int flags)
 {
 	struct file *base = NULL;
 	struct nameidata nd;
@@ -2343,7 +2345,7 @@ path_umountat(int dfd, const char *name, struct path *path, unsigned int flags)
 		}
 	}
 
-	err = umount_lookup_last(&nd, path);
+	err = mntpoint_lookup_last(&nd, path);
 	while (err > 0) {
 		void *cookie;
 		struct path link = *path;
@@ -2354,7 +2356,7 @@ path_umountat(int dfd, const char *name, struct path *path, unsigned int flags)
 		err = follow_link(&link, &nd, &cookie);
 		if (err)
 			break;
-		err = umount_lookup_last(&nd, path);
+		err = mntpoint_lookup_last(&nd, path);
 		put_link(&nd, &link, cookie);
 	}
 out:
@@ -2368,21 +2370,23 @@ out:
 }
 
 /**
- * user_path_umountat - lookup a path from userland in order to umount it
+ * user_path_mntpointat - lookup a mountpoint path from userland without
+ *      revalidating it or calling the managed dentry code
  * @dfd:	directory file descriptor
  * @name:	pathname from userland
  * @flags:	lookup flags
  * @path:	pointer to container to hold result
  *
- * A umount is a special case for path walking. We're not actually interested
- * in the inode in this situation, and ESTALE errors can be a problem. We
- * simply want track down the dentry and vfsmount attached at the mountpoint
- * and avoid revalidating the last component.
+ * A mount point (when being umounted for example) is a special case for path
+ * walking. We're not actually interested in the inode in this situation, and
+ * ESTALE errors can be a problem. We simply want track down the dentry and
+ * vfsmount attached at the mountpoint and avoid revalidating or calling the
+ * managed dentry code for the last component.
  *
  * Returns 0 and populates "path" on success.
  */
 int
-user_path_umountat(int dfd, const char __user *name, unsigned int flags,
+user_path_mntpointat(int dfd, const char __user *name, unsigned int flags,
 			struct path *path)
 {
 	struct filename *s = getname(name);
@@ -2391,11 +2395,11 @@ user_path_umountat(int dfd, const char __user *name, unsigned int flags,
 	if (IS_ERR(s))
 		return PTR_ERR(s);
 
-	error = path_umountat(dfd, s->name, path, flags | LOOKUP_RCU);
+	error = path_mntpointat(dfd, s->name, path, flags | LOOKUP_RCU);
 	if (unlikely(error == -ECHILD))
-		error = path_umountat(dfd, s->name, path, flags);
+		error = path_mntpointat(dfd, s->name, path, flags);
 	if (unlikely(error == -ESTALE))
-		error = path_umountat(dfd, s->name, path, flags | LOOKUP_REVAL);
+		error = path_mntpointat(dfd, s->name, path, flags|LOOKUP_REVAL);
 
 	if (likely(!error))
 		audit_inode(s, path->dentry, 0);
diff --git a/fs/namespace.c b/fs/namespace.c
index fc2b522..c48ad96 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1321,7 +1321,7 @@ SYSCALL_DEFINE2(umount, char __user *, name, int, flags)
 	if (!(flags & UMOUNT_NOFOLLOW))
 		lookup_flags |= LOOKUP_FOLLOW;
 
-	retval = user_path_umountat(AT_FDCWD, name, lookup_flags, &path);
+	retval = user_path_mntpointat(AT_FDCWD, name, lookup_flags, &path);
 	if (retval)
 		goto out;
 	mnt = real_mount(path.mnt);
diff --git a/include/linux/namei.h b/include/linux/namei.h
index cd09751..7908abf 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -58,7 +58,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
 
 extern int user_path_at(int, const char __user *, unsigned, struct path *);
 extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);
-extern int user_path_umountat(int, const char __user *, unsigned int, struct path *);
+extern int user_path_mntpointat(int, const char __user *, unsigned int, struct path *);
 
 #define user_path(name, path) user_path_at(AT_FDCWD, name, LOOKUP_FOLLOW, path)
 #define user_lpath(name, path) user_path_at(AT_FDCWD, name, 0, path)

[PATCH 2/2] autofs4 - fix device ioctl mount lookup

[Ian Kent]

When reconnecting to automounts at startup an autofs ioctl is used
to find the device and inode of existing mounts so they can be used
to open a file descriptor of possibly covered mounts.

At this time the the caller might not yet "own" the mount so it can
trigger calling ->d_automount(). This causes automount to hang when
trying to reconnect to direct or offset mount types.

Consequently kern_path() can't be used but path_mntpointat() can be.

Signed-off-by: Ian Kent <raven@themaw.net>
Cc: Jeff Layton <jlayton@redhat.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
---
 fs/autofs4/dev-ioctl.c |   23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index 9183821..228866f 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -183,13 +183,14 @@ static int autofs_dev_ioctl_protosubver(struct file *fp,
 	return 0;
 }
 
+/* Find the topmost mount satisfying test() */
 static int find_autofs_mount(const char *pathname,
 			     struct path *res,
 			     int test(struct path *path, void *data),
 			     void *data)
 {
 	struct path path;
-	int err = kern_path(pathname, 0, &path);
+	int err = user_path_mntpointat(AT_FDCWD, pathname, 0, &path);
 	if (err)
 		return err;
 	err = -ENOENT;
@@ -197,10 +198,9 @@ static int find_autofs_mount(const char *pathname,
 		if (path.dentry->d_sb->s_magic == AUTOFS_SUPER_MAGIC) {
 			if (test(&path, data)) {
 				path_get(&path);
-				if (!err) /* already found some */
-					path_put(res);
 				*res = path;
 				err = 0;
+				break;
 			}
 		}
 		if (!follow_up(&path))
@@ -498,12 +498,11 @@ static int autofs_dev_ioctl_askumount(struct file *fp,
  * mount if there is one or 0 if it isn't a mountpoint.
  *
  * If we aren't supplied with a file descriptor then we
- * lookup the nameidata of the path and check if it is the
- * root of a mount. If a type is given we are looking for
- * a particular autofs mount and if we don't find a match
- * we return fail. If the located nameidata path is the
- * root of a mount we return 1 along with the super magic
- * of the mount or 0 otherwise.
+ * lookup the path and check if it is the root of a mount.
+ * If a type is given we are looking for a particular autofs
+ * mount and if we don't find a match we return fail. If the
+ * located path is the root of a mount we return 1 along with
+ * the super magic of the mount or 0 otherwise.
  *
  * In both cases the the device number (as returned by
  * new_encode_dev()) is also returned.
@@ -531,9 +530,11 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp,
 
 	if (!fp || param->ioctlfd == -1) {
 		if (autofs_type_any(type))
-			err = kern_path(name, LOOKUP_FOLLOW, &path);
+			err = user_path_mntpointat(AT_FDCWD,
+						   name, LOOKUP_FOLLOW, &path);
 		else
-			err = find_autofs_mount(name, &path, test_by_type, &type);
+			err = find_autofs_mount(name, &path,
+						test_by_type, &type);
 		if (err)
 			goto out;
 		devid = new_encode_dev(path.dentry->d_sb->s_dev);

Re: [PATCH 2/2] autofs4 - fix device ioctl mount lookup

[Jeff Layton]

On Sun, 08 Sep 2013 16:47:23 +0800
Ian Kent <raven@themaw.net> wrote:

> When reconnecting to automounts at startup an autofs ioctl is used
> to find the device and inode of existing mounts so they can be used
> to open a file descriptor of possibly covered mounts.
> 
> At this time the the caller might not yet "own" the mount so it can
> trigger calling ->d_automount(). This causes automount to hang when
> trying to reconnect to direct or offset mount types.
> 
> Consequently kern_path() can't be used but path_mntpointat() can be.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> Cc: Jeff Layton <jlayton@redhat.com>
> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> ---
>  fs/autofs4/dev-ioctl.c |   23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
> index 9183821..228866f 100644
> --- a/fs/autofs4/dev-ioctl.c
> +++ b/fs/autofs4/dev-ioctl.c
> @@ -183,13 +183,14 @@ static int autofs_dev_ioctl_protosubver(struct file *fp,
>  	return 0;
>  }
>  
> +/* Find the topmost mount satisfying test() */
>  static int find_autofs_mount(const char *pathname,
>  			     struct path *res,
>  			     int test(struct path *path, void *data),
>  			     void *data)
>  {
>  	struct path path;
> -	int err = kern_path(pathname, 0, &path);
> +	int err = user_path_mntpointat(AT_FDCWD, pathname, 0, &path);

This looks wrong. "pathname" is a kernel string, not a __user one. I
think what you need to do here is to turn user_path_mntpointat into a
wrapper around a kern_path_mntpointat equivalent and then call that
here.

>  	if (err)
>  		return err;
>  	err = -ENOENT;
> @@ -197,10 +198,9 @@ static int find_autofs_mount(const char *pathname,
>  		if (path.dentry->d_sb->s_magic == AUTOFS_SUPER_MAGIC) {
>  			if (test(&path, data)) {
>  				path_get(&path);
> -				if (!err) /* already found some */
> -					path_put(res);
>  				*res = path;
>  				err = 0;
> +				break;
>  			}
>  		}
>  		if (!follow_up(&path))
> @@ -498,12 +498,11 @@ static int autofs_dev_ioctl_askumount(struct file *fp,
>   * mount if there is one or 0 if it isn't a mountpoint.
>   *
>   * If we aren't supplied with a file descriptor then we
> - * lookup the nameidata of the path and check if it is the
> - * root of a mount. If a type is given we are looking for
> - * a particular autofs mount and if we don't find a match
> - * we return fail. If the located nameidata path is the
> - * root of a mount we return 1 along with the super magic
> - * of the mount or 0 otherwise.
> + * lookup the path and check if it is the root of a mount.
> + * If a type is given we are looking for a particular autofs
> + * mount and if we don't find a match we return fail. If the
> + * located path is the root of a mount we return 1 along with
> + * the super magic of the mount or 0 otherwise.
>   *
>   * In both cases the the device number (as returned by
>   * new_encode_dev()) is also returned.
> @@ -531,9 +530,11 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp,
>  
>  	if (!fp || param->ioctlfd == -1) {
>  		if (autofs_type_any(type))
> -			err = kern_path(name, LOOKUP_FOLLOW, &path);
> +			err = user_path_mntpointat(AT_FDCWD,
> +						   name, LOOKUP_FOLLOW, &path);
>  		else
> -			err = find_autofs_mount(name, &path, test_by_type, &type);
> +			err = find_autofs_mount(name, &path,
> +						test_by_type, &type);


...ditto in these spots of course...

>  		if (err)
>  			goto out;
>  		devid = new_encode_dev(path.dentry->d_sb->s_dev);
> 


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/2] vfs - rename user_path_umountat() to user_path_mntpointat()

[Jeff Layton]

On Sun, 08 Sep 2013 16:47:15 +0800
Ian Kent <raven@themaw.net> wrote:

> The function user_path_umountat() not only avoids revalidation but
> avoids the managed dentry code as well.
> 
> This function has other uses, such as the case where autofs is
> reconnecting to a tree of mounts and needs to avoid the managed
> dentry call since that would incorrectly cause a callback to the
> daemon.
> 
> So give the function a more descriptive name.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> Cc: Jeff Layton <jlayton@redhat.com>
> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> ---
>  fs/namei.c            |   34 +++++++++++++++++++---------------
>  fs/namespace.c        |    2 +-
>  include/linux/namei.h |    2 +-
>  3 files changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index f415c66..24f7562 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2250,7 +2250,7 @@ user_path_parent(int dfd, const char __user *path, struct nameidata *nd,
>   *         to the link, and nd->path will *not* be put.
>   */
>  static int
> -umount_lookup_last(struct nameidata *nd, struct path *path)
> +mntpoint_lookup_last(struct nameidata *nd, struct path *path)
>  {
>  	int error = 0;
>  	struct dentry *dentry;
> @@ -2309,17 +2309,19 @@ error_check:
>  }
>  
>  /**
> - * path_umountat - look up a path to be umounted
> + * path_mntpointat - look up a path to a mount point
>   * @dfd:	directory file descriptor to start walk from
>   * @name:	full pathname to walk
>   * @flags:	lookup flags
>   * @nd:		pathwalk nameidata
>   *
> - * Look up the given name, but don't attempt to revalidate the last component.
> + * Look up the given name, but don't attempt to revalidate the last component
> + * or call the managed dentry code.
>   * Returns 0 and "path" will be valid on success; Retuns error otherwise.
>   */
>  static int
> -path_umountat(int dfd, const char *name, struct path *path, unsigned int flags)
> +path_mntpointat(int dfd, const char *name,
> +		struct path *path, unsigned int flags)
>  {
>  	struct file *base = NULL;
>  	struct nameidata nd;
> @@ -2343,7 +2345,7 @@ path_umountat(int dfd, const char *name, struct path *path, unsigned int flags)
>  		}
>  	}
>  
> -	err = umount_lookup_last(&nd, path);
> +	err = mntpoint_lookup_last(&nd, path);
>  	while (err > 0) {
>  		void *cookie;
>  		struct path link = *path;
> @@ -2354,7 +2356,7 @@ path_umountat(int dfd, const char *name, struct path *path, unsigned int flags)
>  		err = follow_link(&link, &nd, &cookie);
>  		if (err)
>  			break;
> -		err = umount_lookup_last(&nd, path);
> +		err = mntpoint_lookup_last(&nd, path);
>  		put_link(&nd, &link, cookie);
>  	}
>  out:
> @@ -2368,21 +2370,23 @@ out:
>  }
>  
>  /**
> - * user_path_umountat - lookup a path from userland in order to umount it
> + * user_path_mntpointat - lookup a mountpoint path from userland without
> + *      revalidating it or calling the managed dentry code
>   * @dfd:	directory file descriptor
>   * @name:	pathname from userland
>   * @flags:	lookup flags
>   * @path:	pointer to container to hold result
>   *
> - * A umount is a special case for path walking. We're not actually interested
> - * in the inode in this situation, and ESTALE errors can be a problem. We
> - * simply want track down the dentry and vfsmount attached at the mountpoint
> - * and avoid revalidating the last component.
> + * A mount point (when being umounted for example) is a special case for path
> + * walking. We're not actually interested in the inode in this situation, and
> + * ESTALE errors can be a problem. We simply want track down the dentry and
> + * vfsmount attached at the mountpoint and avoid revalidating or calling the
> + * managed dentry code for the last component.
>   *
>   * Returns 0 and populates "path" on success.
>   */
>  int
> -user_path_umountat(int dfd, const char __user *name, unsigned int flags,
> +user_path_mntpointat(int dfd, const char __user *name, unsigned int flags,
>  			struct path *path)
>  {
>  	struct filename *s = getname(name);
> @@ -2391,11 +2395,11 @@ user_path_umountat(int dfd, const char __user *name, unsigned int flags,
>  	if (IS_ERR(s))
>  		return PTR_ERR(s);
>  
> -	error = path_umountat(dfd, s->name, path, flags | LOOKUP_RCU);
> +	error = path_mntpointat(dfd, s->name, path, flags | LOOKUP_RCU);
>  	if (unlikely(error == -ECHILD))
> -		error = path_umountat(dfd, s->name, path, flags);
> +		error = path_mntpointat(dfd, s->name, path, flags);
>  	if (unlikely(error == -ESTALE))
> -		error = path_umountat(dfd, s->name, path, flags | LOOKUP_REVAL);
> +		error = path_mntpointat(dfd, s->name, path, flags|LOOKUP_REVAL);
>  
>  	if (likely(!error))
>  		audit_inode(s, path->dentry, 0);
> diff --git a/fs/namespace.c b/fs/namespace.c
> index fc2b522..c48ad96 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1321,7 +1321,7 @@ SYSCALL_DEFINE2(umount, char __user *, name, int, flags)
>  	if (!(flags & UMOUNT_NOFOLLOW))
>  		lookup_flags |= LOOKUP_FOLLOW;
>  
> -	retval = user_path_umountat(AT_FDCWD, name, lookup_flags, &path);
> +	retval = user_path_mntpointat(AT_FDCWD, name, lookup_flags, &path);
>  	if (retval)
>  		goto out;
>  	mnt = real_mount(path.mnt);
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index cd09751..7908abf 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -58,7 +58,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
>  
>  extern int user_path_at(int, const char __user *, unsigned, struct path *);
>  extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);
> -extern int user_path_umountat(int, const char __user *, unsigned int, struct path *);
> +extern int user_path_mntpointat(int, const char __user *, unsigned int, struct path *);
>  
>  #define user_path(name, path) user_path_at(AT_FDCWD, name, LOOKUP_FOLLOW, path)
>  #define user_lpath(name, path) user_path_at(AT_FDCWD, name, 0, path)
> 

Looks good to me.

Acked-by: Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/2] autofs4 - fix device ioctl mount lookup

[Ian Kent]

On Sun, 2013-09-08 at 07:33 -0400, Jeff Layton wrote:
> On Sun, 08 Sep 2013 16:47:23 +0800
> Ian Kent <raven@themaw.net> wrote:
> 
> > When reconnecting to automounts at startup an autofs ioctl is used
> > to find the device and inode of existing mounts so they can be used
> > to open a file descriptor of possibly covered mounts.
> > 
> > At this time the the caller might not yet "own" the mount so it can
> > trigger calling ->d_automount(). This causes automount to hang when
> > trying to reconnect to direct or offset mount types.
> > 
> > Consequently kern_path() can't be used but path_mntpointat() can be.
> > 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > Cc: Jeff Layton <jlayton@redhat.com>
> > Cc: Al Viro <viro@ZenIV.linux.org.uk>
> > ---
> >  fs/autofs4/dev-ioctl.c |   23 ++++++++++++-----------
> >  1 file changed, 12 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
> > index 9183821..228866f 100644
> > --- a/fs/autofs4/dev-ioctl.c
> > +++ b/fs/autofs4/dev-ioctl.c
> > @@ -183,13 +183,14 @@ static int autofs_dev_ioctl_protosubver(struct file *fp,
> >  	return 0;
> >  }
> >  
> > +/* Find the topmost mount satisfying test() */
> >  static int find_autofs_mount(const char *pathname,
> >  			     struct path *res,
> >  			     int test(struct path *path, void *data),
> >  			     void *data)
> >  {
> >  	struct path path;
> > -	int err = kern_path(pathname, 0, &path);
> > +	int err = user_path_mntpointat(AT_FDCWD, pathname, 0, &path);
> 
> This looks wrong. "pathname" is a kernel string, not a __user one. I
> think what you need to do here is to turn user_path_mntpointat into a
> wrapper around a kern_path_mntpointat equivalent and then call that
> here.

In both cases the path comes from a structure passed from user space.
So I started thinking it wasn't correct previously.

> 
> >  	if (err)
> >  		return err;
> >  	err = -ENOENT;
> > @@ -197,10 +198,9 @@ static int find_autofs_mount(const char *pathname,
> >  		if (path.dentry->d_sb->s_magic == AUTOFS_SUPER_MAGIC) {
> >  			if (test(&path, data)) {
> >  				path_get(&path);
> > -				if (!err) /* already found some */
> > -					path_put(res);
> >  				*res = path;
> >  				err = 0;
> > +				break;
> >  			}
> >  		}
> >  		if (!follow_up(&path))
> > @@ -498,12 +498,11 @@ static int autofs_dev_ioctl_askumount(struct file *fp,
> >   * mount if there is one or 0 if it isn't a mountpoint.
> >   *
> >   * If we aren't supplied with a file descriptor then we
> > - * lookup the nameidata of the path and check if it is the
> > - * root of a mount. If a type is given we are looking for
> > - * a particular autofs mount and if we don't find a match
> > - * we return fail. If the located nameidata path is the
> > - * root of a mount we return 1 along with the super magic
> > - * of the mount or 0 otherwise.
> > + * lookup the path and check if it is the root of a mount.
> > + * If a type is given we are looking for a particular autofs
> > + * mount and if we don't find a match we return fail. If the
> > + * located path is the root of a mount we return 1 along with
> > + * the super magic of the mount or 0 otherwise.
> >   *
> >   * In both cases the the device number (as returned by
> >   * new_encode_dev()) is also returned.
> > @@ -531,9 +530,11 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp,
> >  
> >  	if (!fp || param->ioctlfd == -1) {
> >  		if (autofs_type_any(type))
> > -			err = kern_path(name, LOOKUP_FOLLOW, &path);
> > +			err = user_path_mntpointat(AT_FDCWD,
> > +						   name, LOOKUP_FOLLOW, &path);
> >  		else
> > -			err = find_autofs_mount(name, &path, test_by_type, &type);
> > +			err = find_autofs_mount(name, &path,
> > +						test_by_type, &type);
> 
> 
> ...ditto in these spots of course...
> 
> >  		if (err)
> >  			goto out;
> >  		devid = new_encode_dev(path.dentry->d_sb->s_dev);
> > 
> 
> 

Re: [PATCH 2/2] autofs4 - fix device ioctl mount lookup

[Jeff Layton]

On Mon, 09 Sep 2013 15:18:00 +0800
Ian Kent <raven@themaw.net> wrote:

> On Sun, 2013-09-08 at 07:33 -0400, Jeff Layton wrote:
> > On Sun, 08 Sep 2013 16:47:23 +0800
> > Ian Kent <raven@themaw.net> wrote:
> > 
> > > When reconnecting to automounts at startup an autofs ioctl is used
> > > to find the device and inode of existing mounts so they can be used
> > > to open a file descriptor of possibly covered mounts.
> > > 
> > > At this time the the caller might not yet "own" the mount so it can
> > > trigger calling ->d_automount(). This causes automount to hang when
> > > trying to reconnect to direct or offset mount types.
> > > 
> > > Consequently kern_path() can't be used but path_mntpointat() can be.
> > > 
> > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > Cc: Jeff Layton <jlayton@redhat.com>
> > > Cc: Al Viro <viro@ZenIV.linux.org.uk>
> > > ---
> > >  fs/autofs4/dev-ioctl.c |   23 ++++++++++++-----------
> > >  1 file changed, 12 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
> > > index 9183821..228866f 100644
> > > --- a/fs/autofs4/dev-ioctl.c
> > > +++ b/fs/autofs4/dev-ioctl.c
> > > @@ -183,13 +183,14 @@ static int autofs_dev_ioctl_protosubver(struct file *fp,
> > >  	return 0;
> > >  }
> > >  
> > > +/* Find the topmost mount satisfying test() */
> > >  static int find_autofs_mount(const char *pathname,
> > >  			     struct path *res,
> > >  			     int test(struct path *path, void *data),
> > >  			     void *data)
> > >  {
> > >  	struct path path;
> > > -	int err = kern_path(pathname, 0, &path);
> > > +	int err = user_path_mntpointat(AT_FDCWD, pathname, 0, &path);
> > 
> > This looks wrong. "pathname" is a kernel string, not a __user one. I
> > think what you need to do here is to turn user_path_mntpointat into a
> > wrapper around a kern_path_mntpointat equivalent and then call that
> > here.
> 
> In both cases the path comes from a structure passed from user space.
> So I started thinking it wasn't correct previously.
> 

AFAICT, this is a kernel string by the time it gets here.

_autofs_dev_ioctl calls copy_dev_ioctl, which copies that struct from
userland to a kernel buffer.

> > 
> > >  	if (err)
> > >  		return err;
> > >  	err = -ENOENT;
> > > @@ -197,10 +198,9 @@ static int find_autofs_mount(const char *pathname,
> > >  		if (path.dentry->d_sb->s_magic == AUTOFS_SUPER_MAGIC) {
> > >  			if (test(&path, data)) {
> > >  				path_get(&path);
> > > -				if (!err) /* already found some */
> > > -					path_put(res);
> > >  				*res = path;
> > >  				err = 0;
> > > +				break;
> > >  			}
> > >  		}
> > >  		if (!follow_up(&path))
> > > @@ -498,12 +498,11 @@ static int autofs_dev_ioctl_askumount(struct file *fp,
> > >   * mount if there is one or 0 if it isn't a mountpoint.
> > >   *
> > >   * If we aren't supplied with a file descriptor then we
> > > - * lookup the nameidata of the path and check if it is the
> > > - * root of a mount. If a type is given we are looking for
> > > - * a particular autofs mount and if we don't find a match
> > > - * we return fail. If the located nameidata path is the
> > > - * root of a mount we return 1 along with the super magic
> > > - * of the mount or 0 otherwise.
> > > + * lookup the path and check if it is the root of a mount.
> > > + * If a type is given we are looking for a particular autofs
> > > + * mount and if we don't find a match we return fail. If the
> > > + * located path is the root of a mount we return 1 along with
> > > + * the super magic of the mount or 0 otherwise.
> > >   *
> > >   * In both cases the the device number (as returned by
> > >   * new_encode_dev()) is also returned.
> > > @@ -531,9 +530,11 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp,
> > >  
> > >  	if (!fp || param->ioctlfd == -1) {
> > >  		if (autofs_type_any(type))
> > > -			err = kern_path(name, LOOKUP_FOLLOW, &path);
> > > +			err = user_path_mntpointat(AT_FDCWD,
> > > +						   name, LOOKUP_FOLLOW, &path);
> > >  		else
> > > -			err = find_autofs_mount(name, &path, test_by_type, &type);
> > > +			err = find_autofs_mount(name, &path,
> > > +						test_by_type, &type);
> > 
> > 
> > ...ditto in these spots of course...
> > 
> > >  		if (err)
> > >  			goto out;
> > >  		devid = new_encode_dev(path.dentry->d_sb->s_dev);
> > > 
> > 
> > 
> 
> 


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/2] autofs4 - fix device ioctl mount lookup

[Ian Kent]

On Mon, 2013-09-09 at 06:45 -0400, Jeff Layton wrote:
> On Mon, 09 Sep 2013 15:18:00 +0800
> Ian Kent <raven@themaw.net> wrote:
> 
> > On Sun, 2013-09-08 at 07:33 -0400, Jeff Layton wrote:
> > > On Sun, 08 Sep 2013 16:47:23 +0800
> > > Ian Kent <raven@themaw.net> wrote:
> > > 
> > > > When reconnecting to automounts at startup an autofs ioctl is used
> > > > to find the device and inode of existing mounts so they can be used
> > > > to open a file descriptor of possibly covered mounts.
> > > > 
> > > > At this time the the caller might not yet "own" the mount so it can
> > > > trigger calling ->d_automount(). This causes automount to hang when
> > > > trying to reconnect to direct or offset mount types.
> > > > 
> > > > Consequently kern_path() can't be used but path_mntpointat() can be.
> > > > 
> > > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > > Cc: Jeff Layton <jlayton@redhat.com>
> > > > Cc: Al Viro <viro@ZenIV.linux.org.uk>
> > > > ---
> > > >  fs/autofs4/dev-ioctl.c |   23 ++++++++++++-----------
> > > >  1 file changed, 12 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
> > > > index 9183821..228866f 100644
> > > > --- a/fs/autofs4/dev-ioctl.c
> > > > +++ b/fs/autofs4/dev-ioctl.c
> > > > @@ -183,13 +183,14 @@ static int autofs_dev_ioctl_protosubver(struct file *fp,
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +/* Find the topmost mount satisfying test() */
> > > >  static int find_autofs_mount(const char *pathname,
> > > >  			     struct path *res,
> > > >  			     int test(struct path *path, void *data),
> > > >  			     void *data)
> > > >  {
> > > >  	struct path path;
> > > > -	int err = kern_path(pathname, 0, &path);
> > > > +	int err = user_path_mntpointat(AT_FDCWD, pathname, 0, &path);
> > > 
> > > This looks wrong. "pathname" is a kernel string, not a __user one. I
> > > think what you need to do here is to turn user_path_mntpointat into a
> > > wrapper around a kern_path_mntpointat equivalent and then call that
> > > here.
> > 
> > In both cases the path comes from a structure passed from user space.
> > So I started thinking it wasn't correct previously.
> > 
> 
> AFAICT, this is a kernel string by the time it gets here.
> 
> _autofs_dev_ioctl calls copy_dev_ioctl, which copies that struct from
> userland to a kernel buffer.

Yeah, right you are, doesn't matter now anyway.
I expect we'll go with what Al has in his for-next branch.

> 
> > > 
> > > >  	if (err)
> > > >  		return err;
> > > >  	err = -ENOENT;
> > > > @@ -197,10 +198,9 @@ static int find_autofs_mount(const char *pathname,
> > > >  		if (path.dentry->d_sb->s_magic == AUTOFS_SUPER_MAGIC) {
> > > >  			if (test(&path, data)) {
> > > >  				path_get(&path);
> > > > -				if (!err) /* already found some */
> > > > -					path_put(res);
> > > >  				*res = path;
> > > >  				err = 0;
> > > > +				break;
> > > >  			}
> > > >  		}
> > > >  		if (!follow_up(&path))
> > > > @@ -498,12 +498,11 @@ static int autofs_dev_ioctl_askumount(struct file *fp,
> > > >   * mount if there is one or 0 if it isn't a mountpoint.
> > > >   *
> > > >   * If we aren't supplied with a file descriptor then we
> > > > - * lookup the nameidata of the path and check if it is the
> > > > - * root of a mount. If a type is given we are looking for
> > > > - * a particular autofs mount and if we don't find a match
> > > > - * we return fail. If the located nameidata path is the
> > > > - * root of a mount we return 1 along with the super magic
> > > > - * of the mount or 0 otherwise.
> > > > + * lookup the path and check if it is the root of a mount.
> > > > + * If a type is given we are looking for a particular autofs
> > > > + * mount and if we don't find a match we return fail. If the
> > > > + * located path is the root of a mount we return 1 along with
> > > > + * the super magic of the mount or 0 otherwise.
> > > >   *
> > > >   * In both cases the the device number (as returned by
> > > >   * new_encode_dev()) is also returned.
> > > > @@ -531,9 +530,11 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp,
> > > >  
> > > >  	if (!fp || param->ioctlfd == -1) {
> > > >  		if (autofs_type_any(type))
> > > > -			err = kern_path(name, LOOKUP_FOLLOW, &path);
> > > > +			err = user_path_mntpointat(AT_FDCWD,
> > > > +						   name, LOOKUP_FOLLOW, &path);
> > > >  		else
> > > > -			err = find_autofs_mount(name, &path, test_by_type, &type);
> > > > +			err = find_autofs_mount(name, &path,
> > > > +						test_by_type, &type);
> > > 
> > > 
> > > ...ditto in these spots of course...
> > > 
> > > >  		if (err)
> > > >  			goto out;
> > > >  		devid = new_encode_dev(path.dentry->d_sb->s_dev);
> > > > 
> > > 
> > > 
> > 
> > 
> 
>