[PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[David Howells]

Directly suppress automount on the following:

	[l]stat
	[l]getxattr
	[l]setxattr
	[l]listxattr
	[l]removexattr
	name_to_handle_at
	fstatat
	statfs
	utimes

But provide an AT_AUTOMOUNT_FOLLOW path where possible to override that
behaviour.

This changes the behaviour back to before the in-VFS automount patches were
applied, thus, for example, preventing stat() from causing automounting, but
allows it to be overridden where possible.

However, what about the following:

	umount
	faccessat
	fchmodat, chmod
	fchownat, lchown, chown

Should these or should these not cause automounting?

It's possible that with these additions, the whole decision point in
follow_automount() can be reduced to just a check of LOOKUP_NO_AUTOMOUNT.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/fhandle.c          |    2 +-
 fs/stat.c             |    8 ++++----
 fs/statfs.c           |    3 ++-
 fs/utimes.c           |   10 +++++++---
 fs/xattr.c            |   20 ++++++++++++--------
 include/linux/fcntl.h |    1 +
 6 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/fs/fhandle.c b/fs/fhandle.c
index 6b08864..bc0f327 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -92,7 +92,7 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
 		int, flag)
 {
 	struct path path;
-	int lookup_flags;
+	int lookup_flags = LOOKUP_NO_AUTOMOUNT;
 	int err;
 
 	if ((flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0)
diff --git a/fs/stat.c b/fs/stat.c
index ba5316f..e39b489 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -73,16 +73,16 @@ int vfs_fstatat(int dfd, const char __user *filename, struct kstat *stat,
 {
 	struct path path;
 	int error = -EINVAL;
-	int lookup_flags = 0;
+	int lookup_flags = LOOKUP_NO_AUTOMOUNT;
 
 	if ((flag & ~(AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT |
-		      AT_EMPTY_PATH)) != 0)
+		      AT_EMPTY_PATH | AT_AUTOMOUNT_FOLLOW)) != 0)
 		goto out;
 
 	if (!(flag & AT_SYMLINK_NOFOLLOW))
 		lookup_flags |= LOOKUP_FOLLOW;
-	if (flag & AT_NO_AUTOMOUNT)
-		lookup_flags |= LOOKUP_NO_AUTOMOUNT;
+	if (!(flag & AT_AUTOMOUNT_FOLLOW))
+		lookup_flags &= ~LOOKUP_NO_AUTOMOUNT;
 	if (flag & AT_EMPTY_PATH)
 		lookup_flags |= LOOKUP_EMPTY;
 
diff --git a/fs/statfs.c b/fs/statfs.c
index 8244924..fe64701 100644
--- a/fs/statfs.c
+++ b/fs/statfs.c
@@ -76,7 +76,8 @@ EXPORT_SYMBOL(vfs_statfs);
 int user_statfs(const char __user *pathname, struct kstatfs *st)
 {
 	struct path path;
-	int error = user_path(pathname, &path);
+	int error = user_path_at(AT_FDCWD, pathname, LOOKUP_NO_AUTOMOUNT,
+				 &path);
 	if (!error) {
 		error = vfs_statfs(&path, st);
 		path_put(&path);
diff --git a/fs/utimes.c b/fs/utimes.c
index ba653f3..e9bf196 100644
--- a/fs/utimes.c
+++ b/fs/utimes.c
@@ -136,13 +136,15 @@ long do_utimes(int dfd, const char __user *filename, struct timespec *times,
 		goto out;
 	}
 
-	if (flags & ~AT_SYMLINK_NOFOLLOW)
+	if (flags & ~(AT_SYMLINK_NOFOLLOW |
+		      AT_NO_AUTOMOUNT | AT_AUTOMOUNT_FOLLOW))
 		goto out;
 
 	if (filename == NULL && dfd != AT_FDCWD) {
 		struct file *file;
 
-		if (flags & AT_SYMLINK_NOFOLLOW)
+		if (flags & (AT_SYMLINK_NOFOLLOW |
+			     AT_NO_AUTOMOUNT | AT_AUTOMOUNT_FOLLOW))
 			goto out;
 
 		file = fget(dfd);
@@ -154,10 +156,12 @@ long do_utimes(int dfd, const char __user *filename, struct timespec *times,
 		fput(file);
 	} else {
 		struct path path;
-		int lookup_flags = 0;
+		int lookup_flags = LOOKUP_NO_AUTOMOUNT;
 
 		if (!(flags & AT_SYMLINK_NOFOLLOW))
 			lookup_flags |= LOOKUP_FOLLOW;
+		if (flags & AT_AUTOMOUNT_FOLLOW)
+			lookup_flags &= ~LOOKUP_NO_AUTOMOUNT;
 
 		error = user_path_at(dfd, filename, lookup_flags, &path);
 		if (error)
diff --git a/fs/xattr.c b/fs/xattr.c
index f060663..7c462ae 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -290,7 +290,8 @@ SYSCALL_DEFINE5(setxattr, const char __user *, pathname,
 	struct path path;
 	int error;
 
-	error = user_path(pathname, &path);
+	error = user_path_at(AT_FDCWD, pathname,
+			     LOOKUP_FOLLOW | LOOKUP_NO_AUTOMOUNT, &path);
 	if (error)
 		return error;
 	error = mnt_want_write(path.mnt);
@@ -309,7 +310,7 @@ SYSCALL_DEFINE5(lsetxattr, const char __user *, pathname,
 	struct path path;
 	int error;
 
-	error = user_lpath(pathname, &path);
+	error = user_path_at(AT_FDCWD, pathname, LOOKUP_NO_AUTOMOUNT, &path);
 	if (error)
 		return error;
 	error = mnt_want_write(path.mnt);
@@ -386,7 +387,8 @@ SYSCALL_DEFINE4(getxattr, const char __user *, pathname,
 	struct path path;
 	ssize_t error;
 
-	error = user_path(pathname, &path);
+	error = user_path_at(AT_FDCWD, pathname,
+			     LOOKUP_FOLLOW | LOOKUP_NO_AUTOMOUNT, &path);
 	if (error)
 		return error;
 	error = getxattr(path.dentry, name, value, size);
@@ -400,7 +402,7 @@ SYSCALL_DEFINE4(lgetxattr, const char __user *, pathname,
 	struct path path;
 	ssize_t error;
 
-	error = user_lpath(pathname, &path);
+	error = user_path_at(AT_FDCWD, pathname, LOOKUP_NO_AUTOMOUNT, &path);
 	if (error)
 		return error;
 	error = getxattr(path.dentry, name, value, size);
@@ -459,7 +461,8 @@ SYSCALL_DEFINE3(listxattr, const char __user *, pathname, char __user *, list,
 	struct path path;
 	ssize_t error;
 
-	error = user_path(pathname, &path);
+	error = user_path_at(AT_FDCWD, pathname,
+			     LOOKUP_FOLLOW | LOOKUP_NO_AUTOMOUNT, &path);
 	if (error)
 		return error;
 	error = listxattr(path.dentry, list, size);
@@ -473,7 +476,7 @@ SYSCALL_DEFINE3(llistxattr, const char __user *, pathname, char __user *, list,
 	struct path path;
 	ssize_t error;
 
-	error = user_lpath(pathname, &path);
+	error = user_path_at(AT_FDCWD, pathname, LOOKUP_NO_AUTOMOUNT, &path);
 	if (error)
 		return error;
 	error = listxattr(path.dentry, list, size);
@@ -519,7 +522,8 @@ SYSCALL_DEFINE2(removexattr, const char __user *, pathname,
 	struct path path;
 	int error;
 
-	error = user_path(pathname, &path);
+	error = user_path_at(AT_FDCWD, pathname,
+			     LOOKUP_FOLLOW | LOOKUP_NO_AUTOMOUNT, &path);
 	if (error)
 		return error;
 	error = mnt_want_write(path.mnt);
@@ -537,7 +541,7 @@ SYSCALL_DEFINE2(lremovexattr, const char __user *, pathname,
 	struct path path;
 	int error;
 
-	error = user_lpath(pathname, &path);
+	error = user_path_at(AT_FDCWD, pathname, LOOKUP_NO_AUTOMOUNT, &path);
 	if (error)
 		return error;
 	error = mnt_want_write(path.mnt);
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index f550f89..768fb66 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -47,6 +47,7 @@
 #define AT_SYMLINK_FOLLOW	0x400   /* Follow symbolic links.  */
 #define AT_NO_AUTOMOUNT		0x800	/* Suppress terminal automount traversal */
 #define AT_EMPTY_PATH		0x1000	/* Allow empty relative pathname */
+#define AT_AUTOMOUNT_FOLLOW	0x2000	/* Follow terminal automounts */
 
 #ifdef __KERNEL__
 

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Miklos Szeredi]

David Howells <dhowells@redhat.com> writes:

> Directly suppress automount on the following:
>
> 	[l]stat
> 	[l]getxattr
> 	[l]setxattr
> 	[l]listxattr
> 	[l]removexattr
> 	name_to_handle_at
> 	fstatat
> 	statfs
> 	utimes
>
> But provide an AT_AUTOMOUNT_FOLLOW path where possible to override that
> behaviour.
>
> This changes the behaviour back to before the in-VFS automount patches were
> applied, thus, for example, preventing stat() from causing automounting, but
> allows it to be overridden where possible.
>
> However, what about the following:
>
> 	umount
> 	faccessat
> 	fchmodat, chmod
> 	fchownat, lchown, chown
>
> Should these or should these not cause automounting?

It probably doesn't matter.  Old kernels used to not automount on these
and it didn't cause problems, but I'm quite sure that if they do
automount that will not cause problems either.

>
> It's possible that with these additions, the whole decision point in
> follow_automount() can be reduced to just a check of LOOKUP_NO_AUTOMOUNT.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
>  fs/fhandle.c          |    2 +-
>  fs/stat.c             |    8 ++++----
>  fs/statfs.c           |    3 ++-
>  fs/utimes.c           |   10 +++++++---
>  fs/xattr.c            |   20 ++++++++++++--------
>  include/linux/fcntl.h |    1 +
>  6 files changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 6b08864..bc0f327 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -92,7 +92,7 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
>  		int, flag)
>  {
>  	struct path path;
> -	int lookup_flags;
> +	int lookup_flags = LOOKUP_NO_AUTOMOUNT;

lookup_flags is going to be reinitialized later, so this has no affect.

Otherwise looks good.

Thanks,
Miklos

>  	int err;
>  
>  	if ((flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0)
> diff --git a/fs/stat.c b/fs/stat.c
> index ba5316f..e39b489 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -73,16 +73,16 @@ int vfs_fstatat(int dfd, const char __user *filename, struct kstat *stat,
>  {
>  	struct path path;
>  	int error = -EINVAL;
> -	int lookup_flags = 0;
> +	int lookup_flags = LOOKUP_NO_AUTOMOUNT;
>  
>  	if ((flag & ~(AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT |
> -		      AT_EMPTY_PATH)) != 0)
> +		      AT_EMPTY_PATH | AT_AUTOMOUNT_FOLLOW)) != 0)
>  		goto out;
>  
>  	if (!(flag & AT_SYMLINK_NOFOLLOW))
>  		lookup_flags |= LOOKUP_FOLLOW;
> -	if (flag & AT_NO_AUTOMOUNT)
> -		lookup_flags |= LOOKUP_NO_AUTOMOUNT;
> +	if (!(flag & AT_AUTOMOUNT_FOLLOW))
> +		lookup_flags &= ~LOOKUP_NO_AUTOMOUNT;
>  	if (flag & AT_EMPTY_PATH)
>  		lookup_flags |= LOOKUP_EMPTY;
>  
> diff --git a/fs/statfs.c b/fs/statfs.c
> index 8244924..fe64701 100644
> --- a/fs/statfs.c
> +++ b/fs/statfs.c
> @@ -76,7 +76,8 @@ EXPORT_SYMBOL(vfs_statfs);
>  int user_statfs(const char __user *pathname, struct kstatfs *st)
>  {
>  	struct path path;
> -	int error = user_path(pathname, &path);
> +	int error = user_path_at(AT_FDCWD, pathname, LOOKUP_NO_AUTOMOUNT,
> +				 &path);
>  	if (!error) {
>  		error = vfs_statfs(&path, st);
>  		path_put(&path);
> diff --git a/fs/utimes.c b/fs/utimes.c
> index ba653f3..e9bf196 100644
> --- a/fs/utimes.c
> +++ b/fs/utimes.c
> @@ -136,13 +136,15 @@ long do_utimes(int dfd, const char __user *filename, struct timespec *times,
>  		goto out;
>  	}
>  
> -	if (flags & ~AT_SYMLINK_NOFOLLOW)
> +	if (flags & ~(AT_SYMLINK_NOFOLLOW |
> +		      AT_NO_AUTOMOUNT | AT_AUTOMOUNT_FOLLOW))
>  		goto out;
>  
>  	if (filename == NULL && dfd != AT_FDCWD) {
>  		struct file *file;
>  
> -		if (flags & AT_SYMLINK_NOFOLLOW)
> +		if (flags & (AT_SYMLINK_NOFOLLOW |
> +			     AT_NO_AUTOMOUNT | AT_AUTOMOUNT_FOLLOW))
>  			goto out;
>  
>  		file = fget(dfd);
> @@ -154,10 +156,12 @@ long do_utimes(int dfd, const char __user *filename, struct timespec *times,
>  		fput(file);
>  	} else {
>  		struct path path;
> -		int lookup_flags = 0;
> +		int lookup_flags = LOOKUP_NO_AUTOMOUNT;
>  
>  		if (!(flags & AT_SYMLINK_NOFOLLOW))
>  			lookup_flags |= LOOKUP_FOLLOW;
> +		if (flags & AT_AUTOMOUNT_FOLLOW)
> +			lookup_flags &= ~LOOKUP_NO_AUTOMOUNT;
>  
>  		error = user_path_at(dfd, filename, lookup_flags, &path);
>  		if (error)
> diff --git a/fs/xattr.c b/fs/xattr.c
> index f060663..7c462ae 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -290,7 +290,8 @@ SYSCALL_DEFINE5(setxattr, const char __user *, pathname,
>  	struct path path;
>  	int error;
>  
> -	error = user_path(pathname, &path);
> +	error = user_path_at(AT_FDCWD, pathname,
> +			     LOOKUP_FOLLOW | LOOKUP_NO_AUTOMOUNT, &path);
>  	if (error)
>  		return error;
>  	error = mnt_want_write(path.mnt);
> @@ -309,7 +310,7 @@ SYSCALL_DEFINE5(lsetxattr, const char __user *, pathname,
>  	struct path path;
>  	int error;
>  
> -	error = user_lpath(pathname, &path);
> +	error = user_path_at(AT_FDCWD, pathname, LOOKUP_NO_AUTOMOUNT, &path);
>  	if (error)
>  		return error;
>  	error = mnt_want_write(path.mnt);
> @@ -386,7 +387,8 @@ SYSCALL_DEFINE4(getxattr, const char __user *, pathname,
>  	struct path path;
>  	ssize_t error;
>  
> -	error = user_path(pathname, &path);
> +	error = user_path_at(AT_FDCWD, pathname,
> +			     LOOKUP_FOLLOW | LOOKUP_NO_AUTOMOUNT, &path);
>  	if (error)
>  		return error;
>  	error = getxattr(path.dentry, name, value, size);
> @@ -400,7 +402,7 @@ SYSCALL_DEFINE4(lgetxattr, const char __user *, pathname,
>  	struct path path;
>  	ssize_t error;
>  
> -	error = user_lpath(pathname, &path);
> +	error = user_path_at(AT_FDCWD, pathname, LOOKUP_NO_AUTOMOUNT, &path);
>  	if (error)
>  		return error;
>  	error = getxattr(path.dentry, name, value, size);
> @@ -459,7 +461,8 @@ SYSCALL_DEFINE3(listxattr, const char __user *, pathname, char __user *, list,
>  	struct path path;
>  	ssize_t error;
>  
> -	error = user_path(pathname, &path);
> +	error = user_path_at(AT_FDCWD, pathname,
> +			     LOOKUP_FOLLOW | LOOKUP_NO_AUTOMOUNT, &path);
>  	if (error)
>  		return error;
>  	error = listxattr(path.dentry, list, size);
> @@ -473,7 +476,7 @@ SYSCALL_DEFINE3(llistxattr, const char __user *, pathname, char __user *, list,
>  	struct path path;
>  	ssize_t error;
>  
> -	error = user_lpath(pathname, &path);
> +	error = user_path_at(AT_FDCWD, pathname, LOOKUP_NO_AUTOMOUNT, &path);
>  	if (error)
>  		return error;
>  	error = listxattr(path.dentry, list, size);
> @@ -519,7 +522,8 @@ SYSCALL_DEFINE2(removexattr, const char __user *, pathname,
>  	struct path path;
>  	int error;
>  
> -	error = user_path(pathname, &path);
> +	error = user_path_at(AT_FDCWD, pathname,
> +			     LOOKUP_FOLLOW | LOOKUP_NO_AUTOMOUNT, &path);
>  	if (error)
>  		return error;
>  	error = mnt_want_write(path.mnt);
> @@ -537,7 +541,7 @@ SYSCALL_DEFINE2(lremovexattr, const char __user *, pathname,
>  	struct path path;
>  	int error;
>  
> -	error = user_lpath(pathname, &path);
> +	error = user_path_at(AT_FDCWD, pathname, LOOKUP_NO_AUTOMOUNT, &path);
>  	if (error)
>  		return error;
>  	error = mnt_want_write(path.mnt);
> diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
> index f550f89..768fb66 100644
> --- a/include/linux/fcntl.h
> +++ b/include/linux/fcntl.h
> @@ -47,6 +47,7 @@
>  #define AT_SYMLINK_FOLLOW	0x400   /* Follow symbolic links.  */
>  #define AT_NO_AUTOMOUNT		0x800	/* Suppress terminal automount traversal */
>  #define AT_EMPTY_PATH		0x1000	/* Allow empty relative pathname */
> +#define AT_AUTOMOUNT_FOLLOW	0x2000	/* Follow terminal automounts */
>  
>  #ifdef __KERNEL__
>  

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Ian Kent]

On Thu, 2011-09-22 at 14:45 +0100, David Howells wrote:
> Directly suppress automount on the following:
> 
> 	[l]stat
> 	[l]getxattr
> 	[l]setxattr
> 	[l]listxattr
> 	[l]removexattr
> 	name_to_handle_at
> 	fstatat
> 	statfs
> 	utimes
> 
> But provide an AT_AUTOMOUNT_FOLLOW path where possible to override that
> behaviour.
> 
> This changes the behaviour back to before the in-VFS automount patches were
> applied, thus, for example, preventing stat() from causing automounting, but
> allows it to be overridden where possible.
> 
> However, what about the following:
> 
> 	umount
> 	faccessat
> 	fchmodat, chmod
> 	fchownat, lchown, chown
> 
> Should these or should these not cause automounting?
> 
> It's possible that with these additions, the whole decision point in
> follow_automount() can be reduced to just a check of LOOKUP_NO_AUTOMOUNT.

As I have said I don't agree with any of this, including the original
change from Miklos.

But, I also ended up supporting it.

I haven't checked what the side effects would be but using the
LOOKUP_DIRECTORY flag in walks that get caught by the removal of the
LOOKUP_FOLLOW test in follow_automount() and retaining Miklos's original
patch is probably a more natural solution IMO.

After all, automount points are always directories and if we think they
might also need to be automounted then that would cause them to be
mounted, and always has done so.

> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  fs/fhandle.c          |    2 +-
>  fs/stat.c             |    8 ++++----
>  fs/statfs.c           |    3 ++-
>  fs/utimes.c           |   10 +++++++---
>  fs/xattr.c            |   20 ++++++++++++--------
>  include/linux/fcntl.h |    1 +
>  6 files changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 6b08864..bc0f327 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -92,7 +92,7 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
>  		int, flag)
>  {
>  	struct path path;
> -	int lookup_flags;
> +	int lookup_flags = LOOKUP_NO_AUTOMOUNT;
>  	int err;
>  
>  	if ((flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0)
> diff --git a/fs/stat.c b/fs/stat.c
> index ba5316f..e39b489 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -73,16 +73,16 @@ int vfs_fstatat(int dfd, const char __user *filename, struct kstat *stat,
>  {
>  	struct path path;
>  	int error = -EINVAL;
> -	int lookup_flags = 0;
> +	int lookup_flags = LOOKUP_NO_AUTOMOUNT;
>  
>  	if ((flag & ~(AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT |
> -		      AT_EMPTY_PATH)) != 0)
> +		      AT_EMPTY_PATH | AT_AUTOMOUNT_FOLLOW)) != 0)
>  		goto out;
>  
>  	if (!(flag & AT_SYMLINK_NOFOLLOW))
>  		lookup_flags |= LOOKUP_FOLLOW;
> -	if (flag & AT_NO_AUTOMOUNT)
> -		lookup_flags |= LOOKUP_NO_AUTOMOUNT;
> +	if (!(flag & AT_AUTOMOUNT_FOLLOW))
> +		lookup_flags &= ~LOOKUP_NO_AUTOMOUNT;
>  	if (flag & AT_EMPTY_PATH)
>  		lookup_flags |= LOOKUP_EMPTY;
>  
> diff --git a/fs/statfs.c b/fs/statfs.c
> index 8244924..fe64701 100644
> --- a/fs/statfs.c
> +++ b/fs/statfs.c
> @@ -76,7 +76,8 @@ EXPORT_SYMBOL(vfs_statfs);
>  int user_statfs(const char __user *pathname, struct kstatfs *st)
>  {
>  	struct path path;
> -	int error = user_path(pathname, &path);
> +	int error = user_path_at(AT_FDCWD, pathname, LOOKUP_NO_AUTOMOUNT,
> +				 &path);
>  	if (!error) {
>  		error = vfs_statfs(&path, st);
>  		path_put(&path);
> diff --git a/fs/utimes.c b/fs/utimes.c
> index ba653f3..e9bf196 100644
> --- a/fs/utimes.c
> +++ b/fs/utimes.c
> @@ -136,13 +136,15 @@ long do_utimes(int dfd, const char __user *filename, struct timespec *times,
>  		goto out;
>  	}
>  
> -	if (flags & ~AT_SYMLINK_NOFOLLOW)
> +	if (flags & ~(AT_SYMLINK_NOFOLLOW |
> +		      AT_NO_AUTOMOUNT | AT_AUTOMOUNT_FOLLOW))
>  		goto out;
>  
>  	if (filename == NULL && dfd != AT_FDCWD) {
>  		struct file *file;
>  
> -		if (flags & AT_SYMLINK_NOFOLLOW)
> +		if (flags & (AT_SYMLINK_NOFOLLOW |
> +			     AT_NO_AUTOMOUNT | AT_AUTOMOUNT_FOLLOW))
>  			goto out;
>  
>  		file = fget(dfd);
> @@ -154,10 +156,12 @@ long do_utimes(int dfd, const char __user *filename, struct timespec *times,
>  		fput(file);
>  	} else {
>  		struct path path;
> -		int lookup_flags = 0;
> +		int lookup_flags = LOOKUP_NO_AUTOMOUNT;
>  
>  		if (!(flags & AT_SYMLINK_NOFOLLOW))
>  			lookup_flags |= LOOKUP_FOLLOW;
> +		if (flags & AT_AUTOMOUNT_FOLLOW)
> +			lookup_flags &= ~LOOKUP_NO_AUTOMOUNT;
>  
>  		error = user_path_at(dfd, filename, lookup_flags, &path);
>  		if (error)
> diff --git a/fs/xattr.c b/fs/xattr.c
> index f060663..7c462ae 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -290,7 +290,8 @@ SYSCALL_DEFINE5(setxattr, const char __user *, pathname,
>  	struct path path;
>  	int error;
>  
> -	error = user_path(pathname, &path);
> +	error = user_path_at(AT_FDCWD, pathname,
> +			     LOOKUP_FOLLOW | LOOKUP_NO_AUTOMOUNT, &path);
>  	if (error)
>  		return error;
>  	error = mnt_want_write(path.mnt);
> @@ -309,7 +310,7 @@ SYSCALL_DEFINE5(lsetxattr, const char __user *, pathname,
>  	struct path path;
>  	int error;
>  
> -	error = user_lpath(pathname, &path);
> +	error = user_path_at(AT_FDCWD, pathname, LOOKUP_NO_AUTOMOUNT, &path);
>  	if (error)
>  		return error;
>  	error = mnt_want_write(path.mnt);
> @@ -386,7 +387,8 @@ SYSCALL_DEFINE4(getxattr, const char __user *, pathname,
>  	struct path path;
>  	ssize_t error;
>  
> -	error = user_path(pathname, &path);
> +	error = user_path_at(AT_FDCWD, pathname,
> +			     LOOKUP_FOLLOW | LOOKUP_NO_AUTOMOUNT, &path);
>  	if (error)
>  		return error;
>  	error = getxattr(path.dentry, name, value, size);
> @@ -400,7 +402,7 @@ SYSCALL_DEFINE4(lgetxattr, const char __user *, pathname,
>  	struct path path;
>  	ssize_t error;
>  
> -	error = user_lpath(pathname, &path);
> +	error = user_path_at(AT_FDCWD, pathname, LOOKUP_NO_AUTOMOUNT, &path);
>  	if (error)
>  		return error;
>  	error = getxattr(path.dentry, name, value, size);
> @@ -459,7 +461,8 @@ SYSCALL_DEFINE3(listxattr, const char __user *, pathname, char __user *, list,
>  	struct path path;
>  	ssize_t error;
>  
> -	error = user_path(pathname, &path);
> +	error = user_path_at(AT_FDCWD, pathname,
> +			     LOOKUP_FOLLOW | LOOKUP_NO_AUTOMOUNT, &path);
>  	if (error)
>  		return error;
>  	error = listxattr(path.dentry, list, size);
> @@ -473,7 +476,7 @@ SYSCALL_DEFINE3(llistxattr, const char __user *, pathname, char __user *, list,
>  	struct path path;
>  	ssize_t error;
>  
> -	error = user_lpath(pathname, &path);
> +	error = user_path_at(AT_FDCWD, pathname, LOOKUP_NO_AUTOMOUNT, &path);
>  	if (error)
>  		return error;
>  	error = listxattr(path.dentry, list, size);
> @@ -519,7 +522,8 @@ SYSCALL_DEFINE2(removexattr, const char __user *, pathname,
>  	struct path path;
>  	int error;
>  
> -	error = user_path(pathname, &path);
> +	error = user_path_at(AT_FDCWD, pathname,
> +			     LOOKUP_FOLLOW | LOOKUP_NO_AUTOMOUNT, &path);
>  	if (error)
>  		return error;
>  	error = mnt_want_write(path.mnt);
> @@ -537,7 +541,7 @@ SYSCALL_DEFINE2(lremovexattr, const char __user *, pathname,
>  	struct path path;
>  	int error;
>  
> -	error = user_lpath(pathname, &path);
> +	error = user_path_at(AT_FDCWD, pathname, LOOKUP_NO_AUTOMOUNT, &path);
>  	if (error)
>  		return error;
>  	error = mnt_want_write(path.mnt);
> diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
> index f550f89..768fb66 100644
> --- a/include/linux/fcntl.h
> +++ b/include/linux/fcntl.h
> @@ -47,6 +47,7 @@
>  #define AT_SYMLINK_FOLLOW	0x400   /* Follow symbolic links.  */
>  #define AT_NO_AUTOMOUNT		0x800	/* Suppress terminal automount traversal */
>  #define AT_EMPTY_PATH		0x1000	/* Allow empty relative pathname */
> +#define AT_AUTOMOUNT_FOLLOW	0x2000	/* Follow terminal automounts */
>  
>  #ifdef __KERNEL__
>  
> 

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Thu, Sep 22, 2011 at 9:04 AM, Ian Kent <raven@themaw.net> wrote:
>
> I haven't checked what the side effects would be but using the
> LOOKUP_DIRECTORY flag in walks that get caught by the removal of the
> LOOKUP_FOLLOW test in follow_automount() and retaining Miklos's original
> patch is probably a more natural solution IMO.

Ok, I do have to agree with that.

So instead of adding a new flag, let's just document a few *logical*
rules for what causes auto-mounting:

 - opening the mount-point itself with LOOKUP_DIRECTORY does so

   Logic: when you use LOOKUP_DIRECTORY, you expect to see the
*contents* of the mount-point.

 - looking something up *under* the mount-point does so.

   This may be obvious, but it actually has a non-obvious special
case: what about the pathname "mountpoint" vs "mountpoint/"

And I think the "LOOKUP_DIRECTORY" rule ends up automatically also
resolving that special case: when we have a slash at the end of the
last component, it not only implies that we care about the contents,
it will also automatically set LOOKUP_DIRECTORY.

So I'm getting more and more convinced that LOOKUP_DIRECTORY is
actually the right thing to trigger on. It automatically means that
"opendir()" on the mountpoint will do the right thing (because
O_DIRECTORY results in LOOKUP_DIRECTORY), and it automatically - and
very naturally - gives user processes the ability to choose whether
they want to see auto-monting or not ("path" doesn't get auto-mounted,
but "path/" does).

Yet at the same time it keeps the "stupid default behavior for
processes that only look at each directory entry and don't even think
about automounting" be the "don't auto-mount when not necessary"
behavior.

So: no new flag. Just make nfs4 use LOOKUP_DIRECTORY, and let's add
big documentation notes about this.

Ok?

                     Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Ian Kent]

On Thu, 2011-09-22 at 09:30 -0700, Linus Torvalds wrote:
> On Thu, Sep 22, 2011 at 9:04 AM, Ian Kent <raven@themaw.net> wrote:
> >
> > I haven't checked what the side effects would be but using the
> > LOOKUP_DIRECTORY flag in walks that get caught by the removal of the
> > LOOKUP_FOLLOW test in follow_automount() and retaining Miklos's original
> > patch is probably a more natural solution IMO.
> 
> Ok, I do have to agree with that.
> 
> So instead of adding a new flag, let's just document a few *logical*
> rules for what causes auto-mounting:
> 
>  - opening the mount-point itself with LOOKUP_DIRECTORY does so
> 
>    Logic: when you use LOOKUP_DIRECTORY, you expect to see the
> *contents* of the mount-point.
> 
>  - looking something up *under* the mount-point does so.
> 
>    This may be obvious, but it actually has a non-obvious special
> case: what about the pathname "mountpoint" vs "mountpoint/"
> 
> And I think the "LOOKUP_DIRECTORY" rule ends up automatically also
> resolving that special case: when we have a slash at the end of the
> last component, it not only implies that we care about the contents,
> it will also automatically set LOOKUP_DIRECTORY.
> 
> So I'm getting more and more convinced that LOOKUP_DIRECTORY is
> actually the right thing to trigger on. It automatically means that
> "opendir()" on the mountpoint will do the right thing (because
> O_DIRECTORY results in LOOKUP_DIRECTORY), and it automatically - and
> very naturally - gives user processes the ability to choose whether
> they want to see auto-monting or not ("path" doesn't get auto-mounted,
> but "path/" does).
> 
> Yet at the same time it keeps the "stupid default behavior for
> processes that only look at each directory entry and don't even think
> about automounting" be the "don't auto-mount when not necessary"
> behavior.
> 
> So: no new flag. Just make nfs4 use LOOKUP_DIRECTORY, and let's add
> big documentation notes about this.

Yes, that's what I think is best.

It's late here so I'll survey the code and see if I can find any other
instances of this and post a patch, tomorrow.

Jeff, could you test adding LOOKUP_DIRECTORY to the walk in
nfs_follow_remote_path() please, just in case I've got this all wrong.

> 
> Ok?
> 
>                      Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Jeff Layton]

On Fri, 23 Sep 2011 00:45:35 +0800
Ian Kent <raven@themaw.net> wrote:

> On Thu, 2011-09-22 at 09:30 -0700, Linus Torvalds wrote:
> > On Thu, Sep 22, 2011 at 9:04 AM, Ian Kent <raven@themaw.net> wrote:
> > >
> > > I haven't checked what the side effects would be but using the
> > > LOOKUP_DIRECTORY flag in walks that get caught by the removal of the
> > > LOOKUP_FOLLOW test in follow_automount() and retaining Miklos's original
> > > patch is probably a more natural solution IMO.
> > 
> > Ok, I do have to agree with that.
> > 
> > So instead of adding a new flag, let's just document a few *logical*
> > rules for what causes auto-mounting:
> > 
> >  - opening the mount-point itself with LOOKUP_DIRECTORY does so
> > 
> >    Logic: when you use LOOKUP_DIRECTORY, you expect to see the
> > *contents* of the mount-point.
> > 
> >  - looking something up *under* the mount-point does so.
> > 
> >    This may be obvious, but it actually has a non-obvious special
> > case: what about the pathname "mountpoint" vs "mountpoint/"
> > 
> > And I think the "LOOKUP_DIRECTORY" rule ends up automatically also
> > resolving that special case: when we have a slash at the end of the
> > last component, it not only implies that we care about the contents,
> > it will also automatically set LOOKUP_DIRECTORY.
> > 
> > So I'm getting more and more convinced that LOOKUP_DIRECTORY is
> > actually the right thing to trigger on. It automatically means that
> > "opendir()" on the mountpoint will do the right thing (because
> > O_DIRECTORY results in LOOKUP_DIRECTORY), and it automatically - and
> > very naturally - gives user processes the ability to choose whether
> > they want to see auto-monting or not ("path" doesn't get auto-mounted,
> > but "path/" does).
> > 
> > Yet at the same time it keeps the "stupid default behavior for
> > processes that only look at each directory entry and don't even think
> > about automounting" be the "don't auto-mount when not necessary"
> > behavior.
> > 
> > So: no new flag. Just make nfs4 use LOOKUP_DIRECTORY, and let's add
> > big documentation notes about this.
> 
> Yes, that's what I think is best.
> 
> It's late here so I'll survey the code and see if I can find any other
> instances of this and post a patch, tomorrow.
> 
> Jeff, could you test adding LOOKUP_DIRECTORY to the walk in
> nfs_follow_remote_path() please, just in case I've got this all wrong.
> 

Yep, adding LOOKUP_DIRECTORY also fixes the problem. I'll go ahead and
send a patch to Trond...

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Jeff Layton]

On Thu, 22 Sep 2011 13:35:29 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> On Fri, 23 Sep 2011 00:45:35 +0800
> Ian Kent <raven@themaw.net> wrote:
> 
> > On Thu, 2011-09-22 at 09:30 -0700, Linus Torvalds wrote:
> > > On Thu, Sep 22, 2011 at 9:04 AM, Ian Kent <raven@themaw.net> wrote:
> > > >
> > > > I haven't checked what the side effects would be but using the
> > > > LOOKUP_DIRECTORY flag in walks that get caught by the removal of the
> > > > LOOKUP_FOLLOW test in follow_automount() and retaining Miklos's original
> > > > patch is probably a more natural solution IMO.
> > > 
> > > Ok, I do have to agree with that.
> > > 
> > > So instead of adding a new flag, let's just document a few *logical*
> > > rules for what causes auto-mounting:
> > > 
> > >  - opening the mount-point itself with LOOKUP_DIRECTORY does so
> > > 
> > >    Logic: when you use LOOKUP_DIRECTORY, you expect to see the
> > > *contents* of the mount-point.
> > > 
> > >  - looking something up *under* the mount-point does so.
> > > 
> > >    This may be obvious, but it actually has a non-obvious special
> > > case: what about the pathname "mountpoint" vs "mountpoint/"
> > > 
> > > And I think the "LOOKUP_DIRECTORY" rule ends up automatically also
> > > resolving that special case: when we have a slash at the end of the
> > > last component, it not only implies that we care about the contents,
> > > it will also automatically set LOOKUP_DIRECTORY.
> > > 
> > > So I'm getting more and more convinced that LOOKUP_DIRECTORY is
> > > actually the right thing to trigger on. It automatically means that
> > > "opendir()" on the mountpoint will do the right thing (because
> > > O_DIRECTORY results in LOOKUP_DIRECTORY), and it automatically - and
> > > very naturally - gives user processes the ability to choose whether
> > > they want to see auto-monting or not ("path" doesn't get auto-mounted,
> > > but "path/" does).
> > > 
> > > Yet at the same time it keeps the "stupid default behavior for
> > > processes that only look at each directory entry and don't even think
> > > about automounting" be the "don't auto-mount when not necessary"
> > > behavior.
> > > 
> > > So: no new flag. Just make nfs4 use LOOKUP_DIRECTORY, and let's add
> > > big documentation notes about this.
> > 
> > Yes, that's what I think is best.
> > 
> > It's late here so I'll survey the code and see if I can find any other
> > instances of this and post a patch, tomorrow.
> > 
> > Jeff, could you test adding LOOKUP_DIRECTORY to the walk in
> > nfs_follow_remote_path() please, just in case I've got this all wrong.
> > 
> 
> Yep, adding LOOKUP_DIRECTORY also fixes the problem. I'll go ahead and
> send a patch to Trond...
> 

To be clear... I tested this with LOOKUP_FOLLOW|LOOKUP_DIRECTORY.

Also, Ian requested that I hold off on pushing a patch to Trond until
he can audit the rest of the code for similar problems.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Trond Myklebust]

On Thu, 2011-09-22 at 14:44 -0400, Jeff Layton wrote: 
> On Thu, 22 Sep 2011 13:35:29 -0400
> Jeff Layton <jlayton@redhat.com> wrote:
> 
> > On Fri, 23 Sep 2011 00:45:35 +0800
> > Ian Kent <raven@themaw.net> wrote:
> > 
> > > On Thu, 2011-09-22 at 09:30 -0700, Linus Torvalds wrote:
> > > > On Thu, Sep 22, 2011 at 9:04 AM, Ian Kent <raven@themaw.net> wrote:
> > > > >
> > > > > I haven't checked what the side effects would be but using the
> > > > > LOOKUP_DIRECTORY flag in walks that get caught by the removal of the
> > > > > LOOKUP_FOLLOW test in follow_automount() and retaining Miklos's original
> > > > > patch is probably a more natural solution IMO.
> > > > 
> > > > Ok, I do have to agree with that.
> > > > 
> > > > So instead of adding a new flag, let's just document a few *logical*
> > > > rules for what causes auto-mounting:
> > > > 
> > > >  - opening the mount-point itself with LOOKUP_DIRECTORY does so
> > > > 
> > > >    Logic: when you use LOOKUP_DIRECTORY, you expect to see the
> > > > *contents* of the mount-point.
> > > > 
> > > >  - looking something up *under* the mount-point does so.
> > > > 
> > > >    This may be obvious, but it actually has a non-obvious special
> > > > case: what about the pathname "mountpoint" vs "mountpoint/"
> > > > 
> > > > And I think the "LOOKUP_DIRECTORY" rule ends up automatically also
> > > > resolving that special case: when we have a slash at the end of the
> > > > last component, it not only implies that we care about the contents,
> > > > it will also automatically set LOOKUP_DIRECTORY.
> > > > 
> > > > So I'm getting more and more convinced that LOOKUP_DIRECTORY is
> > > > actually the right thing to trigger on. It automatically means that
> > > > "opendir()" on the mountpoint will do the right thing (because
> > > > O_DIRECTORY results in LOOKUP_DIRECTORY), and it automatically - and
> > > > very naturally - gives user processes the ability to choose whether
> > > > they want to see auto-monting or not ("path" doesn't get auto-mounted,
> > > > but "path/" does).
> > > > 
> > > > Yet at the same time it keeps the "stupid default behavior for
> > > > processes that only look at each directory entry and don't even think
> > > > about automounting" be the "don't auto-mount when not necessary"
> > > > behavior.
> > > > 
> > > > So: no new flag. Just make nfs4 use LOOKUP_DIRECTORY, and let's add
> > > > big documentation notes about this.
> > > 
> > > Yes, that's what I think is best.
> > > 
> > > It's late here so I'll survey the code and see if I can find any other
> > > instances of this and post a patch, tomorrow.
> > > 
> > > Jeff, could you test adding LOOKUP_DIRECTORY to the walk in
> > > nfs_follow_remote_path() please, just in case I've got this all wrong.
> > > 
> > 
> > Yep, adding LOOKUP_DIRECTORY also fixes the problem. I'll go ahead and
> > send a patch to Trond...
> > 
> 
> To be clear... I tested this with LOOKUP_FOLLOW|LOOKUP_DIRECTORY.
> 
> Also, Ian requested that I hold off on pushing a patch to Trond until
> he can audit the rest of the code for similar problems.

We need to audit not only NFS, but a bunch of stuff in the VFS.

For instance, what is the correct behaviour if I do a bind mount with
the source being an automount directory? Currently, that doesn't set
LOOKUP_DIRECTORY (they cant, since file may also be a mountpoint) and so
the current behaviour means that they will fail to automount.

I'm not sure if we care about umount() following a terminal automount,
but that too is affected and suffers from the 'doesn't have to be a
directory' syndrome.

Quotactl seems to be affected too. Dunno if quotactl is allowed to apply
to a file, but if it is, then we can't set LOOKUP_DIRECTORY.

Cheers
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Thu, Sep 22, 2011 at 12:20 PM, Trond Myklebust
<Trond.Myklebust@netapp.com> wrote:
>
> We need to audit not only NFS, but a bunch of stuff in the VFS.

Why? What has *anything*else* got to do with fixing a NFSv4 bug?

IOW, why the hell would we hold up one bugfix just because you might
want to look at something else too?

> For instance, what is the correct behaviour if I do a bind mount with
> the source being an automount directory? Currently, that doesn't set
> LOOKUP_DIRECTORY (they cant, since file may also be a mountpoint) and so
> the current behaviour means that they will fail to automount.

Umm. If there is any question at all what the correct behavior might
be, then pretty clearly the correct behavior is to let the user
decide.

And as pointed out, that's one of the *advantages* of
LOOKUP_DIRECTORY. Because it lets users decide.

Exatly because it gets automatically set for the 'slash at end of
name' case. So suddenly you can literally let the user say one or the
other by just letting the user add or remove the slash: "/mountpoint"
is not automount-followed, while "/mountpoint/" is. Nice, obvious, and
consistent interface. Which is exactly what we'd be looking for.

                 Linus

RE: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Myklebust, Trond]

> -----Original Message-----
> From: Linus Torvalds [mailto:torvalds@linux-foundation.org]
> Sent: Thursday, September 22, 2011 6:57 PM
> To: Myklebust, Trond
> Cc: Jeff Layton; Ian Kent; David Howells; miklos@szeredi.hu;
> viro@zeniv.linux.org.uk; gregkh@suse.de; linux-nfs@vger.kernel.org;
> leonardo.lists@gmail.com
> Subject: Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr,
etc.
> 
> On Thu, Sep 22, 2011 at 12:20 PM, Trond Myklebust
> <Trond.Myklebust@netapp.com> wrote:
> >
> > We need to audit not only NFS, but a bunch of stuff in the VFS.
> 
> Why? What has *anything*else* got to do with fixing a NFSv4 bug?
> 
> IOW, why the hell would we hold up one bugfix just because you might
want
> to look at something else too?

Your LOOKUP_DIRECTORY solution assumes that any and all syscalls that
_might_ want to automount a directory that is the last path element can
always predict in advance that the last path element will be a
directory. I'm challenging that assumption.
So if I ask for quotactl(/foo), why am I suddenly required to know in
advance that /foo might be a directory, and so make that a /foo/ so that
the automounted case suddenly works?

Your assumption is that in the majority of cases, we do _not_ want to
automount the final directory unless we know that we are expecting a
directory. As far as I'm concerned, if doesn't matter if you are
expecting a directory or not: with only a few exceptions, your
application is not supposed to be able to access the directory that will
be automounted over at all. The only reason for allowing the 'stat()'
exception is to optimize away the automount in the common case of an 'ls
-l' where the application doesn't care about accuracy.

  Trond

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Thu, Sep 22, 2011 at 5:56 PM, Myklebust, Trond
<Trond.Myklebust@netapp.com> wrote:
>
> Your assumption is that in the majority of cases, we do _not_ want to
> automount the final directory unless we know that we are expecting a
> directory.

Umm. That is the assumption yes, BUT THAT IS ALSO THE CURRENT STATE.

So it's more than an assumption. It's a fact.

So when you call it "assumption", you are basically ignoring and
trying to belittle current reality. Why?

                     Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Trond Myklebust]

On Thu, 2011-09-22 at 18:04 -0700, Linus Torvalds wrote: 
> On Thu, Sep 22, 2011 at 5:56 PM, Myklebust, Trond
> <Trond.Myklebust@netapp.com> wrote:
> >
> > Your assumption is that in the majority of cases, we do _not_ want to
> > automount the final directory unless we know that we are expecting a
> > directory.
> 
> Umm. That is the assumption yes, BUT THAT IS ALSO THE CURRENT STATE.
> 
> So it's more than an assumption. It's a fact.
> 
> So when you call it "assumption", you are basically ignoring and
> trying to belittle current reality. Why?

AFAICR, the whole point of doing the ->automount() stuff was to fix what
was perceived to be a broken situation in which the application was more
often than not seeing the properties of a directory which it would
_never_ directly access.

David, Ian, Al and I carefully listed the cases where we might want to
optimise away the automount, and designed a system that fit those cases.

I fully accept that we need to address any regressions that may have
introduced, but your fix goes beyond the regressions that were reported:
it basically puts us back where we were before the automount changes. In
that situation, what have we gained by the changes, and why keep them at
all?

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Miklos Szeredi]

Trond Myklebust <Trond.Myklebust@netapp.com> writes:

> On Thu, 2011-09-22 at 18:04 -0700, Linus Torvalds wrote: 
>> On Thu, Sep 22, 2011 at 5:56 PM, Myklebust, Trond
>> <Trond.Myklebust@netapp.com> wrote:
>> >
>> > Your assumption is that in the majority of cases, we do _not_ want to
>> > automount the final directory unless we know that we are expecting a
>> > directory.
>> 
>> Umm. That is the assumption yes, BUT THAT IS ALSO THE CURRENT STATE.
>> 
>> So it's more than an assumption. It's a fact.
>> 
>> So when you call it "assumption", you are basically ignoring and
>> trying to belittle current reality. Why?
>
> AFAICR, the whole point of doing the ->automount() stuff was to fix what
> was perceived to be a broken situation in which the application was more
> often than not seeing the properties of a directory which it would
> _never_ directly access.

We are pitting one set of applications against another.  There's no
Right(TM) behavior here.  In any case there will be applications which
will behave strangely, be extremely slow, etc...

Changing the behavior of stat will cause regressions.  Period.  If we
just fixed up those apps which behave badly with the old kernels, there
wouldn't be any regressions.

> David, Ian, Al and I carefully listed the cases where we might want to
> optimise away the automount, and designed a system that fit those cases.
>
> I fully accept that we need to address any regressions that may have
> introduced, but your fix goes beyond the regressions that were reported:
> it basically puts us back where we were before the automount changes. In
> that situation, what have we gained by the changes, and why keep them at
> all?

You are blowing this out of proportion.  Those patches do a lot more
than change the behavior of stat, etc.  The fact that stat will trigger
an automount is only mentioned in passing, and even there it's not
stated that the behavior was actually *changed* from the old autofs4
implentation:

   "I've also extracted the mount/don't-mount logic from autofs4 and
    included it here.  It makes the mount go ahead anyway if someone
    calls open() or creat(), tries to traverse the directory, tries to
    chdir/chroot/etc. into the directory, or sticks a '/' on the end of
    the pathname.  If they do a stat(), however, they'll only trigger
    the automount if they didn't also say O_NOFOLLOW."

Thanks,
Miklos

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Ian Kent]

On Fri, 2011-09-23 at 09:25 +0200, Miklos Szeredi wrote:
> Trond Myklebust <Trond.Myklebust@netapp.com> writes:
> 
> > On Thu, 2011-09-22 at 18:04 -0700, Linus Torvalds wrote: 
> >> On Thu, Sep 22, 2011 at 5:56 PM, Myklebust, Trond
> >> <Trond.Myklebust@netapp.com> wrote:
> >> >
> >> > Your assumption is that in the majority of cases, we do _not_ want to
> >> > automount the final directory unless we know that we are expecting a
> >> > directory.
> >> 
> >> Umm. That is the assumption yes, BUT THAT IS ALSO THE CURRENT STATE.
> >> 
> >> So it's more than an assumption. It's a fact.
> >> 
> >> So when you call it "assumption", you are basically ignoring and
> >> trying to belittle current reality. Why?
> >
> > AFAICR, the whole point of doing the ->automount() stuff was to fix what
> > was perceived to be a broken situation in which the application was more
> > often than not seeing the properties of a directory which it would
> > _never_ directly access.
> 
> We are pitting one set of applications against another.  There's no
> Right(TM) behavior here.  In any case there will be applications which
> will behave strangely, be extremely slow, etc...
> 
> Changing the behavior of stat will cause regressions.  Period.  If we
> just fixed up those apps which behave badly with the old kernels, there
> wouldn't be any regressions.

That's the point isn't it.

Your patch changes the pre-exiting behavior of NFS and probably CIFS and
AFS, but restores the pre-existing behavior of autofs, which is
something I've wanted to change for years.

My suggestion of using LOOKUP_DIRECTORY at path walk sites that need it
is still a possibility. I'm still looking through path walk call sites.
But that resolution will still leave NFS et.al. with LOOKUP_FOLLOW only
calls having changed semantic behavior.

There is no resolution to this that will satisfy everyone. Passing on
the lookup flags is, I think, the only way to segregate the sub system
behaviors and I'm not in favor of that either.

Ian

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Ian Kent]

On Thu, 2011-09-22 at 18:04 -0700, Linus Torvalds wrote:
> On Thu, Sep 22, 2011 at 5:56 PM, Myklebust, Trond
> <Trond.Myklebust@netapp.com> wrote:
> >
> > Your assumption is that in the majority of cases, we do _not_ want to
> > automount the final directory unless we know that we are expecting a
> > directory.
> 
> Umm. That is the assumption yes, BUT THAT IS ALSO THE CURRENT STATE.

With the patch that Miklos posted, yes.

But the other users of the vfs-automount didn't behave this way before
the vfs-automount changes.

> 
> So it's more than an assumption. It's a fact.

For autofs itself, yes.

> 
> So when you call it "assumption", you are basically ignoring and
> trying to belittle current reality. Why?

Lets take a step back and re-examine what started this.

That is the semantic change automounting LOOKUP_FOLLOW path walks, aka.
stat and related system calls which Miklos wanted to change back.

Unfortunately, the reality is that most of the time (almost always
anyway) that is what should be done. The exceptions are such things as
long listing directory contents where we would also like stat to cause
automounting but cannot because it causes users to complain about lots
of mounts being triggered. Contrast that with the behavior of find(1)
where users sometimes don't want a bunch of mounts done but, in this
case, they must be done for it to function.

So the previous autofs behavior was problematic for find and couldn't be
fixed, but was OK for long directory listings. Pretty much a crap
situation.

There is also the common case where applications perform a stat(2) (or
any LOOKUP_FOLLOW type) call (other than when used in listing a
directory) where the automount should also be done and complains about
that behavior were met with an explanation of why it was so and a
wontfix response. So for the bulk of cases automounting should be done
for LOOKUP_FOLLOW type system calls and not for the ~LOOKUP_FOLLOW.
Which is also a "natural" usage because user space largely does this the
way automounting needs already.

Coming back to the bug that Miklos referred to.

This was a case where long listing a directory caused automount child
directories to be mounted. But for directory listings we want don't want
to automount it's child directories. We also want the details on
symlinks, and don't want to follow them. In the reported bug there were
a couple of lgetxattr(2) calls and suddenly a getxattr(2) call which was
inconsistent usage and was causing the unwanted mounts. Even upstream
conceded that this was inconsistent and accepted a path (well actually
two patches, one for the acl package and one for coreutils), and the
problem was gone.

In the time we have had this changed behavior we haven't had any further
reports of problems that I'm aware of. But there may be more to come, I
expect we will see some complains from GUI uses at some point too but
they will be a problem whether we mount on stat on not, since not
mounting the things they want to see is no good for them and mounting to
many things for them annoys them as well.

Now Trond has identified a number of examples that demonstrates why we
should not merge (or should revert) Miklos's patch, examples I couldn't
provide when he asked for them.

Again, I believe we got it right the first time, and we should work with
user space to provide any information and assistance needed for them to
change to accommodate the semantic difference which has been needed for
many years, and has (had) now been done.

So, sorry Miklos, but I now think your patch should be reverted.

Ian

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[David Howells]

Ian Kent <raven@themaw.net> wrote:

> But the other users of the vfs-automount didn't behave this way before
> the vfs-automount changes.

That's a very good point.  Prior to my d_automount() patches, NFS, AFS and, I
think, CIFS used strict symlink rules for when to automount because the
automounting was done by a directory with a ->follow_link() op.  Therefore,
stat() automounted and lstat() didn't.

Autofs, on the other hand took care to suppress automounting on stat() too.

My d_automount() patches had two purposes, in order of importance:

 (1) Simplify the automount procedure in the kernel by giving it formal VFS
     support, and thus getting rid of directories with follow_link() ops.

 (2) Make the automount semantics consistent.

So my original patches are somewhat at fault here.  I broke autofs's handling
of stat(), getxattr() and kept that of NFS, AFS and CIFS.  Miklos's patch
simply flips the regression the other way.

The only fault I've had reported against my original patches in this respect is
with ls, and that's fixed in userspace upstream now (libacl was using getxattr
when it should've been using lgetxattr).  It has been mentioned that nautilus
(I think it was) may also broken - which makes sense as it's a file manager.

The only fault I've had reported against Miklos's patch is the NFS4 pathwalk
during mount.

Having thought it over some more, I'm leaning towards reverting Miklos's patch,
removing the do we/don't we logic from follow_automount() (or simplifying it)
and having the syscalls suppy the LOOKUP_NO_AUTOMOUNT flag as appropriate -
whatever is meant by 'appropriate'.

I'm also okay with requiring userspace to be fixed up, but I can also see the
arguments against that.

David

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Trond Myklebust]

On Fri, 2011-09-23 at 11:33 +0100, David Howells wrote: 
> The only fault I've had reported against my original patches in this respect is
> with ls, and that's fixed in userspace upstream now (libacl was using getxattr
> when it should've been using lgetxattr).  It has been mentioned that nautilus
> (I think it was) may also broken - which makes sense as it's a file manager.
> 
> The only fault I've had reported against Miklos's patch is the NFS4 pathwalk
> during mount.

If NFSv4 is the only concern, then Linus's solution is easily
implementable. I don't have any problems with adding a LOOKUP_DIRECTORY
flag to the NFSv4 mount path lookup call.

My objections are due to the other cases that I pointed out, where
Miklos's patch introduced changes in behaviour that IMO are unnecessary
and incorrect.

> Having thought it over some more, I'm leaning towards reverting Miklos's patch,
> removing the do we/don't we logic from follow_automount() (or simplifying it)
> and having the syscalls suppy the LOOKUP_NO_AUTOMOUNT flag as appropriate -
> whatever is meant by 'appropriate'.

I agree. That means the default behaviour is the correct one and we're
only adding exceptions in cases where exposing the hidden directory is
acceptable either to address an optimisation concern or an application
regression.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Fri, Sep 23, 2011 at 7:34 AM, Trond Myklebust
<Trond.Myklebust@netapp.com> wrote:
>
> My objections are due to the other cases that I pointed out, where
> Miklos's patch introduced changes in behaviour that IMO are unnecessary
> and incorrect.

Guys, it wasn't Mikos' patch that introduced the changes!

What's so hard to understand here?

That's why I'm so upset. People talk stupid sh*t about "correct
behavior" when clearly no such thing *exists*. And people talk about
Miklos changes as if they were some radical change that changed
behavior, when they were only a revert to old behavior to begin with
(at least as far as autofs is concerned)!

Get a grip, people. Stop over-analyzing things. Stop bothering to
mention what Solaris does, when the *MUCH* bigger issue is what
*Linux* has done for years and years.

Stop saying "we'll revert Miklos patch" in the same sentence where you
then seem to not even understand that the *original* behavior was the
one that Miklos patch re-introduced.

Reverting Miklos patch isn't a revert. THAT is the "semantic change".
That's the one you need to explain why the heck it would be the right
thing to do, rather than imply that we'd be going back to some known
state.

                              Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Trond Myklebust]

On Fri, 2011-09-23 at 07:46 -0700, Linus Torvalds wrote: 
> On Fri, Sep 23, 2011 at 7:34 AM, Trond Myklebust
> <Trond.Myklebust@netapp.com> wrote:
> >
> > My objections are due to the other cases that I pointed out, where
> > Miklos's patch introduced changes in behaviour that IMO are unnecessary
> > and incorrect.
> 
> Guys, it wasn't Mikos' patch that introduced the changes!
> 
> What's so hard to understand here?
> 
> That's why I'm so upset. People talk stupid sh*t about "correct
> behavior" when clearly no such thing *exists*. And people talk about
> Miklos changes as if they were some radical change that changed
> behavior, when they were only a revert to old behavior to begin with
> (at least as far as autofs is concerned)!
> 
> Get a grip, people. Stop over-analyzing things. Stop bothering to
> mention what Solaris does, when the *MUCH* bigger issue is what
> *Linux* has done for years and years.
> 
> Stop saying "we'll revert Miklos patch" in the same sentence where you
> then seem to not even understand that the *original* behavior was the
> one that Miklos patch re-introduced.
> 
> Reverting Miklos patch isn't a revert. THAT is the "semantic change".
> That's the one you need to explain why the heck it would be the right
> thing to do, rather than imply that we'd be going back to some known
> state.

Wrong. Miklos's patch introduces new behaviour that was never present
previously:

- you can now bind mount to the hidden directory.
- you can quotactl the underlying directory.

As I said earlier: adding LOOKUP_DIRECTORY to those operations is not an
option because bind mount needs to work with files too (as long as the
source and target are both files). Ditto with quotactl.


-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Fri, Sep 23, 2011 at 8:01 AM, Trond Myklebust
<Trond.Myklebust@netapp.com> wrote:
>
> Wrong. Miklos's patch introduces new behaviour that was never present
> previously:

Look at the big picture. The real damage wasn't done my Miklos, and
that's something you don't seem to understand.

So stop with the crazy "just revert" crap. That's not how it works. If
you want to talk about reverts, then you need to go all the way back
to the follow_link behavior.

Afaik, the closest we can get to old behavior is just checking
LOOKUP_DIRECTORY. We might want to check LOOKUP_OPEN too and that
solves your "whaa, whaah, it might be a file" thing. But neither stat
nor lstat used to auto-mount with autofs (and quite frankly, some
NFS-side automounting should strive to look the same, imgo).

So please *understand* the problem I have already. My problem is that
you seem to think "revert miklos" just fixes all the issues and is
some "safe" approach. It just re-introduces the *original* regression.

                              Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Ian Kent]

On Fri, 2011-09-23 at 07:46 -0700, Linus Torvalds wrote:
> On Fri, Sep 23, 2011 at 7:34 AM, Trond Myklebust
> <Trond.Myklebust@netapp.com> wrote:
> >
> > My objections are due to the other cases that I pointed out, where
> > Miklos's patch introduced changes in behaviour that IMO are unnecessary
> > and incorrect.
> 
> Guys, it wasn't Mikos' patch that introduced the changes!
> 
> What's so hard to understand here?
> 
> That's why I'm so upset. People talk stupid sh*t about "correct
> behavior" when clearly no such thing *exists*. And people talk about
> Miklos changes as if they were some radical change that changed
> behavior, when they were only a revert to old behavior to begin with
> (at least as far as autofs is concerned)!

Sorry to hear you are upset about all this.

And, yes, it is me that is to blame, since I pushed David to do it. I
did so because I feel strongly that the semantic change to autofs is
needed based on several years of experience with reports of undesirable
behavior of autofs.

I thought I had put forward quite a bit of the reasoning for that, and
on topic with what you've asked for in this post, at least most of the
time. In its simplest form I claim, for autofs it is appropriate to
automount in almost all cases where the LOOKUP_FOLLOW flag is used
(these should be considered as not regression with the semantic change).
For the cases where it is preferred not to automount user space should
(and nowadays almost always does) use ~LOOKUP_FOLLOW type syscalls.

I expected some problems from the semantic change, sure, but I expected
they would come from user space. Those problems just haven't
materialized so far and I'm glad of that.

We haven't had any non-trivial problems reported that I know about. As
I've said before the problem Miklos referred to was an inconsistency in
user space which the new automount code exposed, and a change has been
accepted to fix it and this was done prior to Miklos posting his patch. 

My point is that, since we haven't had any non-trivial problems reported
due to the semantic change, in what six months or more, why change it
now?

Ian

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Miklos Szeredi]

Ian Kent <raven@themaw.net> writes:

> On Fri, 2011-09-23 at 07:46 -0700, Linus Torvalds wrote:
> We haven't had any non-trivial problems reported that I know about. As
> I've said before the problem Miklos referred to was an inconsistency in
> user space which the new automount code exposed, and a change has been
> accepted to fix it and this was done prior to Miklos posting his patch. 
>
> My point is that, since we haven't had any non-trivial problems reported
> due to the semantic change, in what six months or more, why change it
> now?

Six month is a short time in terms of kernel deployment.  It's entirely
possible that many installations with large maps simply haven't yet
upgraded their kernel.

And ordering WRT userspace package upgrades doesn't work on such time
scales either.  It's entirely possible that a distribution will upgrade
their kernel yet pick an older userspace package.

Also, there are file managers out there that haven't yet been fixed that
are known to trigger the bad behavior.  So I don't believe your
arguments justify leaving things as they are.

Thanks,
Miklos

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Fri, Sep 23, 2011 at 8:41 AM, Ian Kent <raven@themaw.net> wrote:
>
> My point is that, since we haven't had any non-trivial problems reported
> due to the semantic change, in what six months or more, why change it
> now?

We may indeed have to revert for sanity at this point, but I have to
say that I think the arguments have been very weak, and the logic for
when to auto-mount and when not to seems to not really be based on any
real logic.

For example, I think the whole "let's consider lstat different from
stat" model is broken. It works for symlinks, but that's because

 - applications *know* about the symlink difference

 - lstat actually *informs* about the symlink

neither of which is true for automounts. So quite frankly, I think
trying to conflate automount behavior with symlink behavior is a
completely bogus model to begin with, and has no actual redeeming
values except for an implementation issue (->follow_link) that no
longer even exists!

So I seriously think that the old autofs model is technically
superior. Which doesn't make it the only right one, of course. But I
really do think it's totally broken to think that lstat works
differently from stat.

So I would argue that using lstat as a differentiator for this is bad, because

 - it's not what we've done historically
 - it's a crazy hack introduced by implementation reasons, not logic
 - it's against all lstat documentation
 - it's inflexible and makes it hard to script

Now, introducing a new internal kernel flag is in many ways even
*worse*, because it's going to make it even more ad-hoc and
implementation issue, and even less visible and logical to actual
users who don't care.

So *that* is why I thought the LOOKUP_DIRECTORY flag was so nice. And
I still suspect that if paired with LOOKUP_OPEN, it would actually be
a complete solution that avoids the crazy issues with "random
implementation detail" and could give us something that is both
accessible from user space *and* something we can explain to a user
from a logical basis.

In other words, if we are going to accept changing semantics (and
apparently we have to), let's just make sure the ones we pick are the
*sensible* ones.

                Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Fri, Sep 23, 2011 at 9:21 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> For example, I think the whole "let's consider lstat different from
> stat" model is broken.

Just to expand on that: I think it's also extra broken to then say "..
but we can randomly fix up user space" for it.

It's like saying "we have introduced a totally broken model, and now
we're going to make old applications change because we changed.

That's very much against the kernel philosophy. I realize other
projects work that way, but we really *really* don't work that way in
the kernel. We don't break old workflow unless there is some seriously
overriding reason, and I don't think "we have a new crazy model" is a
good reason.

If the lstat/stat difference made any sense, we could at least argue
for it. But it just doesn't.

                        Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[David Howells]

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > getxattr and co. are trickier because they don't have an AT_ flags form, and
> > it can be argued that these, lgetxattr and co. included should automount.
> > However, that would seriously upset ls, so I think we have to leave lgetxattr
> > not automounting, and so getxattr should follow.
> 
> Why?
> 
> If you really want to follow, use the '/' at the end.

Sorry, 'follow' as in follow what lgetxattr does, not 'follow' as in
automount.  Bad choise of verb.

David

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Fri, Sep 23, 2011 at 9:54 AM, David Howells <dhowells@redhat.com> wrote:
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> > getxattr and co. are trickier because they don't have an AT_ flags form, and
>> > it can be argued that these, lgetxattr and co. included should automount.
>> > However, that would seriously upset ls, so I think we have to leave lgetxattr
>> > not automounting, and so getxattr should follow.
>>
>> Why?
>>
>> If you really want to follow, use the '/' at the end.
>
> Sorry, 'follow' as in follow what lgetxattr does, not 'follow' as in
> automount.  Bad choise of verb.

Ahh, ok, yeah, that 'follow' in that context to me meant auto-mount,
not "logically follows".

             Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[David Howells]

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> We may indeed have to revert for sanity at this point, but I have to
> say that I think the arguments have been very weak, and the logic for
> when to auto-mount and when not to seems to not really be based on any
> real logic.

Can you look over the patchset I've just posted.

It moves the policy on whether or not to suppress automount out of
follow_automount() - which is probably the wrong place for it anyway.

Depending on which patch you want to stop at it can reinstate the pre-Miklos
behaviour; reinstate the Miklos behaviour, but without the regression; or
reinstate the pre-d_automount behaviour.

> For example, I think the whole "let's consider lstat different from
> stat" model is broken. It works for symlinks, but that's because

That's a reasonable way to look at it; the difference between stat and lstat
is about handling symlinks only.  Furthermore, we have fstatat, so this isn't
really an issue - that can be given flags to let userspace decide what it
should do.

getxattr and co. are trickier because they don't have an AT_ flags form, and
it can be argued that these, lgetxattr and co. included should automount.
However, that would seriously upset ls, so I think we have to leave lgetxattr
not automounting, and so getxattr should follow.

We should consider adding an AT_ flags form of the xattr calls, but that's
another matter we can argue over later.

> So *that* is why I thought the LOOKUP_DIRECTORY flag was so nice. And
> I still suspect that if paired with LOOKUP_OPEN, it would actually be
> a complete solution that avoids the crazy issues with "random
> implementation detail" and could give us something that is both
> accessible from user space *and* something we can explain to a user
> from a logical basis.

I don't like this because it's fragile.  We're giving an implied second
meaning to LOOKUP_DIRECTORY/LOOKUP_OPEN, albeit a reasonable one.

Can we please move the policy to the pathwalk callers instead?

David

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Fri, Sep 23, 2011 at 9:40 AM, David Howells <dhowells@redhat.com> wrote:
>
> getxattr and co. are trickier because they don't have an AT_ flags form, and
> it can be argued that these, lgetxattr and co. included should automount.
> However, that would seriously upset ls, so I think we have to leave lgetxattr
> not automounting, and so getxattr should follow.

Why?

If you really want to follow, use the '/' at the end.

Seriously.  ALMOST NOBODY CARES. And the people that DO care the most
are the people who are telling you: DON'T YOU F*(^ING AUTOMOUNT BY
MISTAKE!

If there's even a slight doubt about what to do, we're generally
*much* better off not auto-mounting. Really.

                   Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[David Howells]

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> Guys, it wasn't Mikos' patch that introduced the changes!

Yes, thank you, we know.  It was my d_automount patch that did it.  I've
admitted this.  It made the behaviour of all internal automounts consistent,
which as it turns out, altered the behaviour of autofs.  We started fixing
this up in userspace (libacl and coreutils have been fixed).

> That's why I'm so upset. People talk stupid sh*t about "correct
> behavior" when clearly no such thing *exists*.

The discussion really revolves around two things:

 (1) We have a potential opportunity to define the correct behaviour and to
     make all the filesystems consistent with that, but what is the correct
     behaviour?

 (2) Whether we should vary the behaviour based on filesystem.

> And people talk about Miklos changes as if they were some radical change
> that changed behavior, when they were only a revert to old behavior to begin
> with (at least as far as autofs is concerned)!

Exactly.  You've made my point precisely with that last parenthesis: _only_ as
far as autofs is concerned.

Miklos's patch instead creates a regression in the behaviour in NFS, AFS and
CIFS hence why NFS4 mount root pathwalk is now acting weird.  That's not
really Miklos's fault, it only occurs in certain cases, and it's not very
obvious that it did happen or that it would happen.

> Get a grip, people. Stop over-analyzing things. Stop bothering to
> mention what Solaris does, when the *MUCH* bigger issue is what
> *Linux* has done for years and years.

So the word is Linux may not change it's behaviour in this regard from what
existed prior to the d_automount patch, period?

If so, we need to vary the behaviour based on the filesystem being
automounted.

> Stop saying "we'll revert Miklos patch" in the same sentence where you
> then seem to not even understand that the *original* behavior was the
> one that Miklos patch re-introduced.

Miklos's patch did *NOT* re-introduce the original behaviour.  It introduced a
third state.  It merely moved the regression and brought about a more serious
one.

> Reverting Miklos patch isn't a revert. THAT is the "semantic change".
> That's the one you need to explain why the heck it would be the right
> thing to do, rather than imply that we'd be going back to some known
> state.

Miklos's patch is also a semantic change.


I'm not keen on the LOOKUP_DIRECTORY fix as it may prove fragile in future.

Can we at least move the automount/no automount logic out of
follow_automount() and into the path walk callers?  That way
follow_automount() becomes more robust and we're less likely to get bitten in
future as we can define the behaviour more precisely.

David

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Miklos Szeredi]

David Howells <dhowells@redhat.com> writes:

>> Get a grip, people. Stop over-analyzing things. Stop bothering to
>> mention what Solaris does, when the *MUCH* bigger issue is what
>> *Linux* has done for years and years.
>
> So the word is Linux may not change it's behaviour in this regard from what
> existed prior to the d_automount patch, period?

You can change behavior if it won't cause regressions.  So fix userspace
first, introduce change afterwards.

>> Stop saying "we'll revert Miklos patch" in the same sentence where you
>> then seem to not even understand that the *original* behavior was the
>> one that Miklos patch re-introduced.
>
> Miklos's patch did *NOT* re-introduce the original behaviour.  It introduced a
> third state.  It merely moved the regression and brought about a more serious
> one.

Yes, and there have been lots of proposals on how to fix it.  There just
doesn't seem to be a consensus.

I suggest that we first restore the original behavior for all
filesystems.  And it doesn't mean all the d_automount infrastructure has
to be thrown out.  A really simple fix is to pass the lookup flags (or
some derived automount flags) to d_automount and fix up the very few
instances to reflect the old semantics.  Untested patch attached.

Thanks,
Miklos


diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
index 6533807..5fe8124 100644
--- a/Documentation/filesystems/Locking
+++ b/Documentation/filesystems/Locking
@@ -19,7 +19,7 @@ prototypes:
 	void (*d_release)(struct dentry *);
 	void (*d_iput)(struct dentry *, struct inode *);
 	char *(*d_dname)((struct dentry *dentry, char *buffer, int buflen);
-	struct vfsmount *(*d_automount)(struct path *path);
+	struct vfsmount *(*d_automount)(struct path *path, int);
 	int (*d_manage)(struct dentry *, bool);
 
 locking rules:
diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index 52d8fb8..15e6a50 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -894,7 +894,7 @@ struct dentry_operations {
 	void (*d_release)(struct dentry *);
 	void (*d_iput)(struct dentry *, struct inode *);
 	char *(*d_dname)(struct dentry *, char *, int);
-	struct vfsmount *(*d_automount)(struct path *);
+	struct vfsmount *(*d_automount)(struct path *, int);
 	int (*d_manage)(struct dentry *, bool);
 };
 
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index d2b0888..e687963 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -592,7 +592,7 @@ extern const struct inode_operations afs_mntpt_inode_operations;
 extern const struct inode_operations afs_autocell_inode_operations;
 extern const struct file_operations afs_mntpt_file_operations;
 
-extern struct vfsmount *afs_d_automount(struct path *);
+extern struct vfsmount *afs_d_automount(struct path *, int);
 extern int afs_mntpt_check_symlink(struct afs_vnode *, struct key *);
 extern void afs_mntpt_kill_timer(void);
 
diff --git a/fs/afs/mntpt.c b/fs/afs/mntpt.c
index aa59184..aba1c56 100644
--- a/fs/afs/mntpt.c
+++ b/fs/afs/mntpt.c
@@ -238,12 +238,19 @@ error_no_devname:
 /*
  * handle an automount point
  */
-struct vfsmount *afs_d_automount(struct path *path)
+struct vfsmount *afs_d_automount(struct path *path, int lookup_flags)
 {
 	struct vfsmount *newmnt;
 
 	_enter("{%s,%s}", path->mnt->mnt_devname, path->dentry->d_name.name);
 
+	/*
+	 * We don't want to mount if someone's just doing an lstat - unless
+	 * they're stat'ing a directory and appended a '/' to the name.
+	 */
+	if (!(lookup_flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY | LOOKUP_FOLLOW)))
+		return ERR_PTR(-EISDIR);
+
 	newmnt = afs_mntpt_do_automount(path->dentry);
 	if (IS_ERR(newmnt))
 		return newmnt;
diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index f55ae23..5335e08 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -322,7 +322,7 @@ static struct dentry *autofs4_mountpoint_changed(struct path *path)
 	return path->dentry;
 }
 
-static struct vfsmount *autofs4_d_automount(struct path *path)
+static struct vfsmount *autofs4_d_automount(struct path *path, int lookup_flags)
 {
 	struct dentry *dentry = path->dentry;
 	struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
@@ -332,6 +332,22 @@ static struct vfsmount *autofs4_d_automount(struct path *path)
 	DPRINTK("dentry=%p %.*s",
 		dentry, dentry->d_name.len, dentry->d_name.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 (!(lookup_flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY |
+			      LOOKUP_OPEN | LOOKUP_CREATE)) &&
+	    path->dentry->d_inode)
+		return ERR_PTR(-EISDIR);
+
 	/* The daemon never triggers a mount. */
 	if (autofs4_oz_mode(sbi))
 		return NULL;
diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
index 6873bb6..2ad1dcb 100644
--- a/fs/cifs/cifs_dfs_ref.c
+++ b/fs/cifs/cifs_dfs_ref.c
@@ -347,12 +347,19 @@ cdda_exit:
 /*
  * Attempt to automount the referral
  */
-struct vfsmount *cifs_dfs_d_automount(struct path *path)
+struct vfsmount *cifs_dfs_d_automount(struct path *path, int lookup_flags)
 {
 	struct vfsmount *newmnt;
 
 	cFYI(1, "in %s", __func__);
 
+	/*
+	 * We don't want to mount if someone's just doing an lstat - unless
+	 * they're stat'ing a directory and appended a '/' to the name.
+	 */
+	if (!(flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY | LOOKUP_FOLLOW)))
+		return ERR_PTR(-EISDIR);
+
 	newmnt = cifs_dfs_do_automount(path->dentry);
 	if (IS_ERR(newmnt)) {
 		cFYI(1, "leaving %s [automount failed]" , __func__);
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index 95da802..0cd1286 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -101,7 +101,7 @@ extern const struct dentry_operations cifs_dentry_ops;
 extern const struct dentry_operations cifs_ci_dentry_ops;
 
 #ifdef CONFIG_CIFS_DFS_UPCALL
-extern struct vfsmount *cifs_dfs_d_automount(struct path *path);
+extern struct vfsmount *cifs_dfs_d_automount(struct path *path, int lookup_flags);
 #else
 #define cifs_dfs_d_automount NULL
 #endif
diff --git a/fs/namei.c b/fs/namei.c
index f478836..a637893 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -727,27 +727,11 @@ static int follow_automount(struct path *path, unsigned flags,
 	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 -
-	 * 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_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;
 
-	mnt = path->dentry->d_op->d_automount(path);
+	mnt = path->dentry->d_op->d_automount(path, flags);
 	if (IS_ERR(mnt)) {
 		/*
 		 * The filesystem is allowed to return -EISDIR here to indicate
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index ab12913..904779f 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -276,7 +276,7 @@ extern void nfs_sb_deactive(struct super_block *sb);
 /* namespace.c */
 extern char *nfs_path(char **p, struct dentry *dentry,
 		      char *buffer, ssize_t buflen);
-extern struct vfsmount *nfs_d_automount(struct path *path);
+extern struct vfsmount *nfs_d_automount(struct path *path, int lookup_flags);
 #ifdef CONFIG_NFS_V4
 rpc_authflavor_t nfs_find_best_sec(struct nfs4_secinfo_flavors *);
 #endif
diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
index 8102391..0c0ef6a 100644
--- a/fs/nfs/namespace.c
+++ b/fs/nfs/namespace.c
@@ -223,7 +223,7 @@ static inline int nfs_lookup_with_sec(struct nfs_server *server,
  * situation, and that different filesystems may want to use
  * different security flavours.
  */
-struct vfsmount *nfs_d_automount(struct path *path)
+struct vfsmount *nfs_d_automount(struct path *path, int lookup_flags)
 {
 	struct vfsmount *mnt;
 	struct nfs_server *server = NFS_SERVER(path->dentry->d_inode);
@@ -235,6 +235,14 @@ struct vfsmount *nfs_d_automount(struct path *path)
 
 	dprintk("--> nfs_d_automount()\n");
 
+	/*
+	 * We don't want to mount if someone's just doing an lstat - unless
+	 * they're stat'ing a directory and appended a '/' to the name.
+	 */
+	mnt = ERR_PTR(-EISDIR);
+	if (!(lookup_flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY | LOOKUP_FOLLOW)))
+		goto out_nofree;
+
 	mnt = ERR_PTR(-ESTALE);
 	if (IS_ROOT(path->dentry))
 		goto out_nofree;
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 62157c0..deeb5b2 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -167,7 +167,7 @@ struct dentry_operations {
 	void (*d_release)(struct dentry *);
 	void (*d_iput)(struct dentry *, struct inode *);
 	char *(*d_dname)(struct dentry *, char *, int);
-	struct vfsmount *(*d_automount)(struct path *);
+	struct vfsmount *(*d_automount)(struct path *, int);
 	int (*d_manage)(struct dentry *, bool);
 } ____cacheline_aligned;
 

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Ian Kent]

On Fri, 2011-09-23 at 18:10 +0200, Miklos Szeredi wrote:
> David Howells <dhowells@redhat.com> writes:
> 
> >> Get a grip, people. Stop over-analyzing things. Stop bothering to
> >> mention what Solaris does, when the *MUCH* bigger issue is what
> >> *Linux* has done for years and years.
> >
> > So the word is Linux may not change it's behaviour in this regard from what
> > existed prior to the d_automount patch, period?
> 
> You can change behavior if it won't cause regressions.  So fix userspace
> first, introduce change afterwards.

Easy to say, not so easy to do.

> 
> >> Stop saying "we'll revert Miklos patch" in the same sentence where you
> >> then seem to not even understand that the *original* behavior was the
> >> one that Miklos patch re-introduced.
> >
> > Miklos's patch did *NOT* re-introduce the original behaviour.  It introduced a
> > third state.  It merely moved the regression and brought about a more serious
> > one.
> 
> Yes, and there have been lots of proposals on how to fix it.  There just
> doesn't seem to be a consensus.

Indeed, that's probably because of (as Linus says) the empirically
derived division of system calls that do and do not trigger an
automount, for cases where there is a question of whether to automount
or not. The old ill defined moving target.

> 
> I suggest that we first restore the original behavior for all
> filesystems.  And it doesn't mean all the d_automount infrastructure has
> to be thrown out.  A really simple fix is to pass the lookup flags (or
> some derived automount flags) to d_automount and fix up the very few
> instances to reflect the old semantics.  Untested patch attached.

Perhaps, but that allows modules to circumvent VFS policy which is what
allowed this situation to come up in the first place.

Ian

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Fri, Sep 23, 2011 at 6:30 PM, Ian Kent <raven@themaw.net> wrote:
>
> Perhaps, but that allows modules to circumvent VFS policy which is what
> allowed this situation to come up in the first place.

So, realistically, what's the downside of just adding LOOKUP_DIRECTORY
(or LOOKUP_OPEN) to the nfs_follow_remote_path() case?

And if we decide that we really *really* must never bind-mount a
automount point, we could certainly add LOOKUP_OPEN to that case too,
but my gut feel is that's a "doctor, doctor, it hurts when I put a
nail in my eye" kind of case - do we really care?  Is it a sane
operation to do to begin with?

My gut feel is that either of (or both) of
LOOKUP_OPEN/LOOKUP_DIRECTORY is a saner flag to check for than
LOOKUP_FOLLOW ever was. Let's keep LOOKUP_FOLLOW as being the "acts on
symlink or the thing it points to", and nothing else.

                  Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Ian Kent]

On Fri, 2011-09-23 at 18:44 -0700, Linus Torvalds wrote:
> On Fri, Sep 23, 2011 at 6:30 PM, Ian Kent <raven@themaw.net> wrote:
> >
> > Perhaps, but that allows modules to circumvent VFS policy which is what
> > allowed this situation to come up in the first place.
> 
> So, realistically, what's the downside of just adding LOOKUP_DIRECTORY
> (or LOOKUP_OPEN) to the nfs_follow_remote_path() case?

I'm continuing to look at code and think about that but can't give a
decent answer yet.

> 
> And if we decide that we really *really* must never bind-mount a
> automount point, we could certainly add LOOKUP_OPEN to that case too,
> but my gut feel is that's a "doctor, doctor, it hurts when I put a
> nail in my eye" kind of case - do we really care?  Is it a sane
> operation to do to begin with?

I'll get to all the bits I need to look at eventually.

Preventing the bind mounting an automount point is something that I
wanted to be able to do for autofs. That's because there is a dependency
on specific path oriented external information which means a bind
mounting elsewhere is undefined. But I'm not sure yet that holds true
across all automount users.

I'm a bit surprised this has come up actually because I thought it was
possible to do that before and after the vfs-automount changes. So it's
a separate problem and I'd rather not get caught up in trying to resolve
an ongoing pre-existing problem at the same time as trying to work out
how to handle the current situation. 

> 
> My gut feel is that either of (or both) of
> LOOKUP_OPEN/LOOKUP_DIRECTORY is a saner flag to check for than
> LOOKUP_FOLLOW ever was. Let's keep LOOKUP_FOLLOW as being the "acts on
> symlink or the thing it points to", and nothing else.

It is hard to listen to what is being said and change direction when you
have committed heavily to a way of doing things but I'm trying, ;)

That's what I'm looking at doing by using the LOOKUP_DIRECTORY flag, all
be it slowly, because I'm trying to listen to what is being said, think
about it in relation to this, and understand the implications.

Let me continue looking at code and thinking about it further for now.
At the moment I fear that it won't quite cover all the cases, time will
tell.

Ian

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Jeff Layton]

On Fri, 23 Sep 2011 18:44:59 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Sep 23, 2011 at 6:30 PM, Ian Kent <raven@themaw.net> wrote:
> >
> > Perhaps, but that allows modules to circumvent VFS policy which is what
> > allowed this situation to come up in the first place.
> 
> So, realistically, what's the downside of just adding LOOKUP_DIRECTORY
> (or LOOKUP_OPEN) to the nfs_follow_remote_path() case?
> 

There's certainly no danger in adding LOOKUP_DIRECTORY to that call. We
are, after all, looking for a directory there.

The problem is that that does not fully fix the NFS regression. NFS
(and CIFS) has always allowed the automounting of submounts to trigger
on stat() -- this is both before and after the d_automount patches went
in. Miklos' patch changes this.

The problem really boils down to this:

The d_automount patches changed autofs' automount trigger behavior to
be like that of NFS and CIFS. Miklos' patch reverts the behavior of
autofs to pre-2.6.38 behavior, but it also changes NFS and CIFS in the
same way, which is also a regression.

If you want to go back to pre-2.6.38 behavior, then you really have no
choice but to do re-introduce filesystem-specific behavior for
automounting. The behavior of autofs was different from that of
NFS and CIFS in earlier kernels.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Sat, Sep 24, 2011 at 4:36 AM, Jeff Layton <jlayton@redhat.com> wrote:
>
> The problem really boils down to this:
>
> The d_automount patches changed autofs' automount trigger behavior to
> be like that of NFS and CIFS. Miklos' patch reverts the behavior of
> autofs to pre-2.6.38 behavior, but it also changes NFS and CIFS in the
> same way, which is also a regression.

Sure.

> If you want to go back to pre-2.6.38 behavior, then you really have no
> choice but to do re-introduce filesystem-specific behavior for
> automounting. The behavior of autofs was different from that of
> NFS and CIFS in earlier kernels.

I have absolutely no problem with changing semantics in sane ways that
don't cause actual real users to complain.

We tried it the NFS way, and users complained. Let's now try it the autofs way.

And quite frankly, I think the autofs semantics are the clearly
superior semantics, so I have at least some hope that maybe they would
work.

The old NFS semantics were bad. And they existed not for a good
reason, but for a silly technical implementation reason. And that
technical reason has gone away, since now they don't use that "fake
symlink" approach any more.

So the reason I think the autofs semantics are clearly superior are:

 - they don't make the insane distinction between 'lstat' and 'stat'.

   Seriously, no sane program should expect lstat to give different
behavior from stat, unless the lstat information actually *tells* you
that there's something special about the file.

   Now, if the auto-mounting actually have a whole different kind of
file type for an unmounted entry (not necessarily S_IFLNK - I could
well imagine a new implementation just saying "we'll return the new
S_IFAUTO marker"), then using lstat/stat the same way as for symlinks
would make sense. And maybe that would have been a good thing: then
"ls" could show those things nicely as "unmounted automount points".

   But that's not how things work today, and while I think it would be
a valid approach, I suspect everybody here agrees that that would
probably be a lot of work, for very little gain, and quite a lot of
pain.

   Anyway, as long as lstat() returns a S_IFDIR, then there is
absolutely no way for an application to say "oh, but maybe I need to
do 'stat()' or something else like readlink() to actually get some
further information".

   So I seriously claim that the lstat/stat difference is just crazy.
It made sense as a "hey, we hook into this other thing we had, it's a
hack, don't look too closely - it works well enough in practice", but
it doesn't really make sense at any other level in my opinion.

 - They *do* get a difference between "[l]stat()" and "fd = open() ; fstat(fd)".

   Why do I then claim that open+fstat inconsistency is "less bad"
than lstat inconsistency? Am I not being intellectually dishonest?

   And yes, I agree, either approach is inconsistent *somewhere*. You
have to be, since the alternative is to automount every time somebody
does a "ls -l", and we know that doesn't work. But why is "[l]stat ->
open+fstat" inconsistency better than "lstat => stat" inconsistency?

   My argument is that there are two reasons open+fstat is the better
place to be inconsistent:

   1) It's later in the game. Delaying the automount as far as
possible is good. We know it's bad to automount too early: it's
expensive, and we don't want oblivious programs to automount something
unless they really have to. Doing a "stat" on things is pretty common,
and it's why people complained about making autofs match NFS. And we
*can* try to avoid automounting at stat. But if you actually do an
"open", we *have* to automount.

   So there's a very fundamental reason why open+fstat is different
from plain stat: the open really has forced our hand.

   2) People are actually somewhat *used* to [l]stat giving different
information from open+fstat. For *any* file type, not just for
symlinks. It's quite common to do a first "careless" check (using
[l]stat or even just the directory entry type), and then doing a
"careful" check with "open+fstat". Why? Because of all the races with
rename.

   So I actually think people are more likely to react reasonably to
open+fstat inconsistency than to lstat/stat inconsistency. Now, the
proof is in the pudding, but I think there are two independent
conceptual reasons to prefer automounting to happen at that stage.

 - finally: the autofs semantics have been around for a long long time
in Linux. So the autofs semantic choice is the really traditional one.

   I don't think that's a very strong argument, but it's at least an
argument for trying.

Anyway, what this all boils down to is that I'd *really* like to avoid
having semantic differences for different filesystems that really do
the same thing. So I think the autofs semantics are the better
semantics, and we do know that people started complaining when those
semantics were changed to the NFS/cifs semantics.

So my argument (by now much too long and verbose) is that we should
*try* to change the semantics the other way.

Yes, maybe that hits somebody else who has a big nfs automount site,
and really depended on the old semantics. And maybe we do need to then
add a mount option (because quite frankly, I don't think it should
depend on filesystem type: if somebody really prefers one over the
other, it should be possible to do it either way on *either*
filesystem type, no?). But before that, I'd really like to see if we
can get the "consistent semantics at least between filesystems" model
to work.

So that's me trying to outline what my standpoint is. This got much
longer than maybe necessary, but I wanted to avoid ambiguity.

Short summary: yes, maybe we need to do per-mount-point options. But
let's try to avoid them unless we have hard data that says that we
really do need it.

                          Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Ian Kent]

On Sat, 2011-09-24 at 08:56 -0700, Linus Torvalds wrote:
> On Sat, Sep 24, 2011 at 4:36 AM, Jeff Layton <jlayton@redhat.com> wrote:
> >
> > The problem really boils down to this:
> >
> > The d_automount patches changed autofs' automount trigger behavior to
> > be like that of NFS and CIFS. Miklos' patch reverts the behavior of
> > autofs to pre-2.6.38 behavior, but it also changes NFS and CIFS in the
> > same way, which is also a regression.
> 
> Sure.

I wasn't quite ready to comment yet but this post is so close to what
I've been thinking I have to offer my thoughts so far (but still no
patches).

> 
> > If you want to go back to pre-2.6.38 behavior, then you really have no
> > choice but to do re-introduce filesystem-specific behavior for
> > automounting. The behavior of autofs was different from that of
> > NFS and CIFS in earlier kernels.
> 
> I have absolutely no problem with changing semantics in sane ways that
> don't cause actual real users to complain.
> 
> We tried it the NFS way, and users complained. Let's now try it the autofs way.
> 
> And quite frankly, I think the autofs semantics are the clearly
> superior semantics, so I have at least some hope that maybe they would
> work.
> 
> The old NFS semantics were bad. And they existed not for a good
> reason, but for a silly technical implementation reason. And that
> technical reason has gone away, since now they don't use that "fake
> symlink" approach any more.
> 
> So the reason I think the autofs semantics are clearly superior are:
> 
>  - they don't make the insane distinction between 'lstat' and 'stat'.
> 
>    Seriously, no sane program should expect lstat to give different
> behavior from stat, unless the lstat information actually *tells* you
> that there's something special about the file.
> 
>    Now, if the auto-mounting actually have a whole different kind of
> file type for an unmounted entry (not necessarily S_IFLNK - I could
> well imagine a new implementation just saying "we'll return the new
> S_IFAUTO marker"), then using lstat/stat the same way as for symlinks
> would make sense. And maybe that would have been a good thing: then
> "ls" could show those things nicely as "unmounted automount points".

And that's the heart of it.

We added VFS automounting but somehow we managed to retain the "second
class VFS citizen" nature historic with automouning.

That has lead to overloading of LOOKUP_FOLLOW, and now thoughts of
overloading LOOKUP_DIRECTORY and maybe LOOKUP_OPEN. That isn't good IMO
because it may interfere with future changes needed to code surrounding
the use of those flags and may have side effects for current users, such
as LOOKUP_OPEN usage in NFS et.all.

Making automount a first class citizen is a fair bit of work but by
starting the move in that direction now we can resolve some of the
current difficulties.

I'm still looking around but (so far) I think this first step amounts to
little more than changing the flag LOOKUP_NO_AUTOMOUNT to
LOOKUP_AUTOMOUNT and including it as a flag at path walk call sites
where it is required which will make automounting distinct and obvious
to the source reader. That should clear up problems around cases like
quotactl(2) that have been described by Trond. It should also allow the
underlying semantic behavior to be set to what we decide as a starting
policy, probably that of the original autofs.

AFAICS there's really not a lot more to do than changing the automount
usage of LOOKUP_FOLLOW to explicit usage of the LOOKUP_AUTOMOUNT walk
flag.

Continuing in that direction there needs to be further work done to add
an automount file type as Linus describes, but that will also (probably,
from my POV hopefully) involve a semantic change.

TBH I'm still not sure what the underlying VFS policy should be, namely
"don't automount if in doubt" or "always automount and decide not to
later". To get to a place that matches the original autofs behavior the
former is probably easiest but maybe not the best choice (inferred
personal bias here).

One thing is clear, this needs to be agreed and cast in stone (for
better or worse) before further changes are made.

> 
>    But that's not how things work today, and while I think it would be
> a valid approach, I suspect everybody here agrees that that would
> probably be a lot of work, for very little gain, and quite a lot of
> pain.

But maybe not so much work to get us to a place where we can work toward
it and restore the original autofs semantic behavior without using a
mechanism that could end up as problems waiting to happen.

> 
>    Anyway, as long as lstat() returns a S_IFDIR, then there is
> absolutely no way for an application to say "oh, but maybe I need to
> do 'stat()' or something else like readlink() to actually get some
> further information".
> 
>    So I seriously claim that the lstat/stat difference is just crazy.
> It made sense as a "hey, we hook into this other thing we had, it's a
> hack, don't look too closely - it works well enough in practice", but
> it doesn't really make sense at any other level in my opinion.
> 
>  - They *do* get a difference between "[l]stat()" and "fd = open() ; fstat(fd)".
> 
>    Why do I then claim that open+fstat inconsistency is "less bad"
> than lstat inconsistency? Am I not being intellectually dishonest?
> 
>    And yes, I agree, either approach is inconsistent *somewhere*. You
> have to be, since the alternative is to automount every time somebody
> does a "ls -l", and we know that doesn't work. But why is "[l]stat ->
> open+fstat" inconsistency better than "lstat => stat" inconsistency?
> 
>    My argument is that there are two reasons open+fstat is the better
> place to be inconsistent:
> 
>    1) It's later in the game. Delaying the automount as far as
> possible is good. We know it's bad to automount too early: it's
> expensive, and we don't want oblivious programs to automount something
> unless they really have to. Doing a "stat" on things is pretty common,
> and it's why people complained about making autofs match NFS. And we
> *can* try to avoid automounting at stat. But if you actually do an
> "open", we *have* to automount.
> 
>    So there's a very fundamental reason why open+fstat is different
> from plain stat: the open really has forced our hand.
> 
>    2) People are actually somewhat *used* to [l]stat giving different
> information from open+fstat. For *any* file type, not just for
> symlinks. It's quite common to do a first "careless" check (using
> [l]stat or even just the directory entry type), and then doing a
> "careful" check with "open+fstat". Why? Because of all the races with
> rename.
> 
>    So I actually think people are more likely to react reasonably to
> open+fstat inconsistency than to lstat/stat inconsistency. Now, the
> proof is in the pudding, but I think there are two independent
> conceptual reasons to prefer automounting to happen at that stage.
> 
>  - finally: the autofs semantics have been around for a long long time
> in Linux. So the autofs semantic choice is the really traditional one.
> 
>    I don't think that's a very strong argument, but it's at least an
> argument for trying.
> 
> Anyway, what this all boils down to is that I'd *really* like to avoid
> having semantic differences for different filesystems that really do
> the same thing. So I think the autofs semantics are the better
> semantics, and we do know that people started complaining when those
> semantics were changed to the NFS/cifs semantics.
> 
> So my argument (by now much too long and verbose) is that we should
> *try* to change the semantics the other way.

I guess that just says that initial policy should be "don't automount if
in doubt".

> 
> Yes, maybe that hits somebody else who has a big nfs automount site,
> and really depended on the old semantics. And maybe we do need to then
> add a mount option (because quite frankly, I don't think it should
> depend on filesystem type: if somebody really prefers one over the
> other, it should be possible to do it either way on *either*
> filesystem type, no?). But before that, I'd really like to see if we
> can get the "consistent semantics at least between filesystems" model
> to work.

So that would be a no, don't add those super block flags (I was thinking
about) to provide a method for automount users to influence
LOOKUP_AUTOMOUNT behavior.

I agree that either approach, using super block flags (or file system
flags in some way, aka. per-mount) or passing down walk flags is not
desirable but if we want to restore that actual previous semantics we
will have to do one or the other.

For my part I think a couple of super block flags would be the lesser
evil so that either mount pseudo options, module parameters or plain old
internal file system defaults, could be used for those that really need
it to be different.

OTOH, due to the fact we have such controversy, maybe that's a case for
doing this from the outset. Thoughts? Dare I say, can we reach agreement
on it?

Is there some other way it could be done?

Ian

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Sun, Sep 25, 2011 at 10:11 PM, Ian Kent <raven@themaw.net> wrote:
> On Sat, 2011-09-24 at 08:56 -0700, Linus Torvalds wrote:
>>
>>    Now, if the auto-mounting actually have a whole different kind of
>> file type for an unmounted entry (not necessarily S_IFLNK - I could
>> well imagine a new implementation just saying "we'll return the new
>> S_IFAUTO marker"), then using lstat/stat the same way as for symlinks
>> would make sense. And maybe that would have been a good thing: then
>> "ls" could show those things nicely as "unmounted automount points".
>
> And that's the heart of it.
>
> We added VFS automounting but somehow we managed to retain the "second
> class VFS citizen" nature historic with automouning.

Well, realistically, we had to.

And we *still* have to.

There is absolutely no way in hell that we will teach user space about
a new "unmounted automount" inode type.

The is a metric sh*tload of programs that know about and use S_ISDIR()
and brethren. We realistically can't just break them because it would
be nice. The annoyance factor is too high, but more importantly, the
*advantage* is too low.

automounts just simply aren't important enough to warrant it. Most
people aren't aware of them, even if they are on a system where they
are in use. And that's relatively rare to begin with.

So I put it out as a "if this was a new design, and we didn't have any
existing code issues, it *should* possibly have been done that way".
But it was purely theoretical, because whil eI think it would be a
"clean solution", I seriously don't think it's a *realistic* solution
these days any more.

Sure, we could do it with a mount flag and let people try to migrate
if they wanted to, but realistically, simplicity is a bigger win than
"hey, give people the option to do it". Especially since there isn't
any real clamor from actual users for this - it's purely a theoretical
'wouldn't it have been nice if..' argument.

> OTOH, due to the fact we have such controversy, maybe that's a case for
> doing this from the outset. Thoughts? Dare I say, can we reach agreement
> on it?

Seriously, I have seen absolutely *zero* arguments for just trying to
go with the "keep it simple, stupid" approach.

What are the downsides of just adding the one or two
LOOKUP_DIRECTORY/LOOKUP_OPEN flags?

Nobody has even *mentioned* any downsides. So I don't think this is
controversial, and I don't understand why you consider it
controversial.

                     Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Trond Myklebust]

On Mon, 2011-09-26 at 13:48 -0700, Linus Torvalds wrote: 
> What are the downsides of just adding the one or two
> LOOKUP_DIRECTORY/LOOKUP_OPEN flags?
> 
> Nobody has even *mentioned* any downsides. So I don't think this is
> controversial, and I don't understand why you consider it
> controversial.

LOOKUP_OPEN flags an open intent and will therefore have side-effects on
NFSv4.

Cheers
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Mon, Sep 26, 2011 at 2:13 PM, Trond Myklebust
<Trond.Myklebust@netapp.com> wrote:
>
> LOOKUP_OPEN flags an open intent and will therefore have side-effects on
> NFSv4.

Yes, and the actual downside for that is exactly what?

If you are talking about the bind mount issue, then what is the
downside with saying "we want the same stronger consistency guarantees
as for a 'file open' operation"?

Especially since we know that the side effects of the LOOKUP_FOLLOW
approach are MUCH BIGGER, and have already caused real complaints from
actual real users.

In comparison with those real downsides, why the heck are people
complaining about LOOKUP_DIRECTORY or LOOKUP_OPEN?

Seriously.

Let me repeat: what are the ACTUAL REAL DOWNSIDES?

               Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Trond Myklebust]

On Mon, 2011-09-26 at 14:24 -0700, Linus Torvalds wrote: 
> On Mon, Sep 26, 2011 at 2:13 PM, Trond Myklebust
> <Trond.Myklebust@netapp.com> wrote:
> >
> > LOOKUP_OPEN flags an open intent and will therefore have side-effects on
> > NFSv4.
> 
> Yes, and the actual downside for that is exactly what?
> 
> If you are talking about the bind mount issue, then what is the
> downside with saying "we want the same stronger consistency guarantees
> as for a 'file open' operation"?

A bind mount doesn't actually want us to set up OPEN state on the NFSv4
server and return a fully initialised 'struct file' in the struct
nameidata. If it did, we would already have set the LOOKUP_OPEN flag
together with the accompanying open intent structure in the struct
nameidata.

Ditto for the quotactl().

Cheers
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Mon, Sep 26, 2011 at 2:31 PM, Trond Myklebust
<Trond.Myklebust@netapp.com> wrote:
>
> A bind mount doesn't actually want us to set up OPEN state on the NFSv4
> server and return a fully initialised 'struct file' in the struct
> nameidata. If it did, we would already have set the LOOKUP_OPEN flag
> together with the accompanying open intent structure in the struct
> nameidata.
>
> Ditto for the quotactl().

Umm. As far as I can see, adding LOOKUP_OPEN doesn't actually even
*do* what you claim.

The whole "allocate struct file in struct nameidata" happens in
path_openat(), it doesn't happen in the normal path creation
(do_path_lookup() and friends). The only thing NFS does with
LOOKUP_OPEN is to check whether it needs to do a revalidate
regardless.

But even more importantly than your apparent confusion about filling
in the 'nameidata.intent' fields, even *if* we were to do unnecessary
filp allocations etc (which we don't do, as mentioned), from a user
standpoint, what would be the actual downside?

It would be extra work for a (very rare) operation, but compared to
the known downside that is LOOKUP_FOLLOW and auto-mounting when we
shouldn't, even that hypothetical downside seems pretty benign,
doesn't it?

In other words, what I don't understand is how you seem to be totally
happy ignoring the known problems with LOOKUP_FOLLOW, but LOOKUP_OPEN
is some monster thing that you don't want to deal with.

WHY?

                        Linus

PS. That said, looking at the *cifs* code, it seems to do some nasty
things on LOOKUP_OPEN, and assumes that it always has a 'file'
pointer. Sad. That might actually argue for another flag, or
alternatively for just fixing CIFS.

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Trond Myklebust]

On Mon, 2011-09-26 at 15:24 -0700, Linus Torvalds wrote: 
> On Mon, Sep 26, 2011 at 2:31 PM, Trond Myklebust
> <Trond.Myklebust@netapp.com> wrote:
> >
> > A bind mount doesn't actually want us to set up OPEN state on the NFSv4
> > server and return a fully initialised 'struct file' in the struct
> > nameidata. If it did, we would already have set the LOOKUP_OPEN flag
> > together with the accompanying open intent structure in the struct
> > nameidata.
> >
> > Ditto for the quotactl().
> 
> Umm. As far as I can see, adding LOOKUP_OPEN doesn't actually even
> *do* what you claim.
> 
> The whole "allocate struct file in struct nameidata" happens in
> path_openat(), it doesn't happen in the normal path creation
> (do_path_lookup() and friends). The only thing NFS does with
> LOOKUP_OPEN is to check whether it needs to do a revalidate
> regardless.

Please see the 'is_atomic_open()' helper and the way that
nfs_atomic_lookup() will substitute an OPEN call for the usual LOOKUP if
it sees that the caller is trying to open a file that might potentially
be a regular file.

> But even more importantly than your apparent confusion about filling
> in the 'nameidata.intent' fields, even *if* we were to do unnecessary
> filp allocations etc (which we don't do, as mentioned), from a user
> standpoint, what would be the actual downside?

Lookup permission checks would be replaced with open permission checks
on the server.

IOW: the operation could potentially fail due to a completely unrelated
issue.

Cheers
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

On Mon, Sep 26, 2011 at 3:33 PM, Trond Myklebust
<Trond.Myklebust@netapp.com> wrote:
>
> Lookup permission checks would be replaced with open permission checks
> on the server.
>
> IOW: the operation could potentially fail due to a completely unrelated
> issue.

Quite frankly, that still sounds like a "I'm trying to make a problem
out of something that isn't actually a problem". And you still seem to
be unwilling to admit that LOOKUP_FOLLOW is a problem that has
actually been reported in real life, so you just cut out that part of
my question about how this would be a bigger issue.

But whatever. I can't really care, since it's a two-liner to add a new
flag, and then it falls down to "if you want to follow automounts, you
can set that flag instead".

Almost nobody is ever going to bother setting the flag anyway, since
LOOKUP_OPEN and LOOKUP_DIRECTORY are going to take care of all the
common cases.

So here. You can set LOOKUP_AUTOMOUNT to force an automount traversal. Ok?

                       Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 984 bytes --]

 fs/namei.c            |    2 +-
 include/linux/namei.h |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index f4788365ea22..09606fd83d57 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -739,7 +739,7 @@ static int follow_automount(struct path *path, unsigned flags,
 	 * of the daemon to instantiate them before they can be used.
 	 */
 	if (!(flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY |
-		     LOOKUP_OPEN | LOOKUP_CREATE)) &&
+		     LOOKUP_OPEN | LOOKUP_CREATE | LOOKUP_AUTOMOUNT)) &&
 	    path->dentry->d_inode)
 		return -EISDIR;
 
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 76fe2c62ae71..e13dac7caab2 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -48,6 +48,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
  */
 #define LOOKUP_FOLLOW		0x0001
 #define LOOKUP_DIRECTORY	0x0002
+#define LOOKUP_AUTOMOUNT	0x0004
 
 #define LOOKUP_PARENT		0x0010
 #define LOOKUP_REVAL		0x0020

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Trond Myklebust]

On Mon, 2011-09-26 at 15:56 -0700, Linus Torvalds wrote: 
> On Mon, Sep 26, 2011 at 3:33 PM, Trond Myklebust
> <Trond.Myklebust@netapp.com> wrote:
> >
> > Lookup permission checks would be replaced with open permission checks
> > on the server.
> >
> > IOW: the operation could potentially fail due to a completely unrelated
> > issue.
> 
> Quite frankly, that still sounds like a "I'm trying to make a problem
> out of something that isn't actually a problem". And you still seem to
> be unwilling to admit that LOOKUP_FOLLOW is a problem that has
> actually been reported in real life, so you just cut out that part of
> my question about how this would be a bigger issue.

I never questioned your argument that we need to address the problem of
regressions. That's why I cut out the LOOKUP_FOLLOW part.

I just questioned your proposed solution because it seemed to introduce
new problems. We're in the 3rd iteration of a bugfix (the first being
the introduction of ->d_automount() and the second being Miklos's
patch), and I'm assuming nobody is interested in seeing a 4th iteration.

> But whatever. I can't really care, since it's a two-liner to add a new
> flag, and then it falls down to "if you want to follow automounts, you
> can set that flag instead".
> 
> Almost nobody is ever going to bother setting the flag anyway, since
> LOOKUP_OPEN and LOOKUP_DIRECTORY are going to take care of all the
> common cases.
> 
> So here. You can set LOOKUP_AUTOMOUNT to force an automount traversal. Ok?

I assume that means that we can get rid of LOOKUP_NO_AUTOMOUNT, and just
replace the convoluted logic in follow_automount() with a test for
LOOKUP_AUTOMOUNT? If we have agreed on a default behaviour, then that
would seem cleaner than enumerating all these exceptions.

Cheers
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Mon, Sep 26, 2011 at 4:09 PM, Trond Myklebust
<Trond.Myklebust@netapp.com> wrote:
>
> I assume that means that we can get rid of LOOKUP_NO_AUTOMOUNT, and just
> replace the convoluted logic in follow_automount() with a test for
> LOOKUP_AUTOMOUNT? If we have agreed on a default behaviour, then that
> would seem cleaner than enumerating all these exceptions.

I do think that LOOKUP_NO_AUTOMOUNT is bogus.  It's not a
kernel-internal flag, it's for that silly "vfs_fstatat()" interface,
that was designed when LOOKUP_FOLLOW would cause an automount.

Now that LOOKUP_FOLLOW doesn't do that, the whole reason for
LOOKUP_NO_AUTOMOUNT really goes away, but then we have that crazy user
interface that was encoded for it (only for the very special vfs

The question then becomes: should we just ignore (and deprecate) that
crazy AT_NO_AUTOMOUNT flag? I say "yup".

Or should we do something *really* crazy, and say "if you *don't* have
AT_SYMLINK_NOFOLLOW *and* you don't have AT_NO_AUTOMOUNT, then we make
fstatat64() follow automounts, even though no sane version of *stat()
does so any more"?

Quite frankly, that just sounds insane. But we could do it, and it
would approximate the old semantics for that system call. Even if
those old semantics were clearly not very good, and the whole point of
AT_NO_AUTOMOUNT was to work around problems people must have had..

So my gut feel is that we should just ignore it for now.
AT_NO_AUTOMOUNT becomes a no-op, and the LOOKUP_NO_AUTOMOUNT flag is
already effectively one (since nothing but vfs_fstatat sets it, and
vfs_fstatat now doesn't follow automounts *anyway*)

The "let's just see if anybody complains when we clean stuff up"
approach, in other words.  Because I bet any user who doesn't have
AT_NO_AUTOMOUNT isn't because they didn't really want an automount,
it's because they weren't even aware of the issue.

                        Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Trond Myklebust]

On Mon, 2011-09-26 at 16:26 -0700, Linus Torvalds wrote: 
> On Mon, Sep 26, 2011 at 4:09 PM, Trond Myklebust
> <Trond.Myklebust@netapp.com> wrote:
> >
> > I assume that means that we can get rid of LOOKUP_NO_AUTOMOUNT, and just
> > replace the convoluted logic in follow_automount() with a test for
> > LOOKUP_AUTOMOUNT? If we have agreed on a default behaviour, then that
> > would seem cleaner than enumerating all these exceptions.
> 
> I do think that LOOKUP_NO_AUTOMOUNT is bogus.  It's not a
> kernel-internal flag, it's for that silly "vfs_fstatat()" interface,
> that was designed when LOOKUP_FOLLOW would cause an automount.
> 
> Now that LOOKUP_FOLLOW doesn't do that, the whole reason for
> LOOKUP_NO_AUTOMOUNT really goes away, but then we have that crazy user
> interface that was encoded for it (only for the very special vfs
> 
> The question then becomes: should we just ignore (and deprecate) that
> crazy AT_NO_AUTOMOUNT flag? I say "yup".
> 
> Or should we do something *really* crazy, and say "if you *don't* have
> AT_SYMLINK_NOFOLLOW *and* you don't have AT_NO_AUTOMOUNT, then we make
> fstatat64() follow automounts, even though no sane version of *stat()
> does so any more"?
> 
> Quite frankly, that just sounds insane. But we could do it, and it
> would approximate the old semantics for that system call. Even if
> those old semantics were clearly not very good, and the whole point of
> AT_NO_AUTOMOUNT was to work around problems people must have had..
> 
> So my gut feel is that we should just ignore it for now.
> AT_NO_AUTOMOUNT becomes a no-op, and the LOOKUP_NO_AUTOMOUNT flag is
> already effectively one (since nothing but vfs_fstatat sets it, and
> vfs_fstatat now doesn't follow automounts *anyway*)
> 
> The "let's just see if anybody complains when we clean stuff up"
> approach, in other words.  Because I bet any user who doesn't have
> AT_NO_AUTOMOUNT isn't because they didn't really want an automount,
> it's because they weren't even aware of the issue.

OK. Then how about something along the lines of the following patch
(compile tested only)?

Cheers
  Trond

8<----------------------------------------------------------------------------------

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Mon, Sep 26, 2011 at 5:59 PM, Trond Myklebust
<Trond.Myklebust@netapp.com> wrote:
>
> OK. Then how about something along the lines of the following patch
> (compile tested only)?

I would seriously suggest aiming for *minimal*.

So if it already has any of LOOKUP_DIRECTORY, LOOKUP_OPEN,
LOOKUP_CREATE or LOOKUP_PARENT, don't add LOOKUP_AUTOMOUNT - because
it's not adding anything.

We *have* to automount on those flags anyway. They are not like
LOOKUP_FOLLOW was, where the automount decision was just a random
(bad) choice.

>From a quick look, pretty much all of the cases you added
LOOKUP_AUTOMOUNT to were already implicit automount events. So I think
the two remaining ones are quotactl Q_QUOTAON case, bind mount in
do_looopback(), and the nfs_follow_remote_path() case.

Sure, we could add it to the others as some kind of "documentation"
thing, but I think that should be separate and is debatable to begin
with.

                        Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Ian Kent]

On Mon, 2011-09-26 at 18:18 -0700, Linus Torvalds wrote:
> On Mon, Sep 26, 2011 at 5:59 PM, Trond Myklebust
> <Trond.Myklebust@netapp.com> wrote:
> >
> > OK. Then how about something along the lines of the following patch
> > (compile tested only)?
> 
> I would seriously suggest aiming for *minimal*.
> 
> So if it already has any of LOOKUP_DIRECTORY, LOOKUP_OPEN,
> LOOKUP_CREATE or LOOKUP_PARENT, don't add LOOKUP_AUTOMOUNT - because
> it's not adding anything.
> 
> We *have* to automount on those flags anyway. They are not like
> LOOKUP_FOLLOW was, where the automount decision was just a random
> (bad) choice.
> 
> From a quick look, pretty much all of the cases you added
> LOOKUP_AUTOMOUNT to were already implicit automount events. So I think
> the two remaining ones are quotactl Q_QUOTAON case, bind mount in
> do_looopback(), and the nfs_follow_remote_path() case.
> 
> Sure, we could add it to the others as some kind of "documentation"
> thing, but I think that should be separate and is debatable to begin
> with.

That was the only question I had when I suggested using LOOKUP_AUTOMOUNT
to restore the previous automounting behavior. The trade off being to
add the flag and simplify the check in follow_automount() or to not add
it to implicit automount cases and retain the check but minimize the
changes to path walk call sites.

Talk about an automount file type was purely a suggestion of something
that would be good to do later along with what would be a lengthy user
space co-ordination and education process, not something I thought was
good to do now.

Making allowances for the change back to the old behavior for those
kernel users that had different semantics at the outset was a second and
separate question and still is.

Ian

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Mon, Sep 26, 2011 at 5:59 PM, Trond Myklebust
<Trond.Myklebust@netapp.com> wrote:
> diff --git a/fs/namei.c b/fs/namei.c
> index f478836..5b1608f 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -724,25 +724,9 @@ static int follow_automount(struct path *path, unsigned flags,
>        /* We don't want to mount if someone supplied AT_NO_AUTOMOUNT
>         * and this is the terminal part of the path.
>         */
> -       if ((flags & LOOKUP_NO_AUTOMOUNT) && !(flags & LOOKUP_PARENT))
> +       if (!(flags & LOOKUP_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 -
> -        * 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_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;

So I think that dropping the d_inode test is wrong, even if we were to
at some point decide that we want explicit LOOKUP_AUTOMOUNT
everywhere.

Of course, it may be that the comment is wrong. But assuming the
comment is correct, then it's really the test above that is
fundamentally wrong.

Ian may know the answer. Ian?

Regardless, I applied the minimal and obviously safe part of the
patch, so hopefully we have no actual regressions left in this area,
and it's just a discussion about possible future cleanups.

                    Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Ian Kent]

On Mon, 2011-09-26 at 20:57 -0700, Linus Torvalds wrote:
> On Mon, Sep 26, 2011 at 5:59 PM, Trond Myklebust
> <Trond.Myklebust@netapp.com> wrote:
> > diff --git a/fs/namei.c b/fs/namei.c
> > index f478836..5b1608f 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -724,25 +724,9 @@ static int follow_automount(struct path *path, unsigned flags,
> >        /* We don't want to mount if someone supplied AT_NO_AUTOMOUNT
> >         * and this is the terminal part of the path.
> >         */
> > -       if ((flags & LOOKUP_NO_AUTOMOUNT) && !(flags & LOOKUP_PARENT))
> > +       if (!(flags & LOOKUP_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 -
> > -        * 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_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;
> 
> So I think that dropping the d_inode test is wrong, even if we were to
> at some point decide that we want explicit LOOKUP_AUTOMOUNT
> everywhere.
> 
> Of course, it may be that the comment is wrong. But assuming the
> comment is correct, then it's really the test above that is
> fundamentally wrong.
> 
> Ian may know the answer. Ian?

Yes, I saw that but got side tracked by the email exchanges of last
night.

This case is the one were there is no existing directory yet for the
automount (it's the age old original automount case where mount point
directories never existed within the automount managed directory before
being automounted). In this case we always want to automount regardless
of the lookup flags. So returning -EISDIR should be conditional on also
having a positive dentry.

Ian

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1513 bytes --]

On Mon, Sep 26, 2011 at 9:16 PM, Ian Kent <raven@themaw.net> wrote:
>
> This case is the one were there is no existing directory yet for the
> automount (it's the age old original automount case where mount point
> directories never existed within the automount managed directory before
> being automounted). In this case we always want to automount regardless
> of the lookup flags. So returning -EISDIR should be conditional on also
> having a positive dentry.

Ok. This all just makes me more conviced that the LOOKUP_NO_AUTOMOUNT
code is not even worth trying to salvage (ie not play any games with
the existing AT_NO_AUTOMOUNT flag and perhaps turning
!AT_SYMLINK_NOFOLLOW + !AT_NOAUTOMOUNT -> LOOKUP_AUTOMOUNT). That's
the case that never tested for d_inode being NULL, so it got the
autofs case wrong.

At some point we might want to expose an AT_AUTOMOUNT flag to user
space _if_ it turns out that glibc or somebody wants really wants it,
but the old flag clearly actually had a buggy implementation and
almost certainly isn't worth worrying about.

Of course, we'll have to see if there are any actual regression
reports in this area.. If something actually breaks, we may not have
all that many choices. But at least tentatively, I think we can plan
on applying the attached cleanup to just rip that out (it's already
effectively dead code apart from the buggy lack of d_inode testing,
since apart from d_inode, it never triggers unless the subsequent test
triggers)..

                           Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1606 bytes --]

 fs/namei.c            |    6 ------
 fs/stat.c             |    2 --
 include/linux/namei.h |    2 +-
 3 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 09606fd83d57..0b3138de2a3b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -721,12 +721,6 @@ static int follow_automount(struct path *path, unsigned flags,
 	if (!path->dentry->d_op || !path->dentry->d_op->d_automount)
 		return -EREMOTE;
 
-	/* We don't want to mount if someone supplied AT_NO_AUTOMOUNT
-	 * and this is the terminal part of the 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 -
 	 * unless they're stat'ing a directory and appended a '/' to
 	 * the name.
diff --git a/fs/stat.c b/fs/stat.c
index ba5316ffac61..78a3aa83c7ea 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -81,8 +81,6 @@ int vfs_fstatat(int dfd, const char __user *filename, struct kstat *stat,
 
 	if (!(flag & AT_SYMLINK_NOFOLLOW))
 		lookup_flags |= LOOKUP_FOLLOW;
-	if (flag & AT_NO_AUTOMOUNT)
-		lookup_flags |= LOOKUP_NO_AUTOMOUNT;
 	if (flag & AT_EMPTY_PATH)
 		lookup_flags |= LOOKUP_EMPTY;
 
diff --git a/include/linux/namei.h b/include/linux/namei.h
index e13dac7caab2..409328d1cbbb 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -53,7 +53,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
 #define LOOKUP_PARENT		0x0010
 #define LOOKUP_REVAL		0x0020
 #define LOOKUP_RCU		0x0040
-#define LOOKUP_NO_AUTOMOUNT	0x0080
+
 /*
  * Intent data
  */

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Ian Kent]

On Mon, 2011-09-26 at 21:35 -0700, Linus Torvalds wrote:
> On Mon, Sep 26, 2011 at 9:16 PM, Ian Kent <raven@themaw.net> wrote:
> >
> > This case is the one were there is no existing directory yet for the
> > automount (it's the age old original automount case where mount point
> > directories never existed within the automount managed directory before
> > being automounted). In this case we always want to automount regardless
> > of the lookup flags. So returning -EISDIR should be conditional on also
> > having a positive dentry.
> 
> Ok. This all just makes me more conviced that the LOOKUP_NO_AUTOMOUNT
> code is not even worth trying to salvage (ie not play any games with
> the existing AT_NO_AUTOMOUNT flag and perhaps turning
> !AT_SYMLINK_NOFOLLOW + !AT_NOAUTOMOUNT -> LOOKUP_AUTOMOUNT). That's
> the case that never tested for d_inode being NULL, so it got the
> autofs case wrong.

Agreed.

> 
> At some point we might want to expose an AT_AUTOMOUNT flag to user
> space _if_ it turns out that glibc or somebody wants really wants it,
> but the old flag clearly actually had a buggy implementation and
> almost certainly isn't worth worrying about.

Yes, I also thought that AT_NO_AUTOMOUNT would need to be removed and
AT_AUTOMOUNT added as part of the change. But that can be done as a
follow up.

> 
> Of course, we'll have to see if there are any actual regression
> reports in this area.. If something actually breaks, we may not have
> all that many choices. But at least tentatively, I think we can plan
> on applying the attached cleanup to just rip that out (it's already
> effectively dead code apart from the buggy lack of d_inode testing,
> since apart from d_inode, it never triggers unless the subsequent test
> triggers)..

Is there a source tree somewhere, I'm not sure now exactly what has been
changed. I can run tests if I can get accurate source.

Ian

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Mon, Sep 26, 2011 at 9:51 PM, Ian Kent <raven@themaw.net> wrote:
>
> Is there a source tree somewhere, I'm not sure now exactly what has been
> changed. I can run tests if I can get accurate source.

It's all at git://github.com/torvalds/linux.

I've only applied two patches so far, though: the "Add
LOOKUP_AUTOMOUNT flag" (that didn't actually set it anywhere), and
then the minimal version of Trond's patch.

And I see that I have a few patches from Trond in my mailbox, but I
haven't looked at them yet.

                       Linus

RE: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Myklebust, Trond]

> -----Original Message-----
> From: Linus Torvalds [mailto:torvalds@linux-foundation.org]
> Sent: Tuesday, September 27, 2011 10:32 AM
> To: Ian Kent
> Cc: Myklebust, Trond; Jeff Layton; Miklos Szeredi; David Howells;
> viro@zeniv.linux.org.uk; gregkh@suse.de; linux-nfs@vger.kernel.org;
> leonardo.lists@gmail.com
> Subject: Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr,
etc.
> 
> On Mon, Sep 26, 2011 at 9:51 PM, Ian Kent <raven@themaw.net> wrote:
> >
> > Is there a source tree somewhere, I'm not sure now exactly what has
> > been changed. I can run tests if I can get accurate source.
> 
> It's all at git://github.com/torvalds/linux.
> 
> I've only applied two patches so far, though: the "Add
> LOOKUP_AUTOMOUNT flag" (that didn't actually set it anywhere), and
then
> the minimal version of Trond's patch.
> 
> And I see that I have a few patches from Trond in my mailbox, but I
haven't
> looked at them yet.

I should have checked the github repository before posting.
The stuff you've already committed appears to obsolete the 1/2 patch,
but it might still be worth your while looking at 2/2: that documents
which lookups we expect might result in an automount of the last
component (but contains no actual functional changes).

Cheers
  Trond

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Ian Kent]

On Tue, 2011-09-27 at 07:32 -0700, Linus Torvalds wrote:
> On Mon, Sep 26, 2011 at 9:51 PM, Ian Kent <raven@themaw.net> wrote:
> >
> > Is there a source tree somewhere, I'm not sure now exactly what has been
> > changed. I can run tests if I can get accurate source.
> 
> It's all at git://github.com/torvalds/linux.
> 
> I've only applied two patches so far, though: the "Add
> LOOKUP_AUTOMOUNT flag" (that didn't actually set it anywhere), and
> then the minimal version of Trond's patch.
> 
> And I see that I have a few patches from Trond in my mailbox, but I
> haven't looked at them yet.

I cloned the repo and it looks like everything was present, three recent
patches in all.

Built and ran my tests without any kernel issue.

The tests I run are limited in scope because they essentially use
opendir(3) and check mtab or /proc/mounts for mount and fs type.

Although I do plenty of NFS mount and umount I don't have anything that
tests NFS automounts.

Ian

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Mon, Sep 26, 2011 at 9:51 PM, Ian Kent <raven@themaw.net> wrote:
>
> Yes, I also thought that AT_NO_AUTOMOUNT would need to be removed and
> AT_AUTOMOUNT added as part of the change. But that can be done as a
> follow up.

In fact, since reasonably automounting will always be an issue only
for directories (yeah, in theory you could extend it to regular files,
but why would you?) it's probably not even worth adding AT_AUTOMOUNT
at all - you can do the same thing by just adding a '/' at the end of
the filename instead, and suddenly it works for *all* the stat()
family system calls, not just the odd statat() ones that almost nobody
uses.

So it would be easy to add the AT_AUTOMOUNT flag, but whoever argues
for it would have to be pretty convincing as to why it would be worth
it.

I committed the patch I sent out with your and Trond's acks.

              Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Greg KH]

On Tue, Sep 27, 2011 at 08:22:24AM -0700, Linus Torvalds wrote:
> On Mon, Sep 26, 2011 at 9:51 PM, Ian Kent <raven@themaw.net> wrote:
> >
> > Yes, I also thought that AT_NO_AUTOMOUNT would need to be removed and
> > AT_AUTOMOUNT added as part of the change. But that can be done as a
> > follow up.
> 
> In fact, since reasonably automounting will always be an issue only
> for directories (yeah, in theory you could extend it to regular files,
> but why would you?) it's probably not even worth adding AT_AUTOMOUNT
> at all - you can do the same thing by just adding a '/' at the end of
> the filename instead, and suddenly it works for *all* the stat()
> family system calls, not just the odd statat() ones that almost nobody
> uses.
> 
> So it would be easy to add the AT_AUTOMOUNT flag, but whoever argues
> for it would have to be pretty convincing as to why it would be worth
> it.
> 
> I committed the patch I sent out with your and Trond's acks.

Does any of this make any sense for the 3.0-stable tree as well?

greg k-h

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Linus Torvalds]

On Tue, Sep 27, 2011 at 10:38 AM, Greg KH <gregkh@suse.de> wrote:
>
> Does any of this make any sense for the 3.0-stable tree as well?

My gut feel is that it's not important enough to worry about too much,
and we should wait to see that nobody complains about the 3.1
behavior.

                  Linus

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Greg KH]

On Tue, Sep 27, 2011 at 10:51:34AM -0700, Linus Torvalds wrote:
> On Tue, Sep 27, 2011 at 10:38 AM, Greg KH <gregkh@suse.de> wrote:
> >
> > Does any of this make any sense for the 3.0-stable tree as well?
> 
> My gut feel is that it's not important enough to worry about too much,
> and we should wait to see that nobody complains about the 3.1
> behavior.

Ok, thanks, will do.

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Miklos Szeredi]

Greg KH <gregkh@suse.de> writes:

> On Tue, Sep 27, 2011 at 10:51:34AM -0700, Linus Torvalds wrote:
>> On Tue, Sep 27, 2011 at 10:38 AM, Greg KH <gregkh@suse.de> wrote:
>> >
>> > Does any of this make any sense for the 3.0-stable tree as well?
>> 
>> My gut feel is that it's not important enough to worry about too much,
>> and we should wait to see that nobody complains about the 3.1
>> behavior.
>
> Ok, thanks, will do.

In that case you should back out

 vfs-automount-should-ignore-lookup_follow.patch

too, otherwise there is strange behavior with NFSv4.

Thanks,
Miklos

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[Greg KH]

On Tue, Sep 27, 2011 at 10:18:11PM +0200, Miklos Szeredi wrote:
> Greg KH <gregkh@suse.de> writes:
> 
> > On Tue, Sep 27, 2011 at 10:51:34AM -0700, Linus Torvalds wrote:
> >> On Tue, Sep 27, 2011 at 10:38 AM, Greg KH <gregkh@suse.de> wrote:
> >> >
> >> > Does any of this make any sense for the 3.0-stable tree as well?
> >> 
> >> My gut feel is that it's not important enough to worry about too much,
> >> and we should wait to see that nobody complains about the 3.1
> >> behavior.
> >
> > Ok, thanks, will do.
> 
> In that case you should back out
> 
>  vfs-automount-should-ignore-lookup_follow.patch
> 
> too, otherwise there is strange behavior with NFSv4.

Good point, I've now dropped that one, thanks.

greg k-h

[PATCH 1/2] VFS: Fix the remaining automounter semantics regressions

[Trond Myklebust]

The concensus seems to be that system calls such as stat() etc should not
trigger an automount. Neither should the l* versions.

This patch therefore adds a LOOKUP_AUTOMOUNT flag to tag those lookups that
don't currently trigger an automount on the last path element but that
should.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
 fs/namei.c            |    8 +-------
 fs/namespace.c        |    2 +-
 fs/nfs/super.c        |    4 ++--
 fs/quota/quota.c      |    3 ++-
 fs/stat.c             |    2 --
 include/linux/namei.h |    2 +-
 6 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index f478836..c076492 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -721,12 +721,6 @@ static int follow_automount(struct path *path, unsigned flags,
 	if (!path->dentry->d_op || !path->dentry->d_op->d_automount)
 		return -EREMOTE;
 
-	/* We don't want to mount if someone supplied AT_NO_AUTOMOUNT
-	 * and this is the terminal part of the 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 -
 	 * unless they're stat'ing a directory and appended a '/' to
 	 * the name.
@@ -738,7 +732,7 @@ static int follow_automount(struct path *path, unsigned flags,
 	 * 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 |
+	if (!(flags & (LOOKUP_AUTOMOUNT | LOOKUP_PARENT | LOOKUP_DIRECTORY |
 		     LOOKUP_OPEN | LOOKUP_CREATE)) &&
 	    path->dentry->d_inode)
 		return -EISDIR;
diff --git a/fs/namespace.c b/fs/namespace.c
index 22bfe82..b4febb2 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1757,7 +1757,7 @@ static int do_loopback(struct path *path, char *old_name,
 		return err;
 	if (!old_name || !*old_name)
 		return -EINVAL;
-	err = kern_path(old_name, LOOKUP_FOLLOW, &old_path);
+	err = kern_path(old_name, LOOKUP_FOLLOW|LOOKUP_AUTOMOUNT, &old_path);
 	if (err)
 		return err;
 
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 9b7dd70..8aa6968 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -2797,8 +2797,8 @@ static struct dentry *nfs_follow_remote_path(struct vfsmount *root_mnt,
 	if (ret != 0)
 		goto out_put_mnt_ns;
 
-	ret = vfs_path_lookup(root_mnt->mnt_root, root_mnt,
-			export_path, LOOKUP_FOLLOW, &path);
+	ret = vfs_path_lookup(root_mnt->mnt_root, root_mnt, export_path,
+			LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path);
 
 	nfs_referral_loop_unprotect();
 	put_mnt_ns(ns_private);
diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index b34bdb2..9ea9fce 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -355,7 +355,8 @@ SYSCALL_DEFINE4(quotactl, unsigned int, cmd, const char __user *, special,
 	 * resolution (think about autofs) and thus deadlocks could arise.
 	 */
 	if (cmds == Q_QUOTAON) {
-		ret = user_path_at(AT_FDCWD, addr, LOOKUP_FOLLOW, &path);
+		ret = user_path_at(AT_FDCWD, addr, LOOKUP_FOLLOW |
+				LOOKUP_AUTOMOUNT, &path);
 		if (ret)
 			pathp = ERR_PTR(ret);
 		else
diff --git a/fs/stat.c b/fs/stat.c
index ba5316f..78a3aa8 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -81,8 +81,6 @@ int vfs_fstatat(int dfd, const char __user *filename, struct kstat *stat,
 
 	if (!(flag & AT_SYMLINK_NOFOLLOW))
 		lookup_flags |= LOOKUP_FOLLOW;
-	if (flag & AT_NO_AUTOMOUNT)
-		lookup_flags |= LOOKUP_NO_AUTOMOUNT;
 	if (flag & AT_EMPTY_PATH)
 		lookup_flags |= LOOKUP_EMPTY;
 
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 76fe2c6..bfcd2e0 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -52,7 +52,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
 #define LOOKUP_PARENT		0x0010
 #define LOOKUP_REVAL		0x0020
 #define LOOKUP_RCU		0x0040
-#define LOOKUP_NO_AUTOMOUNT	0x0080
+#define LOOKUP_AUTOMOUNT	0x0080
 /*
  * Intent data
  */
-- 
1.7.6.2

[PATCH 2/2] VFS: Document automounter semantics

[Trond Myklebust]

Add LOOKUP_AUTOMOUNT flags to all the lookups that _should_ trigger
an automount on the last path element.
IOW: Set it for all cases where we currently have the LOOKUP_OPEN,
LOOKUP_DIRECTORY or LOOKUP_CREATE flags set.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
 fs/cachefiles/bind.c               |    3 ++-
 fs/configfs/symlink.c              |    3 ++-
 fs/ecryptfs/main.c                 |    3 ++-
 fs/namei.c                         |    8 +++++---
 fs/nfs/super.c                     |    3 ++-
 fs/notify/fanotify/fanotify_user.c |    2 +-
 fs/notify/inotify/inotify_user.c   |    2 +-
 fs/open.c                          |    6 +++---
 include/linux/namei.h              |    3 ++-
 9 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/fs/cachefiles/bind.c b/fs/cachefiles/bind.c
index 622f469..3ccc141 100644
--- a/fs/cachefiles/bind.c
+++ b/fs/cachefiles/bind.c
@@ -114,7 +114,8 @@ static int cachefiles_daemon_add_cache(struct cachefiles_cache *cache)
 	_debug("- fsdef %p", fsdef);
 
 	/* look up the directory at the root of the cache */
-	ret = kern_path(cache->rootdirname, LOOKUP_DIRECTORY, &path);
+	ret = kern_path(cache->rootdirname, LOOKUP_DIRECTORY |
+			LOOKUP_AUTOMOUNT, &path);
 	if (ret < 0)
 		goto error_open_root;
 
diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index 0f3eb41..5e1e1d4 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -114,7 +114,8 @@ static int get_target(const char *symname, struct path *path,
 {
 	int ret;
 
-	ret = kern_path(symname, LOOKUP_FOLLOW|LOOKUP_DIRECTORY, path);
+	ret = kern_path(symname, LOOKUP_FOLLOW | LOOKUP_DIRECTORY |
+			LOOKUP_AUTOMOUNT, path);
 	if (!ret) {
 		if (path->dentry->d_sb == configfs_sb) {
 			*target = configfs_get_config_item(path->dentry);
diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
index b4a6bef..e3de2c8 100644
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -519,7 +519,8 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 	s->s_d_op = &ecryptfs_dops;
 
 	err = "Reading sb failed";
-	rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path);
+	rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY |
+			LOOKUP_AUTOMOUNT, &path);
 	if (rc) {
 		ecryptfs_printk(KERN_WARNING, "kern_path() failed\n");
 		goto out1;
diff --git a/fs/namei.c b/fs/namei.c
index c076492..3db5992 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1564,7 +1564,8 @@ out_fail:
 static inline int lookup_last(struct nameidata *nd, struct path *path)
 {
 	if (nd->last_type == LAST_NORM && nd->last.name[nd->last.len])
-		nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
+		nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY |
+			LOOKUP_AUTOMOUNT;
 
 	nd->flags &= ~LOOKUP_PARENT;
 	return walk_component(nd, path, &nd->last, nd->last_type,
@@ -2116,7 +2117,8 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 	if (!(open_flag & O_CREAT)) {
 		int symlink_ok = 0;
 		if (nd->last.name[nd->last.len])
-			nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
+			nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY |
+				LOOKUP_AUTOMOUNT;
 		if (open_flag & O_PATH && !(nd->flags & LOOKUP_FOLLOW))
 			symlink_ok = 1;
 		/* we _can_ be in RCU mode here */
@@ -2376,7 +2378,7 @@ struct dentry *kern_path_create(int dfd, const char *pathname, struct path *path
 	if (nd.last_type != LAST_NORM)
 		goto out;
 	nd.flags &= ~LOOKUP_PARENT;
-	nd.flags |= LOOKUP_CREATE | LOOKUP_EXCL;
+	nd.flags |= LOOKUP_AUTOMOUNT | LOOKUP_CREATE | LOOKUP_EXCL;
 	nd.intent.open.flags = O_EXCL;
 
 	/*
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 8aa6968..b7f1abd 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -2798,7 +2798,8 @@ static struct dentry *nfs_follow_remote_path(struct vfsmount *root_mnt,
 		goto out_put_mnt_ns;
 
 	ret = vfs_path_lookup(root_mnt->mnt_root, root_mnt, export_path,
-			LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path);
+			LOOKUP_FOLLOW | LOOKUP_DIRECTORY | LOOKUP_AUTOMOUNT,
+			&path);
 
 	nfs_referral_loop_unprotect();
 	put_mnt_ns(ns_private);
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 9fde1c0..c7c8891 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -496,7 +496,7 @@ static int fanotify_find_path(int dfd, const char __user *filename,
 		if (!(flags & FAN_MARK_DONT_FOLLOW))
 			lookup_flags |= LOOKUP_FOLLOW;
 		if (flags & FAN_MARK_ONLYDIR)
-			lookup_flags |= LOOKUP_DIRECTORY;
+			lookup_flags |= LOOKUP_DIRECTORY | LOOKUP_AUTOMOUNT;
 
 		ret = user_path_at(dfd, filename, lookup_flags, path);
 		if (ret)
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 8445fbc..145395a 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -774,7 +774,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
 	if (!(mask & IN_DONT_FOLLOW))
 		flags |= LOOKUP_FOLLOW;
 	if (mask & IN_ONLYDIR)
-		flags |= LOOKUP_DIRECTORY;
+		flags |= LOOKUP_DIRECTORY | LOOKUP_AUTOMOUNT;
 
 	ret = inotify_find_inode(pathname, &path, flags);
 	if (ret)
diff --git a/fs/open.c b/fs/open.c
index f711921..cc38b85 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -918,16 +918,16 @@ static inline int build_open_flags(int flags, int mode, struct open_flags *op)
 
 	op->acc_mode = acc_mode;
 
-	op->intent = flags & O_PATH ? 0 : LOOKUP_OPEN;
+	op->intent = flags & O_PATH ? 0 : (LOOKUP_OPEN | LOOKUP_AUTOMOUNT);
 
 	if (flags & O_CREAT) {
-		op->intent |= LOOKUP_CREATE;
+		op->intent |= LOOKUP_CREATE|LOOKUP_AUTOMOUNT;
 		if (flags & O_EXCL)
 			op->intent |= LOOKUP_EXCL;
 	}
 
 	if (flags & O_DIRECTORY)
-		lookup_flags |= LOOKUP_DIRECTORY;
+		lookup_flags |= LOOKUP_DIRECTORY | LOOKUP_AUTOMOUNT;
 	if (!(flags & O_NOFOLLOW))
 		lookup_flags |= LOOKUP_FOLLOW;
 	return lookup_flags;
diff --git a/include/linux/namei.h b/include/linux/namei.h
index bfcd2e0..209ba9f 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -70,7 +70,8 @@ extern int user_path_at(int, const char __user *, unsigned, 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)
 #define user_path_dir(name, path) \
-	user_path_at(AT_FDCWD, name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, path)
+	user_path_at(AT_FDCWD, name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY | \
+			LOOKUP_AUTOMOUNT, path)
 
 extern int kern_path(const char *, unsigned, struct path *);
 
-- 
1.7.6.2

Re: [PATCH] VFS: Suppress automount on [l]stat, [l]getxattr, etc.

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> Can we at least move the automount/no automount logic out of
> follow_automount() and into the path walk callers?  That way
> follow_automount() becomes more robust and we're less likely to get bitten in
> future as we can define the behaviour more precisely.

On the other hand, if we do that, it reduces our ability to control things on
a per-filesystem basis:-/

David