fs - make is_local_mountpoint() usable by others

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace will return ELOOP until the mount is umounted within the container.

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

Signed-off-by Ian Kent <[email protected]>

Cc Al Viro <[email protected]>

Cc Eric W. Biederman <[email protected]>

Cc Omar Sandoval <[email protected]>

---
 fs/autofs4/dev-ioctl.c |    2 +-
 fs/autofs4/expire.c    |    4 ++--
 fs/autofs4/root.c      |   30 +++++++++++++++---------------
 fs/autofs4/waitq.c     |    2 +-
 4 files changed, 19 insertions(+), 19 deletions(-)

fs/autofs4/dev-ioctl.c +1 -1

index c7fcc74..0024e25 100644
@@ -564,7 +564,7 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp,
 
 		devid = new_encode_dev(dev);
 
-		err = have_submounts(path.dentry);
+		err = have_local_submounts(path.dentry);
 
 		if (follow_down_one(&path))
 			magic = path.dentry->d_sb->s_magic;

fs/autofs4/expire.c +2 -2

index d8e6d42..7cc34ef 100644
@@ -236,7 +236,7 @@ static int autofs4_tree_busy(struct vfsmount *mnt,
 		 * count for the autofs dentry.
 		 * If the fs is busy update the expiry counter.
 		 */
-		if (d_mountpoint(p)) {
+		if (is_local_mountpoint(p)) {
 			if (autofs4_mount_busy(mnt, p)) {
 				top_ino->last_used = jiffies;
 				dput(p);

@@ -280,7 +280,7 @@ static struct dentry *autofs4_check_leaves(struct vfsmount *mnt,
 	while ((p = get_next_positive_dentry(p, parent))) {
 		pr_debug("dentry %p %pd\n", p, p);
 
-		if (d_mountpoint(p)) {
+		if (is_local_mountpoint(p)) {
 			/* Can we umount this guy */
 			if (autofs4_mount_busy(mnt, p))
 				continue;

fs/autofs4/root.c +15 -15

index fa84bb8..4150ad6 100644
@@ -123,7 +123,7 @@ static int autofs4_dir_open(struct inode *inode, struct file *file)
 	 * it.
 	 */
 	spin_lock(&sbi->lookup_lock);
-	if (!d_mountpoint(dentry) && simple_empty(dentry)) {
+	if (!is_local_mountpoint(dentry) && simple_empty(dentry)) {
 		spin_unlock(&sbi->lookup_lock);
 		return -ENOENT;
 	}

@@ -370,28 +370,28 @@ static struct vfsmount *autofs4_d_automount(struct path *path)
 
 	/*
 	 * If the dentry is a symlink it's equivalent to a directory
-	 * having d_mountpoint() true, so there's no need to call back
-	 * to the daemon.
+	 * having is_local_mountpoint() true, so there's no need to
+	 * call back to the daemon.
 	 */
 	if (d_really_is_positive(dentry) && d_is_symlink(dentry)) {
 		spin_unlock(&sbi->fs_lock);
 		goto done;
 	}
 
-	if (!d_mountpoint(dentry)) {
+	if (!is_local_mountpoint(dentry)) {
 		/*
 		 * It's possible that user space hasn't removed directories
 		 * after umounting a rootless multi-mount, although it
-		 * should. For v5 have_submounts() is sufficient to handle
-		 * this because the leaves of the directory tree under the
-		 * mount never trigger mounts themselves (they have an autofs
-		 * trigger mount mounted on them). But v4 pseudo direct mounts
-		 * do need the leaves to trigger mounts. In this case we
-		 * have no choice but to use the list_empty() check and
-		 * require user space behave.
+		 * should. For v5 have_local_submounts() is sufficient to
+		 * handle this because the leaves of the directory tree under
+		 * the mount never trigger mounts themselves (they have an
+		 * autofs trigger mount mounted on them). But v4 pseudo
+		 * direct mounts do need the leaves to trigger mounts. In
+		 * this case we have no choice but to use the list_empty()
+		 * check and require user space behave.
 		 */
 		if (sbi->version > 4) {
-			if (have_submounts(dentry)) {
+			if (have_local_submounts(dentry)) {
 				spin_unlock(&sbi->fs_lock);
 				goto done;
 			}

@@ -431,7 +431,7 @@ static int autofs4_d_manage(struct dentry *dentry, bool rcu_walk)
 
 	/* The daemon never waits. */
 	if (autofs4_oz_mode(sbi)) {
-		if (!d_mountpoint(dentry))
+		if (!is_local_mountpoint(dentry))
 			return -EISDIR;
 		return 0;
 	}

@@ -460,7 +460,7 @@ static int autofs4_d_manage(struct dentry *dentry, bool rcu_walk)
 
 		if (ino->flags & AUTOFS_INF_WANT_EXPIRE)
 			return 0;
-		if (d_mountpoint(dentry))
+		if (is_local_mountpoint(dentry))
 			return 0;
 		inode = d_inode_rcu(dentry);
 		if (inode && S_ISLNK(inode->i_mode))

@@ -487,7 +487,7 @@ static int autofs4_d_manage(struct dentry *dentry, bool rcu_walk)
 		 * we can avoid needless calls ->d_automount() and avoid
 		 * an incorrect ELOOP error return.
 		 */
-		if ((!d_mountpoint(dentry) && !simple_empty(dentry)) ||
+		if ((!is_local_mountpoint(dentry) && !simple_empty(dentry)) ||
 		    (d_really_is_positive(dentry) && d_is_symlink(dentry)))
 			status = -EISDIR;
 	}

fs/autofs4/waitq.c +1 -1

index 431fd7e..911f4d5 100644
@@ -333,7 +333,7 @@ static int validate_request(struct autofs_wait_queue **wait,
 					dentry = new;
 			}
 		}
-		if (have_submounts(dentry))
+		if (have_local_submounts(dentry))
 			valid = 0;
 
 		if (new)

Quoted 6 lines · Ian Kent

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

Quoted 7 lines

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace will return ELOOP until the mount is umounted within the container.

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

Overall this appears to be a fairly reasonable set of changes. It does increase the expense when an actual mount point is encountered, but if these are the desired some increase in cost when a dentry is a mountpoint is unavoidable.

May I ask the motiviation for this set of changes? Reading through the changes I don't grasp why we want to change the behavior of autofs. What problem is being solved? What are the benefits?

Eric

Quoted 150 lines

Signed-off-by Ian Kent <[email protected]>

Cc Al Viro <[email protected]>

Cc Eric W. Biederman <[email protected]>

Cc Omar Sandoval <[email protected]>

---
 fs/autofs4/dev-ioctl.c |    2 +-
 fs/autofs4/expire.c    |    4 ++--
 fs/autofs4/root.c      |   30 +++++++++++++++---------------
 fs/autofs4/waitq.c     |    2 +-
 4 files changed, 19 insertions(+), 19 deletions(-)

fs/autofs4/dev-ioctl.c +1 -1

index c7fcc74..0024e25 100644
@@ -564,7 +564,7 @@ static int autofs_dev_ioctl_ismountpoint(struct file
 *fp,
 
 		devid = new_encode_dev(dev);
 
-		err = have_submounts(path.dentry);
+		err = have_local_submounts(path.dentry);
 
 		if (follow_down_one(&path))

fs/autofs4/dev-ioctl.c +0 -0

 			magic = path.dentry->d_sb->s_magic;

fs/autofs4/expire.c +2 -2

index d8e6d42..7cc34ef 100644
@@ -236,7 +236,7 @@ static int autofs4_tree_busy(struct vfsmount *mnt,
 		 * count for the autofs dentry.
 		 * If the fs is busy update the expiry counter.
 		 */
-		if (d_mountpoint(p)) {
+		if (is_local_mountpoint(p)) {
 			if (autofs4_mount_busy(mnt, p)) {
 				top_ino->last_used = jiffies;
 				dput(p);

@@ -280,7 +280,7 @@ static struct dentry *autofs4_check_leaves(struct
 vfsmount *mnt,
 	while ((p = get_next_positive_dentry(p, parent))) {
 		pr_debug("dentry %p %pd\n", p, p);
 
-		if (d_mountpoint(p)) {
+		if (is_local_mountpoint(p)) {
 			/* Can we umount this guy */
 			if (autofs4_mount_busy(mnt, p))

fs/autofs4/expire.c +0 -0

 				continue;

fs/autofs4/root.c +1 -1

index fa84bb8..4150ad6 100644
@@ -123,7 +123,7 @@ static int autofs4_dir_open(struct inode *inode, struct
 file *file)
 	 * it.
 	 */
 	spin_lock(&sbi->lookup_lock);
-	if (!d_mountpoint(dentry) && simple_empty(dentry)) {
+	if (!is_local_mountpoint(dentry) && simple_empty(dentry)) {
 		spin_unlock(&sbi->lookup_lock);
 		return -ENOENT;

fs/autofs4/root.c +3 -6

 	}

@@ -370,28 +370,28 @@ static struct vfsmount *autofs4_d_automount(struct
 path *path)
 
 	/*
 	 * If the dentry is a symlink it's equivalent to a directory
-	 * having d_mountpoint() true, so there's no need to call back
-	 * to the daemon.
+	 * having is_local_mountpoint() true, so there's no need to
+	 * call back to the daemon.
 	 */
 	if (d_really_is_positive(dentry) && d_is_symlink(dentry)) {
 		spin_unlock(&sbi->fs_lock);
 		goto done;
 	}
 
-	if (!d_mountpoint(dentry)) {
+	if (!is_local_mountpoint(dentry)) {
 		/*
 		 * It's possible that user space hasn't removed directories
 		 * after umounting a rootless multi-mount, although it
-		 * should. For v5 have_submounts() is sufficient to handle
-		 * this because the leaves of the directory tree under the
-		 * mount never trigger mounts themselves (they have an

autofs

fs/autofs4/root.c +0 -1

-		 * trigger mount mounted on them). But v4 pseudo direct

mounts

fs/autofs4/root.c +2 -3

-		 * do need the leaves to trigger mounts. In this case we
-		 * have no choice but to use the list_empty() check and
-		 * require user space behave.
+		 * should. For v5 have_local_submounts() is sufficient to
+		 * handle this because the leaves of the directory tree

under

fs/autofs4/root.c +7 -2

+		 * the mount never trigger mounts themselves (they have an
+		 * autofs trigger mount mounted on them). But v4 pseudo
+		 * direct mounts do need the leaves to trigger mounts. In
+		 * this case we have no choice but to use the list_empty()
+		 * check and require user space behave.
 		 */
 		if (sbi->version > 4) {
-			if (have_submounts(dentry)) {
+			if (have_local_submounts(dentry)) {
 				spin_unlock(&sbi->fs_lock);
 				goto done;
 			}

@@ -431,7 +431,7 @@ static int autofs4_d_manage(struct dentry *dentry, bool
 rcu_walk)
 
 	/* The daemon never waits. */
 	if (autofs4_oz_mode(sbi)) {
-		if (!d_mountpoint(dentry))
+		if (!is_local_mountpoint(dentry))
 			return -EISDIR;
 		return 0;

fs/autofs4/root.c +1 -1

 	}

@@ -460,7 +460,7 @@ static int autofs4_d_manage(struct dentry *dentry, bool
 rcu_walk)
 
 		if (ino->flags & AUTOFS_INF_WANT_EXPIRE)
 			return 0;
-		if (d_mountpoint(dentry))
+		if (is_local_mountpoint(dentry))
 			return 0;
 		inode = d_inode_rcu(dentry);

fs/autofs4/root.c +1 -1

 		if (inode && S_ISLNK(inode->i_mode))

@@ -487,7 +487,7 @@ static int autofs4_d_manage(struct dentry *dentry, bool
 rcu_walk)
 		 * we can avoid needless calls ->d_automount() and avoid
 		 * an incorrect ELOOP error return.
 		 */
-		if ((!d_mountpoint(dentry) && !simple_empty(dentry)) ||
+		if ((!is_local_mountpoint(dentry) && !simple_empty(dentry))

||

fs/autofs4/root.c +0 -0

 		    (d_really_is_positive(dentry) && d_is_symlink(dentry)))
 			status = -EISDIR;
 	}

fs/autofs4/waitq.c +1 -1

index 431fd7e..911f4d5 100644
@@ -333,7 +333,7 @@ static int validate_request(struct autofs_wait_queue
 **wait,
 					dentry = new;
 			}
 		}
-		if (have_submounts(dentry))
+		if (have_local_submounts(dentry))
 			valid = 0;
 

Quoted 1 line

if (new)

Quoted 16 lines · Eric W. Biederman

Quoted 6 lines · Ian Kent

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

Quoted 7 lines

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace will return ELOOP until the mount is umounted within the container.

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

Quoted 8 lines

Overall this appears to be a fairly reasonable set of changes. It does increase the expense when an actual mount point is encountered, but if these are the desired some increase in cost when a dentry is a mountpoint is unavoidable.

May I ask the motiviation for this set of changes? Reading through the changes I don't grasp why we want to change the behavior of autofs. What problem is being solved? What are the benefits?

LOL, it's all too easy for me to give a patch description that I think explains a problem I need to solve without realizing it isn't clear to others what the problem is, sorry about that.

For quite a while now, and not that frequently but consistently, I've been getting reports of people using autofs getting ELOOP errors and not being able to mount automounts.

This has been due to the cloning of autofs file systems (that have active automounts at the time of the clone) by other systems.

An unshare, as one example, can easily result in the cloning of an autofs file system that has active mounts which shows this problem.

Once an active mount that has been cloned is expired in the namespace that performed the unshare it can't be (auto)mounted again in the the originating namespace because the mounted check in the autofs module will think it is already mounted.

I'm not sure this is a clear description either, hopefully it is enough to demonstrate the type of problem I'm typing to solve.

Quoted 16 lines · Eric W. Biederman

Quoted 6 lines · Ian Kent

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

Quoted 7 lines

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace will return ELOOP until the mount is umounted within the container.

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

Quoted 8 lines

Overall this appears to be a fairly reasonable set of changes. It does increase the expense when an actual mount point is encountered, but if these are the desired some increase in cost when a dentry is a mountpoint is unavoidable.

May I ask the motiviation for this set of changes? Reading through the changes I don't grasp why we want to change the behavior of autofs. What problem is being solved? What are the benefits?

LOL, it's all too easy for me to give a patch description that I think explains a problem I need to solve without realizing it isn't clear to others what the problem is, sorry about that.

For quite a while now, and not that frequently but consistently, I've been getting reports of people using autofs getting ELOOP errors and not being able to mount automounts.

This has been due to the cloning of autofs file systems (that have active automounts at the time of the clone) by other systems.

An unshare, as one example, can easily result in the cloning of an autofs file system that has active mounts which shows this problem.

Once an active mount that has been cloned is expired in the namespace that performed the unshare it can't be (auto)mounted again in the the originating namespace because the mounted check in the autofs module will think it is already mounted.

I'm not sure this is a clear description either, hopefully it is enough to demonstrate the type of problem I'm typing to solve.

Quoted 44 lines · Ian Kent

Quoted 16 lines · Eric W. Biederman

Quoted 6 lines · Ian Kent

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

Quoted 7 lines

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace will return ELOOP until the mount is umounted within the container.

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

Quoted 8 lines

Overall this appears to be a fairly reasonable set of changes. It does increase the expense when an actual mount point is encountered, but if these are the desired some increase in cost when a dentry is a mountpoint is unavoidable.

May I ask the motiviation for this set of changes? Reading through the changes I don't grasp why we want to change the behavior of autofs. What problem is being solved? What are the benefits?

LOL, it's all too easy for me to give a patch description that I think explains
a problem I need to solve without realizing it isn't clear to others what the
problem is, sorry about that.

For quite a while now, and not that frequently but consistently, I've been getting reports of people using autofs getting ELOOP errors and not being able
to mount automounts.

This has been due to the cloning of autofs file systems (that have active automounts at the time of the clone) by other systems.

An unshare, as one example, can easily result in the cloning of an autofs file
system that has active mounts which shows this problem.

Quoted 7 lines

Once an active mount that has been cloned is expired in the namespace that performed the unshare it can't be (auto)mounted again in the the originating namespace because the mounted check in the autofs module will think it is already mounted.

I'm not sure this is a clear description either, hopefully it is enough to demonstrate the type of problem I'm typing to solve.

So to rephrase the problem is that an autofs instance can stop working properly from the perspective of the mount namespace it is mounted in if the autofs instance is shared between multiple mount namespaces. The problem is that mounts and unmounts do not always propogate between mount namespaces. This lack of symmetric mount/unmount behavior leads to mountpoints that become unusable.

Which leads to the question what is the expected new behavior with your patchset applied. New mounts can be added in the parent mount namespace (because the test is local). Does your change also allow the autofs mountpoints to be used in the other mount namespaces that share the autofs instance if everything becomes unmounted?

Or is it expected that other mount namespaces that share an autofs instance will get changes in their mounts via mount propagation and if mount propagation is insufficient they are on their own.

I believe this is a question of how do notifications of the desire for an automount work after your change, and are those notifications consistent with your desired and/or expected behavior.

Eric

Quoted 55 lines · Eric W. Biederman

Quoted 44 lines · Ian Kent

Quoted 16 lines · Eric W. Biederman

Quoted 6 lines · Ian Kent

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

Quoted 7 lines

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace will return ELOOP until the mount is umounted within the container.

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

Quoted 8 lines

Overall this appears to be a fairly reasonable set of changes. It does increase the expense when an actual mount point is encountered, but if these are the desired some increase in cost when a dentry is a mountpoint is unavoidable.

May I ask the motiviation for this set of changes? Reading through the changes I don't grasp why we want to change the behavior of autofs. What problem is being solved? What are the benefits?

LOL, it's all too easy for me to give a patch description that I think explains
a problem I need to solve without realizing it isn't clear to others what the
problem is, sorry about that.

For quite a while now, and not that frequently but consistently, I've been getting reports of people using autofs getting ELOOP errors and not being able
to mount automounts.

This has been due to the cloning of autofs file systems (that have active automounts at the time of the clone) by other systems.

An unshare, as one example, can easily result in the cloning of an autofs file
system that has active mounts which shows this problem.

Quoted 8 lines

Once an active mount that has been cloned is expired in the namespace that performed the unshare it can't be (auto)mounted again in the the originating
namespace because the mounted check in the autofs module will think it is already mounted.

I'm not sure this is a clear description either, hopefully it is enough to demonstrate the type of problem I'm typing to solve.

Quoted 6 lines

So to rephrase the problem is that an autofs instance can stop working properly from the perspective of the mount namespace it is mounted in if the autofs instance is shared between multiple mount namespaces. The problem is that mounts and unmounts do not always propogate between mount namespaces. This lack of symmetric mount/unmount behavior leads to mountpoints that become unusable.

That's right.

It's also worth considering that symmetric mount propagation is usually not the
behaviour needed either and things like LXC and Docker are set propagation slave
because of problems caused by propagation back to the parent namespace.

So a mount can be triggered within a container, mounted by the automount daemon
in the parent namespace, and propagated to the child and similarly for expires,
which is the common use case now.

Quoted 6 lines

Which leads to the question what is the expected new behavior with your patchset applied. New mounts can be added in the parent mount namespace (because the test is local). Does your change also allow the autofs mountpoints to be used in the other mount namespaces that share the autofs instance if everything becomes unmounted?

The problem occurs when the subordinate namespace doesn't deal with these propagated mounts properly, although they can obviously be used by the subordinate namespace.

Quoted 4 lines

Or is it expected that other mount namespaces that share an autofs instance will get changes in their mounts via mount propagation and if mount propagation is insufficient they are on their own.

Namespaces that receive updates via mount propagation from a parent will continue to function as they do now.

Mounts that don't get updates via mount propagation will retain the mount to use
if they need to, as they would without this change, but the originating namespace will also continue to function as expected.

The child namespace needs cleanup its mounts on exit, which it had to do prior to this change also.

Quoted 4 lines

I believe this is a question of how do notifications of the desire for an automount work after your change, and are those notifications consistent with your desired and/or expected behavior.

It sounds like you might be assuming the service receiving these cloned mounts actually wants to use them or is expecting them to behave like automount mounts.
But that's not what I've seen and is not the way these cloned mounts behave without the change.

However, as has probably occurred to you by now, there is a semantic change with
this for namespaces that don't receive mount propogation.

If a mount request is triggered by an access in the subordinate namespace for a
dentry that is already mounted in the parent namespace it will silently fail (in
that a mount won't appear in the subordinate namespace) rather than getting an ELOOP error as it would now.

Quoted 54 lines · Eric W. Biederman

Quoted 44 lines · Ian Kent

Quoted 16 lines · Eric W. Biederman

Quoted 6 lines · Ian Kent

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

Quoted 7 lines

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace will return ELOOP until the mount is umounted within the container.

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

Quoted 8 lines

Overall this appears to be a fairly reasonable set of changes. It does increase the expense when an actual mount point is encountered, but if these are the desired some increase in cost when a dentry is a mountpoint is unavoidable.

May I ask the motiviation for this set of changes? Reading through the changes I don't grasp why we want to change the behavior of autofs. What problem is being solved? What are the benefits?

LOL, it's all too easy for me to give a patch description that I think explains
a problem I need to solve without realizing it isn't clear to others what the
problem is, sorry about that.

For quite a while now, and not that frequently but consistently, I've been getting reports of people using autofs getting ELOOP errors and not being able
to mount automounts.

This has been due to the cloning of autofs file systems (that have active automounts at the time of the clone) by other systems.

An unshare, as one example, can easily result in the cloning of an autofs file
system that has active mounts which shows this problem.

Quoted 7 lines

Once an active mount that has been cloned is expired in the namespace that performed the unshare it can't be (auto)mounted again in the the originating namespace because the mounted check in the autofs module will think it is already mounted.

I'm not sure this is a clear description either, hopefully it is enough to demonstrate the type of problem I'm typing to solve.

Quoted 6 lines

So to rephrase the problem is that an autofs instance can stop working properly from the perspective of the mount namespace it is mounted in if the autofs instance is shared between multiple mount namespaces. The problem is that mounts and unmounts do not always propogate between mount namespaces. This lack of symmetric mount/unmount behavior leads to mountpoints that become unusable.

That's right.

It's also worth considering that symmetric mount propagation is usually not the behaviour needed either and things like LXC and Docker are set propagation slave because of problems caused by propagation back to the parent namespace.

So a mount can be triggered within a container, mounted by the automount daemon in the parent namespace, and propagated to the child and similarly for expires, which is the common use case now.

Quoted 6 lines

Which leads to the question what is the expected new behavior with your patchset applied. New mounts can be added in the parent mount namespace (because the test is local). Does your change also allow the autofs mountpoints to be used in the other mount namespaces that share the autofs instance if everything becomes unmounted?

The problem occurs when the subordinate namespace doesn't deal with these propagated mounts properly, although they can obviously be used by the subordinate namespace.

Quoted 4 lines

Or is it expected that other mount namespaces that share an autofs instance will get changes in their mounts via mount propagation and if mount propagation is insufficient they are on their own.

Namespaces that receive updates via mount propagation from a parent will continue to function as they do now.

Mounts that don't get updates via mount propagation will retain the mount to use if they need to, as they would without this change, but the originating namespace will also continue to function as expected.

The child namespace needs cleanup its mounts on exit, which it had to do prior to this change also.

Quoted 4 lines

I believe this is a question of how do notifications of the desire for an automount work after your change, and are those notifications consistent with your desired and/or expected behavior.

It sounds like you might be assuming the service receiving these cloned mounts actually wants to use them or is expecting them to behave like automount mounts. But that's not what I've seen and is not the way these cloned mounts behave without the change.

However, as has probably occurred to you by now, there is a semantic change with this for namespaces that don't receive mount propogation.

If a mount request is triggered by an access in the subordinate namespace for a dentry that is already mounted in the parent namespace it will silently fail (in that a mount won't appear in the subordinate namespace) rather than getting an ELOOP error as it would now.

It's also the case that, if such a mount isn't already mounted, it will cause a mount to occur in the parent namespace. But that is also the way it is without the change.

TBH I don't know yet how to resolve that, ideally the cloned mounts would not appear in the subordinate namespace upon creation but that's also not currently possible to do and even if it was it would mean quite a change in to the way things behave now.

All in all I believe the change here solves a problem that needs to be solved without affecting normal usage at the expense of a small behaviour change to cases where automount isn't providing a mounting service.

Quoted 153 lines · Ian Kent

Quoted 66 lines · Eric W. Biederman

Quoted 60 lines · Ian Kent

Quoted 23 lines · Eric W. Biederman

Quoted 6 lines · Ian Kent

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

Quoted 8 lines

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace
will return ELOOP until the mount is umounted within the container.

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

Overall this appears to be a fairly reasonable set of changes. It does
increase the expense when an actual mount point is encountered, but if these are the desired some increase in cost when a dentry is a mountpoint is unavoidable.

Quoted 4 lines

May I ask the motiviation for this set of changes? Reading through the
changes I don't grasp why we want to change the behavior of autofs. What problem is being solved? What are the benefits?

LOL, it's all too easy for me to give a patch description that I think explains
a problem I need to solve without realizing it isn't clear to others what
the
problem is, sorry about that.

For quite a while now, and not that frequently but consistently, I've been
getting reports of people using autofs getting ELOOP errors and not being
able
to mount automounts.

This has been due to the cloning of autofs file systems (that have active
automounts at the time of the clone) by other systems.

An unshare, as one example, can easily result in the cloning of an autofs
file
system that has active mounts which shows this problem.

Once an active mount that has been cloned is expired in the namespace that
performed the unshare it can't be (auto)mounted again in the the originating
namespace because the mounted check in the autofs module will think it is
already mounted.

Quoted 3 lines

I'm not sure this is a clear description either, hopefully it is enough to
demonstrate the type of problem I'm typing to solve.

Quoted 6 lines

So to rephrase the problem is that an autofs instance can stop working properly from the perspective of the mount namespace it is mounted in if the autofs instance is shared between multiple mount namespaces. The problem is that mounts and unmounts do not always propogate between mount namespaces. This lack of symmetric mount/unmount behavior leads to mountpoints that become unusable.

That's right.

It's also worth considering that symmetric mount propagation is usually not the
behaviour needed either and things like LXC and Docker are set propagation slave
because of problems caused by propagation back to the parent namespace.

So a mount can be triggered within a container, mounted by the automount daemon
in the parent namespace, and propagated to the child and similarly for expires,
which is the common use case now.

Quoted 6 lines

Which leads to the question what is the expected new behavior with your patchset applied. New mounts can be added in the parent mount namespace (because the test is local). Does your change also allow the autofs mountpoints to be used in the other mount namespaces that share the autofs instance if everything becomes unmounted?

The problem occurs when the subordinate namespace doesn't deal with these propagated mounts properly, although they can obviously be used by the subordinate namespace.

Quoted 4 lines

Or is it expected that other mount namespaces that share an autofs instance will get changes in their mounts via mount propagation and if mount propagation is insufficient they are on their own.

Namespaces that receive updates via mount propagation from a parent will continue to function as they do now.

Mounts that don't get updates via mount propagation will retain the mount to use
if they need to, as they would without this change, but the originating namespace will also continue to function as expected.

The child namespace needs cleanup its mounts on exit, which it had to do prior
to this change also.

Quoted 4 lines

I believe this is a question of how do notifications of the desire for an automount work after your change, and are those notifications consistent with your desired and/or expected behavior.

It sounds like you might be assuming the service receiving these cloned mounts
actually wants to use them or is expecting them to behave like automount mounts.
But that's not what I've seen and is not the way these cloned mounts behave without the change.

However, as has probably occurred to you by now, there is a semantic change with
this for namespaces that don't receive mount propogation.

If a mount request is triggered by an access in the subordinate namespace for a
dentry that is already mounted in the parent namespace it will silently fail (in
that a mount won't appear in the subordinate namespace) rather than getting an
ELOOP error as it would now.

It's also the case that, if such a mount isn't already mounted, it will cause a
mount to occur in the parent namespace. But that is also the way it is without
the change.

TBH I don't know yet how to resolve that, ideally the cloned mounts would not
appear in the subordinate namespace upon creation but that's also not currently
possible to do and even if it was it would mean quite a change in to the way things behave now.

Quoted 4 lines

All in all I believe the change here solves a problem that needs to be solved
without affecting normal usage at the expense of a small behaviour change to cases where automount isn't providing a mounting service.

That sounds like a reasonable semantic change. Limiting the responses of the autofs mount path to what is present in the mount namespace of the program that actually performs the autofs mounts seems needed.

In fact the entire local mount concept exists because I was solving a very similar problem for rename, unlink and rmdir. Where a cloned mount namespace could cause a denial of service attack on the original mount namespace.

I don't know if this change makes sense for mount expiry.

Unless I am misreading something when a mount namespace is cloned the new mounts are put into the same expiry group as the old mounts.

Furthermore the triggers for mounts are based on the filesystem.

I can think of 3 ways to use mount namespaces that are relevant to this discussion.

- Symmetric mount propagation where everything is identical except for specific mounts such as /tmp.

- Slave mount propagation where all of the mounts are created in the parent and propgated to the slave, except for specific exceptions.

- Disabled mount propagation. Where updates are simply not received by the namespace. The mount namespace is expected to change in ways that are completely independent of the parent (and this breaks autofs).

In the first two cases the desire is to have the same set of mounts except for specific exceptions so it is generally desirable. So having someone using a mount in another mount namespace seems like a good reason not to expire the mount.

Furthermore since the processes can always trigger or hang onto the mounts without using mount namespaces I don't think those cases add anything new to the set of problems.

It seems to me the real problem is when something is unmounted in the original mount namespace and not in the slaves which causes the mount calls to fail and cause all kinds of havoc.

Unless you can see an error in my reasoning I think the local mount tests should be limited to just the mount path. That is sufficient to keep autofs working as expected while still respecting non-problem users in other mount namespaces.

Quoted 171 lines · Eric W. Biederman

Quoted 163 lines · Ian Kent

Quoted 69 lines · Eric W. Biederman

Quoted 62 lines · Ian Kent

Quoted 25 lines · Eric W. Biederman

Quoted 13 lines · Ian Kent

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace
will return ELOOP until the mount is umounted within the container.

Quoted 2 lines

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

Overall this appears to be a fairly reasonable set of changes. It does
increase the expense when an actual mount point is encountered, but if
these are the desired some increase in cost when a dentry is a mountpoint is unavoidable.

Quoted 4 lines

May I ask the motiviation for this set of changes? Reading through the
changes I don't grasp why we want to change the behavior of autofs. What problem is being solved? What are the benefits?

LOL, it's all too easy for me to give a patch description that I think explains
a problem I need to solve without realizing it isn't clear to others what
the
problem is, sorry about that.

For quite a while now, and not that frequently but consistently, I've been
getting reports of people using autofs getting ELOOP errors and not being
able
to mount automounts.

This has been due to the cloning of autofs file systems (that have active
automounts at the time of the clone) by other systems.

An unshare, as one example, can easily result in the cloning of an autofs
file
system that has active mounts which shows this problem.

Once an active mount that has been cloned is expired in the namespace that
performed the unshare it can't be (auto)mounted again in the the originating
namespace because the mounted check in the autofs module will think it is
already mounted.

Quoted 4 lines

I'm not sure this is a clear description either, hopefully it is enough
to
demonstrate the type of problem I'm typing to solve.

Quoted 6 lines

So to rephrase the problem is that an autofs instance can stop working properly from the perspective of the mount namespace it is mounted in if the autofs instance is shared between multiple mount namespaces. The problem is that mounts and unmounts do not always propogate between mount namespaces. This lack of symmetric mount/unmount behavior leads to mountpoints that become unusable.

That's right.

It's also worth considering that symmetric mount propagation is usually not
the
behaviour needed either and things like LXC and Docker are set propagation slave
because of problems caused by propagation back to the parent namespace.

So a mount can be triggered within a container, mounted by the automount daemon
in the parent namespace, and propagated to the child and similarly for expires,
which is the common use case now.

Quoted 6 lines

Which leads to the question what is the expected new behavior with your patchset applied. New mounts can be added in the parent mount namespace (because the test is local). Does your change also allow the autofs mountpoints to be used in the other mount namespaces that share the autofs instance if everything becomes unmounted?

The problem occurs when the subordinate namespace doesn't deal with these propagated mounts properly, although they can obviously be used by the subordinate namespace.

Quoted 4 lines

Or is it expected that other mount namespaces that share an autofs instance will get changes in their mounts via mount propagation and if mount propagation is insufficient they are on their own.

Namespaces that receive updates via mount propagation from a parent will continue to function as they do now.

Mounts that don't get updates via mount propagation will retain the mount to
use
if they need to, as they would without this change, but the originating namespace will also continue to function as expected.

The child namespace needs cleanup its mounts on exit, which it had to do prior
to this change also.

Quoted 4 lines

I believe this is a question of how do notifications of the desire for an automount work after your change, and are those notifications consistent with your desired and/or expected behavior.

It sounds like you might be assuming the service receiving these cloned mounts
actually wants to use them or is expecting them to behave like automount mounts.
But that's not what I've seen and is not the way these cloned mounts behave
without the change.

However, as has probably occurred to you by now, there is a semantic change
with
this for namespaces that don't receive mount propogation.

If a mount request is triggered by an access in the subordinate namespace for a
dentry that is already mounted in the parent namespace it will silently fail
(in
that a mount won't appear in the subordinate namespace) rather than getting
an
ELOOP error as it would now.

It's also the case that, if such a mount isn't already mounted, it will cause a
mount to occur in the parent namespace. But that is also the way it is without
the change.

TBH I don't know yet how to resolve that, ideally the cloned mounts would not
appear in the subordinate namespace upon creation but that's also not currently
possible to do and even if it was it would mean quite a change in to the way
things behave now.

Quoted 5 lines

All in all I believe the change here solves a problem that needs to be solved
without affecting normal usage at the expense of a small behaviour change to
cases where automount isn't providing a mounting service.

Quoted 3 lines

That sounds like a reasonable semantic change. Limiting the responses of the autofs mount path to what is present in the mount namespace of the program that actually performs the autofs mounts seems needed.

Indeed, yes.

Quoted 7 lines

In fact the entire local mount concept exists because I was solving a very similar problem for rename, unlink and rmdir. Where a cloned mount namespace could cause a denial of service attack on the original mount namespace.

I don't know if this change makes sense for mount expiry.

Originally I thought it did but now I think your right, it won't actually make a
difference.

Let me think a little more about it, I thought there was a reason I included the
expire in the changes but I can't remember now.

It may be that originally I thought individual automount(8) instances within containers could be affected by an instance of automount(8) in the root namespace (and visa versa) but now I think these will all be isolated.

Quoted 190 lines · Ian Kent

Quoted 171 lines · Eric W. Biederman

Quoted 163 lines · Ian Kent

Quoted 69 lines · Eric W. Biederman

Quoted 62 lines · Ian Kent

Quoted 25 lines · Eric W. Biederman

Quoted 13 lines · Ian Kent

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace
will return ELOOP until the mount is umounted within the container.

Quoted 2 lines

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

Overall this appears to be a fairly reasonable set of changes. It does
increase the expense when an actual mount point is encountered, but if
these are the desired some increase in cost when a dentry is a mountpoint is unavoidable.

Quoted 4 lines

May I ask the motiviation for this set of changes? Reading through the
changes I don't grasp why we want to change the behavior of autofs. What problem is being solved? What are the benefits?

LOL, it's all too easy for me to give a patch description that I think explains
a problem I need to solve without realizing it isn't clear to others what
the
problem is, sorry about that.

For quite a while now, and not that frequently but consistently, I've been
getting reports of people using autofs getting ELOOP errors and not being
able
to mount automounts.

This has been due to the cloning of autofs file systems (that have active
automounts at the time of the clone) by other systems.

An unshare, as one example, can easily result in the cloning of an autofs
file
system that has active mounts which shows this problem.

Once an active mount that has been cloned is expired in the namespace that
performed the unshare it can't be (auto)mounted again in the the originating
namespace because the mounted check in the autofs module will think it is
already mounted.

Quoted 4 lines

I'm not sure this is a clear description either, hopefully it is enough
to
demonstrate the type of problem I'm typing to solve.

Quoted 6 lines

So to rephrase the problem is that an autofs instance can stop working properly from the perspective of the mount namespace it is mounted in if the autofs instance is shared between multiple mount namespaces. The problem is that mounts and unmounts do not always propogate between mount namespaces. This lack of symmetric mount/unmount behavior leads to mountpoints that become unusable.

That's right.

It's also worth considering that symmetric mount propagation is usually not
the
behaviour needed either and things like LXC and Docker are set propagation slave
because of problems caused by propagation back to the parent namespace.

So a mount can be triggered within a container, mounted by the automount daemon
in the parent namespace, and propagated to the child and similarly for expires,
which is the common use case now.

Quoted 6 lines

Which leads to the question what is the expected new behavior with your patchset applied. New mounts can be added in the parent mount namespace (because the test is local). Does your change also allow the autofs mountpoints to be used in the other mount namespaces that share the autofs instance if everything becomes unmounted?

The problem occurs when the subordinate namespace doesn't deal with these propagated mounts properly, although they can obviously be used by the subordinate namespace.

Quoted 4 lines

Or is it expected that other mount namespaces that share an autofs instance will get changes in their mounts via mount propagation and if mount propagation is insufficient they are on their own.

Namespaces that receive updates via mount propagation from a parent will continue to function as they do now.

Mounts that don't get updates via mount propagation will retain the mount to
use
if they need to, as they would without this change, but the originating namespace will also continue to function as expected.

The child namespace needs cleanup its mounts on exit, which it had to do prior
to this change also.

Quoted 4 lines

I believe this is a question of how do notifications of the desire for an automount work after your change, and are those notifications consistent with your desired and/or expected behavior.

It sounds like you might be assuming the service receiving these cloned mounts
actually wants to use them or is expecting them to behave like automount mounts.
But that's not what I've seen and is not the way these cloned mounts behave
without the change.

However, as has probably occurred to you by now, there is a semantic change
with
this for namespaces that don't receive mount propogation.

If a mount request is triggered by an access in the subordinate namespace for a
dentry that is already mounted in the parent namespace it will silently fail
(in
that a mount won't appear in the subordinate namespace) rather than getting
an
ELOOP error as it would now.

It's also the case that, if such a mount isn't already mounted, it will cause a
mount to occur in the parent namespace. But that is also the way it is without
the change.

TBH I don't know yet how to resolve that, ideally the cloned mounts would not
appear in the subordinate namespace upon creation but that's also not currently
possible to do and even if it was it would mean quite a change in to the way
things behave now.

Quoted 5 lines

All in all I believe the change here solves a problem that needs to be solved
without affecting normal usage at the expense of a small behaviour change to
cases where automount isn't providing a mounting service.

Quoted 3 lines

That sounds like a reasonable semantic change. Limiting the responses of the autofs mount path to what is present in the mount namespace of the program that actually performs the autofs mounts seems needed.

Indeed, yes.

Quoted 7 lines

In fact the entire local mount concept exists because I was solving a very similar problem for rename, unlink and rmdir. Where a cloned mount namespace could cause a denial of service attack on the original mount namespace.

I don't know if this change makes sense for mount expiry.

Originally I thought it did but now I think your right, it won't actually make a
difference.

Quoted 7 lines

Let me think a little more about it, I thought there was a reason I included the
expire in the changes but I can't remember now.

It may be that originally I thought individual automount(8) instances within containers could be affected by an instance of automount(8) in the root namespace (and visa versa) but now I think these will all be isolated.

I also thought that the autofs expire would continue to see the umounted mount and continue calling back to the daemon in an attempt to umount it.

That isn't the case so I can drop the changes to the expire expire code as you recommend.

Quoted 11 lines · Ian Kent

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace will return ELOOP until the mount is umounted within the container.

Quoted 2 lines

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

Overall this appears to be a fairly reasonable set of changes. It does increase the expense when an actual mount point is encountered, but if these are the desired some increase in cost when a dentry is a mountpoint is unavoidable.

May I ask the motiviation for this set of changes? Reading through the changes I don't grasp why we want to change the behavior of autofs. What problem is being solved? What are the benefits?

Eric

Quoted 150 lines

Signed-off-by Ian Kent <[email protected]>

Cc Al Viro <[email protected]>

Cc Eric W. Biederman <[email protected]>

Cc Omar Sandoval <[email protected]>

---
 fs/autofs4/dev-ioctl.c |    2 +-
 fs/autofs4/expire.c    |    4 ++--
 fs/autofs4/root.c      |   30 +++++++++++++++---------------
 fs/autofs4/waitq.c     |    2 +-
 4 files changed, 19 insertions(+), 19 deletions(-)

fs/autofs4/dev-ioctl.c +1 -1

index c7fcc74..0024e25 100644
@@ -564,7 +564,7 @@ static int autofs_dev_ioctl_ismountpoint(struct file
 *fp,
 
 		devid = new_encode_dev(dev);
 
-		err = have_submounts(path.dentry);
+		err = have_local_submounts(path.dentry);
 
 		if (follow_down_one(&path))

fs/autofs4/dev-ioctl.c +0 -0

 			magic = path.dentry->d_sb->s_magic;

fs/autofs4/expire.c +2 -2

index d8e6d42..7cc34ef 100644
@@ -236,7 +236,7 @@ static int autofs4_tree_busy(struct vfsmount *mnt,
 		 * count for the autofs dentry.
 		 * If the fs is busy update the expiry counter.
 		 */
-		if (d_mountpoint(p)) {
+		if (is_local_mountpoint(p)) {
 			if (autofs4_mount_busy(mnt, p)) {
 				top_ino->last_used = jiffies;
 				dput(p);

@@ -280,7 +280,7 @@ static struct dentry *autofs4_check_leaves(struct
 vfsmount *mnt,
 	while ((p = get_next_positive_dentry(p, parent))) {
 		pr_debug("dentry %p %pd\n", p, p);
 
-		if (d_mountpoint(p)) {
+		if (is_local_mountpoint(p)) {
 			/* Can we umount this guy */
 			if (autofs4_mount_busy(mnt, p))

fs/autofs4/expire.c +0 -0

 				continue;

fs/autofs4/root.c +1 -1

index fa84bb8..4150ad6 100644
@@ -123,7 +123,7 @@ static int autofs4_dir_open(struct inode *inode, struct
 file *file)
 	 * it.
 	 */
 	spin_lock(&sbi->lookup_lock);
-	if (!d_mountpoint(dentry) && simple_empty(dentry)) {
+	if (!is_local_mountpoint(dentry) && simple_empty(dentry)) {
 		spin_unlock(&sbi->lookup_lock);
 		return -ENOENT;

fs/autofs4/root.c +3 -6

 	}

@@ -370,28 +370,28 @@ static struct vfsmount *autofs4_d_automount(struct
 path *path)
 
 	/*
 	 * If the dentry is a symlink it's equivalent to a directory
-	 * having d_mountpoint() true, so there's no need to call back
-	 * to the daemon.
+	 * having is_local_mountpoint() true, so there's no need to
+	 * call back to the daemon.
 	 */
 	if (d_really_is_positive(dentry) && d_is_symlink(dentry)) {
 		spin_unlock(&sbi->fs_lock);
 		goto done;
 	}
 
-	if (!d_mountpoint(dentry)) {
+	if (!is_local_mountpoint(dentry)) {
 		/*
 		 * It's possible that user space hasn't removed directories
 		 * after umounting a rootless multi-mount, although it
-		 * should. For v5 have_submounts() is sufficient to handle
-		 * this because the leaves of the directory tree under the
-		 * mount never trigger mounts themselves (they have an

autofs

fs/autofs4/root.c +0 -1

-		 * trigger mount mounted on them). But v4 pseudo direct

mounts

fs/autofs4/root.c +2 -3

-		 * do need the leaves to trigger mounts. In this case we
-		 * have no choice but to use the list_empty() check and
-		 * require user space behave.
+		 * should. For v5 have_local_submounts() is sufficient to
+		 * handle this because the leaves of the directory tree

under

fs/autofs4/root.c +7 -2

+		 * the mount never trigger mounts themselves (they have an
+		 * autofs trigger mount mounted on them). But v4 pseudo
+		 * direct mounts do need the leaves to trigger mounts. In
+		 * this case we have no choice but to use the list_empty()
+		 * check and require user space behave.
 		 */
 		if (sbi->version > 4) {
-			if (have_submounts(dentry)) {
+			if (have_local_submounts(dentry)) {
 				spin_unlock(&sbi->fs_lock);
 				goto done;
 			}

@@ -431,7 +431,7 @@ static int autofs4_d_manage(struct dentry *dentry, bool
 rcu_walk)
 
 	/* The daemon never waits. */
 	if (autofs4_oz_mode(sbi)) {
-		if (!d_mountpoint(dentry))
+		if (!is_local_mountpoint(dentry))
 			return -EISDIR;
 		return 0;

fs/autofs4/root.c +1 -1

 	}

@@ -460,7 +460,7 @@ static int autofs4_d_manage(struct dentry *dentry, bool
 rcu_walk)
 
 		if (ino->flags & AUTOFS_INF_WANT_EXPIRE)
 			return 0;
-		if (d_mountpoint(dentry))
+		if (is_local_mountpoint(dentry))
 			return 0;
 		inode = d_inode_rcu(dentry);

fs/autofs4/root.c +1 -1

 		if (inode && S_ISLNK(inode->i_mode))

@@ -487,7 +487,7 @@ static int autofs4_d_manage(struct dentry *dentry, bool
 rcu_walk)
 		 * we can avoid needless calls ->d_automount() and avoid
 		 * an incorrect ELOOP error return.
 		 */
-		if ((!d_mountpoint(dentry) && !simple_empty(dentry)) ||
+		if ((!is_local_mountpoint(dentry) && !simple_empty(dentry))

||

fs/autofs4/root.c +0 -0

 		    (d_really_is_positive(dentry) && d_is_symlink(dentry)))
 			status = -EISDIR;
 	}

fs/autofs4/waitq.c +1 -1

index 431fd7e..911f4d5 100644
@@ -333,7 +333,7 @@ static int validate_request(struct autofs_wait_queue
 **wait,
 					dentry = new;
 			}
 		}
-		if (have_submounts(dentry))
+		if (have_local_submounts(dentry))
 			valid = 0;
 

Quoted 1 line

if (new)

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace will return ELOOP until the mount is umounted within the container.

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

Signed-off-by Ian Kent <[email protected]>

Cc Al Viro <[email protected]>

Cc Eric W. Biederman <[email protected]>

Cc Omar Sandoval <[email protected]>

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace will return ELOOP until the mount is umounted within the container.

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

Quoted 23 lines · Ian Kent

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace will return ELOOP until the mount is umounted within the container.

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

fs/autofs4/root.c +1 -1

@@ -460,7 +460,7 @@ static int autofs4_d_manage(struct dentry *dentry, bool
 rcu_walk)
 
 		if (ino->flags & AUTOFS_INF_WANT_EXPIRE)
 			return 0;
-		if (d_mountpoint(dentry))
+		if (is_local_mountpoint(dentry))
 			return 0;
 		inode = d_inode_rcu(dentry);

Quoted 1 line

if (inode && S_ISLNK(inode->i_mode))

This change is within RCU lookup.

is_local_mountpoint may end up calling __is_local_mountpoint, which will optionally take the namespace_sem lock, resulting in a splat:

I don't know this code. Perhaps it will be perfectly fine performance wise to just drop out of RCU lookup in this case.

Quoted 28 lines · Mateusz Guzik

Quoted 23 lines · Ian Kent

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace will return ELOOP until the mount is umounted within the container.

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

fs/autofs4/root.c +1 -1

@@ -460,7 +460,7 @@ static int autofs4_d_manage(struct dentry *dentry, bool
 rcu_walk)
 
 		if (ino->flags & AUTOFS_INF_WANT_EXPIRE)
 			return 0;
-		if (d_mountpoint(dentry))
+		if (is_local_mountpoint(dentry))
 			return 0;
 		inode = d_inode_rcu(dentry);

Quoted 1 line

if (inode && S_ISLNK(inode->i_mode))

This change is within RCU lookup.

Quoted 2 lines

is_local_mountpoint may end up calling __is_local_mountpoint, which will optionally take the namespace_sem lock, resulting in a splat:

Yes, that's a serious problem I missed.

snip ...

Quoted 2 lines

I don't know this code. Perhaps it will be perfectly fine performance wise to just drop out of RCU lookup in this case.

It's a bit worse than that.

I think being able to continue the rcu-walk for an already mounted dentry that is not being expired is an important part of the performance improvement given by the series that added this.

Can you confirm that Neil?

But for the case here the existing test can allow rcu-walk to continue for a dentry that would attempt to trigger an automount so it's also a bug in the existing code.

Any thoughts on how we can handle this Neil, I'm having a bit of trouble working out how to resolve this one.

Quoted 51 lines · Ian Kent

Quoted 29 lines · Mateusz Guzik

Quoted 18 lines · Ian Kent

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace will return ELOOP until the mount is umounted within the container.

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

(quoted diff) +0 -0

@@ -460,7 +460,7 @@ static int autofs4_d_manage(struct dentry *dentry,

bool
rcu_walk)

fs/autofs4/root.c static int autofs4_d_manage(struct dentry *dentry, bool rcu_walk) · line 466

Quoted 7 lines

fs/autofs4/root.c +1 -1

 		if (ino->flags & AUTOFS_INF_WANT_EXPIRE)
 			return 0;
-		if (d_mountpoint(dentry))
+		if (is_local_mountpoint(dentry))
 			return 0;
 		inode = d_inode_rcu(dentry);
 		if (inode && S_ISLNK(inode->i_mode))

This change is within RCU lookup.

Quoted 2 lines

is_local_mountpoint may end up calling __is_local_mountpoint, which will optionally take the namespace_sem lock, resulting in a splat:

Yes, that's a serious problem I missed.

snip ...

Quoted 3 lines

I don't know this code. Perhaps it will be perfectly fine performance wise to
just drop out of RCU lookup in this case.

It's a bit worse than that.

I think being able to continue the rcu-walk for an already mounted dentry that
is not being expired is an important part of the performance improvement given
by the series that added this.

Can you confirm that Neil?

Quoted 3 lines

But for the case here the existing test can allow rcu-walk to continue for a dentry that would attempt to trigger an automount so it's also a bug in the existing code.

I don't think the existing code is buggy. As I read __follow_mount_rcu if DCACHE_NEED_AUTOMOUNT is set on the dentry after return from d_manage the code will break out of the rcu walk.

Quoted 4 lines

Any thoughts on how we can handle this Neil, I'm having a bit of trouble working
out how to resolve this one.

I believe in this case d_mountpoint is enough for the rcu walk case. If the mountpoint turns out not to be local __follow_mount_rcu will kick out of the rcu walk as it will return false. Because:

	return !mounted && !(path->dentry->d_flags & DCACHE_NEED_AUTOMOUNT);

I am not quite certain about the non-rcu case. That can't be is_local_mountpoint as that is inside a spinlock and is_local_mountpoint can sleep. Perhaps d_mountpoint is simply correct for autofs4_d_manage.

Eric

Quoted 60 lines · Eric W. Biederman

Quoted 50 lines · Ian Kent

Quoted 29 lines · Mateusz Guzik

Quoted 18 lines · Ian Kent

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace will return ELOOP until the mount is umounted within the container.

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

(quoted diff) +0 -0

@@ -460,7 +460,7 @@ static int autofs4_d_manage(struct dentry *dentry,

bool
rcu_walk)

fs/autofs4/root.c static int autofs4_d_manage(struct dentry *dentry, bool rcu_walk) · line 466

Quoted 7 lines

fs/autofs4/root.c +1 -1

 		if (ino->flags & AUTOFS_INF_WANT_EXPIRE)
 			return 0;
-		if (d_mountpoint(dentry))
+		if (is_local_mountpoint(dentry))
 			return 0;
 		inode = d_inode_rcu(dentry);
 		if (inode && S_ISLNK(inode->i_mode))

This change is within RCU lookup.

Quoted 2 lines

is_local_mountpoint may end up calling __is_local_mountpoint, which will optionally take the namespace_sem lock, resulting in a splat:

Yes, that's a serious problem I missed.

snip ...

Quoted 4 lines

I don't know this code. Perhaps it will be perfectly fine performance wise
to
just drop out of RCU lookup in this case.

It's a bit worse than that.

I think being able to continue the rcu-walk for an already mounted dentry that
is not being expired is an important part of the performance improvement given
by the series that added this.

Quoted 7 lines

Can you confirm that Neil?

But for the case here the existing test can allow rcu-walk to continue for a
dentry that would attempt to trigger an automount so it's also a bug in the
existing code.

Quoted 3 lines

I don't think the existing code is buggy. As I read __follow_mount_rcu if DCACHE_NEED_AUTOMOUNT is set on the dentry after return from d_manage the code will break out of the rcu walk.

That's right.

I'm working on follow up patches now.

Quoted 10 lines

Quoted 4 lines

Any thoughts on how we can handle this Neil, I'm having a bit of trouble working
out how to resolve this one.

I believe in this case d_mountpoint is enough for the rcu walk case. If the mountpoint turns out not to be local __follow_mount_rcu will kick out of the rcu walk as it will return false. Because:

	return !mounted && !(path->dentry->d_flags & DCACHE_NEED_AUTOMOUNT);

Quoted 3 lines

I am not quite certain about the non-rcu case. That can't be is_local_mountpoint as that is inside a spinlock and is_local_mountpoint can sleep. Perhaps d_mountpoint is simply correct for autofs4_d_manage.

That's right, I think I have other mistakes there too.

I'm checking those too.

Quoted 3 lines

Eric

Quoted 88 lines · Ian Kent

Quoted 65 lines · Eric W. Biederman

Quoted 54 lines · Ian Kent

Quoted 31 lines · Mateusz Guzik

Quoted 20 lines · Ian Kent

If an automount mount is clone(2)ed into a file system that is propagation private, when it later expires in the originating namespace subsequent calls to autofs ->d_automount() for that dentry in the original namespace will return ELOOP until the mount is manually umounted in the cloned namespace.

In the same way, if an autofs mount is triggered by automount(8) running within a container the dentry will be seen as mounted in the root init namespace and calls to ->d_automount() in that namespace
will return ELOOP until the mount is umounted within the container.

Also, have_submounts() can return an incorect result when a mount exists in a namespace other than the one being checked.

(quoted diff) +0 -0

@@ -460,7 +460,7 @@ static int autofs4_d_manage(struct dentry
 *dentry,

bool
rcu_walk)

fs/autofs4/root.c static int autofs4_d_manage(struct dentry *dentry, bool rcu_walk) · line 466

Quoted 7 lines

fs/autofs4/root.c +1 -1

 		if (ino->flags & AUTOFS_INF_WANT_EXPIRE)
 			return 0;
-		if (d_mountpoint(dentry))
+		if (is_local_mountpoint(dentry))
 			return 0;
 		inode = d_inode_rcu(dentry);
 		if (inode && S_ISLNK(inode->i_mode))

This change is within RCU lookup.

Quoted 3 lines

is_local_mountpoint may end up calling __is_local_mountpoint, which will
optionally take the namespace_sem lock, resulting in a splat:

Yes, that's a serious problem I missed.

snip ...

Quoted 4 lines

I don't know this code. Perhaps it will be perfectly fine performance wise
to
just drop out of RCU lookup in this case.

It's a bit worse than that.

I think being able to continue the rcu-walk for an already mounted dentry
that
is not being expired is an important part of the performance improvement given
by the series that added this.

Quoted 8 lines

Can you confirm that Neil?

But for the case here the existing test can allow rcu-walk to continue for
a
dentry that would attempt to trigger an automount so it's also a bug in the
existing code.

Quoted 3 lines

I don't think the existing code is buggy. As I read __follow_mount_rcu if DCACHE_NEED_AUTOMOUNT is set on the dentry after return from d_manage the code will break out of the rcu walk.

That's right.

I'm working on follow up patches now.

Quoted 13 lines

Quoted 4 lines

Any thoughts on how we can handle this Neil, I'm having a bit of trouble working
out how to resolve this one.

I believe in this case d_mountpoint is enough for the rcu walk case. If the mountpoint turns out not to be local __follow_mount_rcu will kick out of the rcu walk as it will return false. Because:

	return !mounted && !(path->dentry->d_flags & DCACHE_NEED_AUTOMOUNT);

I am not quite certain about the non-rcu case. That can't be is_local_mountpoint as that is inside a spinlock and is_local_mountpoint can sleep. Perhaps d_mountpoint is simply correct for autofs4_d_manage.

Quoted 3 lines

That's right, I think I have other mistakes there too.

I'm checking those too.

It may be that is_local_mountpoint() isn't the right thing to use for this.

When I originally set out to do this I used a lookup_mnt() type check which should be sufficient in this case so I might need to go back to that.

Eric, Mateusz, I appreciate your spending time on this and particularly pointing out my embarrassingly stupid is_local_mountpoint() usage mistake.

Please accept my apology for the inconvenience.

If all goes well (in testing) I'll have follow up patches to correct this fairly soon.

Quoted 9 lines · Ian Kent

Eric, Mateusz, I appreciate your spending time on this and particularly pointing
out my embarrassingly stupid is_local_mountpoint() usage mistake.

Please accept my apology for the inconvenience.

If all goes well (in testing) I'll have follow up patches to correct this fairly
soon.

Related question. Do you happen to know how many mounts per mount namespace tend to be used? It looks like it is going to be wise to put a configurable limit on that number. And I would like the default to be something high enough most people don't care. I believe autofs is likely where people tend to use the most mounts.

Quoted 12 lines · Eric W. Biederman

Quoted 9 lines · Ian Kent

Eric, Mateusz, I appreciate your spending time on this and particularly pointing
out my embarrassingly stupid is_local_mountpoint() usage mistake.

Please accept my apology for the inconvenience.

If all goes well (in testing) I'll have follow up patches to correct this fairly
soon.

Quoted 5 lines

Related question. Do you happen to know how many mounts per mount namespace tend to be used? It looks like it is going to be wise to put a configurable limit on that number. And I would like the default to be something high enough most people don't care. I believe autofs is likely where people tend to use the most mounts.

That's a good question.

I've been thinking that maybe I should have used a lookup_mnt() type check as I originally started out to, for this reason, as the mnt_namespace list looks to be a linear list.

But there can be a lot of mounts, and not only due to autofs, so maybe that should be considered anyway.

The number of mounts for direct mount maps is usually not very large because of the way they are implemented, large direct mount maps can have performance problems. There can be anywhere from a few (likely case a few hundred) to less than 10000, plus mounts that have been triggered and not yet expired.

Indirect mounts have one autofs mount at the root plus the number of mounts that have been triggered and not yet expired.

The number of autofs indirect map entries can range from a few to the common case of several thousand and in rare cases up to between 30000 and 50000. I've not heard of people with maps larger than 50000 entries.

The larger the number of map entries the greater the possibility for a large number of active mounts so it's not hard to expect cases of a 1000 or somewhat more active mounts.

Quoted 14 lines · Ian Kent

Quoted 13 lines · Eric W. Biederman

Quoted 10 lines · Ian Kent

Eric, Mateusz, I appreciate your spending time on this and particularly pointing
out my embarrassingly stupid is_local_mountpoint() usage mistake.

Please accept my apology for the inconvenience.

If all goes well (in testing) I'll have follow up patches to correct this
fairly
soon.

Quoted 5 lines

Quoted 5 lines

Related question. Do you happen to know how many mounts per mount namespace tend to be used? It looks like it is going to be wise to put a configurable limit on that number. And I would like the default to be something high enough most people don't care. I believe autofs is likely where people tend to use the most mounts.

Quoted 10 lines

That's a good question.

I've been thinking that maybe I should have used a lookup_mnt() type check as I
originally started out to, for this reason, as the mnt_namespace list looks to
be a linear list.

But there can be a lot of mounts, and not only due to autofs, so maybe that should be considered anyway.

There are two reasons for is_local_mountpoint being the way it is.

a) For the cases where you don't have the parent mount.
b) For the cases where you want to stop things if something is mounted on a dentry in the local mount namespace even if it isn't mounted on that dentry at your current mount parent. (Something that was important to not change the semantics of the single mount namespace case).

Both of those cases to apply to unlink, rmdir, and rename. I don't think either of those cases apply to what autofs is trying to do. Certainly not the second.

So if you have the parent mount I really think you want to be using some variation of lookup_mnt(). The fact it is rcu safe may help with some of those weird corner cases as well.

Quoted 16 lines

The number of mounts for direct mount maps is usually not very large because of
the way they are implemented, large direct mount maps can have performance problems. There can be anywhere from a few (likely case a few hundred) to less
than 10000, plus mounts that have been triggered and not yet expired.

Indirect mounts have one autofs mount at the root plus the number of mounts that
have been triggered and not yet expired.

The number of autofs indirect map entries can range from a few to the common case of several thousand and in rare cases up to between 30000 and 50000. I've
not heard of people with maps larger than 50000 entries.

Quoted 4 lines

The larger the number of map entries the greater the possibility for a large number of active mounts so it's not hard to expect cases of a 1000 or somewhat
more active mounts.

Fair. So at least 1000. And there can be enough mounts that a limit of 100,000 might be necessary to give head room for the existings configurations.

Thank you. Now I just need to wrap my head around fs/namespace.c again and see how bad a count like that will be to maintain.

Eric

Quoted 17 lines · Eric W. Biederman

Quoted 15 lines · Ian Kent

Quoted 14 lines · Eric W. Biederman

Quoted 11 lines · Ian Kent

Eric, Mateusz, I appreciate your spending time on this and particularly
pointing
out my embarrassingly stupid is_local_mountpoint() usage mistake.

Please accept my apology for the inconvenience.

If all goes well (in testing) I'll have follow up patches to correct this
fairly
soon.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Related question. Do you happen to know how many mounts per mount namespace tend to be used? It looks like it is going to be wise to put a configurable limit on that number. And I would like the default to be something high enough most people don't care. I believe autofs is likely where people tend to use the most mounts.

Yes, I agree, I did want to try and avoid changing the parameters to ->d_mamange() but passing a struct path pointer might be better in the long run
anyway.

Quoted 26 lines · Ian Kent

Quoted 17 lines · Eric W. Biederman

Quoted 15 lines · Ian Kent

Quoted 14 lines · Eric W. Biederman

Quoted 11 lines · Ian Kent

Eric, Mateusz, I appreciate your spending time on this and particularly
pointing
out my embarrassingly stupid is_local_mountpoint() usage mistake.

Please accept my apology for the inconvenience.

If all goes well (in testing) I'll have follow up patches to correct this
fairly
soon.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Related question. Do you happen to know how many mounts per mount namespace tend to be used? It looks like it is going to be wise to put
a configurable limit on that number. And I would like the default to be
something high enough most people don't care. I believe autofs is likely where people tend to use the most mounts.

Quoted 5 lines

Yes, I agree, I did want to try and avoid changing the parameters to ->d_mamange() but passing a struct path pointer might be better in the long run
anyway.

Andrew, could you please drop patches for this series.

I believe they are:
fs-make-is_local_mountpoint-usable-by-others.patch fs-add-have_local_submounts.patch autofs-make-mountpoint-checks-namespace-aware.patch fs-remove-unused-have_submounts-function.patch

I'm going to have a go at what Eric and I discussed above rather than update this series.

Quoted 16 lines

Quoted 14 lines · Ian Kent

Quoted 13 lines · Eric W. Biederman

Quoted 10 lines · Ian Kent

Eric, Mateusz, I appreciate your spending time on this and particularly pointing
out my embarrassingly stupid is_local_mountpoint() usage mistake.

Please accept my apology for the inconvenience.

If all goes well (in testing) I'll have follow up patches to correct this
fairly
soon.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Related question. Do you happen to know how many mounts per mount namespace tend to be used? It looks like it is going to be wise to put a configurable limit on that number. And I would like the default to be something high enough most people don't care. I believe autofs is likely where people tend to use the most mounts.

Yes, I agree, I did want to try and avoid changing the parameters to ->d_mamange() but passing a struct path pointer might be better in the long run anyway.

Ian Kent <[email protected]> writes:

Quoted 25 lines · Eric W. Biederman

Quoted 24 lines

Quoted 22 lines · Ian Kent

Quoted 21 lines · Eric W. Biederman

Quoted 11 lines · Ian Kent

Eric, Mateusz, I appreciate your spending time on this and particularly
pointing
out my embarrassingly stupid is_local_mountpoint() usage mistake.

Please accept my apology for the inconvenience.

If all goes well (in testing) I'll have follow up patches to correct this
fairly
soon.

Related question. Do you happen to know how many mounts per mount namespace tend to be used? It looks like it is going to be wise to put
a configurable limit on that number. And I would like the default to be
something high enough most people don't care. I believe autofs is likely where people tend to use the most mounts.

Quoted 4 lines

Yes, I agree, I did want to try and avoid changing the parameters to ->d_mamange() but passing a struct path pointer might be better in the long run
anyway.

Given that there is exactly one implementation of d_manage in the tree I don't imagine it will be disruptive to change that.

Eric

Quoted 33 lines · Eric W. Biederman

Ian Kent <[email protected]> writes:

Quoted 29 lines · Eric W. Biederman

Quoted 17 lines

Quoted 15 lines · Ian Kent

Quoted 14 lines · Eric W. Biederman

Quoted 11 lines · Ian Kent

Eric, Mateusz, I appreciate your spending time on this and particularly
pointing
out my embarrassingly stupid is_local_mountpoint() usage mistake.

Please accept my apology for the inconvenience.

If all goes well (in testing) I'll have follow up patches to correct this
fairly
soon.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Related question. Do you happen to know how many mounts per mount namespace tend to be used? It looks like it is going to be wise to put
a configurable limit on that number. And I would like the default to be
something high enough most people don't care. I believe autofs is likely where people tend to use the most mounts.

Yes, I agree, I did want to try and avoid changing the parameters to ->d_mamange() but passing a struct path pointer might be better in the long run
anyway.

Quoted 2 lines

Given that there is exactly one implementation of d_manage in the tree I don't imagine it will be disruptive to change that.

Yes, but it could be used by external modules.

And there's also have_submounts().

I can update that using the existing d_walk() infrastructure or take it (mostly) into the autofs module and get rid of have_submounts().

I'll go with the former to start with and see what people think.

Quoted 39 lines · Ian Kent

Quoted 38 lines · Eric W. Biederman

Ian Kent <[email protected]> writes:

Quoted 34 lines · Eric W. Biederman

Quoted 19 lines

Quoted 17 lines · Ian Kent

Quoted 16 lines · Eric W. Biederman

Quoted 13 lines · Ian Kent

Eric, Mateusz, I appreciate your spending time on this and particularly
pointing
out my embarrassingly stupid is_local_mountpoint() usage mistake.

Please accept my apology for the inconvenience.

If all goes well (in testing) I'll have follow up patches to correct
this
fairly
soon.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Related question. Do you happen to know how many mounts per mount namespace tend to be used? It looks like it is going to be wise to
put
a configurable limit on that number. And I would like the default to
be
something high enough most people don't care. I believe autofs is likely where people tend to use the most mounts.

Yes, I agree, I did want to try and avoid changing the parameters to ->d_mamange() but passing a struct path pointer might be better in the long
run
anyway.

Quoted 6 lines

Quoted 2 lines

Given that there is exactly one implementation of d_manage in the tree I don't imagine it will be disruptive to change that.

Yes, but it could be used by external modules.

And there's also have_submounts().

Good point about have_submounts.

Quoted 4 lines

I can update that using the existing d_walk() infrastructure or take it (mostly)
into the autofs module and get rid of have_submounts().

Quoted 1 line

I'll go with the former to start with and see what people think.

That will be interesting to so. It is not clear to me that if d_walk needs to be updated, and if d_walk doesn't need to be updated I would be surprised to see it take into autofs. But I am happy to look at the end result and see what you come up with.

Eric

Quoted 54 lines · Eric W. Biederman

Quoted 39 lines · Ian Kent

Quoted 38 lines · Eric W. Biederman

Ian Kent <[email protected]> writes:

Quoted 34 lines · Eric W. Biederman

Quoted 19 lines

Quoted 17 lines · Ian Kent

Quoted 16 lines · Eric W. Biederman

Quoted 13 lines · Ian Kent

Eric, Mateusz, I appreciate your spending time on this and particularly
pointing
out my embarrassingly stupid is_local_mountpoint() usage mistake.

Please accept my apology for the inconvenience.

If all goes well (in testing) I'll have follow up patches to correct
this
fairly
soon.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Related question. Do you happen to know how many mounts per mount namespace tend to be used? It looks like it is going to be wise to
put
a configurable limit on that number. And I would like the default to
be
something high enough most people don't care. I believe autofs is likely where people tend to use the most mounts.

Yes, I agree, I did want to try and avoid changing the parameters to ->d_mamange() but passing a struct path pointer might be better in the long
run
anyway.

Quoted 6 lines

Quoted 2 lines

Given that there is exactly one implementation of d_manage in the tree I don't imagine it will be disruptive to change that.

Yes, but it could be used by external modules.

And there's also have_submounts().

Good point about have_submounts.

Quoted 4 lines

I can update that using the existing d_walk() infrastructure or take it (mostly)
into the autofs module and get rid of have_submounts().

Quoted 6 lines

Quoted 1 line

I'll go with the former to start with and see what people think.

That will be interesting to so. It is not clear to me that if d_walk needs to be updated, and if d_walk doesn't need to be updated I would be surprised to see it take into autofs. But I am happy to look at the end result and see what you come up with.

I didn't mean d_walk() itself, just the have_submounts() function that's used only by autofs these days. That's all I'll be changing.

To take this functionality into the autofs module shouldn't be a big deal as it amounts to a directory traversal with a check at each node.

But I vaguely remember talk of wanting to get rid of have_submounts() and autofs being the only remaining user.

So I mentioned it to try and elicit a comment, ;)

Quoted 80 lines · Ian Kent

Quoted 60 lines · Eric W. Biederman

Quoted 44 lines · Ian Kent

Quoted 43 lines · Eric W. Biederman

Ian Kent <[email protected]> writes:

Quoted 39 lines · Eric W. Biederman

Quoted 19 lines

Quoted 17 lines · Ian Kent

Quoted 16 lines · Eric W. Biederman

Quoted 13 lines · Ian Kent

Eric, Mateusz, I appreciate your spending time on this and particularly
pointing
out my embarrassingly stupid is_local_mountpoint() usage mistake.

Please accept my apology for the inconvenience.

If all goes well (in testing) I'll have follow up patches to correct
this
fairly
soon.

Quoted 13 lines

Quoted 5 lines

Quoted 5 lines

Related question. Do you happen to know how many mounts per mount
namespace tend to be used? It looks like it is going to be wise
to

Quoted 8 lines

Quoted 8 lines

put
a configurable limit on that number. And I would like the default
to
be
something high enough most people don't care. I believe autofs is
likely where people tend to use the most mounts.

Yes, I agree, I did want to try and avoid changing the parameters to ->d_mamange() but passing a struct path pointer might be better in the
long
run
anyway.

Quoted 7 lines

Quoted 3 lines

Given that there is exactly one implementation of d_manage in the tree I
don't imagine it will be disruptive to change that.

Yes, but it could be used by external modules.

And there's also have_submounts().

Good point about have_submounts.

Quoted 4 lines

I can update that using the existing d_walk() infrastructure or take it (mostly)
into the autofs module and get rid of have_submounts().

Quoted 6 lines

Quoted 1 line

I'll go with the former to start with and see what people think.

That will be interesting to so. It is not clear to me that if d_walk needs to be updated, and if d_walk doesn't need to be updated I would be surprised to see it take into autofs. But I am happy to look at the end result and see what you come up with.

I didn't mean d_walk() itself, just the have_submounts() function that's used
only by autofs these days. That's all I'll be changing.

To take this functionality into the autofs module shouldn't be a big deal as it
amounts to a directory traversal with a check at each node.

But I vaguely remember talk of wanting to get rid of have_submounts() and autofs
being the only remaining user.

Quoted 1 line

So I mentioned it to try and elicit a comment, ;)

From my perspective the key detail is that d_walk is private to dcache.c

That said you want to look at may_umount_tree, or may_umount that are already exported from fs/namespace.c, and already used by autofs. One of those may already do the job you are trying to do.