linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] autofs4 - fix device ioctl mount lookup
@ 2013-09-04  0:54 Ian Kent
  2013-09-04  0:55 ` [PATCH 2/3] autofs: fix the return value of autofs4_fill_super Ian Kent
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Ian Kent @ 2013-09-04  0:54 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: linux-fsdevel, rui.xiang, autofs mailing list,
	Kernel Mailing List, Al Viro

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.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/autofs4/dev-ioctl.c |   72 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 62 insertions(+), 10 deletions(-)

diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index 743c7c2..1d24e42 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -183,13 +183,67 @@ static int autofs_dev_ioctl_protosubver(struct file *fp,
 	return 0;
 }
 
+/*
+ * Lookup the the topmost path of a (possible) mount stack.
+ *
+ * kern_path() can't be used here because the caller might not
+ * "own" the automount dentry yet and we would end up calling
+ * back to ourself.
+ */
+static int kern_path_top(const char *pathname,
+			 unsigned int flags, struct path *path)
+{
+	struct dentry *dentry;
+	struct qstr name;
+	const char *tmp;
+	unsigned int len;
+	int err;
+
+	len = strlen(pathname);
+	if (len <= 1)
+		return -EINVAL;
+
+	tmp = pathname + len - 1;
+	len = 0;
+	if (*tmp == '/')
+		tmp--;
+	do {
+		if (*tmp == '/')
+			break;
+		len++;
+	} while (--tmp >= pathname);
+	tmp++;
+
+	err = kern_path(pathname, flags | LOOKUP_PARENT, path);
+	if (err)
+		return err;
+
+	name.name = tmp;
+	name.len = len;
+	name.hash = full_name_hash(tmp, len);
+
+	dentry = d_lookup(path->dentry, &name);
+	if (!dentry) {
+		path_put(path);
+		return -ENOENT;
+	}
+	dput(path->dentry);
+	path->dentry = dentry;
+
+	while (follow_down_one(path))
+		;
+
+	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 = kern_path_top(pathname, 0, &path);
 	if (err)
 		return err;
 	err = -ENOENT;
@@ -197,10 +251,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))
@@ -486,12 +539,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.
@@ -519,7 +571,7 @@ 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 = kern_path_top(name, LOOKUP_FOLLOW, &path);
 		else
 			err = find_autofs_mount(name, &path, test_by_type, &type);
 		if (err)

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/3] autofs: fix the return value of autofs4_fill_super
  2013-09-04  0:54 [PATCH 1/3] autofs4 - fix device ioctl mount lookup Ian Kent
@ 2013-09-04  0:55 ` Ian Kent
  2013-09-04  0:55 ` [PATCH 3/3] autofs: use IS_ROOT to replace root dentry checks Ian Kent
  2013-09-04  1:03 ` [PATCH 1/3] autofs4 - fix device ioctl mount lookup Al Viro
  2 siblings, 0 replies; 15+ messages in thread
From: Ian Kent @ 2013-09-04  0:55 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: linux-fsdevel, rui.xiang, autofs mailing list,
	Kernel Mailing List, Al Viro

From: Rui Xiang <rui.xiang@huawei.com>

While kzallocing sbi/ino fails, it should return -ENOMEM.

And it should return the err value from autofs_prepare_pipe.

Signed-off-by: Rui Xiang <rui.xiang@huawei.com>
Acked-by: Ian Kent <raven@themaw.net>
---
 fs/autofs4/inode.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index b104726..fe390ed 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -211,10 +211,11 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 	int pipefd;
 	struct autofs_sb_info *sbi;
 	struct autofs_info *ino;
+	int ret = -EINVAL;
 
 	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
 	if (!sbi)
-		goto fail_unlock;
+		return -ENOMEM;
 	DPRINTK("starting up, sbi = %p",sbi);
 
 	s->s_fs_info = sbi;
@@ -248,8 +249,10 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 	 * Get the root inode and dentry, but defer checking for errors.
 	 */
 	ino = autofs4_new_ino(sbi);
-	if (!ino)
+	if (!ino) {
+		ret = -ENOMEM;
 		goto fail_free;
+	}
 	root_inode = autofs4_get_inode(s, S_IFDIR | 0755);
 	root = d_make_root(root_inode);
 	if (!root)
@@ -296,7 +299,8 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 		printk("autofs: could not open pipe file descriptor\n");
 		goto fail_dput;
 	}
-	if (autofs_prepare_pipe(pipe) < 0)
+	ret = autofs_prepare_pipe(pipe);
+	if (ret < 0)
 		goto fail_fput;
 	sbi->pipe = pipe;
 	sbi->pipefd = pipefd;
@@ -323,8 +327,7 @@ fail_ino:
 fail_free:
 	kfree(sbi);
 	s->s_fs_info = NULL;
-fail_unlock:
-	return -EINVAL;
+	return ret;
 }
 
 struct inode *autofs4_get_inode(struct super_block *sb, umode_t mode)

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/3] autofs: use IS_ROOT to replace root dentry checks
  2013-09-04  0:54 [PATCH 1/3] autofs4 - fix device ioctl mount lookup Ian Kent
  2013-09-04  0:55 ` [PATCH 2/3] autofs: fix the return value of autofs4_fill_super Ian Kent
@ 2013-09-04  0:55 ` Ian Kent
  2013-09-04  1:03 ` [PATCH 1/3] autofs4 - fix device ioctl mount lookup Al Viro
  2 siblings, 0 replies; 15+ messages in thread
From: Ian Kent @ 2013-09-04  0:55 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: linux-fsdevel, rui.xiang, autofs mailing list,
	Kernel Mailing List, Al Viro

From: Rui Xiang <rui.xiang@huawei.com>

Use the helper macro !IS_ROOT to replace parent != dentry->d_parent.
Just clean up.

Signed-off-by: Rui Xiang <rui.xiang@huawei.com>
Acked-by: Ian Kent <raven@themaw.net>
---
 fs/autofs4/root.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index 92ef341..2caf36a 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -558,7 +558,7 @@ static int autofs4_dir_symlink(struct inode *dir,
 	dget(dentry);
 	atomic_inc(&ino->count);
 	p_ino = autofs4_dentry_ino(dentry->d_parent);
-	if (p_ino && dentry->d_parent != dentry)
+	if (p_ino && !IS_ROOT(dentry))
 		atomic_inc(&p_ino->count);
 
 	dir->i_mtime = CURRENT_TIME;
@@ -593,7 +593,7 @@ static int autofs4_dir_unlink(struct inode *dir, struct dentry *dentry)
 
 	if (atomic_dec_and_test(&ino->count)) {
 		p_ino = autofs4_dentry_ino(dentry->d_parent);
-		if (p_ino && dentry->d_parent != dentry)
+		if (p_ino && !IS_ROOT(dentry))
 			atomic_dec(&p_ino->count);
 	}
 	dput(ino->dentry);
@@ -732,7 +732,7 @@ static int autofs4_dir_mkdir(struct inode *dir, struct dentry *dentry, umode_t m
 	dget(dentry);
 	atomic_inc(&ino->count);
 	p_ino = autofs4_dentry_ino(dentry->d_parent);
-	if (p_ino && dentry->d_parent != dentry)
+	if (p_ino && !IS_ROOT(dentry))
 		atomic_inc(&p_ino->count);
 	inc_nlink(dir);
 	dir->i_mtime = CURRENT_TIME;


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] autofs4 - fix device ioctl mount lookup
  2013-09-04  0:54 [PATCH 1/3] autofs4 - fix device ioctl mount lookup Ian Kent
  2013-09-04  0:55 ` [PATCH 2/3] autofs: fix the return value of autofs4_fill_super Ian Kent
  2013-09-04  0:55 ` [PATCH 3/3] autofs: use IS_ROOT to replace root dentry checks Ian Kent
@ 2013-09-04  1:03 ` Al Viro
  2013-09-04  2:00   ` Al Viro
  2 siblings, 1 reply; 15+ messages in thread
From: Al Viro @ 2013-09-04  1:03 UTC (permalink / raw)
  To: Ian Kent
  Cc: Linus Torvalds, linux-fsdevel, rui.xiang, autofs mailing list,
	Kernel Mailing List

On Wed, Sep 04, 2013 at 08:54:57AM +0800, Ian Kent wrote:
> +static int kern_path_top(const char *pathname,
> +			 unsigned int flags, struct path *path)
> +{
> +	struct dentry *dentry;
> +	struct qstr name;
> +	const char *tmp;
> +	unsigned int len;
> +	int err;
> +
> +	len = strlen(pathname);
> +	if (len <= 1)
> +		return -EINVAL;
> +
> +	tmp = pathname + len - 1;
> +	len = 0;
> +	if (*tmp == '/')
> +		tmp--;
> +	do {
> +		if (*tmp == '/')
> +			break;
> +		len++;
> +	} while (--tmp >= pathname);
> +	tmp++;
> +
> +	err = kern_path(pathname, flags | LOOKUP_PARENT, path);
> +	if (err)
> +		return err;
> +
> +	name.name = tmp;
> +	name.len = len;
> +	name.hash = full_name_hash(tmp, len);
> +
> +	dentry = d_lookup(path->dentry, &name);
> +	if (!dentry) {
> +		path_put(path);
> +		return -ENOENT;
> +	}
> +	dput(path->dentry);
> +	path->dentry = dentry;
> +
> +	while (follow_down_one(path))
> +		;
> +
> +	return 0;
> +}

Ewwww...   NAK in that form.  Just what will happen if the last component
given to that sucker will be . or .., for starters?  Or a symlink, with
'/' added to it to force following the damn thing?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] autofs4 - fix device ioctl mount lookup
  2013-09-04  1:03 ` [PATCH 1/3] autofs4 - fix device ioctl mount lookup Al Viro
@ 2013-09-04  2:00   ` Al Viro
  2013-09-04  2:18     ` Linus Torvalds
  0 siblings, 1 reply; 15+ messages in thread
From: Al Viro @ 2013-09-04  2:00 UTC (permalink / raw)
  To: Ian Kent
  Cc: Linus Torvalds, linux-fsdevel, rui.xiang, autofs mailing list,
	Kernel Mailing List

On Wed, Sep 04, 2013 at 02:03:02AM +0100, Al Viro wrote:

> Ewwww...   NAK in that form.  Just what will happen if the last component
> given to that sucker will be . or .., for starters?  Or a symlink, with
> '/' added to it to force following the damn thing?

OK, in more details:
	* we are changing a user-visible ABI here - kern_path() with
LOOKUP_FOLLOW had been there since the introduction of that misc
device (OK, it used to be path_lookup(), but that changes nothing).
IOW, we used to follow symlinks there, now we do not.
	* pathname may end on more than one slash.  Modified that
way, the code won't do anything good (not to mention that d_lookup()
on an empty string is an interesting torture test for fs/dcache.c;
probably you'll just find nothing, but normally the function is never
called with such arguments, so...).  In any case it's an ABI change.
	* pathname may end with something/. or something/..; again,
d_lookup() won't find you anything now, so we have an ABI change.

That aside, I'm really not happy with this kind of games; this stuff clearly
belongs in fs/namei.c where we can simply see the last component.  Doing that
on the level of "let's scan the pathname for slashes, etc." is just plain
wrong.  Let's step back for a minute here; what are you trying to do?
You have a pathname that should resolve to a mountpoint, without triggering
automount (or crossing into the mountpoint, for that matter) and you want
struct path for the bottom of that mount stack?  Or is it something
completely different?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] autofs4 - fix device ioctl mount lookup
  2013-09-04  2:00   ` Al Viro
@ 2013-09-04  2:18     ` Linus Torvalds
  2013-09-04  2:26       ` Al Viro
  2013-09-04  2:46       ` Ian Kent
  0 siblings, 2 replies; 15+ messages in thread
From: Linus Torvalds @ 2013-09-04  2:18 UTC (permalink / raw)
  To: Al Viro
  Cc: Ian Kent, linux-fsdevel, rui.xiang, autofs mailing list,
	Kernel Mailing List

On Tue, Sep 3, 2013 at 7:00 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> That aside, I'm really not happy with this kind of games; this stuff clearly
> belongs in fs/namei.c where we can simply see the last component.  Doing that
> on the level of "let's scan the pathname for slashes, etc." is just plain
> wrong.  Let's step back for a minute here; what are you trying to do?
> You have a pathname that should resolve to a mountpoint, without triggering
> automount (or crossing into the mountpoint, for that matter) and you want
> struct path for the bottom of that mount stack?  Or is it something
> completely different?

Can we add a LOOKUP_NOAUTOMNT bit or something (not exposed to user
space, only used for this particular kern_path() call). Then, if/when
automount gets called recursively (through autofs4_lookup? Or is it
just the autofs4_d_automount() interface?) it can just decide to not
follow that last path.

Hmm? I don't think we pass in the lookup-flags to d_automount, but
that could be changed. Yes?

             Linus

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] autofs4 - fix device ioctl mount lookup
  2013-09-04  2:18     ` Linus Torvalds
@ 2013-09-04  2:26       ` Al Viro
  2013-09-04  2:42         ` Al Viro
  2013-09-04  2:46       ` Ian Kent
  1 sibling, 1 reply; 15+ messages in thread
From: Al Viro @ 2013-09-04  2:26 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Ian Kent, linux-fsdevel, rui.xiang, autofs mailing list,
	Kernel Mailing List

On Tue, Sep 03, 2013 at 07:18:42PM -0700, Linus Torvalds wrote:
> On Tue, Sep 3, 2013 at 7:00 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > That aside, I'm really not happy with this kind of games; this stuff clearly
> > belongs in fs/namei.c where we can simply see the last component.  Doing that
> > on the level of "let's scan the pathname for slashes, etc." is just plain
> > wrong.  Let's step back for a minute here; what are you trying to do?
> > You have a pathname that should resolve to a mountpoint, without triggering
> > automount (or crossing into the mountpoint, for that matter) and you want
> > struct path for the bottom of that mount stack?  Or is it something
> > completely different?
> 
> Can we add a LOOKUP_NOAUTOMNT bit or something (not exposed to user
> space, only used for this particular kern_path() call). Then, if/when
> automount gets called recursively (through autofs4_lookup? Or is it
> just the autofs4_d_automount() interface?) it can just decide to not
> follow that last path.
> 
> Hmm? I don't think we pass in the lookup-flags to d_automount, but
> that could be changed. Yes?

No need, really - what we ought to do, AFAICS, is what umount(2) needs.
I've applied slightly modified variant of Jeff's "vfs: allow umount to handle
mountpoints without revalidating them" (modified by just leaving the
struct path filled with mountpoint and leaving the equivalent of follow_mount()
to caller) to the local queue and I'm pretty sure that it's what we want
here as well.

I'll push for-next tonight and send a pull request your way tomorrow, if you
are OK with that.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] autofs4 - fix device ioctl mount lookup
  2013-09-04  2:26       ` Al Viro
@ 2013-09-04  2:42         ` Al Viro
  2013-09-04  3:53           ` Ian Kent
  2013-09-06  8:38           ` Ian Kent
  0 siblings, 2 replies; 15+ messages in thread
From: Al Viro @ 2013-09-04  2:42 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Ian Kent, linux-fsdevel, rui.xiang, autofs mailing list,
	Kernel Mailing List

On Wed, Sep 04, 2013 at 03:26:17AM +0100, Al Viro wrote:
> I've applied slightly modified variant of Jeff's "vfs: allow umount to handle
> mountpoints without revalidating them" (modified by just leaving the
> struct path filled with mountpoint and leaving the equivalent of follow_mount()
> to caller) to the local queue and I'm pretty sure that it's what we want
> here as well.

... and killed the modifications since the result ends up uglier for
caller(s) anyway.  Reapplied as-is.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] autofs4 - fix device ioctl mount lookup
  2013-09-04  2:18     ` Linus Torvalds
  2013-09-04  2:26       ` Al Viro
@ 2013-09-04  2:46       ` Ian Kent
  1 sibling, 0 replies; 15+ messages in thread
From: Ian Kent @ 2013-09-04  2:46 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Al Viro, linux-fsdevel, rui.xiang, autofs mailing list,
	Kernel Mailing List

On Tue, 2013-09-03 at 19:18 -0700, Linus Torvalds wrote:
> On Tue, Sep 3, 2013 at 7:00 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > That aside, I'm really not happy with this kind of games; this stuff clearly
> > belongs in fs/namei.c where we can simply see the last component.  Doing that
> > on the level of "let's scan the pathname for slashes, etc." is just plain
> > wrong.  Let's step back for a minute here; what are you trying to do?
> > You have a pathname that should resolve to a mountpoint, without triggering
> > automount (or crossing into the mountpoint, for that matter) and you want
> > struct path for the bottom of that mount stack?  Or is it something
> > completely different?
> 
> Can we add a LOOKUP_NOAUTOMNT bit or something (not exposed to user
> space, only used for this particular kern_path() call). Then, if/when
> automount gets called recursively (through autofs4_lookup? Or is it
> just the autofs4_d_automount() interface?) it can just decide to not
> follow that last path.
> 
> Hmm? I don't think we pass in the lookup-flags to d_automount, but
> that could be changed. Yes?

We don't need to.

Al is completely right, Jeff's umount path lookup patch will cover all
the needed cases, AFAICS.

That's a relief.

> 
>              Linus



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] autofs4 - fix device ioctl mount lookup
  2013-09-04  2:42         ` Al Viro
@ 2013-09-04  3:53           ` Ian Kent
  2013-09-04  4:07             ` Ian Kent
  2013-09-06  8:38           ` Ian Kent
  1 sibling, 1 reply; 15+ messages in thread
From: Ian Kent @ 2013-09-04  3:53 UTC (permalink / raw)
  To: Al Viro
  Cc: Linus Torvalds, linux-fsdevel, rui.xiang, autofs mailing list,
	Kernel Mailing List

On Wed, 2013-09-04 at 03:42 +0100, Al Viro wrote:
> On Wed, Sep 04, 2013 at 03:26:17AM +0100, Al Viro wrote:
> > I've applied slightly modified variant of Jeff's "vfs: allow umount to handle
> > mountpoints without revalidating them" (modified by just leaving the
> > struct path filled with mountpoint and leaving the equivalent of follow_mount()
> > to caller) to the local queue and I'm pretty sure that it's what we want
> > here as well.
> 
> ... and killed the modifications since the result ends up uglier for
> caller(s) anyway.  Reapplied as-is.

But isn't that what's needed anyway?

Looks like it fits with the existing code that walks back down looking
for a match.

And that fits in with fixing the bug where we want the first match just
means breaking out early. The match should be close to the top, if not
the first then the second, for the common case.

Ian


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] autofs4 - fix device ioctl mount lookup
  2013-09-04  3:53           ` Ian Kent
@ 2013-09-04  4:07             ` Ian Kent
  2013-09-04 10:35               ` Jeff Layton
  0 siblings, 1 reply; 15+ messages in thread
From: Ian Kent @ 2013-09-04  4:07 UTC (permalink / raw)
  To: Al Viro
  Cc: Linus Torvalds, linux-fsdevel, rui.xiang, autofs mailing list,
	Kernel Mailing List

On Wed, 2013-09-04 at 11:53 +0800, Ian Kent wrote:
> On Wed, 2013-09-04 at 03:42 +0100, Al Viro wrote:
> > On Wed, Sep 04, 2013 at 03:26:17AM +0100, Al Viro wrote:
> > > I've applied slightly modified variant of Jeff's "vfs: allow umount to handle
> > > mountpoints without revalidating them" (modified by just leaving the
> > > struct path filled with mountpoint and leaving the equivalent of follow_mount()
> > > to caller) to the local queue and I'm pretty sure that it's what we want
> > > here as well.
> > 
> > ... and killed the modifications since the result ends up uglier for
> > caller(s) anyway.  Reapplied as-is.
> 
> But isn't that what's needed anyway?

Except for the question of following symlinks which is still bugging me.

Granted, the initial implementation (8d7b48e0) didn't check for a
symlink but probably should have and returned EINVAL if it found one.

That's because the whole process is driven by automount maps that have
specific paths, so needing to follow a symlink is an indication the
caller is doing something wrong.

It's probably not actually harmful to cater for it in kernel .... though
the user will likely crash and burn some time later.

> 
> Looks like it fits with the existing code that walks back down looking
> for a match.
> 
> And that fits in with fixing the bug where we want the first match just
> means breaking out early. The match should be close to the top, if not
> the first then the second, for the common case.
> 
> Ian



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] autofs4 - fix device ioctl mount lookup
  2013-09-04  4:07             ` Ian Kent
@ 2013-09-04 10:35               ` Jeff Layton
  0 siblings, 0 replies; 15+ messages in thread
From: Jeff Layton @ 2013-09-04 10:35 UTC (permalink / raw)
  To: Ian Kent
  Cc: Al Viro, Linus Torvalds, linux-fsdevel, rui.xiang,
	autofs mailing list, Kernel Mailing List

On Wed, 04 Sep 2013 12:07:28 +0800
Ian Kent <raven@themaw.net> wrote:

> On Wed, 2013-09-04 at 11:53 +0800, Ian Kent wrote:
> > On Wed, 2013-09-04 at 03:42 +0100, Al Viro wrote:
> > > On Wed, Sep 04, 2013 at 03:26:17AM +0100, Al Viro wrote:
> > > > I've applied slightly modified variant of Jeff's "vfs: allow umount to handle
> > > > mountpoints without revalidating them" (modified by just leaving the
> > > > struct path filled with mountpoint and leaving the equivalent of follow_mount()
> > > > to caller) to the local queue and I'm pretty sure that it's what we want
> > > > here as well.
> > > 
> > > ... and killed the modifications since the result ends up uglier for
> > > caller(s) anyway.  Reapplied as-is.
> > 

Thanks Al. Note that there's a follow-on fix for the kerneldoc header
on one of the functions that will be floating around in akpm's tree
soon too. No code changes there though.

> > But isn't that what's needed anyway?
> 
> Except for the question of following symlinks which is still bugging me.
> 
> Granted, the initial implementation (8d7b48e0) didn't check for a
> symlink but probably should have and returned EINVAL if it found one.
> 
> That's because the whole process is driven by automount maps that have
> specific paths, so needing to follow a symlink is an indication the
> caller is doing something wrong.
> 
> It's probably not actually harmful to cater for it in kernel .... though
> the user will likely crash and burn some time later.
> 

If you don't want to allow the last component to be a symlink then the
easiest fix is probably to turn user_path_umountat() into a wrapper
around a new kern_path_umountat() function and simply call that without
LOOKUP_FOLLOW set. Then check to see if the last component is a symlink
before you use it. Of course, that means that you can't case symlinks
in intermediate components either. I'm not sure if that's an issue for
you though.

Doing that wrapperization would be a good opportunity to rename the
function as well, but I'm struck with my usual lack of creativity in
that department.

> > 
> > Looks like it fits with the existing code that walks back down looking
> > for a match.
> > 
> > And that fits in with fixing the bug where we want the first match just
> > means breaking out early. The match should be close to the top, if not
> > the first then the second, for the common case.
> > 
> > Ian
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] autofs4 - fix device ioctl mount lookup
  2013-09-04  2:42         ` Al Viro
  2013-09-04  3:53           ` Ian Kent
@ 2013-09-06  8:38           ` Ian Kent
  2013-09-06  8:54             ` Ian Kent
  1 sibling, 1 reply; 15+ messages in thread
From: Ian Kent @ 2013-09-06  8:38 UTC (permalink / raw)
  To: Al Viro
  Cc: Linus Torvalds, linux-fsdevel, rui.xiang, autofs mailing list,
	Kernel Mailing List

On Wed, 2013-09-04 at 03:42 +0100, Al Viro wrote:
> On Wed, Sep 04, 2013 at 03:26:17AM +0100, Al Viro wrote:
> > I've applied slightly modified variant of Jeff's "vfs: allow umount to handle
> > mountpoints without revalidating them" (modified by just leaving the
> > struct path filled with mountpoint and leaving the equivalent of follow_mount()
> > to caller) to the local queue and I'm pretty sure that it's what we want
> > here as well.
> 
> ... and killed the modifications since the result ends up uglier for
> caller(s) anyway.  Reapplied as-is.

Looks like Jeff's patch has been merged, commit 8033426e6.

Revalidation isn't the only thing not done on the last component using
Jeff's user_path_umountat() path walk. It also bypasses the managed
dentry code for the last component, which is why it's what I need as
well.

Encoding umount in the name seems misleading as to what it really does
as would encoding unmanaged or similar since that doesn't properly cover
it either.

I can rename it in a patch to solve my autofs problem, so how about
something like user_path_simple_last(), other suggestions anyone?

Ian


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] autofs4 - fix device ioctl mount lookup
  2013-09-06  8:38           ` Ian Kent
@ 2013-09-06  8:54             ` Ian Kent
  2013-09-06 10:11               ` Jeff Layton
  0 siblings, 1 reply; 15+ messages in thread
From: Ian Kent @ 2013-09-06  8:54 UTC (permalink / raw)
  To: Jeffrey Layton
  Cc: Linus Torvalds, linux-fsdevel, rui.xiang, autofs mailing list,
	Kernel Mailing List, Al Viro


Sorry, I should have added Jeff to the cc for this post.

On Fri, 2013-09-06 at 16:38 +0800, Ian Kent wrote:
> On Wed, 2013-09-04 at 03:42 +0100, Al Viro wrote:
> > On Wed, Sep 04, 2013 at 03:26:17AM +0100, Al Viro wrote:
> > > I've applied slightly modified variant of Jeff's "vfs: allow umount to handle
> > > mountpoints without revalidating them" (modified by just leaving the
> > > struct path filled with mountpoint and leaving the equivalent of follow_mount()
> > > to caller) to the local queue and I'm pretty sure that it's what we want
> > > here as well.
> > 
> > ... and killed the modifications since the result ends up uglier for
> > caller(s) anyway.  Reapplied as-is.
> 
> Looks like Jeff's patch has been merged, commit 8033426e6.
> 
> Revalidation isn't the only thing not done on the last component using
> Jeff's user_path_umountat() path walk. It also bypasses the managed
> dentry code for the last component, which is why it's what I need as
> well.
> 
> Encoding umount in the name seems misleading as to what it really does
> as would encoding unmanaged or similar since that doesn't properly cover
> it either.
> 
> I can rename it in a patch to solve my autofs problem, so how about
> something like user_path_simple_last(), other suggestions anyone?
> 
> Ian



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] autofs4 - fix device ioctl mount lookup
  2013-09-06  8:54             ` Ian Kent
@ 2013-09-06 10:11               ` Jeff Layton
  0 siblings, 0 replies; 15+ messages in thread
From: Jeff Layton @ 2013-09-06 10:11 UTC (permalink / raw)
  To: Ian Kent
  Cc: Linus Torvalds, linux-fsdevel, rui.xiang, autofs mailing list,
	Kernel Mailing List, Al Viro

On Fri, 06 Sep 2013 16:54:19 +0800
Ian Kent <raven@themaw.net> wrote:

> 
> Sorry, I should have added Jeff to the cc for this post.
> 
> On Fri, 2013-09-06 at 16:38 +0800, Ian Kent wrote:
> > On Wed, 2013-09-04 at 03:42 +0100, Al Viro wrote:
> > > On Wed, Sep 04, 2013 at 03:26:17AM +0100, Al Viro wrote:
> > > > I've applied slightly modified variant of Jeff's "vfs: allow umount to handle
> > > > mountpoints without revalidating them" (modified by just leaving the
> > > > struct path filled with mountpoint and leaving the equivalent of follow_mount()
> > > > to caller) to the local queue and I'm pretty sure that it's what we want
> > > > here as well.
> > > 
> > > ... and killed the modifications since the result ends up uglier for
> > > caller(s) anyway.  Reapplied as-is.
> > 
> > Looks like Jeff's patch has been merged, commit 8033426e6.
> > 
> > Revalidation isn't the only thing not done on the last component using
> > Jeff's user_path_umountat() path walk. It also bypasses the managed
> > dentry code for the last component, which is why it's what I need as
> > well.
> > 
> > Encoding umount in the name seems misleading as to what it really does
> > as would encoding unmanaged or similar since that doesn't properly cover
> > it either.
> > 
> > I can rename it in a patch to solve my autofs problem, so how about
> > something like user_path_simple_last(), other suggestions anyone?
> > 
> > Ian
> 
> 

Since the purpose is to find mountpoints, maybe user_path_mntpoint()?

-- 
Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2013-09-06 10:11 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-04  0:54 [PATCH 1/3] autofs4 - fix device ioctl mount lookup Ian Kent
2013-09-04  0:55 ` [PATCH 2/3] autofs: fix the return value of autofs4_fill_super Ian Kent
2013-09-04  0:55 ` [PATCH 3/3] autofs: use IS_ROOT to replace root dentry checks Ian Kent
2013-09-04  1:03 ` [PATCH 1/3] autofs4 - fix device ioctl mount lookup Al Viro
2013-09-04  2:00   ` Al Viro
2013-09-04  2:18     ` Linus Torvalds
2013-09-04  2:26       ` Al Viro
2013-09-04  2:42         ` Al Viro
2013-09-04  3:53           ` Ian Kent
2013-09-04  4:07             ` Ian Kent
2013-09-04 10:35               ` Jeff Layton
2013-09-06  8:38           ` Ian Kent
2013-09-06  8:54             ` Ian Kent
2013-09-06 10:11               ` Jeff Layton
2013-09-04  2:46       ` Ian Kent

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).