[PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[Miklos Szeredi]

From: Miklos Szeredi <mszeredi@suse.cz>

Prior to 2.6.38 automount would not trigger on either stat(2) or
lstat(2) on the automount point.

After 2.6.38, with the introduction of the ->d_automount()
infrastructure, stat(2) and others would start triggering automount
while lstat(2), etc. still would not.  This is a regression and a
userspace ABI change.

Problem originally reported here:

  http://thread.gmane.org/gmane.linux.kernel.autofs/6098

It appears that there was an attempt at fixing various userspace tools
to not trigger the automount.  But since the stat system call is
rather common it is impossible to "fix" all userspace.

This patch reverts the original behavior, which is to not trigger on
stat(2) and other symlink following syscalls.

Reported-by: Leonardo Chiquitto <leonardo.lists@gmail.com>
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: David Howells <dhowells@redhat.com>
CC: Ian Kent <raven@themaw.net>
CC: stable@kernel.org
---
 fs/namei.c |   33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

Index: linux-2.6/fs/namei.c
===================================================================
--- linux-2.6.orig/fs/namei.c	2011-09-05 14:37:25.000000000 +0200
+++ linux-2.6/fs/namei.c	2011-09-05 17:31:14.000000000 +0200
@@ -727,25 +727,22 @@ static int follow_automount(struct path
 	if ((flags & LOOKUP_NO_AUTOMOUNT) && !(flags & LOOKUP_PARENT))
 		return -EISDIR; /* we actually want to stop here */
 
-	/*
-	 * We don't want to mount if someone's just doing a stat and they've
-	 * set AT_SYMLINK_NOFOLLOW - unless they're stat'ing a directory and
-	 * appended a '/' to the name.
+	/* We don't want to mount if someone's just doing a stat -
+	 * unless they're stat'ing a directory and appended a '/' to
+	 * the name.
+	 *
+	 * We do, however, want to mount if someone wants to open or
+	 * create a file of any type under the mountpoint, wants to
+	 * traverse through the mountpoint or wants to open the
+	 * mounted directory.  Also, autofs may mark negative dentries
+	 * as being automount points.  These will need the attentions
+	 * of the daemon to instantiate them before they can be used.
 	 */
-	if (!(flags & LOOKUP_FOLLOW)) {
-		/* We do, however, want to mount if someone wants to open or
-		 * create a file of any type under the mountpoint, wants to
-		 * traverse through the mountpoint or wants to open the mounted
-		 * directory.
-		 * Also, autofs may mark negative dentries as being automount
-		 * points.  These will need the attentions of the daemon to
-		 * instantiate them before they can be used.
-		 */
-		if (!(flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY |
-			     LOOKUP_OPEN | LOOKUP_CREATE)) &&
-		    path->dentry->d_inode)
-			return -EISDIR;
-	}
+	if (!(flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY |
+		     LOOKUP_OPEN | LOOKUP_CREATE)) &&
+	    path->dentry->d_inode)
+		return -EISDIR;
+
 	current->total_link_count++;
 	if (current->total_link_count >= 40)
 		return -ELOOP;

Re: [PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[David Howells]

Miklos Szeredi <miklos@szeredi.hu> wrote:

> After 2.6.38, with the introduction of the ->d_automount()
> infrastructure, stat(2) and others would start triggering automount
> while lstat(2), etc. still would not.  This is a regression and a
> userspace ABI change.

It doesn't necessarily mean it's wrong.  The main class of program that needs
to be prevented from automounting are things that do bulk stat'ing (e.g. ls) -
and they should probably be doing lstat() anyway.

> +	/* We don't want to mount if someone's just doing a stat -
> +	 * unless they're stat'ing a directory and appended a '/' to
> +	 * the name.

Btw, line length is 80 chars.  This comment could easily use one fewer line.

If you use emacs you can do:

	ESC 7 9 C-x f

to set the right margin and then:

	M-q

whilst the cursor is in the comment paragraph to be rearranged and it will
'fill' the paragraph to 79 characters.

David

Re: [PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[Miklos Szeredi]

David Howells <dhowells@redhat.com> writes:

> Miklos Szeredi <miklos@szeredi.hu> wrote:
>
>> After 2.6.38, with the introduction of the ->d_automount()
>> infrastructure, stat(2) and others would start triggering automount
>> while lstat(2), etc. still would not.  This is a regression and a
>> userspace ABI change.
>
> It doesn't necessarily mean it's wrong.  The main class of program that needs
> to be prevented from automounting are things that do bulk stat'ing (e.g. ls) -
> and they should probably be doing lstat() anyway.

Yeah, some of those uses are probably minor bugs in userspace, but there are
probably many of them (Leonardo reported that for example Nautilus also
triggers all child automounts in a directory).  So I don't think it's OK to
just change the behavior here.

If automounting on lstat(2) is the correct behavior (is it?  why?)  then at
least it should be enabled by a global switch or mount option, IMO.

>
>> +	/* We don't want to mount if someone's just doing a stat -
>> +	 * unless they're stat'ing a directory and appended a '/' to
>> +	 * the name.
>
> Btw, line length is 80 chars.  This comment could easily use one fewer line.
>
> If you use emacs you can do:
>
> 	ESC 7 9 C-x f
>
> to set the right margin and then:
>
> 	M-q
>
> whilst the cursor is in the comment paragraph to be rearranged and it will
> 'fill' the paragraph to 79 characters.

Cool.  Updated patch below.

Thanks,
Miklos
----


Subject: vfs: automount should ignore LOOKUP_FOLLOW

From: Miklos Szeredi <mszeredi@suse.cz>

Prior to 2.6.38 automount would not trigger on either stat(2) or
lstat(2) on the automount point.

After 2.6.38, with the introduction of the ->d_automount()
infrastructure, stat(2) and others would start triggering automount
while lstat(2), etc. still would not.  This is a regression and a
userspace ABI change.

Problem originally reported here:

  http://thread.gmane.org/gmane.linux.kernel.autofs/6098

It appears that there was an attempt at fixing various userspace tools
to not trigger the automount.  But since the stat system call is
rather common it is impossible to "fix" all userspace.

This patch reverts the original behavior, which is to not trigger on
stat(2) and other symlink following syscalls.

Reported-by: Leonardo Chiquitto <leonardo.lists@gmail.com>
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: David Howells <dhowells@redhat.com>
CC: Ian Kent <raven@themaw.net>
CC: stable@kernel.org
---
 fs/namei.c |   32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

Index: linux-2.6/fs/namei.c
===================================================================
--- linux-2.6.orig/fs/namei.c	2011-09-05 18:01:59.000000000 +0200
+++ linux-2.6/fs/namei.c	2011-09-05 18:51:25.000000000 +0200
@@ -727,25 +727,21 @@ static int follow_automount(struct path
 	if ((flags & LOOKUP_NO_AUTOMOUNT) && !(flags & LOOKUP_PARENT))
 		return -EISDIR; /* we actually want to stop here */
 
-	/*
-	 * We don't want to mount if someone's just doing a stat and they've
-	 * set AT_SYMLINK_NOFOLLOW - unless they're stat'ing a directory and
-	 * appended a '/' to the name.
+	/* We don't want to mount if someone's just doing a stat - unless
+	 * they're stat'ing a directory and appended a '/' to the name.
+	 *
+	 * We do, however, want to mount if someone wants to open or create a
+	 * file of any type under the mountpoint, wants to traverse through the
+	 * mountpoint or wants to open the mounted directory.  Also, autofs may
+	 * mark negative dentries as being automount points.  These will need
+	 * the attentions of the daemon to instantiate them before they can be
+	 * used.
 	 */
-	if (!(flags & LOOKUP_FOLLOW)) {
-		/* We do, however, want to mount if someone wants to open or
-		 * create a file of any type under the mountpoint, wants to
-		 * traverse through the mountpoint or wants to open the mounted
-		 * directory.
-		 * Also, autofs may mark negative dentries as being automount
-		 * points.  These will need the attentions of the daemon to
-		 * instantiate them before they can be used.
-		 */
-		if (!(flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY |
-			     LOOKUP_OPEN | LOOKUP_CREATE)) &&
-		    path->dentry->d_inode)
-			return -EISDIR;
-	}
+	if (!(flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY |
+		     LOOKUP_OPEN | LOOKUP_CREATE)) &&
+	    path->dentry->d_inode)
+		return -EISDIR;
+
 	current->total_link_count++;
 	if (current->total_link_count >= 40)
 		return -ELOOP;

Re: [PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[Ian Kent]

On Mon, 2011-09-05 at 19:02 +0200, Miklos Szeredi wrote:
> David Howells <dhowells@redhat.com> writes:
> 
> > Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> >> After 2.6.38, with the introduction of the ->d_automount()
> >> infrastructure, stat(2) and others would start triggering automount
> >> while lstat(2), etc. still would not.  This is a regression and a
> >> userspace ABI change.
> >
> > It doesn't necessarily mean it's wrong.  The main class of program that needs
> > to be prevented from automounting are things that do bulk stat'ing (e.g. ls) -
> > and they should probably be doing lstat() anyway.
> 
> Yeah, some of those uses are probably minor bugs in userspace, but there are
> probably many of them (Leonardo reported that for example Nautilus also
> triggers all child automounts in a directory).  So I don't think it's OK to
> just change the behavior here.

Yes, Leonard has been a big help with his recent efforts, thanks
Leonard.

> 
> If automounting on lstat(2) is the correct behavior (is it?  why?)  then at
> least it should be enabled by a global switch or mount option, IMO.

Ideally we wouldn't need to take special precautions for these
operations at all but we can't, especially for GUI environments that
constantly scan file systems on mount/umount activity.

Historically for autofs, neither stat(2) or lstat(2) would trigger a
mount. With the current implementation stat(2) now does but lstat(2)
doesn't which is a step in the right direction IMHO. So, I recommend we
continue to encourage user space to make the needed changes so we
continue to move in the right direction, and yes, I acknowledge it is a
pain but it'll never get done otherwise.

And, yes, other subsystems that need to automount also have the same
problems as autofs now so we do need to take precautions, but that was
already the case before the current implementation.

Hopefully, in time, user space will adopt the use of fcntl(2) and
AT_NO_AUTOMOUNT and we will be able to fix this once and for all, as
this was David's intent in introducing it. Not quite sure how we will go
about promoting that adoption or how much pain that will mean for user
space though.

> 
> >
> >> +	/* We don't want to mount if someone's just doing a stat -
> >> +	 * unless they're stat'ing a directory and appended a '/' to
> >> +	 * the name.
> >
> > Btw, line length is 80 chars.  This comment could easily use one fewer line.
> >
> > If you use emacs you can do:
> >
> > 	ESC 7 9 C-x f
> >
> > to set the right margin and then:
> >
> > 	M-q
> >
> > whilst the cursor is in the comment paragraph to be rearranged and it will
> > 'fill' the paragraph to 79 characters.
> 
> Cool.  Updated patch below.
> 
> Thanks,
> Miklos
> ----
> 
> 
> Subject: vfs: automount should ignore LOOKUP_FOLLOW
> 
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> Prior to 2.6.38 automount would not trigger on either stat(2) or
> lstat(2) on the automount point.
> 
> After 2.6.38, with the introduction of the ->d_automount()
> infrastructure, stat(2) and others would start triggering automount
> while lstat(2), etc. still would not.  This is a regression and a
> userspace ABI change.
> 
> Problem originally reported here:
> 
>   http://thread.gmane.org/gmane.linux.kernel.autofs/6098
> 
> It appears that there was an attempt at fixing various userspace tools
> to not trigger the automount.  But since the stat system call is
> rather common it is impossible to "fix" all userspace.
> 
> This patch reverts the original behavior, which is to not trigger on
> stat(2) and other symlink following syscalls.
> 
> Reported-by: Leonardo Chiquitto <leonardo.lists@gmail.com>
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> CC: David Howells <dhowells@redhat.com>
> CC: Ian Kent <raven@themaw.net>
> CC: stable@kernel.org
> ---
>  fs/namei.c |   32 ++++++++++++++------------------
>  1 file changed, 14 insertions(+), 18 deletions(-)
> 
> Index: linux-2.6/fs/namei.c
> ===================================================================
> --- linux-2.6.orig/fs/namei.c	2011-09-05 18:01:59.000000000 +0200
> +++ linux-2.6/fs/namei.c	2011-09-05 18:51:25.000000000 +0200
> @@ -727,25 +727,21 @@ static int follow_automount(struct path
>  	if ((flags & LOOKUP_NO_AUTOMOUNT) && !(flags & LOOKUP_PARENT))
>  		return -EISDIR; /* we actually want to stop here */
>  
> -	/*
> -	 * We don't want to mount if someone's just doing a stat and they've
> -	 * set AT_SYMLINK_NOFOLLOW - unless they're stat'ing a directory and
> -	 * appended a '/' to the name.
> +	/* We don't want to mount if someone's just doing a stat - unless
> +	 * they're stat'ing a directory and appended a '/' to the name.
> +	 *
> +	 * We do, however, want to mount if someone wants to open or create a
> +	 * file of any type under the mountpoint, wants to traverse through the
> +	 * mountpoint or wants to open the mounted directory.  Also, autofs may
> +	 * mark negative dentries as being automount points.  These will need
> +	 * the attentions of the daemon to instantiate them before they can be
> +	 * used.
>  	 */
> -	if (!(flags & LOOKUP_FOLLOW)) {
> -		/* We do, however, want to mount if someone wants to open or
> -		 * create a file of any type under the mountpoint, wants to
> -		 * traverse through the mountpoint or wants to open the mounted
> -		 * directory.
> -		 * Also, autofs may mark negative dentries as being automount
> -		 * points.  These will need the attentions of the daemon to
> -		 * instantiate them before they can be used.
> -		 */
> -		if (!(flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY |
> -			     LOOKUP_OPEN | LOOKUP_CREATE)) &&
> -		    path->dentry->d_inode)
> -			return -EISDIR;
> -	}
> +	if (!(flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY |
> +		     LOOKUP_OPEN | LOOKUP_CREATE)) &&
> +	    path->dentry->d_inode)
> +		return -EISDIR;
> +
>  	current->total_link_count++;
>  	if (current->total_link_count >= 40)
>  		return -ELOOP;

Re: [PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[Ian Kent]

On Tue, 2011-09-06 at 11:53 +0800, Ian Kent wrote:
> On Mon, 2011-09-05 at 19:02 +0200, Miklos Szeredi wrote:
> > David Howells <dhowells@redhat.com> writes:
> > 
> > > Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > >> After 2.6.38, with the introduction of the ->d_automount()
> > >> infrastructure, stat(2) and others would start triggering automount
> > >> while lstat(2), etc. still would not.  This is a regression and a
> > >> userspace ABI change.
> > >
> > > It doesn't necessarily mean it's wrong.  The main class of program that needs
> > > to be prevented from automounting are things that do bulk stat'ing (e.g. ls) -
> > > and they should probably be doing lstat() anyway.
> > 
> > Yeah, some of those uses are probably minor bugs in userspace, but there are
> > probably many of them (Leonardo reported that for example Nautilus also
> > triggers all child automounts in a directory).  So I don't think it's OK to
> > just change the behavior here.
> 
> Yes, Leonard has been a big help with his recent efforts, thanks
> Leonard.
> 
> > 
> > If automounting on lstat(2) is the correct behavior (is it?  why?)  then at
> > least it should be enabled by a global switch or mount option, IMO.
> 
> Ideally we wouldn't need to take special precautions for these
> operations at all but we can't, especially for GUI environments that
> constantly scan file systems on mount/umount activity.
> 
> Historically for autofs, neither stat(2) or lstat(2) would trigger a
> mount. With the current implementation stat(2) now does but lstat(2)
> doesn't which is a step in the right direction IMHO. So, I recommend we
> continue to encourage user space to make the needed changes so we
> continue to move in the right direction, and yes, I acknowledge it is a
> pain but it'll never get done otherwise.
> 
> And, yes, other subsystems that need to automount also have the same
> problems as autofs now so we do need to take precautions, but that was
> already the case before the current implementation.
> 
> Hopefully, in time, user space will adopt the use of fcntl(2) and
> AT_NO_AUTOMOUNT and we will be able to fix this once and for all, as
> this was David's intent in introducing it. Not quite sure how we will go
> about promoting that adoption or how much pain that will mean for user
> space though.

Actually, maybe that's not entirely sensible either, now that I think
about it. Isn't it so that if we have one process that shouldn't trigger
mounts and sets the flag, other processes that possibly should won't
either ... mmm ... I'll need to check that.

Ian

Re: [PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[Miklos Szeredi]

Ian Kent <raven@themaw.net> writes:

> On Mon, 2011-09-05 at 19:02 +0200, Miklos Szeredi wrote:
>> 
>> If automounting on lstat(2) is the correct behavior (is it?  why?)  then at
>> least it should be enabled by a global switch or mount option, IMO.
>
> Ideally we wouldn't need to take special precautions for these
> operations at all but we can't, especially for GUI environments that
> constantly scan file systems on mount/umount activity.
>
> Historically for autofs, neither stat(2) or lstat(2) would trigger a
> mount. With the current implementation stat(2) now does but lstat(2)
> doesn't which is a step in the right direction IMHO. So, I recommend we
> continue to encourage user space to make the needed changes so we
> continue to move in the right direction, and yes, I acknowledge it is a
> pain but it'll never get done otherwise.

I'm not quite convinced.  What's the advantage of triggering automount
on stat(2)?

Has anybody complained that stat(2) on the mountpoint doesn't cause an
automount?

Thanks,
Miklos

Re: [PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[Ian Kent]

On Tue, 2011-09-06 at 10:09 +0200, Miklos Szeredi wrote:
> Ian Kent <raven@themaw.net> writes:
> 
> > On Mon, 2011-09-05 at 19:02 +0200, Miklos Szeredi wrote:
> >> 
> >> If automounting on lstat(2) is the correct behavior (is it?  why?)  then at
> >> least it should be enabled by a global switch or mount option, IMO.
> >
> > Ideally we wouldn't need to take special precautions for these
> > operations at all but we can't, especially for GUI environments that
> > constantly scan file systems on mount/umount activity.
> >
> > Historically for autofs, neither stat(2) or lstat(2) would trigger a
> > mount. With the current implementation stat(2) now does but lstat(2)
> > doesn't which is a step in the right direction IMHO. So, I recommend we
> > continue to encourage user space to make the needed changes so we
> > continue to move in the right direction, and yes, I acknowledge it is a
> > pain but it'll never get done otherwise.
> 
> I'm not quite convinced.  What's the advantage of triggering automount
> on stat(2)?

You get the information of the directory of the mounted fs and for many
peoples purposes automounting should be transparent so that would be
best.
 
> 
> Has anybody complained that stat(2) on the mountpoint doesn't cause an
> automount?

Yes, based on the reasoning above.

I have plead my case on that a number of times and because I have had no
choice but to continue with it the users have no choice but to accept
it. For these people I recommend not using the "browse" option with
autofs mount maps.

For autofs this is only a problem for mount maps that use "browse"
option. Basically ones where the mount point directory already exists
and is later mounted upon. When the dentry doesn't exist exist a mount
needs to be performed, at least it is needed to maintain previous
semantics. Other users of automounting are usually mounting on
directories that already exist, such as NFS crossing mount point
directories. I'm pretty sure the automount should be transparent for
them and similarly for CIFS DFS mounts.

Ian

Re: [PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[Miklos Szeredi]

Ian Kent <raven@themaw.net> writes:

> On Tue, 2011-09-06 at 10:09 +0200, Miklos Szeredi wrote:
>> Ian Kent <raven@themaw.net> writes:
>> 
>> > On Mon, 2011-09-05 at 19:02 +0200, Miklos Szeredi wrote:
>> >> 
>> >> If automounting on lstat(2) is the correct behavior (is it?  why?)  then at
>> >> least it should be enabled by a global switch or mount option, IMO.
>> >
>> > Ideally we wouldn't need to take special precautions for these
>> > operations at all but we can't, especially for GUI environments that
>> > constantly scan file systems on mount/umount activity.
>> >
>> > Historically for autofs, neither stat(2) or lstat(2) would trigger a
>> > mount. With the current implementation stat(2) now does but lstat(2)
>> > doesn't which is a step in the right direction IMHO. So, I recommend we
>> > continue to encourage user space to make the needed changes so we
>> > continue to move in the right direction, and yes, I acknowledge it is a
>> > pain but it'll never get done otherwise.
>> 
>> I'm not quite convinced.  What's the advantage of triggering automount
>> on stat(2)?
>
> You get the information of the directory of the mounted fs and for many
> peoples purposes automounting should be transparent so that would be
> best.

The same is true for lstat(2).

>  
>> 
>> Has anybody complained that stat(2) on the mountpoint doesn't cause an
>> automount?
>
> Yes, based on the reasoning above.

Would any of those complaints go away if stat(2) did cause an
automount and lstat(2) didn't?

With AT_NO_AUTOMOUNT I can agree with, that's a flag specifically about
not automounting.  Unlike AT_SYMLINK_NOFOLLOW which is about following
symlinks and hasn't got a lot to do with automounts.

Thanks,
Miklos

Re: [PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[Ian Kent]

On Tue, 2011-09-06 at 17:39 +0200, Miklos Szeredi wrote:
> Ian Kent <raven@themaw.net> writes:
> 
> > On Tue, 2011-09-06 at 10:09 +0200, Miklos Szeredi wrote:
> >> Ian Kent <raven@themaw.net> writes:
> >> 
> >> > On Mon, 2011-09-05 at 19:02 +0200, Miklos Szeredi wrote:
> >> >> 
> >> >> If automounting on lstat(2) is the correct behavior (is it?  why?)  then at
> >> >> least it should be enabled by a global switch or mount option, IMO.
> >> >
> >> > Ideally we wouldn't need to take special precautions for these
> >> > operations at all but we can't, especially for GUI environments that
> >> > constantly scan file systems on mount/umount activity.
> >> >
> >> > Historically for autofs, neither stat(2) or lstat(2) would trigger a
> >> > mount. With the current implementation stat(2) now does but lstat(2)
> >> > doesn't which is a step in the right direction IMHO. So, I recommend we
> >> > continue to encourage user space to make the needed changes so we
> >> > continue to move in the right direction, and yes, I acknowledge it is a
> >> > pain but it'll never get done otherwise.
> >> 
> >> I'm not quite convinced.  What's the advantage of triggering automount
> >> on stat(2)?
> >
> > You get the information of the directory of the mounted fs and for many
> > peoples purposes automounting should be transparent so that would be
> > best.
> 
> The same is true for lstat(2).
> 
> >  
> >> 
> >> Has anybody complained that stat(2) on the mountpoint doesn't cause an
> >> automount?
> >
> > Yes, based on the reasoning above.
> 
> Would any of those complaints go away if stat(2) did cause an
> automount and lstat(2) didn't?

Ummm .... please forgive me, I'm confused over which of these cause an
automount to occur, again.

Much of what I've said previously is wrong because stat(2) does cause
the automount and lstat(2) doesn't, which is, I think, the way it should
be, semantically (see bug
https://bugzilla.redhat.com/show_bug.cgi?id=692823 for example).

I'll go back and check the code "again", just to be doubly sure, which I
need to do given my constant confusion on this, oops!

The comments about working through problems that arise in order to
retain this behavior still apply though.

Ian

Re: [PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[Miklos Szeredi]

Ian Kent <raven@themaw.net> writes:

> On Tue, 2011-09-06 at 17:39 +0200, Miklos Szeredi wrote:
>> Ian Kent <raven@themaw.net> writes:
>> 
>> > On Tue, 2011-09-06 at 10:09 +0200, Miklos Szeredi wrote:
>> >> Ian Kent <raven@themaw.net> writes:
>> >> 
>> >> > On Mon, 2011-09-05 at 19:02 +0200, Miklos Szeredi wrote:
>> >> >> 
>> >> >> If automounting on lstat(2) is the correct behavior (is it?  why?)  then at
>> >> >> least it should be enabled by a global switch or mount option, IMO.
>> >> >
>> >> > Ideally we wouldn't need to take special precautions for these
>> >> > operations at all but we can't, especially for GUI environments that
>> >> > constantly scan file systems on mount/umount activity.
>> >> >
>> >> > Historically for autofs, neither stat(2) or lstat(2) would trigger a
>> >> > mount. With the current implementation stat(2) now does but lstat(2)
>> >> > doesn't which is a step in the right direction IMHO. So, I recommend we
>> >> > continue to encourage user space to make the needed changes so we
>> >> > continue to move in the right direction, and yes, I acknowledge it is a
>> >> > pain but it'll never get done otherwise.
>> >> 
>> >> I'm not quite convinced.  What's the advantage of triggering automount
>> >> on stat(2)?
>> >
>> > You get the information of the directory of the mounted fs and for many
>> > peoples purposes automounting should be transparent so that would be
>> > best.
>> 
>> The same is true for lstat(2).
>> 
>> >  
>> >> 
>> >> Has anybody complained that stat(2) on the mountpoint doesn't cause an
>> >> automount?
>> >
>> > Yes, based on the reasoning above.
>> 
>> Would any of those complaints go away if stat(2) did cause an
>> automount and lstat(2) didn't?
>
> Ummm .... please forgive me, I'm confused over which of these cause an
> automount to occur, again.
>
> Much of what I've said previously is wrong because stat(2) does cause
> the automount and lstat(2) doesn't, which is, I think, the way it should
> be, semantically (see bug
> https://bugzilla.redhat.com/show_bug.cgi?id=692823 for example).
>
> I'll go back and check the code "again", just to be doubly sure, which I
> need to do given my constant confusion on this, oops!

Sloppy wording on my part is probably responsible for some of the
confusion.

Yes, 2.6.38 and later kernels do trigger on stat(2) but not on lstat(2).

My question is this:  does this behavior improve anything compared to
kernels before 2.6.38?  Because I don't see that it does, in fact it's
just causing regressions.

You say it's a step in the right direction but I don't see why.  Either
we want stat *and* lstat to both be correct and trigger the automount or
we are satisfied with the incorrect but well established practice of not
triggering on (l)stat.

The middle ground makes no sense IMO, there's nothing gained by the
differentiated behavior based on LOOKUP_FOLLOW.

Can you explain why it's better if stat() tiggers automounts and gives a
correct result but lstat() doesn't?

Thanks,
Miklos

Re: [PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[Linus Torvalds]

On Thu, Sep 8, 2011 at 6:38 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> Yes, 2.6.38 and later kernels do trigger on stat(2) but not on lstat(2).
>
> My question is this:  does this behavior improve anything compared to
> kernels before 2.6.38?  Because I don't see that it does, in fact it's
> just causing regressions.
>
> You say it's a step in the right direction but I don't see why.  Either
> we want stat *and* lstat to both be correct and trigger the automount or
> we are satisfied with the incorrect but well established practice of not
> triggering on (l)stat.
>
> The middle ground makes no sense IMO, there's nothing gained by the
> differentiated behavior based on LOOKUP_FOLLOW.
>
> Can you explain why it's better if stat() tiggers automounts and gives a
> correct result but lstat() doesn't?

I have to say that this is a very cogent question.

The one thing I've not seen in the thread yet is a description of the
failure. What does the regression look like? Just "very slow 'ls' with
some versions of 'ls'" or what?

I'm inclined to apply the patch as a regression fix, but I'll let this
thread try to convince me for another day..

                         Linus

Re: [PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[Al Viro]

On Thu, Sep 08, 2011 at 10:42:28AM -0700, Linus Torvalds wrote:

> > You say it's a step in the right direction but I don't see why. ?Either
> > we want stat *and* lstat to both be correct and trigger the automount or
> > we are satisfied with the incorrect but well established practice of not
> > triggering on (l)stat.
> >
> > The middle ground makes no sense IMO, there's nothing gained by the
> > differentiated behavior based on LOOKUP_FOLLOW.
> >
> > Can you explain why it's better if stat() tiggers automounts and gives a
> > correct result but lstat() doesn't?
> 
> I have to say that this is a very cogent question.
> 
> The one thing I've not seen in the thread yet is a description of the
> failure. What does the regression look like? Just "very slow 'ls' with
> some versions of 'ls'" or what?
> 
> I'm inclined to apply the patch as a regression fix, but I'll let this
> thread try to convince me for another day..

IIRC, that matches traditional SunOS behaviour and it actually does make
sense; you want wildcard expansion and ls -l to be doable even when there's
a stuck NFS server.  IOW, non-triggering lstat(2) is a matter of usability...

Re: [PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[Linus Torvalds]

On Thu, Sep 8, 2011 at 12:50 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Sep 08, 2011 at 10:42:28AM -0700, Linus Torvalds wrote:
>
>> > You say it's a step in the right direction but I don't see why. ?Either
>> > we want stat *and* lstat to both be correct and trigger the automount or
>> > we are satisfied with the incorrect but well established practice of not
>> > triggering on (l)stat.
>> >
>> > The middle ground makes no sense IMO, there's nothing gained by the
>> > differentiated behavior based on LOOKUP_FOLLOW.
>> >
>> > Can you explain why it's better if stat() tiggers automounts and gives a
>> > correct result but lstat() doesn't?
>>
>> I have to say that this is a very cogent question.
>>
>> The one thing I've not seen in the thread yet is a description of the
>> failure. What does the regression look like? Just "very slow 'ls' with
>> some versions of 'ls'" or what?
>>
>> I'm inclined to apply the patch as a regression fix, but I'll let this
>> thread try to convince me for another day..
>
> IIRC, that matches traditional SunOS behaviour and it actually does make
> sense; you want wildcard expansion and ls -l to be doable even when there's
> a stuck NFS server.  IOW, non-triggering lstat(2) is a matter of usability...

non-triggering lstat() isn't the issue, afaik. We never trigger on lstat.

nontriggering *stat()* is the issue. We didn't *use* to trigger on
stat() either. Now in 2.6.38+ we do.

                   Linus

Re: [PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[Al Viro]

On Thu, Sep 08, 2011 at 01:19:49PM -0700, Linus Torvalds wrote:

> >> I'm inclined to apply the patch as a regression fix, but I'll let this
> >> thread try to convince me for another day..
> >
> > IIRC, that matches traditional SunOS behaviour and it actually does make
> > sense; you want wildcard expansion and ls -l to be doable even when there's
> > a stuck NFS server. ?IOW, non-triggering lstat(2) is a matter of usability...
> 
> non-triggering lstat() isn't the issue, afaik. We never trigger on lstat.
> 
> nontriggering *stat()* is the issue. We didn't *use* to trigger on
> stat() either. Now in 2.6.38+ we do.

Yes.  Again, IIRC that's what SunOS implementation had been doing all along;
I think the reason was that stat()+open()+fstat() getting different results
for stat and fstat would spook quite a few programs, but I could be easily
wrong on that.

I've no strong preferences for or against that change; to be honest, the set
of situations when automount triggers always left an impression of voodoo
balancing between usability (== not triggering it too much, lest we end up
with stuck boxen and unhappy admins) and not breaking expectations of userland
(e.g. stat/open/fstat issue)...

Re: [PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[Ian Kent]

On Thu, 2011-09-08 at 15:38 +0200, Miklos Szeredi wrote:
> Ian Kent <raven@themaw.net> writes:
> 
> > On Tue, 2011-09-06 at 17:39 +0200, Miklos Szeredi wrote:
> >> Ian Kent <raven@themaw.net> writes:
> >> 
> >> > On Tue, 2011-09-06 at 10:09 +0200, Miklos Szeredi wrote:
> >> >> Ian Kent <raven@themaw.net> writes:
> >> >> 
> >> >> > On Mon, 2011-09-05 at 19:02 +0200, Miklos Szeredi wrote:
> >> >> >> 
> >> >> >> If automounting on lstat(2) is the correct behavior (is it?  why?)  then at
> >> >> >> least it should be enabled by a global switch or mount option, IMO.
> >> >> >
> >> >> > Ideally we wouldn't need to take special precautions for these
> >> >> > operations at all but we can't, especially for GUI environments that
> >> >> > constantly scan file systems on mount/umount activity.
> >> >> >
> >> >> > Historically for autofs, neither stat(2) or lstat(2) would trigger a
> >> >> > mount. With the current implementation stat(2) now does but lstat(2)
> >> >> > doesn't which is a step in the right direction IMHO. So, I recommend we
> >> >> > continue to encourage user space to make the needed changes so we
> >> >> > continue to move in the right direction, and yes, I acknowledge it is a
> >> >> > pain but it'll never get done otherwise.
> >> >> 
> >> >> I'm not quite convinced.  What's the advantage of triggering automount
> >> >> on stat(2)?
> >> >
> >> > You get the information of the directory of the mounted fs and for many
> >> > peoples purposes automounting should be transparent so that would be
> >> > best.
> >> 
> >> The same is true for lstat(2).
> >> 
> >> >  
> >> >> 
> >> >> Has anybody complained that stat(2) on the mountpoint doesn't cause an
> >> >> automount?
> >> >
> >> > Yes, based on the reasoning above.
> >> 
> >> Would any of those complaints go away if stat(2) did cause an
> >> automount and lstat(2) didn't?
> >
> > Ummm .... please forgive me, I'm confused over which of these cause an
> > automount to occur, again.
> >
> > Much of what I've said previously is wrong because stat(2) does cause
> > the automount and lstat(2) doesn't, which is, I think, the way it should
> > be, semantically (see bug
> > https://bugzilla.redhat.com/show_bug.cgi?id=692823 for example).
> >
> > I'll go back and check the code "again", just to be doubly sure, which I
> > need to do given my constant confusion on this, oops!
> 
> Sloppy wording on my part is probably responsible for some of the
> confusion.
> 
> Yes, 2.6.38 and later kernels do trigger on stat(2) but not on lstat(2).
> 
> My question is this:  does this behavior improve anything compared to
> kernels before 2.6.38?  Because I don't see that it does, in fact it's
> just causing regressions.

OK, I think I'm back on track wrt. the discussion now.

I know I've come out against the change so far but the reality is that
this has always been an issue and, from what your saying there are some
user space applications that are unhappy with it to a significant degree
(I'm actually not yet aware of these reports).

So I support your recommended change. Certainly for the autofs case it
will do nothing more than restore the original behavior, but since
autofs isn't the only user of this functionality now I have recommend we
ask the other subsystem maintainers what impact it would have for them.
I think that's AFS, CIFS and NFS (I'm not adding to the cc since I'm not
sure who to add for CIFS, can someone else reply with the proper ccs
please).

> 
> You say it's a step in the right direction but I don't see why.  Either
> we want stat *and* lstat to both be correct and trigger the automount or
> we are satisfied with the incorrect but well established practice of not
> triggering on (l)stat.

I say it's a step in the right direction only because of what I think
should be "correct" behavior and my interpretation simply doesn't quite
match yours, sorry, ;)

My interpretation is still that (forgive the repetition), in an ideal
world, follow type walks should trigger automounts to occur and no
follow walks should not purely due to the behavioral similarity between
automounts and sym-links.

> 
> The middle ground makes no sense IMO, there's nothing gained by the
> differentiated behavior based on LOOKUP_FOLLOW.

Right, but I don't see it as middle ground, I see it as "correct"
behavior.

I also find the "both follow and no follow walks should trigger mounts"
sensible. Unfortunately that can't work at all without sufficient
context to be able to know when to and when not to trigger a mount and
that just isn't possible AFAICS.

> 
> Can you explain why it's better if stat() tiggers automounts and gives a
> correct result but lstat() doesn't?

Again, my interpretation of what I think is "correct" which is not
necessarily the right one.

Ian

Re: [PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[Ian Kent]

On Thu, 2011-09-08 at 10:42 -0700, Linus Torvalds wrote:
> On Thu, Sep 8, 2011 at 6:38 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > Yes, 2.6.38 and later kernels do trigger on stat(2) but not on lstat(2).
> >
> > My question is this:  does this behavior improve anything compared to
> > kernels before 2.6.38?  Because I don't see that it does, in fact it's
> > just causing regressions.
> >
> > You say it's a step in the right direction but I don't see why.  Either
> > we want stat *and* lstat to both be correct and trigger the automount or
> > we are satisfied with the incorrect but well established practice of not
> > triggering on (l)stat.
> >
> > The middle ground makes no sense IMO, there's nothing gained by the
> > differentiated behavior based on LOOKUP_FOLLOW.
> >
> > Can you explain why it's better if stat() tiggers automounts and gives a
> > correct result but lstat() doesn't?
> 
> I have to say that this is a very cogent question.
> 
> The one thing I've not seen in the thread yet is a description of the
> failure. What does the regression look like? Just "very slow 'ls' with
> some versions of 'ls'" or what?

The problem that is seen is everything in a directory being mounted upon
listing the directory. This isn't really a problem for small automount
maps but maps with more than a few dozen entries start becoming
problematic and a few hundred entries or more (quite common in many
environments) is an obvious nightmare.

Clearly this is a problem that I have introduced by the idea of the
"browse" option of autofs, whereby the potential automount points of an
indirect automount are visible within the automount managed directory
but it also applies to NFS crossing mount points (since those mount
point directories must also pre-exist).

Note that this isn't as problem if the "browse" option isn't used since
we just se an empty directory and specific access to a path causes only
those mounts to occur. But Solaris has been able to do this for years
now so it is expected by the user community.

> 
> I'm inclined to apply the patch as a regression fix, but I'll let this
> thread try to convince me for another day..

I have to agree, although it would be good to get some input from other
subsystem maintainers.

Ian

Re: [PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[Ian Kent]

On Thu, 2011-09-08 at 22:54 +0100, Al Viro wrote:
> On Thu, Sep 08, 2011 at 01:19:49PM -0700, Linus Torvalds wrote:
> 
> > >> I'm inclined to apply the patch as a regression fix, but I'll let this
> > >> thread try to convince me for another day..
> > >
> > > IIRC, that matches traditional SunOS behaviour and it actually does make
> > > sense; you want wildcard expansion and ls -l to be doable even when there's
> > > a stuck NFS server. ?IOW, non-triggering lstat(2) is a matter of usability...
> > 
> > non-triggering lstat() isn't the issue, afaik. We never trigger on lstat.
> > 
> > nontriggering *stat()* is the issue. We didn't *use* to trigger on
> > stat() either. Now in 2.6.38+ we do.
> 
> Yes.  Again, IIRC that's what SunOS implementation had been doing all along;
> I think the reason was that stat()+open()+fstat() getting different results
> for stat and fstat would spook quite a few programs, but I could be easily
> wrong on that.

Yes, that the sort of problem that we see.

For example, find(1) has had problems with this sort of inconsistency
with before and after comparisons it makes as it walks the directory
tree.

Ian

Re: [PATCH] vfs: automount should ignore LOOKUP_FOLLOW

[Jeff Layton]

On Mon, 05 Sep 2011 18:06:26 +0200
Miklos Szeredi <miklos@szeredi.hu> wrote:

> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> Prior to 2.6.38 automount would not trigger on either stat(2) or
> lstat(2) on the automount point.
> 
> After 2.6.38, with the introduction of the ->d_automount()
> infrastructure, stat(2) and others would start triggering automount
> while lstat(2), etc. still would not.  This is a regression and a
> userspace ABI change.
> 
> Problem originally reported here:
> 
>   http://thread.gmane.org/gmane.linux.kernel.autofs/6098
> 
> It appears that there was an attempt at fixing various userspace tools
> to not trigger the automount.  But since the stat system call is
> rather common it is impossible to "fix" all userspace.
> 
> This patch reverts the original behavior, which is to not trigger on
> stat(2) and other symlink following syscalls.
> 
> Reported-by: Leonardo Chiquitto <leonardo.lists@gmail.com>
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> CC: David Howells <dhowells@redhat.com>
> CC: Ian Kent <raven@themaw.net>
> CC: stable@kernel.org
> ---
>  fs/namei.c |   33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
> 

This patch is causing a regression with NFSv4.

When I mount up a filesystem, and do any activity on it, a new
automount gets triggered on top of the original mountpoint.

The first mount ends up getting the fsid of the pseudo root (fsid=0),
rather than the correct one for the fs. Once it traverses into the
actual filesystem the fsid's look different and d_automount gets
triggered.

I think the problem is that nfs_follow_remote_path() uses
LOOKUP_FOLLOW, and with the above patch this is now ignored.

Let me know if you need details on how to reproduce this.

-- 
Jeff Layton <jlayton@redhat.com>