[PATCH 1/7] autofs4 - indirect dentry must almost always be positive

[PATCH 1/7] autofs4 - indirect dentry must almost always be positive

[Ian Kent]

We have been seeing mount requests comming to the automount daemon
for keys of the form "<map key>/<non key directory>" which are
lookups for invalid map keys. But we can check for this in the
kernel module and return a fail immediately, without having to
send a request to the daemon.

It is possible to recognise these requests are invalid based on
whether the request dentry is negative and its relation to the
autofs file system root.

For example, given the indirect multi-mount map entry:

idm1  \
    /mm1  <server>:/<path1>
    /mm2  <server>:/<path2>

For a request to mount idm1, IS_ROOT((idm1)->d_parent) will be
always be true and the dentry may be negative. But directories
idm1/mm1 and idm1/mm2 will always be created as part of the
mount request for idm1. So any mount request within idm1 itself
must have a positive dentry otherwise the map key is invalid.

In version 4 these multi-mount entries are all mounted and umounted
as a single request and in version 5 the directories idm1/mm1 and
idm1/mm2 are created and an autofs fs mounted on them to act as a
mount trigger so the above is also true.

This also holds true for the autofs version 4 pseudo direct mount
feature. When this feature is used without the "--ghost" option
automount(8) will create internal submounts as we go down the
map key paths which are essentially normal indirect mounts for
which the above holds. If the "--ghost" option is given the
directories for map keys are created at daemon startup so valid
map entries correspond to postive dentries in the autofs fs.

autofs version 5 direct mount maps are similar except that the
IS_ROOT check is not needed. This has been addressed in a previous
patch tittled "autofs4 - detect invalid direct mount requests".

For example, given the direct multi-mount map entry:

/test/dm1  \
    /mm1  <server>:/<path1>
    /mm2  <server>:/<path2>

An autofs fs is mounted on /test/dm1 as a trigger mount and when
a mount is triggered for /test/dm1, the multi-mount offset
directories /test/dm1/mm1 and /test/dm1/mm2 are created and an
autofs fs is mounted on them to act as mount triggers. So valid
direct mount requests must always have a positive dentry if they
correspond to a valid map entry.

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

---

 fs/autofs4/waitq.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)


diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index bcb6c52..35216d1 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -328,9 +328,20 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 	if (sbi->catatonic)
 		return -ENOENT;
 
-	if (!dentry->d_inode &&
-	    (sbi->type & (AUTOFS_TYPE_DIRECT | AUTOFS_TYPE_OFFSET)))
-		return -ENOENT;
+	if (!dentry->d_inode) {
+		/*
+		 * A wait for a negative dentry is invalid for certain
+		 * cases. A direct or offset mount "always" has its mount
+		 * point directory created and so the request dentry must
+		 * be positive or the map key doesn't exist. The situation
+		 * is very similar for indirect mounts except only dentrys
+		 * in the root of the autofs file system may be negative.
+		 */
+		if (sbi->type & (AUTOFS_TYPE_DIRECT|AUTOFS_TYPE_OFFSET))
+			return -ENOENT;
+		else if (!IS_ROOT(dentry->d_parent))
+			return -ENOENT;
+	}
 
 	name = kmalloc(NAME_MAX + 1, GFP_KERNEL);
 	if (!name)

[PATCH 2/7] autofs4 - cleanup redundant readir code

[Ian Kent]

The mount triggering functionality of readdir and related functions
is no longer used (and is quite broken as well). The unused portions
have been removed.

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

---

 fs/autofs4/root.c |  149 ++++++-----------------------------------------------
 1 files changed, 16 insertions(+), 133 deletions(-)


diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index 2a1a631..e842b0a 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -25,8 +25,6 @@ static int autofs4_dir_rmdir(struct inode *,struct dentry *);
 static int autofs4_dir_mkdir(struct inode *,struct dentry *,int);
 static int autofs4_root_ioctl(struct inode *, struct file *,unsigned int,unsigned long);
 static int autofs4_dir_open(struct inode *inode, struct file *file);
-static int autofs4_dir_close(struct inode *inode, struct file *file);
-static int autofs4_dir_readdir(struct file * filp, void * dirent, filldir_t filldir);
 static int autofs4_root_readdir(struct file * filp, void * dirent, filldir_t filldir);
 static struct dentry *autofs4_lookup(struct inode *,struct dentry *, struct nameidata *);
 static void *autofs4_follow_link(struct dentry *, struct nameidata *);
@@ -44,9 +42,9 @@ const struct file_operations autofs4_root_operations = {
 
 const struct file_operations autofs4_dir_operations = {
 	.open		= autofs4_dir_open,
-	.release	= autofs4_dir_close,
+	.release	= dcache_dir_close,
 	.read		= generic_read_dir,
-	.readdir	= autofs4_dir_readdir,
+	.readdir	= dcache_readdir,
 };
 
 const struct inode_operations autofs4_indirect_root_inode_operations = {
@@ -98,17 +96,7 @@ static int autofs4_root_readdir(struct file *file, void *dirent,
 static int autofs4_dir_open(struct inode *inode, struct file *file)
 {
 	struct dentry *dentry = file->f_path.dentry;
-	struct vfsmount *mnt = file->f_path.mnt;
 	struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
-	struct dentry *cursor;
-	int status;
-
-	status = dcache_dir_open(inode, file);
-	if (status)
-		goto out;
-
-	cursor = file->private_data;
-	cursor->d_fsdata = NULL;
 
 	DPRINTK("file=%p dentry=%p %.*s",
 		file, dentry, dentry->d_name.len, dentry->d_name.name);
@@ -116,129 +104,24 @@ static int autofs4_dir_open(struct inode *inode, struct file *file)
 	if (autofs4_oz_mode(sbi))
 		goto out;
 
-	if (autofs4_ispending(dentry)) {
-		DPRINTK("dentry busy");
-		dcache_dir_close(inode, file);
-		status = -EBUSY;
-		goto out;
-	}
-
-	status = -ENOENT;
-	if (!d_mountpoint(dentry) && dentry->d_op && dentry->d_op->d_revalidate) {
-		struct nameidata nd;
-		int empty, ret;
-
-		/* In case there are stale directory dentrys from a failed mount */
-		spin_lock(&dcache_lock);
-		empty = list_empty(&dentry->d_subdirs);
+	/*
+	 * An empty directory in an autofs file system is always a
+	 * mount point. The daemon must have failed to mount this
+	 * during lookup so it doesn't exist. This can happen, for
+	 * example, if user space returns an incorrect status for a
+	 * mount request. Otherwise we're doing a readdir on the
+	 * autofs file system so just let the libfs routines handle
+	 * it.
+	 */
+	spin_lock(&dcache_lock);
+	if (!d_mountpoint(dentry) && __simple_empty(dentry)) {
 		spin_unlock(&dcache_lock);
-
-		if (!empty)
-			d_invalidate(dentry);
-
-		nd.flags = LOOKUP_DIRECTORY;
-		ret = (dentry->d_op->d_revalidate)(dentry, &nd);
-
-		if (ret <= 0) {
-			if (ret < 0)
-				status = ret;
-			dcache_dir_close(inode, file);
-			goto out;
-		}
+		return -ENOENT;
 	}
+	spin_unlock(&dcache_lock);
 
-	if (d_mountpoint(dentry)) {
-		struct file *fp = NULL;
-		struct path fp_path = { .dentry = dentry, .mnt = mnt };
-
-		path_get(&fp_path);
-
-		if (!autofs4_follow_mount(&fp_path.mnt, &fp_path.dentry)) {
-			path_put(&fp_path);
-			dcache_dir_close(inode, file);
-			goto out;
-		}
-
-		fp = dentry_open(fp_path.dentry, fp_path.mnt, file->f_flags);
-		status = PTR_ERR(fp);
-		if (IS_ERR(fp)) {
-			dcache_dir_close(inode, file);
-			goto out;
-		}
-		cursor->d_fsdata = fp;
-	}
-	return 0;
-out:
-	return status;
-}
-
-static int autofs4_dir_close(struct inode *inode, struct file *file)
-{
-	struct dentry *dentry = file->f_path.dentry;
-	struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
-	struct dentry *cursor = file->private_data;
-	int status = 0;
-
-	DPRINTK("file=%p dentry=%p %.*s",
-		file, dentry, dentry->d_name.len, dentry->d_name.name);
-
-	if (autofs4_oz_mode(sbi))
-		goto out;
-
-	if (autofs4_ispending(dentry)) {
-		DPRINTK("dentry busy");
-		status = -EBUSY;
-		goto out;
-	}
-
-	if (d_mountpoint(dentry)) {
-		struct file *fp = cursor->d_fsdata;
-		if (!fp) {
-			status = -ENOENT;
-			goto out;
-		}
-		filp_close(fp, current->files);
-	}
-out:
-	dcache_dir_close(inode, file);
-	return status;
-}
-
-static int autofs4_dir_readdir(struct file *file, void *dirent, filldir_t filldir)
-{
-	struct dentry *dentry = file->f_path.dentry;
-	struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
-	struct dentry *cursor = file->private_data;
-	int status;
-
-	DPRINTK("file=%p dentry=%p %.*s",
-		file, dentry, dentry->d_name.len, dentry->d_name.name);
-
-	if (autofs4_oz_mode(sbi))
-		goto out;
-
-	if (autofs4_ispending(dentry)) {
-		DPRINTK("dentry busy");
-		return -EBUSY;
-	}
-
-	if (d_mountpoint(dentry)) {
-		struct file *fp = cursor->d_fsdata;
-
-		if (!fp)
-			return -ENOENT;
-
-		if (!fp->f_op || !fp->f_op->readdir)
-			goto out;
-
-		status = vfs_readdir(fp, filldir, dirent);
-		file->f_pos = fp->f_pos;
-		if (status)
-			autofs4_copy_atime(file, fp);
-		return status;
-	}
 out:
-	return dcache_readdir(file, dirent, filldir);
+	return dcache_dir_open(inode, file);
 }
 
 static int try_to_fill_dentry(struct dentry *dentry, int flags)

[PATCH 3/7] autofs4 - fix pending checks

[Ian Kent]

There are two cases for which a dentry that has a pending mount request
does not wait for completion. One is via autofs4_revalidate() and the
other via autofs4_follow_link().

In revalidate, after the mount point directory is created, but before
the mount is done, the check in try_to_fill_dentry() can can fail to
send the dentry to the wait queue since the dentry is positive and the
lookup flags may contain only LOOKUP_FOLLOW. Although we don't trigger
a mount for the LOOKUP_FOLLOW flag, if ther's one pending we might as
well wait and use the mounted dentry for the lookup.

In autofs4_follow_link() the dentry is not checked to see if it is
pending so it may fail to call try_to_fill_dentry() and not wait
for mount completion.

A dentry that is pending must always be sent to the wait queue.

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

---

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


diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index e842b0a..2944b28 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -177,7 +177,8 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
 			return status;
 		}
 	/* Trigger mount for path component or follow link */
-	} else if (flags & (TRIGGER_FLAGS | TRIGGER_INTENTS) ||
+	} else if (dentry->d_flags & DCACHE_AUTOFS_PENDING ||
+			flags & (TRIGGER_FLAGS | TRIGGER_INTENTS) ||
 			current->link_count) {
 		DPRINTK("waiting for mount name=%.*s",
 			dentry->d_name.len, dentry->d_name.name);
@@ -223,7 +224,8 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
 
 	/* If it's our master or we shouldn't trigger a mount we're done */
 	lookup_type = nd->flags & (TRIGGER_FLAGS | TRIGGER_INTENTS);
-	if (oz_mode || !lookup_type)
+	if (oz_mode ||
+	    !(lookup_type || dentry->d_flags & DCACHE_AUTOFS_PENDING))
 		goto done;
 
 	/* If an expire request is pending wait for it. */
@@ -242,7 +244,8 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
 	 * don't try to mount it again.
 	 */
 	spin_lock(&dcache_lock);
-	if (!d_mountpoint(dentry) && __simple_empty(dentry)) {
+	if (dentry->d_flags & DCACHE_AUTOFS_PENDING ||
+	    (!d_mountpoint(dentry) && __simple_empty(dentry))) {
 		spin_unlock(&dcache_lock);
 
 		status = try_to_fill_dentry(dentry, 0);

[PATCH 4/7] autofs4 - fix indirect mount pending expire race

[Ian Kent]

The selection of a dentry for expiration and the setting of the
AUTOFS_INF_EXPIRING flag isn't done atomically which can lead to
lookups walking into an expiring mount.

What happens is that an expire is initiated by the daemon and
a dentry is selected for expire but, since there is no lock
held between the selection and setting of the expiring flag,
a process may find the flag clear and continue walking into
the mount tree at the same time the daemon attempts the expire
it.

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

---

 fs/autofs4/autofs_i.h |   10 +++-------
 fs/autofs4/expire.c   |   46 +++++++++++++++++++++++++++++++++++-----------
 fs/autofs4/root.c     |   32 +++++++++++++++++++++++++++-----
 3 files changed, 65 insertions(+), 23 deletions(-)


diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index 058e180..5d90ed3 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -138,18 +138,14 @@ static inline int autofs4_oz_mode(struct autofs_sb_info *sbi) {
 static inline int autofs4_ispending(struct dentry *dentry)
 {
 	struct autofs_info *inf = autofs4_dentry_ino(dentry);
-	int pending = 0;
 
 	if (dentry->d_flags & DCACHE_AUTOFS_PENDING)
 		return 1;
 
-	if (inf) {
-		spin_lock(&inf->sbi->fs_lock);
-		pending = inf->flags & AUTOFS_INF_EXPIRING;
-		spin_unlock(&inf->sbi->fs_lock);
-	}
+	if (inf->flags & AUTOFS_INF_EXPIRING)
+		return 1;
 
-	return pending;
+	return 0;
 }
 
 static inline void autofs4_copy_atime(struct file *src, struct file *dst)
diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c
index 894fee5..19f5bea 100644
--- a/fs/autofs4/expire.c
+++ b/fs/autofs4/expire.c
@@ -292,6 +292,8 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
 	struct list_head *next;
 	int do_now = how & AUTOFS_EXP_IMMEDIATE;
 	int exp_leaves = how & AUTOFS_EXP_LEAVES;
+	struct autofs_info *ino;
+	unsigned int ino_count;
 
 	if (!root)
 		return NULL;
@@ -316,6 +318,9 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
 		dentry = dget(dentry);
 		spin_unlock(&dcache_lock);
 
+		spin_lock(&sbi->fs_lock);
+		ino = autofs4_dentry_ino(dentry);
+
 		/*
 		 * Case 1: (i) indirect mount or top level pseudo direct mount
 		 *	   (autofs-4.1).
@@ -326,6 +331,11 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
 			DPRINTK("checking mountpoint %p %.*s",
 				dentry, (int)dentry->d_name.len, dentry->d_name.name);
 
+			/* Path walk currently on this dentry? */
+			ino_count = atomic_read(&ino->count) + 2;
+			if (atomic_read(&dentry->d_count) > ino_count)
+				goto next;
+
 			/* Can we umount this guy */
 			if (autofs4_mount_busy(mnt, dentry))
 				goto next;
@@ -343,23 +353,25 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
 
 		/* Case 2: tree mount, expire iff entire tree is not busy */
 		if (!exp_leaves) {
-			/* Lock the tree as we must expire as a whole */
-			spin_lock(&sbi->fs_lock);
-			if (!autofs4_tree_busy(mnt, dentry, timeout, do_now)) {
-				struct autofs_info *inf = autofs4_dentry_ino(dentry);
+			/* Path walk currently on this dentry? */
+			ino_count = atomic_read(&ino->count) + 1;
+			if (atomic_read(&dentry->d_count) > ino_count)
+				goto next;
 
-				/* Set this flag early to catch sys_chdir and the like */
-				inf->flags |= AUTOFS_INF_EXPIRING;
-				spin_unlock(&sbi->fs_lock);
+			if (!autofs4_tree_busy(mnt, dentry, timeout, do_now)) {
 				expired = dentry;
 				goto found;
 			}
-			spin_unlock(&sbi->fs_lock);
 		/*
 		 * Case 3: pseudo direct mount, expire individual leaves
 		 *	   (autofs-4.1).
 		 */
 		} else {
+			/* Path walk currently on this dentry? */
+			ino_count = atomic_read(&ino->count) + 1;
+			if (atomic_read(&dentry->d_count) > ino_count)
+				goto next;
+
 			expired = autofs4_check_leaves(mnt, dentry, timeout, do_now);
 			if (expired) {
 				dput(dentry);
@@ -367,6 +379,7 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
 			}
 		}
 next:
+		spin_unlock(&sbi->fs_lock);
 		dput(dentry);
 		spin_lock(&dcache_lock);
 		next = next->next;
@@ -377,6 +390,9 @@ next:
 found:
 	DPRINTK("returning %p %.*s",
 		expired, (int)expired->d_name.len, expired->d_name.name);
+	ino = autofs4_dentry_ino(expired);
+	ino->flags |= AUTOFS_INF_EXPIRING;
+	spin_unlock(&sbi->fs_lock);
 	spin_lock(&dcache_lock);
 	list_move(&expired->d_parent->d_subdirs, &expired->d_u.d_child);
 	spin_unlock(&dcache_lock);
@@ -390,7 +406,9 @@ int autofs4_expire_run(struct super_block *sb,
 		      struct autofs_packet_expire __user *pkt_p)
 {
 	struct autofs_packet_expire pkt;
+	struct autofs_info *ino;
 	struct dentry *dentry;
+	int ret = 0;
 
 	memset(&pkt,0,sizeof pkt);
 
@@ -406,9 +424,14 @@ int autofs4_expire_run(struct super_block *sb,
 	dput(dentry);
 
 	if ( copy_to_user(pkt_p, &pkt, sizeof(struct autofs_packet_expire)) )
-		return -EFAULT;
+		ret = -EFAULT;
 
-	return 0;
+	spin_lock(&sbi->fs_lock);
+	ino = autofs4_dentry_ino(dentry);
+	ino->flags &= ~AUTOFS_INF_EXPIRING;
+	spin_unlock(&sbi->fs_lock);
+
+	return ret;
 }
 
 /* Call repeatedly until it returns -EAGAIN, meaning there's nothing
@@ -433,9 +456,10 @@ int autofs4_expire_multi(struct super_block *sb, struct vfsmount *mnt,
 
 		/* This is synchronous because it makes the daemon a
                    little easier */
-		ino->flags |= AUTOFS_INF_EXPIRING;
 		ret = autofs4_wait(sbi, dentry, NFY_EXPIRE);
+		spin_lock(&sbi->fs_lock);
 		ino->flags &= ~AUTOFS_INF_EXPIRING;
+		spin_unlock(&sbi->fs_lock);
 		dput(dentry);
 	}
 
diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index 2944b28..2ed2a51 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -133,7 +133,10 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
 	/* Block on any pending expiry here; invalidate the dentry
            when expiration is done to trigger mount request with a new
            dentry */
-	if (ino && (ino->flags & AUTOFS_INF_EXPIRING)) {
+	spin_lock(&sbi->fs_lock);
+	if (ino->flags & AUTOFS_INF_EXPIRING) {
+		spin_unlock(&sbi->fs_lock);
+
 		DPRINTK("waiting for expire %p name=%.*s",
 			 dentry, dentry->d_name.len, dentry->d_name.name);
 
@@ -149,8 +152,11 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
 		status = d_invalidate(dentry);
 		if (status != -EBUSY)
 			return -EAGAIN;
-	}
 
+		goto cont;
+	}
+	spin_unlock(&sbi->fs_lock);
+cont:
 	DPRINTK("dentry=%p %.*s ino=%p",
 		 dentry, dentry->d_name.len, dentry->d_name.name, dentry->d_inode);
 
@@ -229,15 +235,21 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
 		goto done;
 
 	/* If an expire request is pending wait for it. */
-	if (ino && (ino->flags & AUTOFS_INF_EXPIRING)) {
+	spin_lock(&sbi->fs_lock);
+	if (ino->flags & AUTOFS_INF_EXPIRING) {
+		spin_unlock(&sbi->fs_lock);
+
 		DPRINTK("waiting for active request %p name=%.*s",
 			dentry, dentry->d_name.len, dentry->d_name.name);
 
 		status = autofs4_wait(sbi, dentry, NFY_NONE);
 
 		DPRINTK("request done status=%d", status);
-	}
 
+		goto cont;
+	}
+	spin_unlock(&sbi->fs_lock);
+cont:
 	/*
 	 * If the dentry contains directories then it is an
 	 * autofs multi-mount with no root mount offset. So
@@ -292,8 +304,11 @@ static int autofs4_revalidate(struct dentry *dentry, struct nameidata *nd)
 	int status = 1;
 
 	/* Pending dentry */
+	spin_lock(&sbi->fs_lock);
 	if (autofs4_ispending(dentry)) {
 		/* The daemon never causes a mount to trigger */
+		spin_unlock(&sbi->fs_lock);
+
 		if (oz_mode)
 			return 1;
 
@@ -316,6 +331,7 @@ static int autofs4_revalidate(struct dentry *dentry, struct nameidata *nd)
 
 		return status;
 	}
+	spin_unlock(&sbi->fs_lock);
 
 	/* Negative dentry.. invalidate if "old" */
 	if (dentry->d_inode == NULL)
@@ -329,6 +345,7 @@ static int autofs4_revalidate(struct dentry *dentry, struct nameidata *nd)
 		DPRINTK("dentry=%p %.*s, emptydir",
 			 dentry, dentry->d_name.len, dentry->d_name.name);
 		spin_unlock(&dcache_lock);
+
 		/* The daemon never causes a mount to trigger */
 		if (oz_mode)
 			return 1;
@@ -521,13 +538,18 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
 		 * so it must have been successful, so just wait for it.
 		 */
 		ino = autofs4_dentry_ino(expiring);
-		while (ino && (ino->flags & AUTOFS_INF_EXPIRING)) {
+		spin_lock(&sbi->fs_lock);
+		if (ino->flags & AUTOFS_INF_EXPIRING) {
+			spin_unlock(&sbi->fs_lock);
 			DPRINTK("wait for incomplete expire %p name=%.*s",
 				expiring, expiring->d_name.len,
 				expiring->d_name.name);
 			autofs4_wait(sbi, expiring, NFY_NONE);
 			DPRINTK("request completed");
+			goto cont;
 		}
+		spin_unlock(&sbi->fs_lock);
+cont:
 		spin_lock(&sbi->lookup_lock);
 		if (!list_empty(&ino->expiring))
 			list_del_init(&ino->expiring);

[PATCH 5/7] autofs4 - fix direct mount pending expire race

[Ian Kent]

For direct and offset type mounts that are covered by another mount
we cannot check the AUTOFS_INF_EXPIRING flag during a path walk
which leads to lookups walking into an expiring mount while it is
being expired.

For example, for the direct multi-mount map entry with a couple of
offsets:

/race/mm1  /      <server1>:/<path1>
           /om1   <server2>:/<path2>
           /om2   <server1>:/<path3>

an autofs trigger mount is mounted on /race/mm1 and when accessed
it is over mounted and trigger mounts made for /race/mm1/om1 and
/race/mm1/om2. So it isn't possible for path walks to see the
expiring flag at all and they happily walk into the file system
while it is expiring.

When expiring these mounts follow_down() must stop at the autofs
mount and all processes must block in the ->follow_link() method
(except the daemon) until the expire is complete. This is done by
decrementing the d_mounted field of the autofs trigger mount root
dentry until the expire is completed. In ->follow_link() all
processes wait on the expire and the mount following is completed
for the daemon until the expire is complete.

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

---

 fs/autofs4/autofs_i.h |    3 ++
 fs/autofs4/expire.c   |   16 +++++++++--
 fs/autofs4/root.c     |   72 +++++++++++++++++++++++++++++++++----------------
 3 files changed, 65 insertions(+), 26 deletions(-)


diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index 5d90ed3..4b40cbc 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -52,6 +52,8 @@ struct autofs_info {
 
 	int		flags;
 
+	struct completion expire_complete;
+
 	struct list_head active;
 	struct list_head expiring;
 
@@ -69,6 +71,7 @@ struct autofs_info {
 };
 
 #define AUTOFS_INF_EXPIRING	(1<<0) /* dentry is in the process of expiring */
+#define AUTOFS_INF_MOUNTPOINT	(1<<1) /* mountpoint status for direct expire */
 
 struct autofs_wait_queue {
 	wait_queue_head_t queue;
diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c
index 19f5bea..705b9f0 100644
--- a/fs/autofs4/expire.c
+++ b/fs/autofs4/expire.c
@@ -259,13 +259,15 @@ static struct dentry *autofs4_expire_direct(struct super_block *sb,
 	now = jiffies;
 	timeout = sbi->exp_timeout;
 
-	/* Lock the tree as we must expire as a whole */
 	spin_lock(&sbi->fs_lock);
 	if (!autofs4_direct_busy(mnt, root, timeout, do_now)) {
 		struct autofs_info *ino = autofs4_dentry_ino(root);
-
-		/* Set this flag early to catch sys_chdir and the like */
+		if (d_mountpoint(root)) {
+			ino->flags |= AUTOFS_INF_MOUNTPOINT;
+			root->d_mounted--;
+		}
 		ino->flags |= AUTOFS_INF_EXPIRING;
+		init_completion(&ino->expire_complete);
 		spin_unlock(&sbi->fs_lock);
 		return root;
 	}
@@ -392,6 +394,7 @@ found:
 		expired, (int)expired->d_name.len, expired->d_name.name);
 	ino = autofs4_dentry_ino(expired);
 	ino->flags |= AUTOFS_INF_EXPIRING;
+	init_completion(&ino->expire_complete);
 	spin_unlock(&sbi->fs_lock);
 	spin_lock(&dcache_lock);
 	list_move(&expired->d_parent->d_subdirs, &expired->d_u.d_child);
@@ -429,6 +432,7 @@ int autofs4_expire_run(struct super_block *sb,
 	spin_lock(&sbi->fs_lock);
 	ino = autofs4_dentry_ino(dentry);
 	ino->flags &= ~AUTOFS_INF_EXPIRING;
+	complete_all(&ino->expire_complete);
 	spin_unlock(&sbi->fs_lock);
 
 	return ret;
@@ -457,8 +461,14 @@ int autofs4_expire_multi(struct super_block *sb, struct vfsmount *mnt,
 		/* This is synchronous because it makes the daemon a
                    little easier */
 		ret = autofs4_wait(sbi, dentry, NFY_EXPIRE);
+
 		spin_lock(&sbi->fs_lock);
+		if (ino->flags & AUTOFS_INF_MOUNTPOINT) {
+			sb->s_root->d_mounted++;
+			ino->flags &= ~AUTOFS_INF_MOUNTPOINT;
+		}
 		ino->flags &= ~AUTOFS_INF_EXPIRING;
+		complete_all(&ino->expire_complete);
 		spin_unlock(&sbi->fs_lock);
 		dput(dentry);
 	}
diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index 2ed2a51..bdf190b 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -141,6 +141,7 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
 			 dentry, dentry->d_name.len, dentry->d_name.name);
 
 		status = autofs4_wait(sbi, dentry, NFY_NONE);
+		wait_for_completion(&ino->expire_complete);
 
 		DPRINTK("expire done status=%d", status);
 
@@ -227,14 +228,32 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
 	DPRINTK("dentry=%p %.*s oz_mode=%d nd->flags=%d",
 		dentry, dentry->d_name.len, dentry->d_name.name, oz_mode,
 		nd->flags);
-
-	/* If it's our master or we shouldn't trigger a mount we're done */
-	lookup_type = nd->flags & (TRIGGER_FLAGS | TRIGGER_INTENTS);
-	if (oz_mode ||
-	    !(lookup_type || dentry->d_flags & DCACHE_AUTOFS_PENDING))
+	/*
+	 * For an expire of a covered direct or offset mount we need
+	 * to beeak out of follow_down() at the autofs mount trigger
+	 * (d_mounted--), so we can see the expiring flag, and manage
+	 * the blocking and following here until the expire is completed.
+	 */
+	if (oz_mode) {
+		spin_lock(&sbi->fs_lock);
+		if (ino->flags & AUTOFS_INF_EXPIRING) {
+			spin_unlock(&sbi->fs_lock);
+			/* Follow down to our covering mount. */
+			if (!follow_down(&nd->path.mnt, &nd->path.dentry))
+				goto done;
+			/*
+			 * We shouldn't need to do this but we have no way
+			 * of knowing what may have been done so try a follow
+			 * just in case.
+			 */
+			autofs4_follow_mount(&nd->path.mnt, &nd->path.dentry);
+			goto done;
+		}
+		spin_unlock(&sbi->fs_lock);
 		goto done;
+	}
 
-	/* If an expire request is pending wait for it. */
+	/* If an expire request is pending everyone must wait. */
 	spin_lock(&sbi->fs_lock);
 	if (ino->flags & AUTOFS_INF_EXPIRING) {
 		spin_unlock(&sbi->fs_lock);
@@ -243,6 +262,7 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
 			dentry, dentry->d_name.len, dentry->d_name.name);
 
 		status = autofs4_wait(sbi, dentry, NFY_NONE);
+		wait_for_completion(&ino->expire_complete);
 
 		DPRINTK("request done status=%d", status);
 
@@ -250,10 +270,15 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
 	}
 	spin_unlock(&sbi->fs_lock);
 cont:
+	/* We trigger a mount for almost all flags */
+	lookup_type = nd->flags & (TRIGGER_FLAGS | TRIGGER_INTENTS);
+	if (!(lookup_type || dentry->d_flags & DCACHE_AUTOFS_PENDING))
+		goto done;
+
 	/*
-	 * If the dentry contains directories then it is an
-	 * autofs multi-mount with no root mount offset. So
-	 * don't try to mount it again.
+	 * If the dentry contains directories then it is an autofs
+	 * multi-mount with no root mount offset. So don't try to
+	 * mount it again.
 	 */
 	spin_lock(&dcache_lock);
 	if (dentry->d_flags & DCACHE_AUTOFS_PENDING ||
@@ -264,22 +289,22 @@ cont:
 		if (status)
 			goto out_error;
 
-		/*
-		 * The mount succeeded but if there is no root mount
-		 * it must be an autofs multi-mount with no root offset
-		 * so we don't need to follow the mount.
-		 */
-		if (d_mountpoint(dentry)) {
-			if (!autofs4_follow_mount(&nd->path.mnt,
-						  &nd->path.dentry)) {
-				status = -ENOENT;
-				goto out_error;
-			}
-		}
-
-		goto done;
+		goto follow;
 	}
 	spin_unlock(&dcache_lock);
+follow:
+	/*
+	 * If there is no root mount it must be an autofs
+	 * multi-mount with no root offset so we don't need
+	 * to follow it.
+	 */
+	if (d_mountpoint(dentry)) {
+		if (!autofs4_follow_mount(&nd->path.mnt,
+					  &nd->path.dentry)) {
+			status = -ENOENT;
+			goto out_error;
+		}
+	}
 
 done:
 	return NULL;
@@ -545,6 +570,7 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
 				expiring, expiring->d_name.len,
 				expiring->d_name.name);
 			autofs4_wait(sbi, expiring, NFY_NONE);
+			wait_for_completion(&ino->expire_complete);
 			DPRINTK("request completed");
 			goto cont;
 		}

[PATCH 6/7] autofs4 - reorganize expire pending wait function calls

[Ian Kent]

This patch re-orgnirzes the checking for and waiting on
active expires and elininates redundant checks.

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

---

 fs/autofs4/autofs_i.h |    1 +
 fs/autofs4/expire.c   |   29 +++++++++++++++++++
 fs/autofs4/root.c     |   75 +++++++------------------------------------------
 3 files changed, 40 insertions(+), 65 deletions(-)


diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index 4b40cbc..69a2f5c 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -163,6 +163,7 @@ void autofs4_free_ino(struct autofs_info *);
 
 /* Expiration */
 int is_autofs4_dentry(struct dentry *);
+int autofs4_expire_wait(struct dentry *dentry);
 int autofs4_expire_run(struct super_block *, struct vfsmount *,
 			struct autofs_sb_info *,
 			struct autofs_packet_expire __user *);
diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c
index 705b9f0..cdabb79 100644
--- a/fs/autofs4/expire.c
+++ b/fs/autofs4/expire.c
@@ -402,6 +402,35 @@ found:
 	return expired;
 }
 
+int autofs4_expire_wait(struct dentry *dentry)
+{
+	struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
+	struct autofs_info *ino = autofs4_dentry_ino(dentry);
+	int status;
+
+	/* Block on any pending expire */
+	spin_lock(&sbi->fs_lock);
+	if (ino->flags & AUTOFS_INF_EXPIRING) {
+		spin_unlock(&sbi->fs_lock);
+
+		DPRINTK("waiting for expire %p name=%.*s",
+			 dentry, dentry->d_name.len, dentry->d_name.name);
+
+		status = autofs4_wait(sbi, dentry, NFY_NONE);
+		wait_for_completion(&ino->expire_complete);
+
+		DPRINTK("expire done status=%d", status);
+
+		if (d_unhashed(dentry))
+			return -EAGAIN;
+
+		return status;
+	}
+	spin_unlock(&sbi->fs_lock);
+
+	return 0;
+}
+
 /* Perform an expiry operation */
 int autofs4_expire_run(struct super_block *sb,
 		      struct vfsmount *mnt,
diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index bdf190b..9c39fa7 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -130,34 +130,6 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
 	struct autofs_info *ino = autofs4_dentry_ino(dentry);
 	int status;
 
-	/* Block on any pending expiry here; invalidate the dentry
-           when expiration is done to trigger mount request with a new
-           dentry */
-	spin_lock(&sbi->fs_lock);
-	if (ino->flags & AUTOFS_INF_EXPIRING) {
-		spin_unlock(&sbi->fs_lock);
-
-		DPRINTK("waiting for expire %p name=%.*s",
-			 dentry, dentry->d_name.len, dentry->d_name.name);
-
-		status = autofs4_wait(sbi, dentry, NFY_NONE);
-		wait_for_completion(&ino->expire_complete);
-
-		DPRINTK("expire done status=%d", status);
-
-		/*
-		 * If the directory still exists the mount request must
-		 * continue otherwise it can't be followed at the right
-		 * time during the walk.
-		 */
-		status = d_invalidate(dentry);
-		if (status != -EBUSY)
-			return -EAGAIN;
-
-		goto cont;
-	}
-	spin_unlock(&sbi->fs_lock);
-cont:
 	DPRINTK("dentry=%p %.*s ino=%p",
 		 dentry, dentry->d_name.len, dentry->d_name.name, dentry->d_inode);
 
@@ -254,22 +226,8 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
 	}
 
 	/* If an expire request is pending everyone must wait. */
-	spin_lock(&sbi->fs_lock);
-	if (ino->flags & AUTOFS_INF_EXPIRING) {
-		spin_unlock(&sbi->fs_lock);
-
-		DPRINTK("waiting for active request %p name=%.*s",
-			dentry, dentry->d_name.len, dentry->d_name.name);
-
-		status = autofs4_wait(sbi, dentry, NFY_NONE);
-		wait_for_completion(&ino->expire_complete);
+	autofs4_expire_wait(dentry);
 
-		DPRINTK("request done status=%d", status);
-
-		goto cont;
-	}
-	spin_unlock(&sbi->fs_lock);
-cont:
 	/* We trigger a mount for almost all flags */
 	lookup_type = nd->flags & (TRIGGER_FLAGS | TRIGGER_INTENTS);
 	if (!(lookup_type || dentry->d_flags & DCACHE_AUTOFS_PENDING))
@@ -338,6 +296,14 @@ static int autofs4_revalidate(struct dentry *dentry, struct nameidata *nd)
 			return 1;
 
 		/*
+		 * If the directory has gone away due to an expire
+		 * we have been called as ->d_revalidate() and so
+		 * we need to return false and proceed to ->lookup().
+		 */
+		if (autofs4_expire_wait(dentry) == -EAGAIN)
+			return 0;
+
+		/*
 		 * A zero status is success otherwise we have a
 		 * negative error code.
 		 */
@@ -345,15 +311,6 @@ static int autofs4_revalidate(struct dentry *dentry, struct nameidata *nd)
 		if (status == 0)
 			return 1;
 
-		/*
-		 * A status of EAGAIN here means that the dentry has gone
-		 * away while waiting for an expire to complete. If we are
-		 * racing with expire lookup will wait for it so this must
-		 * be a revalidate and we need to send it to lookup.
-		 */
-		if (status == -EAGAIN)
-			return 0;
-
 		return status;
 	}
 	spin_unlock(&sbi->fs_lock);
@@ -563,19 +520,7 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
 		 * so it must have been successful, so just wait for it.
 		 */
 		ino = autofs4_dentry_ino(expiring);
-		spin_lock(&sbi->fs_lock);
-		if (ino->flags & AUTOFS_INF_EXPIRING) {
-			spin_unlock(&sbi->fs_lock);
-			DPRINTK("wait for incomplete expire %p name=%.*s",
-				expiring, expiring->d_name.len,
-				expiring->d_name.name);
-			autofs4_wait(sbi, expiring, NFY_NONE);
-			wait_for_completion(&ino->expire_complete);
-			DPRINTK("request completed");
-			goto cont;
-		}
-		spin_unlock(&sbi->fs_lock);
-cont:
+		autofs4_expire_wait(expiring);
 		spin_lock(&sbi->lookup_lock);
 		if (!list_empty(&ino->expiring))
 			list_del_init(&ino->expiring);

[PATCH 7/7] autofs4 - remove unused ioctls

[Ian Kent]

The ioctls AUTOFS_IOC_TOGGLEREGHOST and AUTOFS_IOC_ASKREGHOST were
added several years ago but what they were intended for has never
been implemented (as far as I'm aware noone uses them) so remove them.

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

---

 fs/autofs4/root.c        |   68 +---------------------------------------------
 fs/compat_ioctl.c        |    2 -
 include/linux/auto_fs4.h |    2 -
 3 files changed, 1 insertions(+), 71 deletions(-)


diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index 9c39fa7..b1536c1 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -25,7 +25,6 @@ static int autofs4_dir_rmdir(struct inode *,struct dentry *);
 static int autofs4_dir_mkdir(struct inode *,struct dentry *,int);
 static int autofs4_root_ioctl(struct inode *, struct file *,unsigned int,unsigned long);
 static int autofs4_dir_open(struct inode *inode, struct file *file);
-static int autofs4_root_readdir(struct file * filp, void * dirent, filldir_t filldir);
 static struct dentry *autofs4_lookup(struct inode *,struct dentry *, struct nameidata *);
 static void *autofs4_follow_link(struct dentry *, struct nameidata *);
 
@@ -36,7 +35,7 @@ const struct file_operations autofs4_root_operations = {
 	.open		= dcache_dir_open,
 	.release	= dcache_dir_close,
 	.read		= generic_read_dir,
-	.readdir	= autofs4_root_readdir,
+	.readdir	= dcache_readdir,
 	.ioctl		= autofs4_root_ioctl,
 };
 
@@ -71,28 +70,6 @@ const struct inode_operations autofs4_dir_inode_operations = {
 	.rmdir		= autofs4_dir_rmdir,
 };
 
-static int autofs4_root_readdir(struct file *file, void *dirent,
-				filldir_t filldir)
-{
-	struct autofs_sb_info *sbi = autofs4_sbi(file->f_path.dentry->d_sb);
-	int oz_mode = autofs4_oz_mode(sbi);
-
-	DPRINTK("called, filp->f_pos = %lld", file->f_pos);
-
-	/*
-	 * Don't set reghost flag if:
-	 * 1) f_pos is larger than zero -- we've already been here.
-	 * 2) we haven't even enabled reghosting in the 1st place.
-	 * 3) this is the daemon doing a readdir
-	 */
-	if (oz_mode && file->f_pos == 0 && sbi->reghost_enabled)
-		sbi->needs_reghost = 1;
-
-	DPRINTK("needs_reghost = %d", sbi->needs_reghost);
-
-	return dcache_readdir(file, dirent, filldir);
-}
-
 static int autofs4_dir_open(struct inode *inode, struct file *file)
 {
 	struct dentry *dentry = file->f_path.dentry;
@@ -865,44 +842,6 @@ static inline int autofs4_get_protosubver(struct autofs_sb_info *sbi, int __user
 }
 
 /*
- * Tells the daemon whether we need to reghost or not. Also, clears
- * the reghost_needed flag.
- */
-static inline int autofs4_ask_reghost(struct autofs_sb_info *sbi, int __user *p)
-{
-	int status;
-
-	DPRINTK("returning %d", sbi->needs_reghost);
-
-	status = put_user(sbi->needs_reghost, p);
-	if (status)
-		return status;
-
-	sbi->needs_reghost = 0;
-	return 0;
-}
-
-/*
- * Enable / Disable reghosting ioctl() operation
- */
-static inline int autofs4_toggle_reghost(struct autofs_sb_info *sbi, int __user *p)
-{
-	int status;
-	int val;
-
-	status = get_user(val, p);
-
-	DPRINTK("reghost = %d", val);
-
-	if (status)
-		return status;
-
-	/* turn on/off reghosting, with the val */
-	sbi->reghost_enabled = val;
-	return 0;
-}
-
-/*
 * Tells the daemon whether it can umount the autofs mount.
 */
 static inline int autofs4_ask_umount(struct vfsmount *mnt, int __user *p)
@@ -966,11 +905,6 @@ static int autofs4_root_ioctl(struct inode *inode, struct file *filp,
 	case AUTOFS_IOC_SETTIMEOUT:
 		return autofs4_get_set_timeout(sbi, p);
 
-	case AUTOFS_IOC_TOGGLEREGHOST:
-		return autofs4_toggle_reghost(sbi, p);
-	case AUTOFS_IOC_ASKREGHOST:
-		return autofs4_ask_reghost(sbi, p);
-
 	case AUTOFS_IOC_ASKUMOUNT:
 		return autofs4_ask_umount(filp->f_path.mnt, p);
 
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index 44e9f05..c6663c5 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -2290,8 +2290,6 @@ COMPATIBLE_IOCTL(AUTOFS_IOC_PROTOVER)
 COMPATIBLE_IOCTL(AUTOFS_IOC_EXPIRE)
 COMPATIBLE_IOCTL(AUTOFS_IOC_EXPIRE_MULTI)
 COMPATIBLE_IOCTL(AUTOFS_IOC_PROTOSUBVER)
-COMPATIBLE_IOCTL(AUTOFS_IOC_ASKREGHOST)
-COMPATIBLE_IOCTL(AUTOFS_IOC_TOGGLEREGHOST)
 COMPATIBLE_IOCTL(AUTOFS_IOC_ASKUMOUNT)
 /* Raw devices */
 COMPATIBLE_IOCTL(RAW_SETBIND)
diff --git a/include/linux/auto_fs4.h b/include/linux/auto_fs4.h
index 31a2954..b785c6f 100644
--- a/include/linux/auto_fs4.h
+++ b/include/linux/auto_fs4.h
@@ -98,8 +98,6 @@ union autofs_v5_packet_union {
 #define AUTOFS_IOC_EXPIRE_INDIRECT	AUTOFS_IOC_EXPIRE_MULTI
 #define AUTOFS_IOC_EXPIRE_DIRECT	AUTOFS_IOC_EXPIRE_MULTI
 #define AUTOFS_IOC_PROTOSUBVER		_IOR(0x93,0x67,int)
-#define AUTOFS_IOC_ASKREGHOST           _IOR(0x93,0x68,int)
-#define AUTOFS_IOC_TOGGLEREGHOST        _IOR(0x93,0x69,int)
 #define AUTOFS_IOC_ASKUMOUNT		_IOR(0x93,0x70,int)
 
 

Re: [PATCH 1/7] autofs4 - indirect dentry must almost always be positive

[Jeff Moyer]

Ian Kent <raven@themaw.net> writes:

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

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Cheers,

Jeff

>
> ---
>
>  fs/autofs4/waitq.c |   17 ++++++++++++++---
>  1 files changed, 14 insertions(+), 3 deletions(-)
>
>
> diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> index bcb6c52..35216d1 100644
> --- a/fs/autofs4/waitq.c
> +++ b/fs/autofs4/waitq.c
> @@ -328,9 +328,20 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
>  	if (sbi->catatonic)
>  		return -ENOENT;
>  
> -	if (!dentry->d_inode &&
> -	    (sbi->type & (AUTOFS_TYPE_DIRECT | AUTOFS_TYPE_OFFSET)))
> -		return -ENOENT;
> +	if (!dentry->d_inode) {
> +		/*
> +		 * A wait for a negative dentry is invalid for certain
> +		 * cases. A direct or offset mount "always" has its mount
> +		 * point directory created and so the request dentry must
> +		 * be positive or the map key doesn't exist. The situation
> +		 * is very similar for indirect mounts except only dentrys
> +		 * in the root of the autofs file system may be negative.
> +		 */
> +		if (sbi->type & (AUTOFS_TYPE_DIRECT|AUTOFS_TYPE_OFFSET))
> +			return -ENOENT;
> +		else if (!IS_ROOT(dentry->d_parent))
> +			return -ENOENT;
> +	}
>  
>  	name = kmalloc(NAME_MAX + 1, GFP_KERNEL);
>  	if (!name)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/7] autofs4 - cleanup redundant readir code

[Jeff Moyer]

Ian Kent <raven@themaw.net> writes:

> The mount triggering functionality of readdir and related functions
> is no longer used (and is quite broken as well). The unused portions
> have been removed.

I agree that this should be safe (and I also agree that the code was
broken!), but I'm not sure why you say that the code was no longer used.
It was still wired up until this patch, right?

Anyway:

Acked-by: Jeff Moyer <jmoyer@redhat.com>

>
> Signed-off-by: Ian Kent <raven@themaw.net>
>
> ---
>
>  fs/autofs4/root.c |  149 ++++++-----------------------------------------------
>  1 files changed, 16 insertions(+), 133 deletions(-)
>
>
> diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
> index 2a1a631..e842b0a 100644
> --- a/fs/autofs4/root.c
> +++ b/fs/autofs4/root.c
> @@ -25,8 +25,6 @@ static int autofs4_dir_rmdir(struct inode *,struct dentry *);
>  static int autofs4_dir_mkdir(struct inode *,struct dentry *,int);
>  static int autofs4_root_ioctl(struct inode *, struct file *,unsigned int,unsigned long);
>  static int autofs4_dir_open(struct inode *inode, struct file *file);
> -static int autofs4_dir_close(struct inode *inode, struct file *file);
> -static int autofs4_dir_readdir(struct file * filp, void * dirent, filldir_t filldir);
>  static int autofs4_root_readdir(struct file * filp, void * dirent, filldir_t filldir);
>  static struct dentry *autofs4_lookup(struct inode *,struct dentry *, struct nameidata *);
>  static void *autofs4_follow_link(struct dentry *, struct nameidata *);
> @@ -44,9 +42,9 @@ const struct file_operations autofs4_root_operations = {
>  
>  const struct file_operations autofs4_dir_operations = {
>  	.open		= autofs4_dir_open,
> -	.release	= autofs4_dir_close,
> +	.release	= dcache_dir_close,
>  	.read		= generic_read_dir,
> -	.readdir	= autofs4_dir_readdir,
> +	.readdir	= dcache_readdir,
>  };
>  
>  const struct inode_operations autofs4_indirect_root_inode_operations = {
> @@ -98,17 +96,7 @@ static int autofs4_root_readdir(struct file *file, void *dirent,
>  static int autofs4_dir_open(struct inode *inode, struct file *file)
>  {
>  	struct dentry *dentry = file->f_path.dentry;
> -	struct vfsmount *mnt = file->f_path.mnt;
>  	struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
> -	struct dentry *cursor;
> -	int status;
> -
> -	status = dcache_dir_open(inode, file);
> -	if (status)
> -		goto out;
> -
> -	cursor = file->private_data;
> -	cursor->d_fsdata = NULL;
>  
>  	DPRINTK("file=%p dentry=%p %.*s",
>  		file, dentry, dentry->d_name.len, dentry->d_name.name);
> @@ -116,129 +104,24 @@ static int autofs4_dir_open(struct inode *inode, struct file *file)
>  	if (autofs4_oz_mode(sbi))
>  		goto out;
>  
> -	if (autofs4_ispending(dentry)) {
> -		DPRINTK("dentry busy");
> -		dcache_dir_close(inode, file);
> -		status = -EBUSY;
> -		goto out;
> -	}
> -
> -	status = -ENOENT;
> -	if (!d_mountpoint(dentry) && dentry->d_op && dentry->d_op->d_revalidate) {
> -		struct nameidata nd;
> -		int empty, ret;
> -
> -		/* In case there are stale directory dentrys from a failed mount */
> -		spin_lock(&dcache_lock);
> -		empty = list_empty(&dentry->d_subdirs);
> +	/*
> +	 * An empty directory in an autofs file system is always a
> +	 * mount point. The daemon must have failed to mount this
> +	 * during lookup so it doesn't exist. This can happen, for
> +	 * example, if user space returns an incorrect status for a
> +	 * mount request. Otherwise we're doing a readdir on the
> +	 * autofs file system so just let the libfs routines handle
> +	 * it.
> +	 */
> +	spin_lock(&dcache_lock);
> +	if (!d_mountpoint(dentry) && __simple_empty(dentry)) {
>  		spin_unlock(&dcache_lock);
> -
> -		if (!empty)
> -			d_invalidate(dentry);
> -
> -		nd.flags = LOOKUP_DIRECTORY;
> -		ret = (dentry->d_op->d_revalidate)(dentry, &nd);
> -
> -		if (ret <= 0) {
> -			if (ret < 0)
> -				status = ret;
> -			dcache_dir_close(inode, file);
> -			goto out;
> -		}
> +		return -ENOENT;
>  	}
> +	spin_unlock(&dcache_lock);
>  
> -	if (d_mountpoint(dentry)) {
> -		struct file *fp = NULL;
> -		struct path fp_path = { .dentry = dentry, .mnt = mnt };
> -
> -		path_get(&fp_path);
> -
> -		if (!autofs4_follow_mount(&fp_path.mnt, &fp_path.dentry)) {
> -			path_put(&fp_path);
> -			dcache_dir_close(inode, file);
> -			goto out;
> -		}
> -
> -		fp = dentry_open(fp_path.dentry, fp_path.mnt, file->f_flags);
> -		status = PTR_ERR(fp);
> -		if (IS_ERR(fp)) {
> -			dcache_dir_close(inode, file);
> -			goto out;
> -		}
> -		cursor->d_fsdata = fp;
> -	}
> -	return 0;
> -out:
> -	return status;
> -}
> -
> -static int autofs4_dir_close(struct inode *inode, struct file *file)
> -{
> -	struct dentry *dentry = file->f_path.dentry;
> -	struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
> -	struct dentry *cursor = file->private_data;
> -	int status = 0;
> -
> -	DPRINTK("file=%p dentry=%p %.*s",
> -		file, dentry, dentry->d_name.len, dentry->d_name.name);
> -
> -	if (autofs4_oz_mode(sbi))
> -		goto out;
> -
> -	if (autofs4_ispending(dentry)) {
> -		DPRINTK("dentry busy");
> -		status = -EBUSY;
> -		goto out;
> -	}
> -
> -	if (d_mountpoint(dentry)) {
> -		struct file *fp = cursor->d_fsdata;
> -		if (!fp) {
> -			status = -ENOENT;
> -			goto out;
> -		}
> -		filp_close(fp, current->files);
> -	}
> -out:
> -	dcache_dir_close(inode, file);
> -	return status;
> -}
> -
> -static int autofs4_dir_readdir(struct file *file, void *dirent, filldir_t filldir)
> -{
> -	struct dentry *dentry = file->f_path.dentry;
> -	struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
> -	struct dentry *cursor = file->private_data;
> -	int status;
> -
> -	DPRINTK("file=%p dentry=%p %.*s",
> -		file, dentry, dentry->d_name.len, dentry->d_name.name);
> -
> -	if (autofs4_oz_mode(sbi))
> -		goto out;
> -
> -	if (autofs4_ispending(dentry)) {
> -		DPRINTK("dentry busy");
> -		return -EBUSY;
> -	}
> -
> -	if (d_mountpoint(dentry)) {
> -		struct file *fp = cursor->d_fsdata;
> -
> -		if (!fp)
> -			return -ENOENT;
> -
> -		if (!fp->f_op || !fp->f_op->readdir)
> -			goto out;
> -
> -		status = vfs_readdir(fp, filldir, dirent);
> -		file->f_pos = fp->f_pos;
> -		if (status)
> -			autofs4_copy_atime(file, fp);
> -		return status;
> -	}
>  out:
> -	return dcache_readdir(file, dirent, filldir);
> +	return dcache_dir_open(inode, file);
>  }
>  
>  static int try_to_fill_dentry(struct dentry *dentry, int flags)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/7] autofs4 - fix pending checks

[Jeff Moyer]

Ian Kent <raven@themaw.net> writes:

> There are two cases for which a dentry that has a pending mount request
> does not wait for completion. One is via autofs4_revalidate() and the
> other via autofs4_follow_link().
>
> In revalidate, after the mount point directory is created, but before
> the mount is done, the check in try_to_fill_dentry() can can fail to
> send the dentry to the wait queue since the dentry is positive and the
> lookup flags may contain only LOOKUP_FOLLOW. Although we don't trigger
> a mount for the LOOKUP_FOLLOW flag, if ther's one pending we might as
> well wait and use the mounted dentry for the lookup.
>
> In autofs4_follow_link() the dentry is not checked to see if it is
> pending so it may fail to call try_to_fill_dentry() and not wait
> for mount completion.
>
> A dentry that is pending must always be sent to the wait queue.
>
> Signed-off-by: Ian Kent <raven@themaw.net>

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>


>
> ---
>
>  fs/autofs4/root.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
>
>
> diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
> index e842b0a..2944b28 100644
> --- a/fs/autofs4/root.c
> +++ b/fs/autofs4/root.c
> @@ -177,7 +177,8 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
>  			return status;
>  		}
>  	/* Trigger mount for path component or follow link */
> -	} else if (flags & (TRIGGER_FLAGS | TRIGGER_INTENTS) ||
> +	} else if (dentry->d_flags & DCACHE_AUTOFS_PENDING ||
> +			flags & (TRIGGER_FLAGS | TRIGGER_INTENTS) ||
>  			current->link_count) {
>  		DPRINTK("waiting for mount name=%.*s",
>  			dentry->d_name.len, dentry->d_name.name);
> @@ -223,7 +224,8 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
>  
>  	/* If it's our master or we shouldn't trigger a mount we're done */
>  	lookup_type = nd->flags & (TRIGGER_FLAGS | TRIGGER_INTENTS);
> -	if (oz_mode || !lookup_type)
> +	if (oz_mode ||
> +	    !(lookup_type || dentry->d_flags & DCACHE_AUTOFS_PENDING))
>  		goto done;
>  
>  	/* If an expire request is pending wait for it. */
> @@ -242,7 +244,8 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
>  	 * don't try to mount it again.
>  	 */
>  	spin_lock(&dcache_lock);
> -	if (!d_mountpoint(dentry) && __simple_empty(dentry)) {
> +	if (dentry->d_flags & DCACHE_AUTOFS_PENDING ||
> +	    (!d_mountpoint(dentry) && __simple_empty(dentry))) {
>  		spin_unlock(&dcache_lock);
>  
>  		status = try_to_fill_dentry(dentry, 0);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/7] autofs4 - cleanup redundant readir code

[Ian Kent]

On Fri, 2008-07-18 at 10:47 -0400, Jeff Moyer wrote:
> Ian Kent <raven@themaw.net> writes:
> 
> > The mount triggering functionality of readdir and related functions
> > is no longer used (and is quite broken as well). The unused portions
> > have been removed.
> 
> I agree that this should be safe (and I also agree that the code was
> broken!), but I'm not sure why you say that the code was no longer used.
> It was still wired up until this patch, right?

Of course that's true but the times that this was called were
essentially due to races in the lookup code. So, partly due to our
recent changes in the lookup (and revalidate), and also due to
observations and suggestions from Al Viro, I believe we catch all cases
their now, as it should be.

> 
> Anyway:
> 
> Acked-by: Jeff Moyer <jmoyer@redhat.com>
> 
> >
> > Signed-off-by: Ian Kent <raven@themaw.net>
> >
> > ---
> >
> >  fs/autofs4/root.c |  149 ++++++-----------------------------------------------
> >  1 files changed, 16 insertions(+), 133 deletions(-)
> >
> >
> > diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
> > index 2a1a631..e842b0a 100644
> > --- a/fs/autofs4/root.c
> > +++ b/fs/autofs4/root.c
> > @@ -25,8 +25,6 @@ static int autofs4_dir_rmdir(struct inode *,struct dentry *);
> >  static int autofs4_dir_mkdir(struct inode *,struct dentry *,int);
> >  static int autofs4_root_ioctl(struct inode *, struct file *,unsigned int,unsigned long);
> >  static int autofs4_dir_open(struct inode *inode, struct file *file);
> > -static int autofs4_dir_close(struct inode *inode, struct file *file);
> > -static int autofs4_dir_readdir(struct file * filp, void * dirent, filldir_t filldir);
> >  static int autofs4_root_readdir(struct file * filp, void * dirent, filldir_t filldir);
> >  static struct dentry *autofs4_lookup(struct inode *,struct dentry *, struct nameidata *);
> >  static void *autofs4_follow_link(struct dentry *, struct nameidata *);
> > @@ -44,9 +42,9 @@ const struct file_operations autofs4_root_operations = {
> >  
> >  const struct file_operations autofs4_dir_operations = {
> >  	.open		= autofs4_dir_open,
> > -	.release	= autofs4_dir_close,
> > +	.release	= dcache_dir_close,
> >  	.read		= generic_read_dir,
> > -	.readdir	= autofs4_dir_readdir,
> > +	.readdir	= dcache_readdir,
> >  };
> >  
> >  const struct inode_operations autofs4_indirect_root_inode_operations = {
> > @@ -98,17 +96,7 @@ static int autofs4_root_readdir(struct file *file, void *dirent,
> >  static int autofs4_dir_open(struct inode *inode, struct file *file)
> >  {
> >  	struct dentry *dentry = file->f_path.dentry;
> > -	struct vfsmount *mnt = file->f_path.mnt;
> >  	struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
> > -	struct dentry *cursor;
> > -	int status;
> > -
> > -	status = dcache_dir_open(inode, file);
> > -	if (status)
> > -		goto out;
> > -
> > -	cursor = file->private_data;
> > -	cursor->d_fsdata = NULL;
> >  
> >  	DPRINTK("file=%p dentry=%p %.*s",
> >  		file, dentry, dentry->d_name.len, dentry->d_name.name);
> > @@ -116,129 +104,24 @@ static int autofs4_dir_open(struct inode *inode, struct file *file)
> >  	if (autofs4_oz_mode(sbi))
> >  		goto out;
> >  
> > -	if (autofs4_ispending(dentry)) {
> > -		DPRINTK("dentry busy");
> > -		dcache_dir_close(inode, file);
> > -		status = -EBUSY;
> > -		goto out;
> > -	}
> > -
> > -	status = -ENOENT;
> > -	if (!d_mountpoint(dentry) && dentry->d_op && dentry->d_op->d_revalidate) {
> > -		struct nameidata nd;
> > -		int empty, ret;
> > -
> > -		/* In case there are stale directory dentrys from a failed mount */
> > -		spin_lock(&dcache_lock);
> > -		empty = list_empty(&dentry->d_subdirs);
> > +	/*
> > +	 * An empty directory in an autofs file system is always a
> > +	 * mount point. The daemon must have failed to mount this
> > +	 * during lookup so it doesn't exist. This can happen, for
> > +	 * example, if user space returns an incorrect status for a
> > +	 * mount request. Otherwise we're doing a readdir on the
> > +	 * autofs file system so just let the libfs routines handle
> > +	 * it.
> > +	 */
> > +	spin_lock(&dcache_lock);
> > +	if (!d_mountpoint(dentry) && __simple_empty(dentry)) {
> >  		spin_unlock(&dcache_lock);
> > -
> > -		if (!empty)
> > -			d_invalidate(dentry);
> > -
> > -		nd.flags = LOOKUP_DIRECTORY;
> > -		ret = (dentry->d_op->d_revalidate)(dentry, &nd);
> > -
> > -		if (ret <= 0) {
> > -			if (ret < 0)
> > -				status = ret;
> > -			dcache_dir_close(inode, file);
> > -			goto out;
> > -		}
> > +		return -ENOENT;
> >  	}
> > +	spin_unlock(&dcache_lock);
> >  
> > -	if (d_mountpoint(dentry)) {
> > -		struct file *fp = NULL;
> > -		struct path fp_path = { .dentry = dentry, .mnt = mnt };
> > -
> > -		path_get(&fp_path);
> > -
> > -		if (!autofs4_follow_mount(&fp_path.mnt, &fp_path.dentry)) {
> > -			path_put(&fp_path);
> > -			dcache_dir_close(inode, file);
> > -			goto out;
> > -		}
> > -
> > -		fp = dentry_open(fp_path.dentry, fp_path.mnt, file->f_flags);
> > -		status = PTR_ERR(fp);
> > -		if (IS_ERR(fp)) {
> > -			dcache_dir_close(inode, file);
> > -			goto out;
> > -		}
> > -		cursor->d_fsdata = fp;
> > -	}
> > -	return 0;
> > -out:
> > -	return status;
> > -}
> > -
> > -static int autofs4_dir_close(struct inode *inode, struct file *file)
> > -{
> > -	struct dentry *dentry = file->f_path.dentry;
> > -	struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
> > -	struct dentry *cursor = file->private_data;
> > -	int status = 0;
> > -
> > -	DPRINTK("file=%p dentry=%p %.*s",
> > -		file, dentry, dentry->d_name.len, dentry->d_name.name);
> > -
> > -	if (autofs4_oz_mode(sbi))
> > -		goto out;
> > -
> > -	if (autofs4_ispending(dentry)) {
> > -		DPRINTK("dentry busy");
> > -		status = -EBUSY;
> > -		goto out;
> > -	}
> > -
> > -	if (d_mountpoint(dentry)) {
> > -		struct file *fp = cursor->d_fsdata;
> > -		if (!fp) {
> > -			status = -ENOENT;
> > -			goto out;
> > -		}
> > -		filp_close(fp, current->files);
> > -	}
> > -out:
> > -	dcache_dir_close(inode, file);
> > -	return status;
> > -}
> > -
> > -static int autofs4_dir_readdir(struct file *file, void *dirent, filldir_t filldir)
> > -{
> > -	struct dentry *dentry = file->f_path.dentry;
> > -	struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
> > -	struct dentry *cursor = file->private_data;
> > -	int status;
> > -
> > -	DPRINTK("file=%p dentry=%p %.*s",
> > -		file, dentry, dentry->d_name.len, dentry->d_name.name);
> > -
> > -	if (autofs4_oz_mode(sbi))
> > -		goto out;
> > -
> > -	if (autofs4_ispending(dentry)) {
> > -		DPRINTK("dentry busy");
> > -		return -EBUSY;
> > -	}
> > -
> > -	if (d_mountpoint(dentry)) {
> > -		struct file *fp = cursor->d_fsdata;
> > -
> > -		if (!fp)
> > -			return -ENOENT;
> > -
> > -		if (!fp->f_op || !fp->f_op->readdir)
> > -			goto out;
> > -
> > -		status = vfs_readdir(fp, filldir, dirent);
> > -		file->f_pos = fp->f_pos;
> > -		if (status)
> > -			autofs4_copy_atime(file, fp);
> > -		return status;
> > -	}
> >  out:
> > -	return dcache_readdir(file, dirent, filldir);
> > +	return dcache_dir_open(inode, file);
> >  }
> >  
> >  static int try_to_fill_dentry(struct dentry *dentry, int flags)
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/7] autofs4 - fix indirect mount pending expire race

[Jeff Moyer]

Ian Kent <raven@themaw.net> writes:

> The selection of a dentry for expiration and the setting of the
> AUTOFS_INF_EXPIRING flag isn't done atomically which can lead to
> lookups walking into an expiring mount.
>
> What happens is that an expire is initiated by the daemon and
> a dentry is selected for expire but, since there is no lock
> held between the selection and setting of the expiring flag,
> a process may find the flag clear and continue walking into
> the mount tree at the same time the daemon attempts the expire
> it.
>
> Signed-off-by: Ian Kent <raven@themaw.net>
>
> ---

[...]
>  static inline void autofs4_copy_atime(struct file *src, struct file *dst)
> diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c
> index 894fee5..19f5bea 100644
> --- a/fs/autofs4/expire.c
> +++ b/fs/autofs4/expire.c
> @@ -292,6 +292,8 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
>  	struct list_head *next;
>  	int do_now = how & AUTOFS_EXP_IMMEDIATE;
>  	int exp_leaves = how & AUTOFS_EXP_LEAVES;
> +	struct autofs_info *ino;
> +	unsigned int ino_count;
>  
>  	if (!root)
>  		return NULL;
> @@ -316,6 +318,9 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
>  		dentry = dget(dentry);
>  		spin_unlock(&dcache_lock);
>  
> +		spin_lock(&sbi->fs_lock);
> +		ino = autofs4_dentry_ino(dentry);
> +
>  		/*
>  		 * Case 1: (i) indirect mount or top level pseudo direct mount
>  		 *	   (autofs-4.1).
> @@ -326,6 +331,11 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
>  			DPRINTK("checking mountpoint %p %.*s",
>  				dentry, (int)dentry->d_name.len, dentry->d_name.name);
>  
> +			/* Path walk currently on this dentry? */
> +			ino_count = atomic_read(&ino->count) + 2;
> +			if (atomic_read(&dentry->d_count) > ino_count)
> +				goto next;
> +

It would be nice to document the +2.  The +1s (below) may be evident
given that we did a dget above, but still might merit mention.

>  			/* Can we umount this guy */
>  			if (autofs4_mount_busy(mnt, dentry))
>  				goto next;
> @@ -343,23 +353,25 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
>  
>  		/* Case 2: tree mount, expire iff entire tree is not busy */
>  		if (!exp_leaves) {
> -			/* Lock the tree as we must expire as a whole */
> -			spin_lock(&sbi->fs_lock);
> -			if (!autofs4_tree_busy(mnt, dentry, timeout, do_now)) {
> -				struct autofs_info *inf = autofs4_dentry_ino(dentry);
> +			/* Path walk currently on this dentry? */
> +			ino_count = atomic_read(&ino->count) + 1;
> +			if (atomic_read(&dentry->d_count) > ino_count)
> +				goto next;
>  
> -				/* Set this flag early to catch sys_chdir and the like */
> -				inf->flags |= AUTOFS_INF_EXPIRING;
> -				spin_unlock(&sbi->fs_lock);
> +			if (!autofs4_tree_busy(mnt, dentry, timeout, do_now)) {
>  				expired = dentry;
>  				goto found;
>  			}
> -			spin_unlock(&sbi->fs_lock);
>  		/*
>  		 * Case 3: pseudo direct mount, expire individual leaves
>  		 *	   (autofs-4.1).
>  		 */
>  		} else {
> +			/* Path walk currently on this dentry? */
> +			ino_count = atomic_read(&ino->count) + 1;
> +			if (atomic_read(&dentry->d_count) > ino_count)
> +				goto next;
> +
>  			expired = autofs4_check_leaves(mnt, dentry, timeout, do_now);
>  			if (expired) {
>  				dput(dentry);
> @@ -367,6 +379,7 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
>  			}
>  		}
>  next:
> +		spin_unlock(&sbi->fs_lock);
>  		dput(dentry);
>  		spin_lock(&dcache_lock);
>  		next = next->next;
> @@ -377,6 +390,9 @@ next:
>  found:
>  	DPRINTK("returning %p %.*s",
>  		expired, (int)expired->d_name.len, expired->d_name.name);
> +	ino = autofs4_dentry_ino(expired);

If we get here, ino is already set to the autofs4_dentry_ino(expired),
so this statement is redundant.

[...]
> diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
> index 2944b28..2ed2a51 100644
> --- a/fs/autofs4/root.c
> +++ b/fs/autofs4/root.c
> @@ -133,7 +133,10 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
>  	/* Block on any pending expiry here; invalidate the dentry
>             when expiration is done to trigger mount request with a new
>             dentry */
> -	if (ino && (ino->flags & AUTOFS_INF_EXPIRING)) {
> +	spin_lock(&sbi->fs_lock);
> +	if (ino->flags & AUTOFS_INF_EXPIRING) {
> +		spin_unlock(&sbi->fs_lock);
> +
>  		DPRINTK("waiting for expire %p name=%.*s",
>  			 dentry, dentry->d_name.len, dentry->d_name.name);

This is okay, since we wait on the AUTOFS_INF_EXPIRING flag in
validate_request.  That check is done outside the lock, but I doubt
there are issues with not seeing an update since you perform a schedule
there.

> @@ -149,8 +152,11 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
>  		status = d_invalidate(dentry);
>  		if (status != -EBUSY)
>  			return -EAGAIN;
> -	}
>  
> +		goto cont;
> +	}
> +	spin_unlock(&sbi->fs_lock);
> +cont:
>  	DPRINTK("dentry=%p %.*s ino=%p",
>  		 dentry, dentry->d_name.len, dentry->d_name.name, dentry->d_inode);
>  
> @@ -229,15 +235,21 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
>  		goto done;
>  
>  	/* If an expire request is pending wait for it. */
> -	if (ino && (ino->flags & AUTOFS_INF_EXPIRING)) {
> +	spin_lock(&sbi->fs_lock);
> +	if (ino->flags & AUTOFS_INF_EXPIRING) {
> +		spin_unlock(&sbi->fs_lock);
> +
>  		DPRINTK("waiting for active request %p name=%.*s",
>  			dentry, dentry->d_name.len, dentry->d_name.name);
>  
>  		status = autofs4_wait(sbi, dentry, NFY_NONE);
>  
>  		DPRINTK("request done status=%d", status);
> -	}
>  
> +		goto cont;
> +	}
> +	spin_unlock(&sbi->fs_lock);
> +cont:

could've done an:
} else
	spin_unlock(&sbi->fs_lock);

But, whatever...

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Re: [PATCH 4/7] autofs4 - fix indirect mount pending expire race

[Ian Kent]

On Fri, 2008-07-18 at 14:52 -0400, Jeff Moyer wrote:
> Ian Kent <raven@themaw.net> writes:
> 
> > The selection of a dentry for expiration and the setting of the
> > AUTOFS_INF_EXPIRING flag isn't done atomically which can lead to
> > lookups walking into an expiring mount.
> >
> > What happens is that an expire is initiated by the daemon and
> > a dentry is selected for expire but, since there is no lock
> > held between the selection and setting of the expiring flag,
> > a process may find the flag clear and continue walking into
> > the mount tree at the same time the daemon attempts the expire
> > it.
> >
> > Signed-off-by: Ian Kent <raven@themaw.net>
> >
> > ---
> 
> [...]
> >  static inline void autofs4_copy_atime(struct file *src, struct file *dst)
> > diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c
> > index 894fee5..19f5bea 100644
> > --- a/fs/autofs4/expire.c
> > +++ b/fs/autofs4/expire.c
> > @@ -292,6 +292,8 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
> >  	struct list_head *next;
> >  	int do_now = how & AUTOFS_EXP_IMMEDIATE;
> >  	int exp_leaves = how & AUTOFS_EXP_LEAVES;
> > +	struct autofs_info *ino;
> > +	unsigned int ino_count;
> >  
> >  	if (!root)
> >  		return NULL;
> > @@ -316,6 +318,9 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
> >  		dentry = dget(dentry);
> >  		spin_unlock(&dcache_lock);
> >  
> > +		spin_lock(&sbi->fs_lock);
> > +		ino = autofs4_dentry_ino(dentry);
> > +
> >  		/*
> >  		 * Case 1: (i) indirect mount or top level pseudo direct mount
> >  		 *	   (autofs-4.1).
> > @@ -326,6 +331,11 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
> >  			DPRINTK("checking mountpoint %p %.*s",
> >  				dentry, (int)dentry->d_name.len, dentry->d_name.name);
> >  
> > +			/* Path walk currently on this dentry? */
> > +			ino_count = atomic_read(&ino->count) + 2;
> > +			if (atomic_read(&dentry->d_count) > ino_count)
> > +				goto next;
> > +
> 
> It would be nice to document the +2.  The +1s (below) may be evident
> given that we did a dget above, but still might merit mention.
> 
> >  			/* Can we umount this guy */
> >  			if (autofs4_mount_busy(mnt, dentry))
> >  				goto next;
> > @@ -343,23 +353,25 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
> >  
> >  		/* Case 2: tree mount, expire iff entire tree is not busy */
> >  		if (!exp_leaves) {
> > -			/* Lock the tree as we must expire as a whole */
> > -			spin_lock(&sbi->fs_lock);
> > -			if (!autofs4_tree_busy(mnt, dentry, timeout, do_now)) {
> > -				struct autofs_info *inf = autofs4_dentry_ino(dentry);
> > +			/* Path walk currently on this dentry? */
> > +			ino_count = atomic_read(&ino->count) + 1;
> > +			if (atomic_read(&dentry->d_count) > ino_count)
> > +				goto next;
> >  
> > -				/* Set this flag early to catch sys_chdir and the like */
> > -				inf->flags |= AUTOFS_INF_EXPIRING;
> > -				spin_unlock(&sbi->fs_lock);
> > +			if (!autofs4_tree_busy(mnt, dentry, timeout, do_now)) {
> >  				expired = dentry;
> >  				goto found;
> >  			}
> > -			spin_unlock(&sbi->fs_lock);
> >  		/*
> >  		 * Case 3: pseudo direct mount, expire individual leaves
> >  		 *	   (autofs-4.1).
> >  		 */
> >  		} else {
> > +			/* Path walk currently on this dentry? */
> > +			ino_count = atomic_read(&ino->count) + 1;
> > +			if (atomic_read(&dentry->d_count) > ino_count)
> > +				goto next;
> > +
> >  			expired = autofs4_check_leaves(mnt, dentry, timeout, do_now);
> >  			if (expired) {
> >  				dput(dentry);

The expired dentry may be a different dentry!

> > @@ -367,6 +379,7 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
> >  			}
> >  		}
> >  next:
> > +		spin_unlock(&sbi->fs_lock);
> >  		dput(dentry);
> >  		spin_lock(&dcache_lock);
> >  		next = next->next;
> > @@ -377,6 +390,9 @@ next:
> >  found:
> >  	DPRINTK("returning %p %.*s",
> >  		expired, (int)expired->d_name.len, expired->d_name.name);
> > +	ino = autofs4_dentry_ino(expired);
> 
> If we get here, ino is already set to the autofs4_dentry_ino(expired),
> so this statement is redundant.

Almost but see above.

> 
> [...]
> > diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
> > index 2944b28..2ed2a51 100644
> > --- a/fs/autofs4/root.c
> > +++ b/fs/autofs4/root.c
> > @@ -133,7 +133,10 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
> >  	/* Block on any pending expiry here; invalidate the dentry
> >             when expiration is done to trigger mount request with a new
> >             dentry */
> > -	if (ino && (ino->flags & AUTOFS_INF_EXPIRING)) {
> > +	spin_lock(&sbi->fs_lock);
> > +	if (ino->flags & AUTOFS_INF_EXPIRING) {
> > +		spin_unlock(&sbi->fs_lock);
> > +
> >  		DPRINTK("waiting for expire %p name=%.*s",
> >  			 dentry, dentry->d_name.len, dentry->d_name.name);
> 
> This is okay, since we wait on the AUTOFS_INF_EXPIRING flag in
> validate_request.  That check is done outside the lock, but I doubt
> there are issues with not seeing an update since you perform a schedule
> there.
> 
> > @@ -149,8 +152,11 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
> >  		status = d_invalidate(dentry);
> >  		if (status != -EBUSY)
> >  			return -EAGAIN;
> > -	}
> >  
> > +		goto cont;
> > +	}
> > +	spin_unlock(&sbi->fs_lock);
> > +cont:
> >  	DPRINTK("dentry=%p %.*s ino=%p",
> >  		 dentry, dentry->d_name.len, dentry->d_name.name, dentry->d_inode);
> >  
> > @@ -229,15 +235,21 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
> >  		goto done;
> >  
> >  	/* If an expire request is pending wait for it. */
> > -	if (ino && (ino->flags & AUTOFS_INF_EXPIRING)) {
> > +	spin_lock(&sbi->fs_lock);
> > +	if (ino->flags & AUTOFS_INF_EXPIRING) {
> > +		spin_unlock(&sbi->fs_lock);
> > +
> >  		DPRINTK("waiting for active request %p name=%.*s",
> >  			dentry, dentry->d_name.len, dentry->d_name.name);
> >  
> >  		status = autofs4_wait(sbi, dentry, NFY_NONE);
> >  
> >  		DPRINTK("request done status=%d", status);
> > -	}
> >  
> > +		goto cont;
> > +	}
> > +	spin_unlock(&sbi->fs_lock);
> > +cont:
> 
> could've done an:
> } else
> 	spin_unlock(&sbi->fs_lock);
> 
> But, whatever...
> 
> Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Re: [PATCH 4/7] autofs4 - fix indirect mount pending expire race

[Jeff Moyer]

Ian Kent <raven@themaw.net> writes:

> On Fri, 2008-07-18 at 14:52 -0400, Jeff Moyer wrote:
>> Ian Kent <raven@themaw.net> writes:
>> >  		} else {
>> > +			/* Path walk currently on this dentry? */
>> > +			ino_count = atomic_read(&ino->count) + 1;
>> > +			if (atomic_read(&dentry->d_count) > ino_count)
>> > +				goto next;
>> > +
>> >  			expired = autofs4_check_leaves(mnt, dentry, timeout, do_now);
>> >  			if (expired) {
>> >  				dput(dentry);
>
> The expired dentry may be a different dentry!
>
>> >  found:
>> >  	DPRINTK("returning %p %.*s",
>> >  		expired, (int)expired->d_name.len, expired->d_name.name);
>> > +	ino = autofs4_dentry_ino(expired);
>> 
>> If we get here, ino is already set to the autofs4_dentry_ino(expired),
>> so this statement is redundant.
>
> Almost but see above.

Ah, you're right, I missed that.

Cheers,

Jeff

Re: [PATCH 5/7] autofs4 - fix direct mount pending expire race

[Jeff Moyer]

Ian Kent <raven@themaw.net> writes:

> For direct and offset type mounts that are covered by another mount
> we cannot check the AUTOFS_INF_EXPIRING flag during a path walk
> which leads to lookups walking into an expiring mount while it is
> being expired.
>
> For example, for the direct multi-mount map entry with a couple of
> offsets:
>
> /race/mm1  /      <server1>:/<path1>
>            /om1   <server2>:/<path2>
>            /om2   <server1>:/<path3>
>
> an autofs trigger mount is mounted on /race/mm1 and when accessed
> it is over mounted and trigger mounts made for /race/mm1/om1 and
> /race/mm1/om2. So it isn't possible for path walks to see the
> expiring flag at all and they happily walk into the file system
> while it is expiring.
>
> When expiring these mounts follow_down() must stop at the autofs
> mount and all processes must block in the ->follow_link() method
> (except the daemon) until the expire is complete. This is done by
> decrementing the d_mounted field of the autofs trigger mount root
> dentry until the expire is completed. In ->follow_link() all
> processes wait on the expire and the mount following is completed
> for the daemon until the expire is complete.
>
> Signed-off-by: Ian Kent <raven@themaw.net>
>
> ---
>
>  fs/autofs4/autofs_i.h |    3 ++
>  fs/autofs4/expire.c   |   16 +++++++++--
>  fs/autofs4/root.c     |   72 +++++++++++++++++++++++++++++++++----------------
>  3 files changed, 65 insertions(+), 26 deletions(-)
>
>
> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> index 5d90ed3..4b40cbc 100644
> --- a/fs/autofs4/autofs_i.h
> +++ b/fs/autofs4/autofs_i.h
> @@ -52,6 +52,8 @@ struct autofs_info {
>  
>  	int		flags;
>  
> +	struct completion expire_complete;
> +
>  	struct list_head active;
>  	struct list_head expiring;
>  
> @@ -69,6 +71,7 @@ struct autofs_info {
>  };
>  
>  #define AUTOFS_INF_EXPIRING	(1<<0) /* dentry is in the process of expiring */
> +#define AUTOFS_INF_MOUNTPOINT	(1<<1) /* mountpoint status for direct expire */
>  
>  struct autofs_wait_queue {
>  	wait_queue_head_t queue;
> diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c
> index 19f5bea..705b9f0 100644
> --- a/fs/autofs4/expire.c
> +++ b/fs/autofs4/expire.c
> @@ -259,13 +259,15 @@ static struct dentry *autofs4_expire_direct(struct super_block *sb,
>  	now = jiffies;
>  	timeout = sbi->exp_timeout;
>  
> -	/* Lock the tree as we must expire as a whole */
>  	spin_lock(&sbi->fs_lock);
>  	if (!autofs4_direct_busy(mnt, root, timeout, do_now)) {
>  		struct autofs_info *ino = autofs4_dentry_ino(root);
> -
> -		/* Set this flag early to catch sys_chdir and the like */
> +		if (d_mountpoint(root)) {
> +			ino->flags |= AUTOFS_INF_MOUNTPOINT;
> +			root->d_mounted--;
> +		}

This makes me uneasy.  This should take d_mounted to zero.  Then, when
the daemon actually does the unmount, won't the d_mounted drop below
zero?  Following calls to d_mountpoint will return a negative value, but
everyone treats it as a boolean, so it will evaluate to true for a brief
time.  Or did I miss something?

Cheers,

Jeff

Re: [PATCH 7/7] autofs4 - remove unused ioctls

[Jeff Moyer]

Ian Kent <raven@themaw.net> writes:

> The ioctls AUTOFS_IOC_TOGGLEREGHOST and AUTOFS_IOC_ASKREGHOST were
> added several years ago but what they were intended for has never
> been implemented (as far as I'm aware noone uses them) so remove them.

Yeah, the only other consumer of the autofs interfaces (that I know) is
autodir, and that doesn't use the reghost ioctls.

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

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Re: [PATCH 5/7] autofs4 - fix direct mount pending expire race

[Ian Kent]

On Fri, 2008-07-18 at 16:08 -0400, Jeff Moyer wrote:
> Ian Kent <raven@themaw.net> writes:
> 
> > For direct and offset type mounts that are covered by another mount
> > we cannot check the AUTOFS_INF_EXPIRING flag during a path walk
> > which leads to lookups walking into an expiring mount while it is
> > being expired.
> >
> > For example, for the direct multi-mount map entry with a couple of
> > offsets:
> >
> > /race/mm1  /      <server1>:/<path1>
> >            /om1   <server2>:/<path2>
> >            /om2   <server1>:/<path3>
> >
> > an autofs trigger mount is mounted on /race/mm1 and when accessed
> > it is over mounted and trigger mounts made for /race/mm1/om1 and
> > /race/mm1/om2. So it isn't possible for path walks to see the
> > expiring flag at all and they happily walk into the file system
> > while it is expiring.
> >
> > When expiring these mounts follow_down() must stop at the autofs
> > mount and all processes must block in the ->follow_link() method
> > (except the daemon) until the expire is complete. This is done by
> > decrementing the d_mounted field of the autofs trigger mount root
> > dentry until the expire is completed. In ->follow_link() all
> > processes wait on the expire and the mount following is completed
> > for the daemon until the expire is complete.
> >
> > Signed-off-by: Ian Kent <raven@themaw.net>
> >
> > ---
> >
> >  fs/autofs4/autofs_i.h |    3 ++
> >  fs/autofs4/expire.c   |   16 +++++++++--
> >  fs/autofs4/root.c     |   72 +++++++++++++++++++++++++++++++++----------------
> >  3 files changed, 65 insertions(+), 26 deletions(-)
> >
> >
> > diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> > index 5d90ed3..4b40cbc 100644
> > --- a/fs/autofs4/autofs_i.h
> > +++ b/fs/autofs4/autofs_i.h
> > @@ -52,6 +52,8 @@ struct autofs_info {
> >  
> >  	int		flags;
> >  
> > +	struct completion expire_complete;
> > +
> >  	struct list_head active;
> >  	struct list_head expiring;
> >  
> > @@ -69,6 +71,7 @@ struct autofs_info {
> >  };
> >  
> >  #define AUTOFS_INF_EXPIRING	(1<<0) /* dentry is in the process of expiring */
> > +#define AUTOFS_INF_MOUNTPOINT	(1<<1) /* mountpoint status for direct expire */
> >  
> >  struct autofs_wait_queue {
> >  	wait_queue_head_t queue;
> > diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c
> > index 19f5bea..705b9f0 100644
> > --- a/fs/autofs4/expire.c
> > +++ b/fs/autofs4/expire.c
> > @@ -259,13 +259,15 @@ static struct dentry *autofs4_expire_direct(struct super_block *sb,
> >  	now = jiffies;
> >  	timeout = sbi->exp_timeout;
> >  
> > -	/* Lock the tree as we must expire as a whole */
> >  	spin_lock(&sbi->fs_lock);
> >  	if (!autofs4_direct_busy(mnt, root, timeout, do_now)) {
> >  		struct autofs_info *ino = autofs4_dentry_ino(root);
> > -
> > -		/* Set this flag early to catch sys_chdir and the like */
> > +		if (d_mountpoint(root)) {
> > +			ino->flags |= AUTOFS_INF_MOUNTPOINT;
> > +			root->d_mounted--;
> > +		}
> 
> This makes me uneasy.  This should take d_mounted to zero.  Then, when
> the daemon actually does the unmount, won't the d_mounted drop below
> zero?  Following calls to d_mountpoint will return a negative value, but
> everyone treats it as a boolean, so it will evaluate to true for a brief
> time.  Or did I miss something?

Yes, I thought about doing exactly that.

But the thing that effects d_mounted is mounted on the dentry and so
d_mounted may be decremented during the expire. So if we set it
explicitly it would be incorrect at the end. While the
decrement/increment isn't always correct throughout the expire we need
to handle the mount following in ->follow_link() anyway and then the
decrement/increment ends up with the correct value once the expire is
complete.

One problem that has occurred to me is that user space could could
manually umount it just when we change it. So a follow up patch to add a
lock around the increment/decrement in fs/namespace.c and
fs/autofs4/expire.c is in order. I'm having a look at that now.

Ian

Re: [PATCH 5/7] autofs4 - fix direct mount pending expire race

[Ian Kent]

On Sat, 2008-07-19 at 10:09 +0800, Ian Kent wrote:
> On Fri, 2008-07-18 at 16:08 -0400, Jeff Moyer wrote:
> > 
> > This makes me uneasy.  This should take d_mounted to zero.  Then, when
> > the daemon actually does the unmount, won't the d_mounted drop below
> > zero?  Following calls to d_mountpoint will return a negative value, but
> > everyone treats it as a boolean, so it will evaluate to true for a brief
> > time.  Or did I miss something?
> 
> Yes, I thought about doing exactly that.
> 
> But the thing that effects d_mounted is mounted on the dentry and so
> d_mounted may be decremented during the expire. So if we set it
> explicitly it would be incorrect at the end. While the
> decrement/increment isn't always correct throughout the expire we need
> to handle the mount following in ->follow_link() anyway and then the
> decrement/increment ends up with the correct value once the expire is
> complete.
> 
> One problem that has occurred to me is that user space could could
> manually umount it just when we change it. So a follow up patch to add a
> lock around the increment/decrement in fs/namespace.c and
> fs/autofs4/expire.c is in order. I'm having a look at that now.

DOH, this is rubbish.

I often wonder why I forget what I've done so quickly.

Of course user space can't umount this because every process except the
daemon is blocked in ->follow_link() during the expire. But we can't
rely on the the return from user space and cannot know if the mount was
actually umounted so the decrement/increment will maintain the
status-quo. If d_mounted does become negative during the expire then we
will still be sent to ->follow_link() as follow_down() will fail.

Ian