[PATCH 1/7] autofs4 - don't make expiring dentry negative

[PATCH 1/7] autofs4 - don't make expiring dentry negative

[Ian Kent]

Correct the error of making a positive dentry negative after it has been
instantiated.

The code that makes this error attempts to re-use the dentry from a
concurrent expire and mount to resolve a race and the dentry used for
the lookup must be negative for mounts to trigger in the required
cases. The fact is that the dentry doesn't need to be re-used because
all that is needed is to preserve the flag that indicates an expire is
still incomplete at the time of the mount request.

This change uses the the dentry to check the flag and wait for the
expire to complete then discards it instead of attempting to re-use it.

Signed-off-by: Ian Kent <raven@themaw.net>

---

 fs/autofs4/autofs_i.h |    6 +--
 fs/autofs4/inode.c    |    6 +--
 fs/autofs4/root.c     |  115 ++++++++++++++++++-------------------------------
 3 files changed, 49 insertions(+), 78 deletions(-)


diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index c3d352d..69b1497 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -52,7 +52,7 @@ struct autofs_info {
 
 	int		flags;
 
-	struct list_head rehash;
+	struct list_head expiring;
 
 	struct autofs_sb_info *sbi;
 	unsigned long last_used;
@@ -112,8 +112,8 @@ struct autofs_sb_info {
 	struct mutex wq_mutex;
 	spinlock_t fs_lock;
 	struct autofs_wait_queue *queues; /* Wait queue pointer */
-	spinlock_t rehash_lock;
-	struct list_head rehash_list;
+	spinlock_t lookup_lock;
+	struct list_head expiring_list;
 };
 
 static inline struct autofs_sb_info *autofs4_sbi(struct super_block *sb)
diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index 2fdcf5e..94bfc15 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -47,7 +47,7 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
 	ino->dentry = NULL;
 	ino->size = 0;
 
-	INIT_LIST_HEAD(&ino->rehash);
+	INIT_LIST_HEAD(&ino->expiring);
 
 	ino->last_used = jiffies;
 	atomic_set(&ino->count, 0);
@@ -338,8 +338,8 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 	mutex_init(&sbi->wq_mutex);
 	spin_lock_init(&sbi->fs_lock);
 	sbi->queues = NULL;
-	spin_lock_init(&sbi->rehash_lock);
-	INIT_LIST_HEAD(&sbi->rehash_list);
+	spin_lock_init(&sbi->lookup_lock);
+	INIT_LIST_HEAD(&sbi->expiring_list);
 	s->s_blocksize = 1024;
 	s->s_blocksize_bits = 10;
 	s->s_magic = AUTOFS_SUPER_MAGIC;
diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index edf5b6b..2e8959c 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -493,10 +493,10 @@ void autofs4_dentry_release(struct dentry *de)
 		struct autofs_sb_info *sbi = autofs4_sbi(de->d_sb);
 
 		if (sbi) {
-			spin_lock(&sbi->rehash_lock);
-			if (!list_empty(&inf->rehash))
-				list_del(&inf->rehash);
-			spin_unlock(&sbi->rehash_lock);
+			spin_lock(&sbi->lookup_lock);
+			if (!list_empty(&inf->expiring))
+				list_del(&inf->expiring);
+			spin_unlock(&sbi->lookup_lock);
 		}
 
 		inf->dentry = NULL;
@@ -518,7 +518,7 @@ static struct dentry_operations autofs4_dentry_operations = {
 	.d_release	= autofs4_dentry_release,
 };
 
-static struct dentry *autofs4_lookup_unhashed(struct autofs_sb_info *sbi, struct dentry *parent, struct qstr *name)
+static struct dentry *autofs4_lookup_expiring(struct autofs_sb_info *sbi, struct dentry *parent, struct qstr *name)
 {
 	unsigned int len = name->len;
 	unsigned int hash = name->hash;
@@ -526,14 +526,14 @@ static struct dentry *autofs4_lookup_unhashed(struct autofs_sb_info *sbi, struct
 	struct list_head *p, *head;
 
 	spin_lock(&dcache_lock);
-	spin_lock(&sbi->rehash_lock);
-	head = &sbi->rehash_list;
+	spin_lock(&sbi->lookup_lock);
+	head = &sbi->expiring_list;
 	list_for_each(p, head) {
 		struct autofs_info *ino;
 		struct dentry *dentry;
 		struct qstr *qstr;
 
-		ino = list_entry(p, struct autofs_info, rehash);
+		ino = list_entry(p, struct autofs_info, expiring);
 		dentry = ino->dentry;
 
 		spin_lock(&dentry->d_lock);
@@ -555,33 +555,17 @@ static struct dentry *autofs4_lookup_unhashed(struct autofs_sb_info *sbi, struct
 			goto next;
 
 		if (d_unhashed(dentry)) {
-			struct inode *inode = dentry->d_inode;
-
-			ino = autofs4_dentry_ino(dentry);
-			list_del_init(&ino->rehash);
+			list_del_init(&ino->expiring);
 			dget(dentry);
-			/*
-			 * Make the rehashed dentry negative so the VFS
-			 * behaves as it should.
-			 */
-			if (inode) {
-				dentry->d_inode = NULL;
-				list_del_init(&dentry->d_alias);
-				spin_unlock(&dentry->d_lock);
-				spin_unlock(&sbi->rehash_lock);
-				spin_unlock(&dcache_lock);
-				iput(inode);
-				return dentry;
-			}
 			spin_unlock(&dentry->d_lock);
-			spin_unlock(&sbi->rehash_lock);
+			spin_unlock(&sbi->lookup_lock);
 			spin_unlock(&dcache_lock);
 			return dentry;
 		}
 next:
 		spin_unlock(&dentry->d_lock);
 	}
-	spin_unlock(&sbi->rehash_lock);
+	spin_unlock(&sbi->lookup_lock);
 	spin_unlock(&dcache_lock);
 
 	return NULL;
@@ -591,7 +575,7 @@ next:
 static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
 {
 	struct autofs_sb_info *sbi;
-	struct dentry *unhashed;
+	struct dentry *expiring;
 	int oz_mode;
 
 	DPRINTK("name = %.*s",
@@ -607,44 +591,40 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
 	DPRINTK("pid = %u, pgrp = %u, catatonic = %d, oz_mode = %d",
 		 current->pid, task_pgrp_nr(current), sbi->catatonic, oz_mode);
 
-	unhashed = autofs4_lookup_unhashed(sbi, dentry->d_parent, &dentry->d_name);
-	if (!unhashed) {
-		/*
-		 * Mark the dentry incomplete but don't hash it. We do this
-		 * to serialize our inode creation operations (symlink and
-		 * mkdir) which prevents deadlock during the callback to
-		 * the daemon. Subsequent user space lookups for the same
-		 * dentry are placed on the wait queue while the daemon
-		 * itself is allowed passage unresticted so the create
-		 * operation itself can then hash the dentry. Finally,
-		 * we check for the hashed dentry and return the newly
-		 * hashed dentry.
-		 */
-		dentry->d_op = &autofs4_root_dentry_operations;
-
-		dentry->d_fsdata = NULL;
-		d_instantiate(dentry, NULL);
-	} else {
-		struct autofs_info *ino = autofs4_dentry_ino(unhashed);
-		DPRINTK("rehash %p with %p", dentry, unhashed);
+	expiring = autofs4_lookup_expiring(sbi, dentry->d_parent, &dentry->d_name);
+	if (expiring) {
+		struct autofs_info *ino = autofs4_dentry_ino(expiring);
 		/*
 		 * If we are racing with expire the request might not
 		 * be quite complete but the directory has been removed
 		 * so it must have been successful, so just wait for it.
-		 * We need to ensure the AUTOFS_INF_EXPIRING flag is clear
-		 * before continuing as revalidate may fail when calling
-		 * try_to_fill_dentry (returning EAGAIN) if we don't.
 		 */
 		while (ino && (ino->flags & AUTOFS_INF_EXPIRING)) {
 			DPRINTK("wait for incomplete expire %p name=%.*s",
-				unhashed, unhashed->d_name.len,
-				unhashed->d_name.name);
-			autofs4_wait(sbi, unhashed, NFY_NONE);
+				expiring, expiring->d_name.len,
+				expiring->d_name.name);
+			autofs4_wait(sbi, expiring, NFY_NONE);
 			DPRINTK("request completed");
 		}
-		dentry = unhashed;
+		dput(expiring);
 	}
 
+	/*
+	 * Mark the dentry incomplete but don't hash it. We do this
+	 * to serialize our inode creation operations (symlink and
+	 * mkdir) which prevents deadlock during the callback to
+	 * the daemon. Subsequent user space lookups for the same
+	 * dentry are placed on the wait queue while the daemon
+	 * itself is allowed passage unresticted so the create
+	 * operation itself can then hash the dentry. Finally,
+	 * we check for the hashed dentry and return the newly
+	 * hashed dentry.
+	 */
+	dentry->d_op = &autofs4_root_dentry_operations;
+
+	dentry->d_fsdata = NULL;
+	d_instantiate(dentry, NULL);
+
 	if (!oz_mode) {
 		spin_lock(&dentry->d_lock);
 		dentry->d_flags |= DCACHE_AUTOFS_PENDING;
@@ -668,8 +648,6 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
 			if (sigismember (sigset, SIGKILL) ||
 			    sigismember (sigset, SIGQUIT) ||
 			    sigismember (sigset, SIGINT)) {
-			    if (unhashed)
-				dput(unhashed);
 			    return ERR_PTR(-ERESTARTNOINTR);
 			}
 		}
@@ -699,15 +677,9 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
 		else
 			dentry = ERR_PTR(-ENOENT);
 
-		if (unhashed)
-			dput(unhashed);
-
 		return dentry;
 	}
 
-	if (unhashed)
-		return dentry;
-
 	return NULL;
 }
 
@@ -769,9 +741,8 @@ static int autofs4_dir_symlink(struct inode *dir,
  * that the file no longer exists. However, doing that means that the
  * VFS layer can turn the dentry into a negative dentry.  We don't want
  * this, because the unlink is probably the result of an expire.
- * We simply d_drop it and add it to a rehash candidates list in the
- * super block, which allows the dentry lookup to reuse it retaining
- * the flags, such as expire in progress, in case we're racing with expire.
+ * We simply d_drop it and add it to a expiring list in the super block,
+ * which allows the dentry lookup to check for an incomplete expire.
  *
  * If a process is blocked on the dentry waiting for the expire to finish,
  * it will invalidate the dentry and try to mount with a new one.
@@ -801,9 +772,9 @@ static int autofs4_dir_unlink(struct inode *dir, struct dentry *dentry)
 	dir->i_mtime = CURRENT_TIME;
 
 	spin_lock(&dcache_lock);
-	spin_lock(&sbi->rehash_lock);
-	list_add(&ino->rehash, &sbi->rehash_list);
-	spin_unlock(&sbi->rehash_lock);
+	spin_lock(&sbi->lookup_lock);
+	list_add(&ino->expiring, &sbi->expiring_list);
+	spin_unlock(&sbi->lookup_lock);
 	spin_lock(&dentry->d_lock);
 	__d_drop(dentry);
 	spin_unlock(&dentry->d_lock);
@@ -829,9 +800,9 @@ static int autofs4_dir_rmdir(struct inode *dir, struct dentry *dentry)
 		spin_unlock(&dcache_lock);
 		return -ENOTEMPTY;
 	}
-	spin_lock(&sbi->rehash_lock);
-	list_add(&ino->rehash, &sbi->rehash_list);
-	spin_unlock(&sbi->rehash_lock);
+	spin_lock(&sbi->lookup_lock);
+	list_add(&ino->expiring, &sbi->expiring_list);
+	spin_unlock(&sbi->lookup_lock);
 	spin_lock(&dentry->d_lock);
 	__d_drop(dentry);
 	spin_unlock(&dentry->d_lock);

[PATCH 2/7] autofs4 - revert - redo lookup in ttfd

[Ian Kent]

This patch series enables the use of a single dentry for lookups
prior to the dentry being hashed and so we no longer need to
redo the lookup. This patch reverts the patch of commit
033790449ba9c4dcf8478a87693d33df625c23b5.

Signed-off-by: Ian Kent <raven@themaw.net>

---

 fs/autofs4/root.c |   21 ---------------------
 1 files changed, 0 insertions(+), 21 deletions(-)


diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index 2e8959c..8511cb2 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -242,7 +242,6 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
 {
 	struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
 	struct autofs_info *ino = autofs4_dentry_ino(dentry);
-	struct dentry *new;
 	int status;
 
 	/* Block on any pending expiry here; invalidate the dentry
@@ -320,26 +319,6 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
 	dentry->d_flags &= ~DCACHE_AUTOFS_PENDING;
 	spin_unlock(&dentry->d_lock);
 
-	/*
-	 * The dentry that is passed in from lookup may not be the one
-	 * we end up using, as mkdir can create a new one.  If this
-	 * happens, and another process tries the lookup at the same time,
-	 * it will set the PENDING flag on this new dentry, but add itself
-	 * to our waitq.  Then, if after the lookup succeeds, the first
-	 * process that requested the mount performs another lookup of the
-	 * same directory, it will show up as still pending!  So, we need
-	 * to redo the lookup here and clear pending on that dentry.
-	 */
-	if (d_unhashed(dentry)) {
-		new = d_lookup(dentry->d_parent, &dentry->d_name);
-		if (new) {
-			spin_lock(&new->d_lock);
-			new->d_flags &= ~DCACHE_AUTOFS_PENDING;
-			spin_unlock(&new->d_lock);
-			dput(new);
-		}
-	}
-
 	return 0;
 }
 

[PATCH 3/7] autofs4 - use look aside list for lookups

[Ian Kent]

A while ago a patch to resolve a deadlock during directory creation
was merged. This delayed the hashing of lookup dentrys until the
->mkdir() (or ->symlink()) operation completed to ensure we always
went through ->lookup() instead of also having processes go through
->revalidate() so our VFS locking remained consistent.

Now we are seeing a couple of side affects of that change in situations
with heavy mount activity.

Two cases have been identified:

1) When a mount request is triggered, due to the delayed hashing, the
directory created by user space for the mount point doesn't have the
DCACHE_AUTOFS_PENDING flag set. In the case of an autofs multi-mount
where a tree of mount point directories are created this can lead to
the path walk continuing rather than the dentry being sent to the wait
queue to wait for request completion. This is because, if the pending
flag isn't set, the criteria for deciding this is a mount in progress
fails to hold, namely that the dentry is not a mount point and has no
subdirectories.

2) A mount request dentry is initially created negative and unhashed.
It remains this way until the ->mkdir() callback completes. Since it is
unhashed a fresh dentry is used when the user space mount request creates
the mount point directory. This leaves the original dentry negative and
unhashed. But revalidate has no way to tell the VFS that the dentry has
changed, other than to force another ->lookup() by returning false, which
is at best wastefull and at worst not possible. This results in an
-ENOENT return from the original path walk when in fact the mount succeeded.

To resolve this we need to ensure that the same dentry is used in all
calls to ->lookup() during the course of a mount request. This patch
achieves that by adding the initial dentry to a look aside list and
removes it at ->mkdir() or ->symlink() completion (or when the dentry
is released), since these are the only create operations autofs4 supports.

Signed-off-by: Ian Kent <raven@themaw.net>

---

 fs/autofs4/autofs_i.h |    2 +
 fs/autofs4/inode.c    |   25 ++++---
 fs/autofs4/root.c     |  169 ++++++++++++++++++++++++++++++++++++++++---------
 3 files changed, 156 insertions(+), 40 deletions(-)


diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index 69b1497..2dce233 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -52,6 +52,7 @@ struct autofs_info {
 
 	int		flags;
 
+	struct list_head active;
 	struct list_head expiring;
 
 	struct autofs_sb_info *sbi;
@@ -113,6 +114,7 @@ struct autofs_sb_info {
 	spinlock_t fs_lock;
 	struct autofs_wait_queue *queues; /* Wait queue pointer */
 	spinlock_t lookup_lock;
+	struct list_head active_list;
 	struct list_head expiring_list;
 };
 
diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index 94bfc15..e3e7099 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -24,8 +24,10 @@
 
 static void ino_lnkfree(struct autofs_info *ino)
 {
-	kfree(ino->u.symlink);
-	ino->u.symlink = NULL;
+	if (ino->u.symlink) {
+		kfree(ino->u.symlink);
+		ino->u.symlink = NULL;
+	}
 }
 
 struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
@@ -41,16 +43,18 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
 	if (ino == NULL)
 		return NULL;
 
-	ino->flags = 0;
-	ino->mode = mode;
-	ino->inode = NULL;
-	ino->dentry = NULL;
-	ino->size = 0;
-
-	INIT_LIST_HEAD(&ino->expiring);
+	if (!reinit) {
+		ino->flags = 0;
+		ino->inode = NULL;
+		ino->dentry = NULL;
+		ino->size = 0;
+		INIT_LIST_HEAD(&ino->active);
+		INIT_LIST_HEAD(&ino->expiring);
+		atomic_set(&ino->count, 0);
+	}
 
+	ino->mode = mode;
 	ino->last_used = jiffies;
-	atomic_set(&ino->count, 0);
 
 	ino->sbi = sbi;
 
@@ -339,6 +343,7 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 	spin_lock_init(&sbi->fs_lock);
 	sbi->queues = NULL;
 	spin_lock_init(&sbi->lookup_lock);
+	INIT_LIST_HEAD(&sbi->active_list);
 	INIT_LIST_HEAD(&sbi->expiring_list);
 	s->s_blocksize = 1024;
 	s->s_blocksize_bits = 10;
diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index 8511cb2..7fa303d 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -473,6 +473,8 @@ void autofs4_dentry_release(struct dentry *de)
 
 		if (sbi) {
 			spin_lock(&sbi->lookup_lock);
+			if (!list_empty(&inf->active))
+				list_del(&inf->active);
 			if (!list_empty(&inf->expiring))
 				list_del(&inf->expiring);
 			spin_unlock(&sbi->lookup_lock);
@@ -497,6 +499,58 @@ static struct dentry_operations autofs4_dentry_operations = {
 	.d_release	= autofs4_dentry_release,
 };
 
+static struct dentry *autofs4_lookup_active(struct autofs_sb_info *sbi, struct dentry *parent, struct qstr *name)
+{
+	unsigned int len = name->len;
+	unsigned int hash = name->hash;
+	const unsigned char *str = name->name;
+	struct list_head *p, *head;
+
+	spin_lock(&dcache_lock);
+	spin_lock(&sbi->lookup_lock);
+	head = &sbi->active_list;
+	list_for_each(p, head) {
+		struct autofs_info *ino;
+		struct dentry *dentry;
+		struct qstr *qstr;
+
+		ino = list_entry(p, struct autofs_info, active);
+		dentry = ino->dentry;
+
+		spin_lock(&dentry->d_lock);
+
+		/* Already gone? */
+		if (atomic_read(&dentry->d_count) == 0)
+			goto next;
+
+		qstr = &dentry->d_name;
+
+		if (dentry->d_name.hash != hash)
+			goto next;
+		if (dentry->d_parent != parent)
+			goto next;
+
+		if (qstr->len != len)
+			goto next;
+		if (memcmp(qstr->name, str, len))
+			goto next;
+
+		if (d_unhashed(dentry)) {
+			dget(dentry);
+			spin_unlock(&dentry->d_lock);
+			spin_unlock(&sbi->lookup_lock);
+			spin_unlock(&dcache_lock);
+			return dentry;
+		}
+next:
+		spin_unlock(&dentry->d_lock);
+	}
+	spin_unlock(&sbi->lookup_lock);
+	spin_unlock(&dcache_lock);
+
+	return NULL;
+}
+
 static struct dentry *autofs4_lookup_expiring(struct autofs_sb_info *sbi, struct dentry *parent, struct qstr *name)
 {
 	unsigned int len = name->len;
@@ -554,7 +608,8 @@ next:
 static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
 {
 	struct autofs_sb_info *sbi;
-	struct dentry *expiring;
+	struct autofs_info *ino;
+	struct dentry *expiring, *unhashed;
 	int oz_mode;
 
 	DPRINTK("name = %.*s",
@@ -572,12 +627,12 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
 
 	expiring = autofs4_lookup_expiring(sbi, dentry->d_parent, &dentry->d_name);
 	if (expiring) {
-		struct autofs_info *ino = autofs4_dentry_ino(expiring);
 		/*
 		 * If we are racing with expire the request might not
 		 * be quite complete but the directory has been removed
 		 * so it must have been successful, so just wait for it.
 		 */
+		ino = autofs4_dentry_ino(expiring);
 		while (ino && (ino->flags & AUTOFS_INF_EXPIRING)) {
 			DPRINTK("wait for incomplete expire %p name=%.*s",
 				expiring, expiring->d_name.len,
@@ -588,21 +643,41 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
 		dput(expiring);
 	}
 
-	/*
-	 * Mark the dentry incomplete but don't hash it. We do this
-	 * to serialize our inode creation operations (symlink and
-	 * mkdir) which prevents deadlock during the callback to
-	 * the daemon. Subsequent user space lookups for the same
-	 * dentry are placed on the wait queue while the daemon
-	 * itself is allowed passage unresticted so the create
-	 * operation itself can then hash the dentry. Finally,
-	 * we check for the hashed dentry and return the newly
-	 * hashed dentry.
-	 */
-	dentry->d_op = &autofs4_root_dentry_operations;
+	unhashed = autofs4_lookup_active(sbi, dentry->d_parent, &dentry->d_name);
+	if (unhashed)
+		dentry = unhashed;
+	else {
+		/*
+		 * Mark the dentry incomplete but don't hash it. We do this
+		 * to serialize our inode creation operations (symlink and
+		 * mkdir) which prevents deadlock during the callback to
+		 * the daemon. Subsequent user space lookups for the same
+		 * dentry are placed on the wait queue while the daemon
+		 * itself is allowed passage unresticted so the create
+		 * operation itself can then hash the dentry. Finally,
+		 * we check for the hashed dentry and return the newly
+		 * hashed dentry.
+		 */
+		dentry->d_op = &autofs4_root_dentry_operations;
 
-	dentry->d_fsdata = NULL;
-	d_instantiate(dentry, NULL);
+		/*
+		 * And we need to ensure that the same dentry is used for
+		 * all following lookup calls until it is hashed so that
+		 * the dentry flags are persistent throughout the request.
+		 */
+		ino = autofs4_init_ino(NULL, sbi, 0555);
+		if (!ino)
+			return ERR_PTR(-ENOMEM);
+
+		dentry->d_fsdata = ino;
+		ino->dentry = dentry;
+
+		spin_lock(&sbi->lookup_lock);
+		list_add(&ino->active, &sbi->active_list);
+		spin_unlock(&sbi->lookup_lock);
+
+		d_instantiate(dentry, NULL);
+	}
 
 	if (!oz_mode) {
 		spin_lock(&dentry->d_lock);
@@ -627,12 +702,16 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
 			if (sigismember (sigset, SIGKILL) ||
 			    sigismember (sigset, SIGQUIT) ||
 			    sigismember (sigset, SIGINT)) {
+			    if (unhashed)
+				dput(unhashed);
 			    return ERR_PTR(-ERESTARTNOINTR);
 			}
 		}
-		spin_lock(&dentry->d_lock);
-		dentry->d_flags &= ~DCACHE_AUTOFS_PENDING;
-		spin_unlock(&dentry->d_lock);
+		if (!oz_mode) {
+			spin_lock(&dentry->d_lock);
+			dentry->d_flags &= ~DCACHE_AUTOFS_PENDING;
+			spin_unlock(&dentry->d_lock);
+		}
 	}
 
 	/*
@@ -656,9 +735,15 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
 		else
 			dentry = ERR_PTR(-ENOENT);
 
+		if (unhashed)
+			dput(unhashed);
+
 		return dentry;
 	}
 
+	if (unhashed)
+		return unhashed;
+
 	return NULL;
 }
 
@@ -679,20 +764,30 @@ static int autofs4_dir_symlink(struct inode *dir,
 		return -EACCES;
 
 	ino = autofs4_init_ino(ino, sbi, S_IFLNK | 0555);
-	if (ino == NULL)
-		return -ENOSPC;
+	if (!ino)
+		return -ENOMEM;
 
-	ino->size = strlen(symname);
-	ino->u.symlink = cp = kmalloc(ino->size + 1, GFP_KERNEL);
+	spin_lock(&sbi->lookup_lock);
+	if (!list_empty(&ino->active))
+		list_del_init(&ino->active);
+	spin_unlock(&sbi->lookup_lock);
 
-	if (cp == NULL) {
-		kfree(ino);
-		return -ENOSPC;
+	cp = kmalloc(ino->size + 1, GFP_KERNEL);
+	if (!cp) {
+		if (!dentry->d_fsdata)
+			kfree(ino);
+		return -ENOMEM;
 	}
 
 	strcpy(cp, symname);
 
 	inode = autofs4_get_inode(dir->i_sb, ino);
+	if (!inode) {
+		kfree(cp);
+		if (!dentry->d_fsdata)
+			kfree(ino);
+		return -ENOMEM;
+	}
 	d_add(dentry, inode);
 
 	if (dir == dir->i_sb->s_root->d_inode)
@@ -708,6 +803,8 @@ static int autofs4_dir_symlink(struct inode *dir,
 		atomic_inc(&p_ino->count);
 	ino->inode = inode;
 
+	ino->size = strlen(symname);
+	ino->u.symlink = cp;
 	dir->i_mtime = CURRENT_TIME;
 
 	return 0;
@@ -752,7 +849,8 @@ static int autofs4_dir_unlink(struct inode *dir, struct dentry *dentry)
 
 	spin_lock(&dcache_lock);
 	spin_lock(&sbi->lookup_lock);
-	list_add(&ino->expiring, &sbi->expiring_list);
+	if (list_empty(&ino->expiring))
+		list_add(&ino->expiring, &sbi->expiring_list);
 	spin_unlock(&sbi->lookup_lock);
 	spin_lock(&dentry->d_lock);
 	__d_drop(dentry);
@@ -780,7 +878,8 @@ static int autofs4_dir_rmdir(struct inode *dir, struct dentry *dentry)
 		return -ENOTEMPTY;
 	}
 	spin_lock(&sbi->lookup_lock);
-	list_add(&ino->expiring, &sbi->expiring_list);
+	if (list_empty(&ino->expiring))
+		list_add(&ino->expiring, &sbi->expiring_list);
 	spin_unlock(&sbi->lookup_lock);
 	spin_lock(&dentry->d_lock);
 	__d_drop(dentry);
@@ -816,10 +915,20 @@ static int autofs4_dir_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 		dentry, dentry->d_name.len, dentry->d_name.name);
 
 	ino = autofs4_init_ino(ino, sbi, S_IFDIR | 0555);
-	if (ino == NULL)
-		return -ENOSPC;
+	if (!ino)
+		return -ENOMEM;
+
+	spin_lock(&sbi->lookup_lock);
+	if (!list_empty(&ino->active))
+		list_del_init(&ino->active);
+	spin_unlock(&sbi->lookup_lock);
 
 	inode = autofs4_get_inode(dir->i_sb, ino);
+	if (!inode) {
+		if (!dentry->d_fsdata)
+			kfree(ino);
+		return -ENOMEM;
+	}
 	d_add(dentry, inode);
 
 	if (dir == dir->i_sb->s_root->d_inode)

[PATCH 4/7] autofs4 - don't release directory mutex if called in oz_mode

[Ian Kent]

Since we now delay hashing of dentrys until the ->mkdir() call,
droping and re-taking the directory mutex within the ->lookup()
function when we are being called by user space is not needed.
This can lead to a race when other processes are attempting to
access the same directory during mount point directory creation.

In this case we need to hang onto the mutex to ensure we don't
get user processes trying to create a mount request for a newly
created dentry after the mount point entry has already been
created. This ensures that when we need to check a dentry passed
to autofs4_wait(), if it is hashed, it is always the mount point
dentry and not a new dentry created by another lookup during
directory creation.

Signed-off-by: Ian Kent <raven@themaw.net>

---

 fs/autofs4/root.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)


diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index 7fa303d..236eb9b 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -683,12 +683,11 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
 		spin_lock(&dentry->d_lock);
 		dentry->d_flags |= DCACHE_AUTOFS_PENDING;
 		spin_unlock(&dentry->d_lock);
-	}
-
-	if (dentry->d_op && dentry->d_op->d_revalidate) {
-		mutex_unlock(&dir->i_mutex);
-		(dentry->d_op->d_revalidate)(dentry, nd);
-		mutex_lock(&dir->i_mutex);
+		if (dentry->d_op && dentry->d_op->d_revalidate) {
+			mutex_unlock(&dir->i_mutex);
+			(dentry->d_op->d_revalidate)(dentry, nd);
+			mutex_lock(&dir->i_mutex);
+		}
 	}
 
 	/*

[PATCH 5/7] autofs4 - use lookup intent flags to trigger mounts

[Ian Kent]

When an open(2) call is made on an autofs mount point directory that
already exists and the O_DIRECTORY flag is not used the needed mount
callback to the daemon is not done. This leads to the path walk
continuing resulting in a callback to the daemon with an incorrect
key. open(2) is called without O_DIRECTORY by the "find" utility but
this should be handled properly anyway.

This happens because autofs needs to use the lookup flags to decide
when to callback to the daemon to perform a mount to prevent mount
storms. For example, an autofs indirect mount map that has the "browse"
option will have the mount point directories are pre-created and the
stat(2) call made by a color ls against each directory will cause all
these directories to be mounted. It is unfortunate we need to resort
to this but mount maps can be quite large. Additionally, if a user
manually umounts an autofs indirect mount the directory isn't removed
which also leads to this situation.

To resolve this autofs needs to use the lookup intent flags to enable
it to make this decision. This patch adds this check and triggers a
call back if any of the lookup intent flags are set as all these calls
warrant a mount attempt be requested.

I know that external VFS code which uses the lookup flags is something
that the VFS would like to eliminate but I have no choice as I can't
see any other way to do this. A VFS dentry or inode operation callback
which returns the lookup "type" (requires a definition) would be
sufficient. But this change is needed now and I'm not aware of the form
that coming VFS changes will take so I'm not willing to propose anything
along these lines.

If anyone can provide an alternate method I would be happy to use it.

Signed-off-by: Ian Kent <raven@themaw.net>

---

 fs/autofs4/root.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)


diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index 236eb9b..7f3ebf1 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -31,6 +31,9 @@ static int autofs4_root_readdir(struct file * filp, void * dirent, filldir_t fil
 static struct dentry *autofs4_lookup(struct inode *,struct dentry *, struct nameidata *);
 static void *autofs4_follow_link(struct dentry *, struct nameidata *);
 
+#define TRIGGER_FLAGS   (LOOKUP_CONTINUE | LOOKUP_DIRECTORY)
+#define TRIGGER_INTENTS (LOOKUP_OPEN | LOOKUP_ACCESS | LOOKUP_CREATE)
+
 const struct file_operations autofs4_root_operations = {
 	.open		= dcache_dir_open,
 	.release	= dcache_dir_close,
@@ -291,7 +294,7 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
 			return status;
 		}
 	/* Trigger mount for path component or follow link */
-	} else if (flags & (LOOKUP_CONTINUE | LOOKUP_DIRECTORY) ||
+	} else if (flags & (TRIGGER_FLAGS | TRIGGER_INTENTS) ||
 			current->link_count) {
 		DPRINTK("waiting for mount name=%.*s",
 			dentry->d_name.len, dentry->d_name.name);
@@ -336,7 +339,7 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
 		nd->flags);
 
 	/* If it's our master or we shouldn't trigger a mount we're done */
-	lookup_type = nd->flags & (LOOKUP_CONTINUE | LOOKUP_DIRECTORY);
+	lookup_type = nd->flags & (TRIGGER_FLAGS | TRIGGER_INTENTS);
 	if (oz_mode || !lookup_type)
 		goto done;

[PATCH 6/7] autofs4 - use struct qstr in waitq.c

[Ian Kent]

From: Jeff Moyer <jmoyer@redhat.com>

The autofs_wait_queue already contains all of the fields of the
struct qstr, so change it into a qstr.

This patch, from Jeff Moyer, has been modified a liitle by myself.

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Ian Kent <raven@themaw.net>

---

 fs/autofs4/autofs_i.h |    4 +-
 fs/autofs4/waitq.c    |   86 ++++++++++++++++++++++++++-----------------------
 2 files changed, 46 insertions(+), 44 deletions(-)


diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index 2dce233..da8882f 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -75,9 +75,7 @@ struct autofs_wait_queue {
 	struct autofs_wait_queue *next;
 	autofs_wqt_t wait_queue_token;
 	/* We use the following to see what we are waiting for */
-	unsigned int hash;
-	unsigned int len;
-	char *name;
+	struct qstr name;
 	u32 dev;
 	u64 ino;
 	uid_t uid;
diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index 75e5955..5208cfb 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -36,8 +36,10 @@ void autofs4_catatonic_mode(struct autofs_sb_info *sbi)
 	while (wq) {
 		nwq = wq->next;
 		wq->status = -ENOENT; /* Magic is gone - report failure */
-		kfree(wq->name);
-		wq->name = NULL;
+		if (wq->name.name) {
+			kfree(wq->name.name);
+			wq->name.name = NULL;
+		}
 		wake_up_interruptible(&wq->queue);
 		wq = nwq;
 	}
@@ -92,7 +94,7 @@ static void autofs4_notify_daemon(struct autofs_sb_info *sbi,
 	size_t pktsz;
 
 	DPRINTK("wait id = 0x%08lx, name = %.*s, type=%d",
-		wq->wait_queue_token, wq->len, wq->name, type);
+		wq->wait_queue_token, wq->name.len, wq->name.name, type);
 
 	memset(&pkt,0,sizeof pkt); /* For security reasons */
 
@@ -107,9 +109,9 @@ static void autofs4_notify_daemon(struct autofs_sb_info *sbi,
 		pktsz = sizeof(*mp);
 
 		mp->wait_queue_token = wq->wait_queue_token;
-		mp->len = wq->len;
-		memcpy(mp->name, wq->name, wq->len);
-		mp->name[wq->len] = '\0';
+		mp->len = wq->name.len;
+		memcpy(mp->name, wq->name.name, wq->name.len);
+		mp->name[wq->name.len] = '\0';
 		break;
 	}
 	case autofs_ptype_expire_multi:
@@ -119,9 +121,9 @@ static void autofs4_notify_daemon(struct autofs_sb_info *sbi,
 		pktsz = sizeof(*ep);
 
 		ep->wait_queue_token = wq->wait_queue_token;
-		ep->len = wq->len;
-		memcpy(ep->name, wq->name, wq->len);
-		ep->name[wq->len] = '\0';
+		ep->len = wq->name.len;
+		memcpy(ep->name, wq->name.name, wq->name.len);
+		ep->name[wq->name.len] = '\0';
 		break;
 	}
 	/*
@@ -138,9 +140,9 @@ static void autofs4_notify_daemon(struct autofs_sb_info *sbi,
 		pktsz = sizeof(*packet);
 
 		packet->wait_queue_token = wq->wait_queue_token;
-		packet->len = wq->len;
-		memcpy(packet->name, wq->name, wq->len);
-		packet->name[wq->len] = '\0';
+		packet->len = wq->name.len;
+		memcpy(packet->name, wq->name.name, wq->name.len);
+		packet->name[wq->name.len] = '\0';
 		packet->dev = wq->dev;
 		packet->ino = wq->ino;
 		packet->uid = wq->uid;
@@ -191,15 +193,15 @@ static int autofs4_getpath(struct autofs_sb_info *sbi,
 }
 
 static struct autofs_wait_queue *
-autofs4_find_wait(struct autofs_sb_info *sbi,
-		  char *name, unsigned int hash, unsigned int len)
+autofs4_find_wait(struct autofs_sb_info *sbi, struct qstr *qstr)
 {
 	struct autofs_wait_queue *wq;
 
 	for (wq = sbi->queues; wq; wq = wq->next) {
-		if (wq->hash == hash &&
-		    wq->len == len &&
-		    wq->name && !memcmp(wq->name, name, len))
+		if (wq->name.hash == qstr->hash &&
+		    wq->name.len == qstr->len &&
+		    wq->name.name &&
+			 !memcmp(wq->name.name, qstr->name, qstr->len))
 			break;
 	}
 	return wq;
@@ -210,9 +212,8 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 {
 	struct autofs_info *ino;
 	struct autofs_wait_queue *wq;
+	struct qstr qstr;
 	char *name;
-	unsigned int len = 0;
-	unsigned int hash = 0;
 	int status, type;
 
 	/* In catatonic mode, we don't wait for nobody */
@@ -225,22 +226,23 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 
 	/* If this is a direct mount request create a dummy name */
 	if (IS_ROOT(dentry) && (sbi->type & AUTOFS_TYPE_DIRECT))
-		len = sprintf(name, "%p", dentry);
+		qstr.len = sprintf(name, "%p", dentry);
 	else {
-		len = autofs4_getpath(sbi, dentry, &name);
-		if (!len) {
+		qstr.len = autofs4_getpath(sbi, dentry, &name);
+		if (!qstr.len) {
 			kfree(name);
 			return -ENOENT;
 		}
 	}
-	hash = full_name_hash(name, len);
+	qstr.name = name;
+	qstr.hash = full_name_hash(name, qstr.len);
 
 	if (mutex_lock_interruptible(&sbi->wq_mutex)) {
-		kfree(name);
+		kfree(qstr.name);
 		return -EINTR;
 	}
 
-	wq = autofs4_find_wait(sbi, name, hash, len);
+	wq = autofs4_find_wait(sbi, &qstr);
 	ino = autofs4_dentry_ino(dentry);
 	if (!wq && ino && notify == NFY_NONE) {
 		/*
@@ -254,10 +256,10 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 			mutex_unlock(&sbi->wq_mutex);
 			schedule_timeout_interruptible(HZ/10);
 			if (mutex_lock_interruptible(&sbi->wq_mutex)) {
-				kfree(name);
+				kfree(qstr.name);
 				return -EINTR;
 			}
-			wq = autofs4_find_wait(sbi, name, hash, len);
+			wq = autofs4_find_wait(sbi, &qstr);
 			if (wq)
 				break;
 		}
@@ -268,7 +270,7 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 		 * return status of the wait.
 		 */
 		if (!wq) {
-			kfree(name);
+			kfree(qstr.name);
 			mutex_unlock(&sbi->wq_mutex);
 			return 0;
 		}
@@ -278,7 +280,7 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 		/* Create a new wait queue */
 		wq = kmalloc(sizeof(struct autofs_wait_queue),GFP_KERNEL);
 		if (!wq) {
-			kfree(name);
+			kfree(qstr.name);
 			mutex_unlock(&sbi->wq_mutex);
 			return -ENOMEM;
 		}
@@ -289,9 +291,7 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 		wq->next = sbi->queues;
 		sbi->queues = wq;
 		init_waitqueue_head(&wq->queue);
-		wq->hash = hash;
-		wq->name = name;
-		wq->len = len;
+		memcpy(&wq->name, &qstr, sizeof(struct qstr));
 		wq->dev = autofs4_get_dev(sbi);
 		wq->ino = autofs4_get_ino(sbi);
 		wq->uid = current->uid;
@@ -319,16 +319,18 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 		}
 
 		DPRINTK("new wait id = 0x%08lx, name = %.*s, nfy=%d\n",
-			(unsigned long) wq->wait_queue_token, wq->len, wq->name, notify);
+			(unsigned long) wq->wait_queue_token, wq->name.len,
+			wq->name.name, notify);
 
 		/* autofs4_notify_daemon() may block */
 		autofs4_notify_daemon(sbi, wq, type);
 	} else {
 		atomic_inc(&wq->wait_ctr);
 		mutex_unlock(&sbi->wq_mutex);
-		kfree(name);
+		kfree(qstr.name);
 		DPRINTK("existing wait id = 0x%08lx, name = %.*s, nfy=%d",
-			(unsigned long) wq->wait_queue_token, wq->len, wq->name, notify);
+			(unsigned long) wq->wait_queue_token, wq->name.len,
+			wq->name.name, notify);
 	}
 
 	/* wq->name is NULL if and only if the lock is already released */
@@ -336,11 +338,13 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 	if (sbi->catatonic) {
 		/* We might have slept, so check again for catatonic mode */
 		wq->status = -ENOENT;
-		kfree(wq->name);
-		wq->name = NULL;
+		if (wq->name.name) {
+			kfree(wq->name.name);
+			wq->name.name = NULL;
+		}
 	}
 
-	if (wq->name) {
+	if (wq->name.name) {
 		/* Block all but "shutdown" signals while waiting */
 		sigset_t oldset;
 		unsigned long irqflags;
@@ -351,7 +355,7 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 		recalc_sigpending();
 		spin_unlock_irqrestore(&current->sighand->siglock, irqflags);
 
-		wait_event_interruptible(wq->queue, wq->name == NULL);
+		wait_event_interruptible(wq->queue, wq->name.name == NULL);
 
 		spin_lock_irqsave(&current->sighand->siglock, irqflags);
 		current->blocked = oldset;
@@ -388,8 +392,8 @@ int autofs4_wait_release(struct autofs_sb_info *sbi, autofs_wqt_t wait_queue_tok
 
 	*wql = wq->next;	/* Unlink from chain */
 	mutex_unlock(&sbi->wq_mutex);
-	kfree(wq->name);
-	wq->name = NULL;	/* Do not wait on this queue */
+	kfree(wq->name.name);
+	wq->name.name = NULL;	/* Do not wait on this queue */
 
 	wq->status = status;
 

[PATCH 7/7] autofs4 - fix pending mount race.

[Ian Kent]

Close a race between a pending mount that is about to finish and a new
lookup for the same directory.

Process P1 triggers a mount of directory foo.
It sets DCACHE_AUTOFS_PENDING in the ->lookup routine, creates a waitq
entry for 'foo', and calls out to the daemon to perform the mount.
The autofs daemon will then create the directory 'foo', using a new dentry
that will be hashed in the dcache.

Before the mount completes, another process, P2, tries to walk into the
'foo' directory. The vfs path walking code finds an entry for 'foo' and
calls the revalidate method. Revalidate finds that the entry is not
PENDING (because PENDING was never set on the dentry created by the mkdir),
but it does find the directory is empty. Revalidate calls try_to_fill_dentry,
which sets the PENDING flag and then calls into the autofs4 wait code to
trigger or wait for a mount of 'foo'. The wait code finds the entry for
'foo' and goes to sleep waiting for the completion of the mount.

Yet another process, P3, tries to walk into the 'foo' directory. This
process again finds a dentry in the dcache for 'foo', and calls into
the autofs revalidate code.

The revalidate code finds that the PENDING flag is set, and so calls
try_to_fill_dentry.

a) try_to_fill_dentry sets the PENDING flag redundantly for this dentry,
   then calls into the autofs4 wait code.
b) the autofs4 wait code takes the waitq mutex and searches for an entry
   for 'foo'

Between a and b, P1 is woken up because the mount completed.
P1 takes the wait queue mutex, clears the PENDING flag from the dentry,
and removes the waitqueue entry for 'foo' from the list.

When it releases the waitq mutex, P3 (eventually) acquires it.  At this
time, it looks for an existing waitq for 'foo', finds none, and so
creates a new one and calls out to the daemon to mount the 'foo' directory.

Now, the reason that three processes are required to trigger this race
is that, because the PENDING flag is not set on the dentry created by
mkdir, the window for the race would be way to slim for it to ever occur.
Basically, between the testing of d_mountpoint(dentry) and the taking of
the waitq mutex, the mount would have to complete and the daemon would
have to be woken up, and that in turn would have to wake up P1.  This is
simply impossible.  Add the third process, though, and it becomes slightly
more likely.

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Ian Kent <raven@themaw.net>

---

 fs/autofs4/waitq.c |  135 +++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 97 insertions(+), 38 deletions(-)


diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index 5208cfb..cd21fd4 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -207,19 +207,106 @@ autofs4_find_wait(struct autofs_sb_info *sbi, struct qstr *qstr)
 	return wq;
 }
 
+/*
+ * Check if we have a valid request.
+ * Returns
+ * 1 if the request should continue.
+ *   In this case we can return an autofs_wait_queue entry if one is
+ *   found or NULL to idicate a new wait needs to be created.
+ * 0 or a negative errno if the request shouldn't continue.
+ */
+static int validate_request(struct autofs_wait_queue **wait,
+			    struct autofs_sb_info *sbi,
+			    struct qstr *qstr,
+			    struct dentry*dentry, enum autofs_notify notify)
+{
+	struct autofs_wait_queue *wq;
+	struct autofs_info *ino;
+
+	/* Wait in progress, continue; */
+	wq = autofs4_find_wait(sbi, qstr);
+	if (wq) {
+		*wait = wq;
+		return 1;
+	}
+
+	*wait = NULL;
+
+	/* If we don't yet have any info this is a new request */
+	ino = autofs4_dentry_ino(dentry);
+	if (!ino)
+		return 1;
+
+	/*
+	 * If we've been asked to wait on an existing expire (NFY_NONE)
+	 * but there is no wait in the queue ...
+	 */
+	if (notify == NFY_NONE) {
+		/*
+		 * Either we've betean the pending expire to post it's
+		 * wait or it finished while we waited on the mutex.
+		 * So we need to wait till either, the wait appears
+		 * or the expire finishes.
+		 */
+
+		while (ino->flags & AUTOFS_INF_EXPIRING) {
+			mutex_unlock(&sbi->wq_mutex);
+			schedule_timeout_interruptible(HZ/10);
+			if (mutex_lock_interruptible(&sbi->wq_mutex))
+				return -EINTR;
+
+			wq = autofs4_find_wait(sbi, qstr);
+			if (wq) {
+				*wait = wq;
+				return 1;
+			}
+		}
+
+		/*
+		 * Not ideal but the status has already gone. Of the two
+		 * cases where we wait on NFY_NONE neither depend on the
+		 * return status of the wait.
+		 */
+		return 0;
+	}
+
+	/*
+	 * If we've been asked to trigger a mount and the request
+	 * completed while we waited on the mutex ...
+	 */
+	if (notify == NFY_MOUNT) {
+		/*
+		 * If the dentry isn't hashed just go ahead and try the
+		 * mount again with a new wait (not much else we can do).
+		*/
+		if (!d_unhashed(dentry)) {
+			/*
+			 * But if the dentry is hashed, that means that we
+			 * got here through the revalidate path.  Thus, we
+			 * need to check if the dentry has been mounted
+			 * while we waited on the wq_mutex. If it has,
+			 * simply return success.
+			 */
+			if (d_mountpoint(dentry))
+				return 0;
+		}
+	}
+
+	return 1;
+}
+
 int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 		enum autofs_notify notify)
 {
-	struct autofs_info *ino;
 	struct autofs_wait_queue *wq;
 	struct qstr qstr;
 	char *name;
-	int status, type;
+	int status, ret, type;
 
 	/* In catatonic mode, we don't wait for nobody */
 	if (sbi->catatonic)
 		return -ENOENT;
-	
+
 	name = kmalloc(NAME_MAX + 1, GFP_KERNEL);
 	if (!name)
 		return -ENOMEM;
@@ -237,43 +324,15 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 	qstr.name = name;
 	qstr.hash = full_name_hash(name, qstr.len);
 
-	if (mutex_lock_interruptible(&sbi->wq_mutex)) {
-		kfree(qstr.name);
+	if (mutex_lock_interruptible(&sbi->wq_mutex))
 		return -EINTR;
-	}
-
-	wq = autofs4_find_wait(sbi, &qstr);
-	ino = autofs4_dentry_ino(dentry);
-	if (!wq && ino && notify == NFY_NONE) {
-		/*
-		 * Either we've betean the pending expire to post it's
-		 * wait or it finished while we waited on the mutex.
-		 * So we need to wait till either, the wait appears
-		 * or the expire finishes.
-		 */
 
-		while (ino->flags & AUTOFS_INF_EXPIRING) {
-			mutex_unlock(&sbi->wq_mutex);
-			schedule_timeout_interruptible(HZ/10);
-			if (mutex_lock_interruptible(&sbi->wq_mutex)) {
-				kfree(qstr.name);
-				return -EINTR;
-			}
-			wq = autofs4_find_wait(sbi, &qstr);
-			if (wq)
-				break;
-		}
-
-		/*
-		 * Not ideal but the status has already gone. Of the two
-		 * cases where we wait on NFY_NONE neither depend on the
-		 * return status of the wait.
-		 */
-		if (!wq) {
-			kfree(qstr.name);
+	ret = validate_request(&wq, sbi, &qstr, dentry, notify);
+	if (ret <= 0) {
+		if (ret == 0)
 			mutex_unlock(&sbi->wq_mutex);
-			return 0;
-		}
+		kfree(qstr.name);
+		return ret;
 	}
 
 	if (!wq) {
@@ -391,9 +450,9 @@ int autofs4_wait_release(struct autofs_sb_info *sbi, autofs_wqt_t wait_queue_tok
 	}
 
 	*wql = wq->next;	/* Unlink from chain */
-	mutex_unlock(&sbi->wq_mutex);
 	kfree(wq->name.name);
 	wq->name.name = NULL;	/* Do not wait on this queue */
+	mutex_unlock(&sbi->wq_mutex);
 
 	wq->status = status;
 

Re: [PATCH 7/7] autofs4 - fix pending mount race.

[Andrew Morton]

On Tue, 17 Jun 2008 20:24:12 +0800 Ian Kent <raven@themaw.net> wrote:

> Close a race between a pending mount that is about to finish and a new
> lookup for the same directory.
> 
> Process P1 triggers a mount of directory foo.
> It sets DCACHE_AUTOFS_PENDING in the ->lookup routine, creates a waitq
> entry for 'foo', and calls out to the daemon to perform the mount.
> The autofs daemon will then create the directory 'foo', using a new dentry
> that will be hashed in the dcache.
> 
> Before the mount completes, another process, P2, tries to walk into the
> 'foo' directory. The vfs path walking code finds an entry for 'foo' and
> calls the revalidate method. Revalidate finds that the entry is not
> PENDING (because PENDING was never set on the dentry created by the mkdir),
> but it does find the directory is empty. Revalidate calls try_to_fill_dentry,
> which sets the PENDING flag and then calls into the autofs4 wait code to
> trigger or wait for a mount of 'foo'. The wait code finds the entry for
> 'foo' and goes to sleep waiting for the completion of the mount.
> 
> Yet another process, P3, tries to walk into the 'foo' directory. This
> process again finds a dentry in the dcache for 'foo', and calls into
> the autofs revalidate code.
> 
> The revalidate code finds that the PENDING flag is set, and so calls
> try_to_fill_dentry.
> 
> a) try_to_fill_dentry sets the PENDING flag redundantly for this dentry,
>    then calls into the autofs4 wait code.
> b) the autofs4 wait code takes the waitq mutex and searches for an entry
>    for 'foo'
> 
> Between a and b, P1 is woken up because the mount completed.
> P1 takes the wait queue mutex, clears the PENDING flag from the dentry,
> and removes the waitqueue entry for 'foo' from the list.
> 
> When it releases the waitq mutex, P3 (eventually) acquires it.  At this
> time, it looks for an existing waitq for 'foo', finds none, and so
> creates a new one and calls out to the daemon to mount the 'foo' directory.
> 
> Now, the reason that three processes are required to trigger this race
> is that, because the PENDING flag is not set on the dentry created by
> mkdir, the window for the race would be way to slim for it to ever occur.
> Basically, between the testing of d_mountpoint(dentry) and the taking of
> the waitq mutex, the mount would have to complete and the daemon would
> have to be woken up, and that in turn would have to wake up P1.  This is
> simply impossible.  Add the third process, though, and it becomes slightly
> more likely.
> 
> ...
>
> diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> index 5208cfb..cd21fd4 100644
> --- a/fs/autofs4/waitq.c
> +++ b/fs/autofs4/waitq.c
> @@ -207,19 +207,106 @@ autofs4_find_wait(struct autofs_sb_info *sbi, struct qstr *qstr)
>  	return wq;
>  }
>  
> +/*
> + * Check if we have a valid request.
> + * Returns
> + * 1 if the request should continue.
> + *   In this case we can return an autofs_wait_queue entry if one is
> + *   found or NULL to idicate a new wait needs to be created.
> + * 0 or a negative errno if the request shouldn't continue.
> + */
> +static int validate_request(struct autofs_wait_queue **wait,
> +			    struct autofs_sb_info *sbi,
> +			    struct qstr *qstr,
> +			    struct dentry*dentry, enum autofs_notify notify)
> +{
> +	struct autofs_wait_queue *wq;
> +	struct autofs_info *ino;
> +
> +	/* Wait in progress, continue; */
> +	wq = autofs4_find_wait(sbi, qstr);
> +	if (wq) {
> +		*wait = wq;
> +		return 1;

Returns 1 with the mutex held.

> +	}
> +
> +	*wait = NULL;
> +
> +	/* If we don't yet have any info this is a new request */
> +	ino = autofs4_dentry_ino(dentry);
> +	if (!ino)
> +		return 1;
> +
> +	/*
> +	 * If we've been asked to wait on an existing expire (NFY_NONE)
> +	 * but there is no wait in the queue ...
> +	 */
> +	if (notify == NFY_NONE) {
> +		/*
> +		 * Either we've betean the pending expire to post it's
> +		 * wait or it finished while we waited on the mutex.
> +		 * So we need to wait till either, the wait appears
> +		 * or the expire finishes.
> +		 */

Wanna have another go at that comment?  The grammar and spelling should
cause an oops or something.

> +		while (ino->flags & AUTOFS_INF_EXPIRING) {
> +			mutex_unlock(&sbi->wq_mutex);
> +			schedule_timeout_interruptible(HZ/10);
> +			if (mutex_lock_interruptible(&sbi->wq_mutex))
> +				return -EINTR;

Returns -EFOO with the mutex not held.

> +
> +			wq = autofs4_find_wait(sbi, qstr);
> +			if (wq) {
> +				*wait = wq;
> +				return 1;
> +			}
> +		}
> +
> +		/*
> +		 * Not ideal but the status has already gone. Of the two
> +		 * cases where we wait on NFY_NONE neither depend on the
> +		 * return status of the wait.
> +		 */
> +		return 0;

Returns zero with the mutex held.

> +	}
> +
> +	/*
> +	 * If we've been asked to trigger a mount and the request
> +	 * completed while we waited on the mutex ...
> +	 */
> +	if (notify == NFY_MOUNT) {
> +		/*
> +		 * If the dentry isn't hashed just go ahead and try the
> +		 * mount again with a new wait (not much else we can do).
> +		*/
> +		if (!d_unhashed(dentry)) {
> +			/*
> +			 * But if the dentry is hashed, that means that we
> +			 * got here through the revalidate path.  Thus, we
> +			 * need to check if the dentry has been mounted
> +			 * while we waited on the wq_mutex. If it has,
> +			 * simply return success.
> +			 */
> +			if (d_mountpoint(dentry))
> +				return 0;
> +		}
> +	}
> +
> +	return 1;
> +}
>
> ...
>
> +	ret = validate_request(&wq, sbi, &qstr, dentry, notify);
> +	if (ret <= 0) {
> +		if (ret == 0)
>  			mutex_unlock(&sbi->wq_mutex);
> -			return 0;
> -		}
> +		kfree(qstr.name);
> +		return ret;
>  	}

Leave the mutex held if it returned 1.  Doesn't unlock the mutex if it
returned -EFOO.  Presumably callers of this function will unlock the
mutex if it returned zero.

Or something like that.  My brain just exploded.

Please double-check the locking protocol here and then document the
sorry thing.

Re: [PATCH 6/7] autofs4 - use struct qstr in waitq.c

[Andrew Morton]

On Tue, 17 Jun 2008 20:24:06 +0800
Ian Kent <raven@themaw.net> wrote:

> From: Jeff Moyer <jmoyer@redhat.com>
> 
> The autofs_wait_queue already contains all of the fields of the
> struct qstr, so change it into a qstr.
> 
> This patch, from Jeff Moyer, has been modified a liitle by myself.
> 
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
> Signed-off-by: Ian Kent <raven@themaw.net>

So this patch which had been happily sitting in -mm for a month has
suddenly broken because linux-next's three-day-old
4bce7ce7c7d0d57b78dacc3a2bd87ec63b2d9b4c has removed LOOKUP_ACCESS.

This is suboptimal.

Now what do I do?

Re: [PATCH 6/7] autofs4 - use struct qstr in waitq.c

[Ian Kent]

On Tue, 2008-07-22 at 16:13 -0700, Andrew Morton wrote:
> On Tue, 17 Jun 2008 20:24:06 +0800
> Ian Kent <raven@themaw.net> wrote:
> 
> > From: Jeff Moyer <jmoyer@redhat.com>
> > 
> > The autofs_wait_queue already contains all of the fields of the
> > struct qstr, so change it into a qstr.
> > 
> > This patch, from Jeff Moyer, has been modified a liitle by myself.
> > 
> > Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
> > Signed-off-by: Ian Kent <raven@themaw.net>
> 
> So this patch which had been happily sitting in -mm for a month has
> suddenly broken because linux-next's three-day-old
> 4bce7ce7c7d0d57b78dacc3a2bd87ec63b2d9b4c has removed LOOKUP_ACCESS.
> 
> This is suboptimal.
> 
> Now what do I do?

Ummm .. I'm confused.

Your patch autofs4-use-lookup-intent-flags-to-trigger-mounts-fix.patch
allows the linux-next kernel to build with all the autofs4 patches
currently posted for inclusion in mm but the patch you mention here
isn't concerned with the lookup flags?

The removal of LOOKUP_ACCESS is quite interesting. AFAIKS it effectively
prevents the patch
autofs4-use-lookup-intent-flags-to-trigger-mounts.patch from also
resolving an issue with recursive autofs mounts while still resolving
the issue that the patch was actually meant to address.

It's hard to get exited about the former issue as Al Viro has NACKed a
previous patch that added the LOOKUP_ACCESS check, indicating the
availability of the lookup flags will be changing. Also there is a
question as to whether autofs will support the use mount points in
automount maps that themselves refer to an automount path (the recursive
bit).

Ian

Re: [PATCH 6/7] autofs4 - use struct qstr in waitq.c

[Andrew Morton]

On Wed, 23 Jul 2008 13:08:40 +0800 Ian Kent <raven@themaw.net> wrote:

> 
> On Tue, 2008-07-22 at 16:13 -0700, Andrew Morton wrote:
> > On Tue, 17 Jun 2008 20:24:06 +0800
> > Ian Kent <raven@themaw.net> wrote:
> > 
> > > From: Jeff Moyer <jmoyer@redhat.com>
> > > 
> > > The autofs_wait_queue already contains all of the fields of the
> > > struct qstr, so change it into a qstr.
> > > 
> > > This patch, from Jeff Moyer, has been modified a liitle by myself.
> > > 
> > > Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
> > > Signed-off-by: Ian Kent <raven@themaw.net>
> > 
> > So this patch which had been happily sitting in -mm for a month has
> > suddenly broken because linux-next's three-day-old
> > 4bce7ce7c7d0d57b78dacc3a2bd87ec63b2d9b4c has removed LOOKUP_ACCESS.
> > 
> > This is suboptimal.
> > 
> > Now what do I do?
> 
> Ummm .. I'm confused.
> 
> Your patch autofs4-use-lookup-intent-flags-to-trigger-mounts-fix.patch
> allows the linux-next kernel to build with all the autofs4 patches
> currently posted for inclusion in mm but the patch you mention here
> isn't concerned with the lookup flags?

Yeah, I picked the wrong patch to reply to.

> The removal of LOOKUP_ACCESS is quite interesting. AFAIKS it effectively
> prevents the patch
> autofs4-use-lookup-intent-flags-to-trigger-mounts.patch from also
> resolving an issue with recursive autofs mounts while still resolving
> the issue that the patch was actually meant to address.
> 
> It's hard to get exited about the former issue as Al Viro has NACKed a
> previous patch that added the LOOKUP_ACCESS check, indicating the
> availability of the lookup flags will be changing. Also there is a
> question as to whether autofs will support the use mount points in
> automount maps that themselves refer to an automount path (the recursive
> bit).

So what do we do?

Seems that a great pile of newish-looking stuff hit the vfs tree yesterday.
Al, is that material supposed to be going into 2.6.27?

Re: [PATCH 6/7] autofs4 - use struct qstr in waitq.c

[Ian Kent]

On Tue, 2008-07-22 at 22:17 -0700, Andrew Morton wrote:
> On Wed, 23 Jul 2008 13:08:40 +0800 Ian Kent <raven@themaw.net> wrote:
> 
> > 
> > On Tue, 2008-07-22 at 16:13 -0700, Andrew Morton wrote:
> > > On Tue, 17 Jun 2008 20:24:06 +0800
> > > Ian Kent <raven@themaw.net> wrote:
> > > 
> > > > From: Jeff Moyer <jmoyer@redhat.com>
> > > > 
> > > > The autofs_wait_queue already contains all of the fields of the
> > > > struct qstr, so change it into a qstr.
> > > > 
> > > > This patch, from Jeff Moyer, has been modified a liitle by myself.
> > > > 
> > > > Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
> > > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > 
> > > So this patch which had been happily sitting in -mm for a month has
> > > suddenly broken because linux-next's three-day-old
> > > 4bce7ce7c7d0d57b78dacc3a2bd87ec63b2d9b4c has removed LOOKUP_ACCESS.
> > > 
> > > This is suboptimal.
> > > 
> > > Now what do I do?
> > 
> > Ummm .. I'm confused.
> > 
> > Your patch autofs4-use-lookup-intent-flags-to-trigger-mounts-fix.patch
> > allows the linux-next kernel to build with all the autofs4 patches
> > currently posted for inclusion in mm but the patch you mention here
> > isn't concerned with the lookup flags?
> 
> Yeah, I picked the wrong patch to reply to.

Hahaha, ;)

> 
> > The removal of LOOKUP_ACCESS is quite interesting. AFAIKS it effectively
> > prevents the patch
> > autofs4-use-lookup-intent-flags-to-trigger-mounts.patch from also
> > resolving an issue with recursive autofs mounts while still resolving
> > the issue that the patch was actually meant to address.
> > 
> > It's hard to get exited about the former issue as Al Viro has NACKed a
> > previous patch that added the LOOKUP_ACCESS check, indicating the
> > availability of the lookup flags will be changing. Also there is a
> > question as to whether autofs will support the use mount points in
> > automount maps that themselves refer to an automount path (the recursive
> > bit).
> 
> So what do we do?

Yeah, best thing to do is to go with the patch that you did to resolve
it. I'm going to need to deal with changes to the availability of the
lookup flags to modules as they come up anyway.

I don't want to hold things up due to the recursive mount issue as Al
has pointed out it doesn't work correctly now anyway. I haven't had time
to look into it as it isn't at the top of my priority list.

Ian