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

[Miklos Szeredi <miklos@szeredi.hu>]

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;