contained_in_local_fs will *always* return true

Commit d3ce65e05d1 (autofs-5.0.3 - allow directory create on NFS root), introduced a bug where contained_in_local_fs would *always* return true.

Since the goal of contained_in_local_fs is only to determine if the path would be created on a local file system, it's enough to check a few things directly without parsing the mount table at all.

We can check via stat calls:
1) If parent is the root directory
2) If the parent is located on the root file system
3) If the parent is located within a file system mounted via a block device

Large numbers of direct mounts when using /proc/self/mounts instead of /etc/mtab result in very slow startup time. In testing, we observed ~8k mounts taking over 6 hours to complete.

This is due to reading /proc/self/mounts for every path component of every mount. For my test case, that turns out to be about 119k times for a total of just under 400 GB read. If it were a flat file, it would be also be slow, but /proc/self/mounts is dynamically generated every time it's read.

By avoiding reading /proc/self/mounts many times, startup time with 8k direct mounts drops from ~6 hours to about 25 seconds.

Signed-off-by Jeff Mahoney <[email protected]>

---
 daemon/automount.c |   47 ++++++++++++++++++++++++++++++++++++++++++++---
 include/mounts.h   |    1 -
 lib/mounts.c       |   45 ---------------------------------------------
 3 files changed, 44 insertions(+), 49 deletions(-)

daemon/automount.c +44 -3

@@ -98,6 +98,46 @@ static int umount_all(struct autofs_poin
 
 extern struct master *master_list;
 
+#define SYSFS_PATTERN "/sys/dev/block/%d:%d"
+static int contained_in_local_fs(const char *parent)
+{
+	char buf[128];
+	struct stat st, root;
+	int ret;
+
+	/* The root directory is always considered local, even if it's not. */
+	if (!*parent)
+		return 1;
+
+	ret = stat("/", &root);
+	if (ret != 0) {
+		logerr("error: stat failed on /: %s", strerror(errno));
+		return 0;
+	}
+
+	ret = stat(parent, &st);
+	if (ret)
+		return 0;
+	/*
+	 * If the parent is on the root file system, it's local.
+	 */
+	if (root.st_dev == st.st_dev)
+		return 1;
+
+	/*
+	 * If the sysfs node for the block device exists, it's local.
+	 */
+	ret = snprintf(buf, sizeof(buf), SYSFS_PATTERN,
+		       major(st.st_dev), minor(st.st_dev));
+	if (ret != 0) {
+		logerr("error: not enough space for sysfs path");
+		return 0;
+	}
+
+	ret = stat(buf, &st);
+	return (ret == 0);
+}
+
 static int do_mkdir(const char *parent, const char *path, mode_t mode)
 {
 	int status;

@@ -114,14 +154,15 @@ static int do_mkdir(const char *parent,
 	}
 
 	/*
-	 * If we're trying to create a directory within an autofs fs
-	 * or the path is contained in a localy mounted fs go ahead.
+	 * If we're trying to create a directory within an autofs fs,
+	 * the parent is the root directory, or the path is contained
+	 * in a locally mounted fs go ahead.
 	 */
 	status = -1;
 	if (*parent)
 		status = statfs(parent, &fs);
 	if ((status != -1 && fs.f_type == (__SWORD_TYPE) AUTOFS_SUPER_MAGIC) ||
-	    contained_in_local_fs(path)) {
+	    contained_in_local_fs(parent)) {
 		mode_t mask = umask(0022);
 		int ret = mkdir(path, mode);
 		(void) umask(mask);

include/mounts.h +0 -1

@@ -96,7 +96,6 @@ char *make_mnt_name_string(char *path);
 struct mnt_list *get_mnt_list(const char *table, const char *path, int include);
 struct mnt_list *reverse_mnt_list(struct mnt_list *list);
 void free_mnt_list(struct mnt_list *list);
-int contained_in_local_fs(const char *path);
 int is_mounted(const char *table, const char *path, unsigned int type);
 int has_fstab_option(const char *opt);
 char *get_offset(const char *prefix, char *offset,

lib/mounts.c +0 -45

@@ -636,51 +636,6 @@ void free_mnt_list(struct mnt_list *list
 	}
 }
 
-int contained_in_local_fs(const char *path)
-{
-	struct mnt_list *mnts, *this;
-	size_t pathlen = strlen(path);
-	int ret;
-
-	if (!path || !pathlen || pathlen > PATH_MAX)
-		return 0;
-
-	mnts = get_mnt_list(_PATH_MOUNTED, "/", 1);
-	if (!mnts)
-		return 0;
-
-	ret = 0;
-
-	for (this = mnts; this != NULL; this = this->next) {
-		size_t len = strlen(this->path);
-
-		if (!strncmp(path, this->path, len)) {
-			if (len > 1 && pathlen > len && path[len] != '/')
-				continue;
-			else if (len == 1 && this->path[0] == '/') {
-				/*
-				 * always return true on rootfs, we don't
-				 * want to break diskless clients.
-				 */
-				ret = 1;
-			} else if (this->fs_name[0] == '/') {
-				if (strlen(this->fs_name) > 1) {
-					if (this->fs_name[1] != '/')
-						ret = 1;
-				} else
-					ret = 1;
-			} else if (!strncmp("LABEL=", this->fs_name, 6) ||
-				   !strncmp("UUID=", this->fs_name, 5))
-				ret = 1;
-			break;
-		}
-	}
-
-	free_mnt_list(mnts);
-
-	return ret;
-}
-
 static int table_is_mounted(const char *table, const char *path, unsigned int type)
 {
 	struct mntent *mnt;

Hi autofs maintainers -

I'm investigating a bug in which the user has > 8k direct mounts set up.
As documented in the README, this performs very, very badly and I'm
looking at ways to improve that situation.

In my research, I discovered commit d3ce65e05d1 (autofs-5.0.3 - allow directory create on NFS root), which was intended to return true for rootfs. It would pass that test. It also returns true for every other
case as long as there is *any* root file system in the mount table.
So,
ultimately, we read the mount table for every path component of every direct mount for no benefit whatsoever.

====8<====

for (this = mnts; this != NULL; this = this->next) {
        size_t len = strlen(this->path);

        if (!strncmp(path, this->path, len)) {
                if (len > 1 && pathlen > len && path[len] != '/')
                        continue;
                else if (len == 1 && this->path[0] == '/') {

			/*
			 * jeffm -> This will cause the entire
			 * function to always return true.  It
			 * turns the loop into a test to see if
			 * anything is mounted on /.
			 */

                        /*
                         * always return true on rootfs, we don't
                         * want to break diskless clients.
                         */
                        ret = 1;
                } else if (this->fs_name[0] == '/') {
                        if (strlen(this->fs_name) > 1) {
                                if (this->fs_name[1] != '/')
                                        ret = 1;
                        } else
                                ret = 1;
                } else if (!strncmp("LABEL=", this->fs_name, 6) ||
                           !strncmp("UUID=", this->fs_name, 5))
                        ret = 1;
                break;
        }
}

====>8====

This commit landed in 2008 and, since it hasn't changed since then, I assume there haven't been any bugs reported. I had trouble trying to locate a mailing list archive for the autofs list, so I can't confirm that. Googling for contained_in_local_fs really only showed the thread
that proposed the patch initially.

So, I suppose my question is this: Since this bug has existed for nearly 8 years already, would fixing it at this point be considered a regression?

If so, we can skip all the checking in do_mkdir entirely, which also removes the performance impact of large numbers of direct mounts while using /proc/self/mounts.

If not, how do we move forward ensuring minimal breakage? I have a patch worked up that skips the mount table parsing entirely and just tests to see if the parent is either on the root file system, regardless
of file system type, or contained in a file system mounted on a block device (grab st_dev and see if it exists in /sys/dev/block). Is that enough or is there a corner case I'm missing?

Commit d3ce65e05d1 (autofs-5.0.3 - allow directory create on NFS root),
introduced a bug where contained_in_local_fs would *always* return true.

Since the goal of contained_in_local_fs is only to determine if the path would be created on a local file system, it's enough to check a few things directly without parsing the mount table at all.

We can check via stat calls:
1) If parent is the root directory
2) If the parent is located on the root file system
3) If the parent is located within a file system mounted via a block device

Large numbers of direct mounts when using /proc/self/mounts instead of /etc/mtab result in very slow startup time. In testing, we observed
~8k mounts taking over 6 hours to complete.

This is due to reading /proc/self/mounts for every path component of every mount. For my test case, that turns out to be about 119k times for a total of just under 400 GB read. If it were a flat file, it would be also be slow, but /proc/self/mounts is dynamically generated every time it's read.

By avoiding reading /proc/self/mounts many times, startup time with 8k direct mounts drops from ~6 hours to about 25 seconds.

Signed-off-by Jeff Mahoney <[email protected]>

---
 daemon/automount.c |   47
++++++++++++++++++++++++++++++++++++++++++++---
 include/mounts.h   |    1 -
 lib/mounts.c       |   45 -------------------------------------------
--
 3 files changed, 44 insertions(+), 49 deletions(-)

daemon/automount.c +8 -0

@@ -98,6 +98,46 @@ static int umount_all(struct autofs_poin
 
 extern struct master *master_list;
 
+#define SYSFS_PATTERN "/sys/dev/block/%d:%d"
+static int contained_in_local_fs(const char *parent)
+{
+	char buf[128];
+	struct stat st, root;
+	int ret;
+
+	/* The root directory is always considered local, even if

it's not. */

daemon/automount.c +6 -0

+	if (!*parent)
+		return 1;
+
+	ret = stat("/", &root);
+	if (ret != 0) {
+		logerr("error: stat failed on /: %s",

strerror(errno));

daemon/automount.c +1 -0

+		return 0;

daemon/automount.c +5 -0

+
+	ret = stat(buf, &st);
+	return (ret == 0);
+}
+
 static int do_mkdir(const char *parent, const char *path, mode_t

mode)
{

daemon/automount.c +3 -2

 	int status;

@@ -114,14 +154,15 @@ static int do_mkdir(const char *parent,
 	}
 
 	/*
-	 * If we're trying to create a directory within an autofs fs
-	 * or the path is contained in a localy mounted fs go ahead.
+	 * If we're trying to create a directory within an autofs fs,
+	 * the parent is the root directory, or the path is contained
+	 * in a locally mounted fs go ahead.
 	 */
 	status = -1;
 	if (*parent)
 		status = statfs(parent, &fs);
 	if ((status != -1 && fs.f_type == (__SWORD_TYPE)

AUTOFS_SUPER_MAGIC) ||

daemon/automount.c +1 -1

-	    contained_in_local_fs(path)) {
+	    contained_in_local_fs(parent)) {
 		mode_t mask = umask(0022);
 		int ret = mkdir(path, mode);
 		(void) umask(mask);

include/mounts.h +0 -1

@@ -96,7 +96,6 @@ char *make_mnt_name_string(char *path);
 struct mnt_list *get_mnt_list(const char *table, const char *path,
 int include);
 struct mnt_list *reverse_mnt_list(struct mnt_list *list);
 void free_mnt_list(struct mnt_list *list);
-int contained_in_local_fs(const char *path);
 int is_mounted(const char *table, const char *path, unsigned int
 type);

int has_fstab_option(const char *opt); char *get_offset(const char *prefix, char *offset,

lib/mounts.c +0 -24

@@ -636,51 +636,6 @@ void free_mnt_list(struct mnt_list *list
 	}
 }
 
-int contained_in_local_fs(const char *path)
-{
-	struct mnt_list *mnts, *this;
-	size_t pathlen = strlen(path);
-	int ret;
-
-	if (!path || !pathlen || pathlen > PATH_MAX)
-		return 0;
-
-	mnts = get_mnt_list(_PATH_MOUNTED, "/", 1);
-	if (!mnts)
-		return 0;
-
-	ret = 0;
-
-	for (this = mnts; this != NULL; this = this->next) {
-		size_t len = strlen(this->path);
-
-		if (!strncmp(path, this->path, len)) {
-			if (len > 1 && pathlen > len && path[len] !=
 '/')
-				continue;
-			else if (len == 1 && this->path[0] == '/') {
-				/*
-				 * always return true on rootfs, we

don't

lib/mounts.c +0 -10

-				 * want to break diskless clients.
-				 */
-				ret = 1;
-			} else if (this->fs_name[0] == '/') {
-				if (strlen(this->fs_name) > 1) {
-					if (this->fs_name[1] != '/')
-						ret = 1;
-				} else
-					ret = 1;
-			} else if (!strncmp("LABEL=", this->fs_name,

6) ||

lib/mounts.c +0 -1

-				   !strncmp("UUID=", this->fs_name,

5))

lib/mounts.c +0 -10

-				ret = 1;
-			break;
-		}
-	}
-
-	free_mnt_list(mnts);
-
-	return ret;
-}
-
 static int table_is_mounted(const char *table, const char *path,

unsigned int type)
{
struct mntent *mnt;

Quoted 15 lines · Jeff Mahoney

Commit d3ce65e05d1 (autofs-5.0.3 - allow directory create on NFS root),
introduced a bug where contained_in_local_fs would *always* return true.

Since the goal of contained_in_local_fs is only to determine if the path would be created on a local file system, it's enough to check a few things directly without parsing the mount table at all.

We can check via stat calls:
1) If parent is the root directory
2) If the parent is located on the root file system
3) If the parent is located within a file system mounted via a block device

Quoted 4 lines

Large numbers of direct mounts when using /proc/self/mounts instead of /etc/mtab result in very slow startup time. In testing, we observed
~8k mounts taking over 6 hours to complete.

When this was originally done the mtab was (usually) an actual file so the autofs mounts weren't present. So mostly it was fairly small.

But now, with the symlinking of the mtab to a proc filesystem, this is a big problem.

Quoted 9 lines

This is due to reading /proc/self/mounts for every path component of every mount. For my test case, that turns out to be about 119k times for a total of just under 400 GB read. If it were a flat file, it would be also be slow, but /proc/self/mounts is dynamically generated every time it's read.

By avoiding reading /proc/self/mounts many times, startup time with 8k direct mounts drops from ~6 hours to about 25 seconds.

Yeah, I get that.

In some tests done before the symlinking became the norm I saw direct mount tables of over 20k entries taking between 20-30 seconds or a bit more (can't quite remember now, it was a long time ago) but nothing like this.

I thought that was bad so this really needs to be fixed.

I am puzzled why I haven't seen other bug reports.

Quoted 33 lines

Signed-off-by Jeff Mahoney <[email protected]>

---
 daemon/automount.c |   47
++++++++++++++++++++++++++++++++++++++++++++---
 include/mounts.h   |    1 -
 lib/mounts.c       |   45 -------------------------------------------
--
 3 files changed, 44 insertions(+), 49 deletions(-)

daemon/automount.c +8 -0

@@ -98,6 +98,46 @@ static int umount_all(struct autofs_poin
 
 extern struct master *master_list;
 
+#define SYSFS_PATTERN "/sys/dev/block/%d:%d"
+static int contained_in_local_fs(const char *parent)
+{
+	char buf[128];
+	struct stat st, root;
+	int ret;
+
+	/* The root directory is always considered local, even if

it's not. */

daemon/automount.c +6 -0

+	if (!*parent)
+		return 1;
+
+	ret = stat("/", &root);
+	if (ret != 0) {
+		logerr("error: stat failed on /: %s",

strerror(errno));

daemon/automount.c +1 -0

+		return 0;

daemon/automount.c static int umount_all(struct autofs_poin · line 125

Quoted 10 lines

daemon/automount.c +10 -0

+	}
+
+	ret = stat(parent, &st);
+	if (ret)
+		return 0;
+	/*
+	 * If the parent is on the root file system, it's local.
+	 */
+	if (root.st_dev == st.st_dev)
+		return 1;

I'm struggling to remember now.

I think the problem was only when the root itself was an NFS file system and automount was trying to create a directory.

daemon/automount.c static int umount_all(struct autofs_poin · line 135

Quoted 10 lines

daemon/automount.c +10 -0

+
+	/*
+	 * If the sysfs node for the block device exists, it's local.
+	 */
+	ret = snprintf(buf, sizeof(buf), SYSFS_PATTERN,
+		       major(st.st_dev), minor(st.st_dev));
+	if (ret != 0) {
+		logerr("error: not enough space for sysfs path");
+		return 0;
+	}

This is the only bit that concerns me.

Not because I think it's wrong, on the contrary, it looks ok.

I just can't think of cases where it might not cover what's needed.

After all, it's already been checked if the parent is an autofs filesystem which should take care of the bulk of cases except for longer direct mount paths, but will often not be the case for offset mounts.

For example a mount entry with offset mounts like:

<direct or indirect key>     /      server:/path \
                             /other server:/otherpath \
                             ...

But the intermediate path components of a direct mount path (and offset mount path) must already exist if within a remote mount. So the existenc e case, checked before this, should cover these cases.

Let me think about it a little longer.

Quoted 48 lines · Ian Kent

Quoted 17 lines · Jeff Mahoney

Commit d3ce65e05d1 (autofs-5.0.3 - allow directory create on NFS root),
introduced a bug where contained_in_local_fs would *always* return true.

Since the goal of contained_in_local_fs is only to determine if the
path would be created on a local file system, it's enough to check a
few things directly without parsing the mount table at all.

We can check via stat calls:
1) If parent is the root directory
2) If the parent is located on the root file system
3) If the parent is located within a file system mounted via a block device

Quoted 5 lines

Large numbers of direct mounts when using /proc/self/mounts instead
of /etc/mtab result in very slow startup time. In testing, we observed
~8k mounts taking over 6 hours to complete.

When this was originally done the mtab was (usually) an actual file so
the autofs mounts weren't present. So mostly it was fairly small.

But now, with the symlinking of the mtab to a proc filesystem, this is a
big problem.

Quoted 10 lines

This is due to reading /proc/self/mounts for every path component of
every mount. For my test case, that turns out to be about 119k times
for a total of just under 400 GB read. If it were a flat file, it would be also be slow, but /proc/self/mounts is dynamically generated
every time it's read.

Quoted 3 lines

By avoiding reading /proc/self/mounts many times, startup time with 8k
direct mounts drops from ~6 hours to about 25 seconds.

Yeah, I get that.

Quoted 7 lines

In some tests done before the symlinking became the norm I saw direct
mount tables of over 20k entries taking between 20-30 seconds or a bit
more (can't quite remember now, it was a long time ago) but nothing like
this.

Yep, I figured you'd remember what was up since it's in the README, but
the context is mostly for a complete commit log entry.

Quoted 3 lines

I thought that was bad so this really needs to be fixed.

I am puzzled why I haven't seen other bug reports.

Yeah, I usually see all of the autofs bugs reported against our products
and it's the first time I've seen it too.

Quoted 47 lines

Quoted 34 lines

Signed-off-by Jeff Mahoney <[email protected]>

---
 daemon/automount.c |   47
++++++++++++++++++++++++++++++++++++++++++++---
 include/mounts.h   |    1 -
 lib/mounts.c       |   45 ---------------------------------------
----
--
 3 files changed, 44 insertions(+), 49 deletions(-)

daemon/automount.c +8 -0

@@ -98,6 +98,46 @@ static int umount_all(struct autofs_poin
 
 extern struct master *master_list;
 
+#define SYSFS_PATTERN "/sys/dev/block/%d:%d"
+static int contained_in_local_fs(const char *parent)
+{
+	char buf[128];
+	struct stat st, root;
+	int ret;
+
+	/* The root directory is always considered local, even if

it's not. */

daemon/automount.c +6 -0

+	if (!*parent)
+		return 1;
+
+	ret = stat("/", &root);
+	if (ret != 0) {
+		logerr("error: stat failed on /: %s",

strerror(errno));

daemon/automount.c +1 -0

+		return 0;

daemon/automount.c static int umount_all(struct autofs_poin · line 125

Quoted 10 lines

daemon/automount.c +10 -0

+	}
+
+	ret = stat(parent, &st);
+	if (ret)
+		return 0;
+	/*
+	 * If the parent is on the root file system, it's local.
+	 */
+	if (root.st_dev == st.st_dev)
+		return 1;

I'm struggling to remember now.

Quoted 3 lines

I think the problem was only when the root itself was an NFS file system
and automount was trying to create a directory.

That sounds like the problem that the patch that caused it to always return true was supposed to solve.

Git tells me that contained_in_local_fs matches up with:
commit 8293b030944eb402893a008a7d2ed1f8969ebee8

Author: Ian Kent <[email protected]>
Date:   Wed Oct 25 15:51:13 2006 +0800

- deal with changed semantics of mkdir in 2.6.19.

Does that ring a bell? Google associates it with a patch called "don't
create remote dirs" which makes sense on its own but I can't find anything in the kernel changelog between v2.6.18 and v2.6.19 that stands
out as changed mkdir semantics that would necessitate the work being done here.

At the point where that commit was added, the test for an autofs parent
wasn't in the caller. That searched through the mount table to see if there was a match and if it was autofs. The statfs call was originally
in the loop and was rightly moved out of it since statfs() doesn't actually require that its path argument be a mountpoint. So that just makes the loop test whether it's a block device. Sort of, since it uses
/[^/] as a pattern to mean block device. Later that was extended to include UUID/LABEL and also with the patch that was intended to match NFS roots.

So, I suppose the question is this: if that patch was accepted based on
its intent, it should be ok to write to a remote file system if it's the
root file system, right? The root file system should be assumed to be private, but once we follow a path off of the root file system we shouldn't write anymore.

Quoted 15 lines

daemon/automount.c static int umount_all(struct autofs_poin · line 135

Quoted 12 lines

daemon/automount.c +3 -0

+
+	/*
+	 * If the sysfs node for the block device exists, it's

local.

daemon/automount.c +7 -0

+	 */
+	ret = snprintf(buf, sizeof(buf), SYSFS_PATTERN,
+		       major(st.st_dev), minor(st.st_dev));
+	if (ret != 0) {
+		logerr("error: not enough space for sysfs path");
+		return 0;
+	}

This is the only bit that concerns me.

Quoted 3 lines

Not because I think it's wrong, on the contrary, it looks ok.

I just can't think of cases where it might not cover what's needed.

I realized shortly after posting the patch that anything using anonymous
device nodes would fail the lookup test since they don't register a node
in /sys/dev/block (nor should they.) This includes btrfs since it uses
anonymous devices to account for the fact that virtualizes the file system across multiple devices and also needs separate namespace for inode numbers between snapshots. Ubifs also uses an anonymous device, but I've never actually seen an ubifs file system in the wild.

Overlayfs will use an anonymous device for directories since they're also virtualized. That's a separate issue, though, since it reports the
device as 'none' in the mount table so direct mounts don't work on overlayfs yet anyway.

Quoted 12 lines

After all, it's already been checked if the parent is an autofs filesystem which should take care of the bulk of cases except for longer
direct mount paths, but will often not be the case for offset mounts.

For example a mount entry with offset mounts like:

<direct or indirect key>     /      server:/path \
                             /other server:/otherpath \
                             ...

Quoted 7 lines

But the intermediate path components of a direct mount path (and offset
mount path) must already exist if within a remote mount. So the existenc
e case, checked before this, should cover these cases.

Let me think about it a little longer.

Sure. It seems like the solution, whatever it ends up being, will simplify this code a lot. It seems like it boils down to "don't create
directories on a remote file system unless it's the root file system."

As noted above, my patch is incomplete. I think a complete solution would look like this:

1/ Test whether the parent is autofs.
2/ Test whether the parent resides on a type of file system known to be
local. This is mostly because we'll need to at least test for btrfs, and if we're already testing file system type, we might as well test for
the common local file systems as well.
3/ Test whether the parent resides on the root file system. 4/ Test whether the file system is mounted on a block device using the /sys/dev/block lookup. This is mostly a fallback for the test in (2) being imperfect WRT new file systems.

Is this missing any other cases?

-Jeff

Quoted 3 lines

I thought that was bad so this really needs to be fixed.

I am puzzled why I haven't seen other bug reports.

Yeah, I usually see all of the autofs bugs reported against our products and it's the first time I've seen it too.

Quoted 130 lines · Jeff Mahoney

Quoted 48 lines · Ian Kent

Quoted 17 lines · Jeff Mahoney

Commit d3ce65e05d1 (autofs-5.0.3 - allow directory create on NFS root),
introduced a bug where contained_in_local_fs would *always* return true.

Since the goal of contained_in_local_fs is only to determine if the
path would be created on a local file system, it's enough to check a
few things directly without parsing the mount table at all.

We can check via stat calls:
1) If parent is the root directory
2) If the parent is located on the root file system
3) If the parent is located within a file system mounted via a block device

Quoted 5 lines

Large numbers of direct mounts when using /proc/self/mounts instead
of /etc/mtab result in very slow startup time. In testing, we observed
~8k mounts taking over 6 hours to complete.

When this was originally done the mtab was (usually) an actual file so
the autofs mounts weren't present. So mostly it was fairly small.

But now, with the symlinking of the mtab to a proc filesystem, this is a
big problem.

Quoted 10 lines

This is due to reading /proc/self/mounts for every path component of
every mount. For my test case, that turns out to be about 119k times
for a total of just under 400 GB read. If it were a flat file, it would be also be slow, but /proc/self/mounts is dynamically generated
every time it's read.

Quoted 3 lines

By avoiding reading /proc/self/mounts many times, startup time with 8k
direct mounts drops from ~6 hours to about 25 seconds.

Yeah, I get that.

Quoted 7 lines

In some tests done before the symlinking became the norm I saw direct
mount tables of over 20k entries taking between 20-30 seconds or a bit
more (can't quite remember now, it was a long time ago) but nothing like
this.

Yep, I figured you'd remember what was up since it's in the README, but
the context is mostly for a complete commit log entry.

Quoted 3 lines

I thought that was bad so this really needs to be fixed.

I am puzzled why I haven't seen other bug reports.

Yeah, I usually see all of the autofs bugs reported against our products
and it's the first time I've seen it too.

Quoted 47 lines

Quoted 34 lines

Signed-off-by Jeff Mahoney <[email protected]>

---
 daemon/automount.c |   47
++++++++++++++++++++++++++++++++++++++++++++---
 include/mounts.h   |    1 -
 lib/mounts.c       |   45 ---------------------------------------
----
--
 3 files changed, 44 insertions(+), 49 deletions(-)

daemon/automount.c +8 -0

@@ -98,6 +98,46 @@ static int umount_all(struct autofs_poin
 
 extern struct master *master_list;
 
+#define SYSFS_PATTERN "/sys/dev/block/%d:%d"
+static int contained_in_local_fs(const char *parent)
+{
+	char buf[128];
+	struct stat st, root;
+	int ret;
+
+	/* The root directory is always considered local, even if

it's not. */

daemon/automount.c +6 -0

+	if (!*parent)
+		return 1;
+
+	ret = stat("/", &root);
+	if (ret != 0) {
+		logerr("error: stat failed on /: %s",

strerror(errno));

daemon/automount.c +1 -0

+		return 0;

daemon/automount.c static int umount_all(struct autofs_poin · line 125

Quoted 10 lines

daemon/automount.c +10 -0

+	}
+
+	ret = stat(parent, &st);
+	if (ret)
+		return 0;
+	/*
+	 * If the parent is on the root file system, it's local.
+	 */
+	if (root.st_dev == st.st_dev)
+		return 1;

I'm struggling to remember now.

Quoted 3 lines

I think the problem was only when the root itself was an NFS file system
and automount was trying to create a directory.

That sounds like the problem that the patch that caused it to always return true was supposed to solve.

Git tells me that contained_in_local_fs matches up with:
commit 8293b030944eb402893a008a7d2ed1f8969ebee8

Author: Ian Kent <[email protected]>
Date:   Wed Oct 25 15:51:13 2006 +0800

- deal with changed semantics of mkdir in 2.6.19.

Quoted 7 lines

Does that ring a bell? Google associates it with a patch called "don't
create remote dirs" which makes sense on its own but I can't find anything in the kernel changelog between v2.6.18 and v2.6.19 that stands
out as changed mkdir semantics that would necessitate the work being done here.

There was a log going on at that time, it was around the initial v5 development.

I had started cataloguing the patches for each release at that time but by the look of the path order there's nothing else related that went in with it. Not only that, at that time, my change log entries were either non-existent or less than stellar.

You can see the patch series for each release at:
https://www.kernel.org/pub/linux/daemons/autofs/v5/

I think this all arose from a discussion on the NFS mailing list due to problems when trying to create mount point directories that resulted in something like "automount shouldn't be creating directories on NFS file systems". So the changed semantics were probably wrt. NFS and not the VFS.

But the same logic extends to remote file systems in general.

Quoted 13 lines

At the point where that commit was added, the test for an autofs parent
wasn't in the caller. That searched through the mount table to see if there was a match and if it was autofs. The statfs call was originally
in the loop and was rightly moved out of it since statfs() doesn't actually require that its path argument be a mountpoint. So that just makes the loop test whether it's a block device. Sort of, since it uses
/[^/] as a pattern to mean block device. Later that was extended to include UUID/LABEL and also with the patch that was intended to match NFS roots.

There's not really enough context, we'll just need to resolve it based on what's sensible.

Quoted 8 lines

So, I suppose the question is this: if that patch was accepted based on
its intent, it should be ok to write to a remote file system if it's the
root file system, right? The root file system should be assumed to be private, but once we follow a path off of the root file system we shouldn't write anymore.

The mount table scan has to go, whatever we come up with is going to get committed with the next bunch of patches.

Quoted 19 lines

Quoted 15 lines

daemon/automount.c static int umount_all(struct autofs_poin · line 135

Quoted 12 lines

daemon/automount.c +3 -0

+
+	/*
+	 * If the sysfs node for the block device exists, it's

local.

daemon/automount.c +7 -0

+	 */
+	ret = snprintf(buf, sizeof(buf), SYSFS_PATTERN,
+		       major(st.st_dev), minor(st.st_dev));
+	if (ret != 0) {
+		logerr("error: not enough space for sysfs path");
+		return 0;
+	}

This is the only bit that concerns me.

Quoted 3 lines

Not because I think it's wrong, on the contrary, it looks ok.

I just can't think of cases where it might not cover what's needed.

Quoted 10 lines

I realized shortly after posting the patch that anything using anonymous
device nodes would fail the lookup test since they don't register a node
in /sys/dev/block (nor should they.) This includes btrfs since it uses
anonymous devices to account for the fact that virtualizes the file system across multiple devices and also needs separate namespace for inode numbers between snapshots. Ubifs also uses an anonymous device, but I've never actually seen an ubifs file system in the wild.

Or perhaps pass down the statfs buffer and check for remote fs magic numbers. That's got to be a smaller list and should be much less likely to change.

Quoted 46 lines

Overlayfs will use an anonymous device for directories since they're also virtualized. That's a separate issue, though, since it reports the
device as 'none' in the mount table so direct mounts don't work on overlayfs yet anyway.

Quoted 12 lines

After all, it's already been checked if the parent is an autofs filesystem which should take care of the bulk of cases except for longer
direct mount paths, but will often not be the case for offset mounts.

For example a mount entry with offset mounts like:

<direct or indirect key>     /      server:/path \
                             /other server:/otherpath \
                             ...

Quoted 7 lines

But the intermediate path components of a direct mount path (and offset
mount path) must already exist if within a remote mount. So the existenc
e case, checked before this, should cover these cases.

Let me think about it a little longer.

Sure. It seems like the solution, whatever it ends up being, will simplify this code a lot. It seems like it boils down to "don't create
directories on a remote file system unless it's the root file system."

As noted above, my patch is incomplete. I think a complete solution would look like this:

1/ Test whether the parent is autofs.
2/ Test whether the parent resides on a type of file system known to be
local. This is mostly because we'll need to at least test for btrfs, and if we're already testing file system type, we might as well test for
the common local file systems as well.
3/ Test whether the parent resides on the root file system. 4/ Test whether the file system is mounted on a block device using the /sys/dev/block lookup. This is mostly a fallback for the test in (2) being imperfect WRT new file systems.

Quoted 1 line

Is this missing any other cases?

The only bit that looks tricky is the test for a block device, the rest covers many of the cases.

And the point is to prohibit mount point directory creation on remote file systems, so if we get that far then just checking the magic number of of the statfs struct we already have for known remote file systems should be enough and be a fairly short list.

With direct mounts, we want to avoid creating path components on remote file systems unless that file system is the root file system.

The check boils down to allowing the mkdir if:
1/ If it's the root directory, or
2/ If it's not a remote file system, or
3/ If it's a remote file system that's the root file system

We can do that without parsing the mount table and can improve startup time for all cases.

Signed-off-by Jeff Mahoney <[email protected]>

---
 daemon/automount.c  | 56 +++++++++++++++++++++++++++++++++++++++--------------
 include/automount.h |  2 ++
 include/mounts.h    |  1 -
 lib/mounts.c        | 45 ------------------------------------------
 4 files changed, 44 insertions(+), 60 deletions(-)

daemon/automount.c +42 -14

index 74e62a1..9d07425 100644
@@ -98,10 +98,25 @@ static int umount_all(struct autofs_point *ap, int force);
 
 extern struct master *master_list;
 
+static int is_remote_fstype(unsigned int fs_type)
+{
+	int ret = 0;
+	switch (fs_type) {
+	case SMB_SUPER_MAGIC:
+	case CIFS_MAGIC_NUMBER:
+	case NCP_SUPER_MAGIC:
+	case NFS_SUPER_MAGIC:
+		ret = 1;
+		break;
+	};
+	return ret;
+}
+
 static int do_mkdir(const char *parent, const char *path, mode_t mode)
 {
 	int status;
-	struct stat st;
+	mode_t mask;
+	struct stat st, root;
 	struct statfs fs;
 
 	/* If path exists we're done */

@@ -114,24 +129,37 @@ static int do_mkdir(const char *parent, const char *path, mode_t mode)
 	}
 
 	/*
-	 * If we're trying to create a directory within an autofs fs
-	 * or the path is contained in a localy mounted fs go ahead.
+	 * We don't want to create the path on a remote file system
+	 * unless it's the root file system.
+	 * An empty parent means it's the root directory and always ok.
 	 */
-	status = -1;
-	if (*parent)
+	if (*parent) {
 		status = statfs(parent, &fs);
-	if ((status != -1 && fs.f_type == (__SWORD_TYPE) AUTOFS_SUPER_MAGIC) ||
-	    contained_in_local_fs(path)) {
-		mode_t mask = umask(0022);
-		int ret = mkdir(path, mode);
-		(void) umask(mask);
-		if (ret == -1) {
-			errno = EACCES;
-			return 0;
+		if (status == -1)
+			goto fail;
+
+		if (is_remote_fstype(fs.f_type)) {
+			status = stat(parent, &st);
+			if (status == -1)
+				goto fail;
+
+			status = stat("/", &root);
+			if (status == -1)
+				goto fail;
+
+			if (st.st_dev != root.st_dev)
+				goto fail;
 		}
-		return 1;
 	}
 
+	mask = umask(0022);
+	status = mkdir(path, mode);
+	(void) umask(mask);
+	if (status == -1)
+		goto fail;
+
+	return 1;
+fail:
 	errno = EACCES;
 	return 0;
 }

include/automount.h +2 -0

index 2cf0611..c0f5fbf 100644
@@ -75,6 +75,8 @@ int load_autofs4_module(void);
 #define AUTOFS_SUPER_MAGIC 0x00000187L
 #define SMB_SUPER_MAGIC    0x0000517BL
 #define CIFS_MAGIC_NUMBER  0xFF534D42L
+#define NCP_SUPER_MAGIC    0x0000564CL
+#define NFS_SUPER_MAGIC    0x00006969L
 
 /* This sould be enough for at least 20 host aliases */
 #define HOST_ENT_BUF_SIZE	2048

include/mounts.h +0 -1

index cce646a..489025c 100644
@@ -101,7 +101,6 @@ int ext_mount_remove(struct list_head *, const char *);
 struct mnt_list *get_mnt_list(const char *table, const char *path, int include);
 struct mnt_list *reverse_mnt_list(struct mnt_list *list);
 void free_mnt_list(struct mnt_list *list);
-int contained_in_local_fs(const char *path);
 int is_mounted(const char *table, const char *path, unsigned int type);
 int has_fstab_option(const char *opt);
 void tree_free_mnt_tree(struct mnt_list *tree);

lib/mounts.c +0 -45

index 455bdca..1d1b4da 100644
@@ -941,51 +941,6 @@ void free_mnt_list(struct mnt_list *list)
 	}
 }
 
-int contained_in_local_fs(const char *path)
-{
-	struct mnt_list *mnts, *this;
-	size_t pathlen = strlen(path);
-	int ret;
-
-	if (!path || !pathlen || pathlen > PATH_MAX)
-		return 0;
-
-	mnts = get_mnt_list(_PATH_MOUNTED, "/", 1);
-	if (!mnts)
-		return 0;
-
-	ret = 0;
-
-	for (this = mnts; this != NULL; this = this->next) {
-		size_t len = strlen(this->path);
-
-		if (!strncmp(path, this->path, len)) {
-			if (len > 1 && pathlen > len && path[len] != '/')
-				continue;
-			else if (len == 1 && this->path[0] == '/') {
-				/*
-				 * always return true on rootfs, we don't
-				 * want to break diskless clients.
-				 */
-				ret = 1;
-			} else if (this->fs_name[0] == '/') {
-				if (strlen(this->fs_name) > 1) {
-					if (this->fs_name[1] != '/')
-						ret = 1;
-				} else
-					ret = 1;
-			} else if (!strncmp("LABEL=", this->fs_name, 6) ||
-				   !strncmp("UUID=", this->fs_name, 5))
-				ret = 1;
-			break;
-		}
-	}
-
-	free_mnt_list(mnts);
-
-	return ret;
-}
-
 static int table_is_mounted(const char *table, const char *path, unsigned int type)
 {
 	struct mntent *mnt;

With direct mounts, we want to avoid creating path components on remote file systems unless that file system is the root file system.

The check boils down to allowing the mkdir if:
1/ If it's the root directory, or
2/ If it's not a remote file system, or
3/ If it's a remote file system that's the root file system

We can do that without parsing the mount table and can improve startup time for all cases.

Signed-off-by Jeff Mahoney <[email protected]>

---
 daemon/automount.c  | 56 +++++++++++++++++++++++++++++++++++++++-----
---------
 include/automount.h |  2 ++
 include/mounts.h    |  1 -
 lib/mounts.c        | 45 ------------------------------------------
 4 files changed, 44 insertions(+), 60 deletions(-)

daemon/automount.c +16 -1

index 74e62a1..9d07425 100644
@@ -98,10 +98,25 @@ static int umount_all(struct autofs_point *ap, int
 force);
 
 extern struct master *master_list;
 
+static int is_remote_fstype(unsigned int fs_type)
+{
+	int ret = 0;
+	switch (fs_type) {
+	case SMB_SUPER_MAGIC:
+	case CIFS_MAGIC_NUMBER:
+	case NCP_SUPER_MAGIC:
+	case NFS_SUPER_MAGIC:
+		ret = 1;
+		break;
+	};
+	return ret;
+}
+
 static int do_mkdir(const char *parent, const char *path, mode_t
 mode)
 {
 	int status;
-	struct stat st;
+	mode_t mask;
+	struct stat st, root;
 	struct statfs fs;

daemon/automount.c +3 -2

 	/* If path exists we're done */

@@ -114,24 +129,37 @@ static int do_mkdir(const char *parent, const
 char *path, mode_t mode)
 	}
 
 	/*
-	 * If we're trying to create a directory within an autofs fs
-	 * or the path is contained in a localy mounted fs go ahead.
+	 * We don't want to create the path on a remote file system
+	 * unless it's the root file system.
+	 * An empty parent means it's the root directory and always

ok.

daemon/automount.c +1 -3

 	 */
-	status = -1;
-	if (*parent)
+	if (*parent) {
 		status = statfs(parent, &fs);
-	if ((status != -1 && fs.f_type == (__SWORD_TYPE)

AUTOFS_SUPER_MAGIC) ||

daemon/automount.c +22 -8

-	    contained_in_local_fs(path)) {
-		mode_t mask = umask(0022);
-		int ret = mkdir(path, mode);
-		(void) umask(mask);
-		if (ret == -1) {
-			errno = EACCES;
-			return 0;
+		if (status == -1)
+			goto fail;
+
+		if (is_remote_fstype(fs.f_type)) {
+			status = stat(parent, &st);
+			if (status == -1)
+				goto fail;
+
+			status = stat("/", &root);
+			if (status == -1)
+				goto fail;
+
+			if (st.st_dev != root.st_dev)
+				goto fail;
 		}
-		return 1;
 	}
 
+	mask = umask(0022);
+	status = mkdir(path, mode);
+	(void) umask(mask);
+	if (status == -1)
+		goto fail;
+
+	return 1;
+fail:
 	errno = EACCES;
 	return 0;
 }

include/automount.h +2 -0

index 2cf0611..c0f5fbf 100644
@@ -75,6 +75,8 @@ int load_autofs4_module(void);
 #define AUTOFS_SUPER_MAGIC 0x00000187L
 #define SMB_SUPER_MAGIC    0x0000517BL
 #define CIFS_MAGIC_NUMBER  0xFF534D42L
+#define NCP_SUPER_MAGIC    0x0000564CL
+#define NFS_SUPER_MAGIC    0x00006969L
 
 /* This sould be enough for at least 20 host aliases */
 #define HOST_ENT_BUF_SIZE	2048

include/mounts.h +0 -1

index cce646a..489025c 100644
@@ -101,7 +101,6 @@ int ext_mount_remove(struct list_head *, const
 char *);
 struct mnt_list *get_mnt_list(const char *table, const char *path,
 int include);
 struct mnt_list *reverse_mnt_list(struct mnt_list *list);
 void free_mnt_list(struct mnt_list *list);
-int contained_in_local_fs(const char *path);
 int is_mounted(const char *table, const char *path, unsigned int

type);
int has_fstab_option(const char *opt);
void tree_free_mnt_tree(struct mnt_list *tree);

lib/mounts.c +0 -24

index 455bdca..1d1b4da 100644
@@ -941,51 +941,6 @@ void free_mnt_list(struct mnt_list *list)
 	}
 }
 
-int contained_in_local_fs(const char *path)
-{
-	struct mnt_list *mnts, *this;
-	size_t pathlen = strlen(path);
-	int ret;
-
-	if (!path || !pathlen || pathlen > PATH_MAX)
-		return 0;
-
-	mnts = get_mnt_list(_PATH_MOUNTED, "/", 1);
-	if (!mnts)
-		return 0;
-
-	ret = 0;
-
-	for (this = mnts; this != NULL; this = this->next) {
-		size_t len = strlen(this->path);
-
-		if (!strncmp(path, this->path, len)) {
-			if (len > 1 && pathlen > len && path[len] !=
 '/')
-				continue;
-			else if (len == 1 && this->path[0] == '/') {
-				/*
-				 * always return true on rootfs, we

don't

lib/mounts.c +0 -10

-				 * want to break diskless clients.
-				 */
-				ret = 1;
-			} else if (this->fs_name[0] == '/') {
-				if (strlen(this->fs_name) > 1) {
-					if (this->fs_name[1] != '/')
-						ret = 1;
-				} else
-					ret = 1;
-			} else if (!strncmp("LABEL=", this->fs_name,

6) ||

lib/mounts.c +0 -1

-				   !strncmp("UUID=", this->fs_name,

5))

lib/mounts.c +0 -10

-				ret = 1;
-			break;
-		}
-	}
-
-	free_mnt_list(mnts);
-
-	return ret;
-}
-
 static int table_is_mounted(const char *table, const char *path,

unsigned int type)
{
struct mntent *mnt;