[BUG] contained_in_local_fs will *always* return true

[BUG] contained_in_local_fs will *always* return true

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 2963 bytes --]

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?

Thanks,

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 881 bytes --]

[PATCH] autofs: fix contained_in_local_fs and improve scalability

[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

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 <jeffm@suse.com>
---
 daemon/automount.c |   47 ++++++++++++++++++++++++++++++++++++++++++++---
 include/mounts.h   |    1 -
 lib/mounts.c       |   45 ---------------------------------------------
 3 files changed, 44 insertions(+), 49 deletions(-)

--- a/daemon/automount.c
+++ b/daemon/automount.c
@@ -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);
--- a/include/mounts.h
+++ b/include/mounts.h
@@ -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,
--- a/lib/mounts.c
+++ b/lib/mounts.c
@@ -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;
--
To unsubscribe from this list: send the line "unsubscribe autofs" in

Re: [PATCH] autofs: fix contained_in_local_fs and improve scalability

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 5576 bytes --]

On 3/18/16 5:25 PM, Jeff Mahoney wrote:
> 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

This test will actually fail for btrfs, but I suppose that could be
special cased.

-Jeff

> 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 <jeffm@suse.com>
> ---
>  daemon/automount.c |   47 ++++++++++++++++++++++++++++++++++++++++++++---
>  include/mounts.h   |    1 -
>  lib/mounts.c       |   45 ---------------------------------------------
>  3 files changed, 44 insertions(+), 49 deletions(-)
> 
> --- a/daemon/automount.c
> +++ b/daemon/automount.c
> @@ -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);
> --- a/include/mounts.h
> +++ b/include/mounts.h
> @@ -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,
> --- a/lib/mounts.c
> +++ b/lib/mounts.c
> @@ -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;
> 


-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 881 bytes --]

Re: [BUG] contained_in_local_fs will *always* return true

[Ian Kent]

On Fri, 2016-03-18 at 16:44 -0400, Jeff Mahoney wrote:
> 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 /.
> 			 */

Yes, that's not what we want.


>                         /*
>                          * 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?

I'd prefer to fix it to do what it's supposed to do.

> 
> 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.

As you can see scanning the mount table is something that must be
avoided wherever possible so I would like to find another way to perform
that check.

I don't really care about keeping the (modified, fixed) scan for those
using the ioctl interface rather than the miscellaneous device. But for
those using the miscellaneous device interface the scan of the mount
table really needs to go.

I'll need a bit of time to even remember what the intent of the change
was.

> 
> 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?

As I say, I'll need some time to reacquaint myself with what is being
done with contained_in_local_fs() so I'm just guessing below.

IIRC the whole point of this (from a bigger picture POV) was due to
complains about autofs even trying to create directories within an NFS
file system being bad form (and autofs can potentially do so quite a
bit) so it was necessary to require the mount point directory already
exist when mounting into an NFS file system.

But that should be possible without scanning the mount table so I'm not
sure what I was thinking, again I'll need to have a look around to see
if I can recall the reason behind contained_in_local_fs() as well as the
bad change that occurred later.

Send over the patch and I'll have a look at that too.

Ian
--
To unsubscribe from this list: send the line "unsubscribe autofs" in

Re: [PATCH] autofs: fix contained_in_local_fs and improve scalability

[Ian Kent]

On Fri, 2016-03-18 at 17:25 -0400, Jeff Mahoney wrote:
> 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.

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. 

> 
> 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.

> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  daemon/automount.c |   47
> ++++++++++++++++++++++++++++++++++++++++++++---
>  include/mounts.h   |    1 -
>  lib/mounts.c       |   45 -------------------------------------------
> --
>  3 files changed, 44 insertions(+), 49 deletions(-)
> 
> --- a/daemon/automount.c
> +++ b/daemon/automount.c
> @@ -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;

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.

> +
> +	/*
> +	 * 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.

> +
> +	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);
> --- a/include/mounts.h
> +++ b/include/mounts.h
> @@ -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,
> --- a/lib/mounts.c
> +++ b/lib/mounts.c
> @@ -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;
--
To unsubscribe from this list: send the line "unsubscribe autofs" in

Re: [PATCH] autofs: fix contained_in_local_fs and improve scalability

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 7959 bytes --]

On 3/20/16 11:44 PM, Ian Kent wrote:
> On Fri, 2016-03-18 at 17:25 -0400, Jeff Mahoney wrote:
>> 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.
> 
> 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. 
> 
>>
>> 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.

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.

> 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.

>>
>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>> ---
>>  daemon/automount.c |   47
>> ++++++++++++++++++++++++++++++++++++++++++++---
>>  include/mounts.h   |    1 -
>>  lib/mounts.c       |   45 -------------------------------------------
>> --
>>  3 files changed, 44 insertions(+), 49 deletions(-)
>>
>> --- a/daemon/automount.c
>> +++ b/daemon/automount.c
>> @@ -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;
> 
> 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.

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 <raven@raven.themaw.net>
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.

>> +
>> +	/*
>> +	 * 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.

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.

> 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.

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

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 881 bytes --]

Re: [PATCH] autofs: fix contained_in_local_fs and improve scalability

[Ian Kent]

On Mon, 2016-03-21 at 18:08 -0400, Jeff Mahoney wrote:
> On 3/20/16 11:44 PM, Ian Kent wrote:
> > On Fri, 2016-03-18 at 17:25 -0400, Jeff Mahoney wrote:
> > > 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.
> > 
> > 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. 
> > 
> > > 
> > > 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.
> 
> 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.
> 
> > 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.
> 
> > > 
> > > Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> > > ---
> > >  daemon/automount.c |   47
> > > ++++++++++++++++++++++++++++++++++++++++++++---
> > >  include/mounts.h   |    1 -
> > >  lib/mounts.c       |   45 ---------------------------------------
> > > ----
> > > --
> > >  3 files changed, 44 insertions(+), 49 deletions(-)
> > > 
> > > --- a/daemon/automount.c
> > > +++ b/daemon/automount.c
> > > @@ -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;
> > 
> > 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.
> 
> 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 <raven@raven.themaw.net>
> 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.

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.

> 
> 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.

> 
> 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.

> 
> > > +
> > > +	/*
> > > +	 * 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.
> 
> 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.

> 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.
> 
> > 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.
> 
> 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?

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.

> 
> -Jeff
> 
--
To unsubscribe from this list: send the line "unsubscribe autofs" in

Re: [PATCH] autofs: fix contained_in_local_fs and improve scalability

[Marion, Mike]

On Mon, Mar 21, 2016 at 06:08:54PM -0400, Jeff Mahoney wrote:

> > 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.

Probably because enterprise customers tend to be behind the curve
already.  I know the engineers we support are even further behind
because the tools they're using are behind even where we'd like to be.
When the first moves to bring out "Enterprise Linux" distros started, the
big EDA ISVs we work with (Synopsys, Cadence, etc) thought the 18 month
time-frames being discussed then were too fast.  If you can believe it.

The SLE12 (and RH7.x) stuff is another special case though because our
internal CM systems needed to be updated to handle the big change that
systemd was, but still work on the older stuff as well.  We didn't get 
that to the point where we could test with our namespaces until SP1 
had come out.

We're the/a customer that raised this issue BTW.  We have pretty huge
direct maps that have been working fine for quite a few years, until the
/etc/mtab symlink.  Our large maps and heavy autofs usage has led us
into breaking it in many... "interesting" ways over the years that
helped the Suse guys get patches upstream that benefit everyone though,
so there's that.

-- 
Mike Marion-Unix SysAdmin/Sr. Staff IT Engineer-http://www.qualcomm.com
Homer: "Oh.. I hate having parties.  The toilet always gets backed up!"
==> Simpsons--
To unsubscribe from this list: send the line "unsubscribe autofs" in

Re: [PATCH] autofs: fix contained_in_local_fs and improve scalability

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 10375 bytes --]

On 3/21/16 8:24 PM, Ian Kent wrote:
> On Mon, 2016-03-21 at 18:08 -0400, Jeff Mahoney wrote:
>> On 3/20/16 11:44 PM, Ian Kent wrote:
>>> On Fri, 2016-03-18 at 17:25 -0400, Jeff Mahoney wrote:
>>>> 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.
>>>
>>> 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. 
>>>
>>>>
>>>> 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.
>>
>> 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.
>>
>>> 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.
>>
>>>>
>>>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>>>> ---
>>>>  daemon/automount.c |   47
>>>> ++++++++++++++++++++++++++++++++++++++++++++---
>>>>  include/mounts.h   |    1 -
>>>>  lib/mounts.c       |   45 ---------------------------------------
>>>> ----
>>>> --
>>>>  3 files changed, 44 insertions(+), 49 deletions(-)
>>>>
>>>> --- a/daemon/automount.c
>>>> +++ b/daemon/automount.c
>>>> @@ -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;
>>>
>>> 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.
>>
>> 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 <raven@raven.themaw.net>
>> 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.
> 
> 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.
> 
>>
>> 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.
> 
>>
>> 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.
> 
>>
>>>> +
>>>> +	/*
>>>> +	 * 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.
>>
>> 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.
> 
>> 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.
>>
>>> 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.
>>
>> 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?
> 
> 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.

Ok, that's an easy enough change to code up.  I'll post an updated patch.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

[PATCH] autofs: improve scalability of direct mount path component creation

[Jeff Mahoney]

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 <jeffm@suse.com>
---
 daemon/automount.c  | 56 +++++++++++++++++++++++++++++++++++++++--------------
 include/automount.h |  2 ++
 include/mounts.h    |  1 -
 lib/mounts.c        | 45 ------------------------------------------
 4 files changed, 44 insertions(+), 60 deletions(-)

diff --git a/daemon/automount.c b/daemon/automount.c
index 74e62a1..9d07425 100644
--- a/daemon/automount.c
+++ b/daemon/automount.c
@@ -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;
 }
diff --git a/include/automount.h b/include/automount.h
index 2cf0611..c0f5fbf 100644
--- a/include/automount.h
+++ b/include/automount.h
@@ -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
diff --git a/include/mounts.h b/include/mounts.h
index cce646a..489025c 100644
--- a/include/mounts.h
+++ b/include/mounts.h
@@ -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);
diff --git a/lib/mounts.c b/lib/mounts.c
index 455bdca..1d1b4da 100644
--- a/lib/mounts.c
+++ b/lib/mounts.c
@@ -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;
-- 
2.7.1


--
To unsubscribe from this list: send the line "unsubscribe autofs" in

Re: [PATCH] autofs: improve scalability of direct mount path component creation

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 5930 bytes --]

On 3/22/16 10:13 AM, Jeff Mahoney wrote:
> 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.

I did test this in the expected scenarios:

- With an NFS (ch)root, direct mount on NFS root, created.
- With an NFS (ch)root, direct mount on NFS mount, did not create.
- With a local root, direct mount on root fs, created.
- With a local root, direct mount on another local fs, created.
- With a local root, direct mount on NFS, did not create.

-Jeff

> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  daemon/automount.c  | 56 +++++++++++++++++++++++++++++++++++++++--------------
>  include/automount.h |  2 ++
>  include/mounts.h    |  1 -
>  lib/mounts.c        | 45 ------------------------------------------
>  4 files changed, 44 insertions(+), 60 deletions(-)
> 
> diff --git a/daemon/automount.c b/daemon/automount.c
> index 74e62a1..9d07425 100644
> --- a/daemon/automount.c
> +++ b/daemon/automount.c
> @@ -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;
>  }
> diff --git a/include/automount.h b/include/automount.h
> index 2cf0611..c0f5fbf 100644
> --- a/include/automount.h
> +++ b/include/automount.h
> @@ -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
> diff --git a/include/mounts.h b/include/mounts.h
> index cce646a..489025c 100644
> --- a/include/mounts.h
> +++ b/include/mounts.h
> @@ -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);
> diff --git a/lib/mounts.c b/lib/mounts.c
> index 455bdca..1d1b4da 100644
> --- a/lib/mounts.c
> +++ b/lib/mounts.c
> @@ -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;
> 


-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 881 bytes --]

Re: [PATCH] autofs: improve scalability of direct mount path component creation

[Ian Kent]

On Tue, 2016-03-22 at 10:13 -0400, Jeff Mahoney wrote:
> 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.

This looks like it covers the important cases.

It's in the queue of patches I have for the next commit but I can't say
when that will be.

If there are other special cases that have been missed they'll need to
be reported and handled as they come up, the mount table traversal has
to go.

Thanks for your help.
Ian

> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  daemon/automount.c  | 56 +++++++++++++++++++++++++++++++++++++++-----
> ---------
>  include/automount.h |  2 ++
>  include/mounts.h    |  1 -
>  lib/mounts.c        | 45 ------------------------------------------
>  4 files changed, 44 insertions(+), 60 deletions(-)
> 
> diff --git a/daemon/automount.c b/daemon/automount.c
> index 74e62a1..9d07425 100644
> --- a/daemon/automount.c
> +++ b/daemon/automount.c
> @@ -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;
>  }
> diff --git a/include/automount.h b/include/automount.h
> index 2cf0611..c0f5fbf 100644
> --- a/include/automount.h
> +++ b/include/automount.h
> @@ -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
> diff --git a/include/mounts.h b/include/mounts.h
> index cce646a..489025c 100644
> --- a/include/mounts.h
> +++ b/include/mounts.h
> @@ -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);
> diff --git a/lib/mounts.c b/lib/mounts.c
> index 455bdca..1d1b4da 100644
> --- a/lib/mounts.c
> +++ b/lib/mounts.c
> @@ -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;
--
To unsubscribe from this list: send the line "unsubscribe autofs" in