autofs4 - cleanup autofs mount type usage

Patch to track the uid and gid of the last process to request a mount for on an autofs dentry.

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

---

 fs/autofs4/autofs_i.h |    3 +++
 fs/autofs4/inode.c    |    2 ++
 fs/autofs4/waitq.c    |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 0 deletions(-)

fs/autofs4/autofs_i.h +3 -0

index ea024d8..fa76d18 100644
@@ -63,6 +63,9 @@ struct autofs_info {
 	unsigned long last_used;
 	atomic_t count;
 
+	uid_t uid;
+	gid_t gid;
+
 	mode_t	mode;
 	size_t	size;
 

fs/autofs4/inode.c +2 -0

index 9ca2d07..9408507 100644
@@ -53,6 +53,8 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
 		atomic_set(&ino->count, 0);
 	}
 
+	ino->uid = 0;
+	ino->gid = 0;
 	ino->mode = mode;
 	ino->last_used = jiffies;
 

fs/autofs4/waitq.c +0 -0

index 6d87bb1..7c60c0b 100644

fs/autofs4/waitq.c +22 -0

+	 * uid and gid in the dentry info struct. This is so it can be
+	 * supplied, on request, by the misc device ioctl interface.
+	 * This is needed during daemon resatart when reconnecting
+	 * to existing, active, autofs mounts. The uid and gid (and
+	 * related string values) may be used for macro substitution
+	 * in autofs mount maps.
+	 */
+	if (!status) {
+		struct autofs_info *ino;
+		struct dentry *de = NULL;
+
+		/* direct mount or browsable map */
+		ino = autofs4_dentry_ino(dentry);
+		if (!ino) {
+			/* If not lookup actual dentry used */
+			de = d_lookup(dentry->d_parent, &dentry->d_name);
+			if (de)
+				ino = autofs4_dentry_ino(de);
+		}
+
+		/* Set mount requester */
+		if (ino) {

Quoted 2 lines · On Thu, 07 Aug 2008 19:40:14 +0800 Ian Kent <[email protected]> wrote:

Patch to track the uid and gid of the last process to request a mount for on an autofs dentry.

pet peeve: changelog should not tell the reader that this is a "patch". Because when someone is reading the changelog in the git repository, they hopefully already know that.

Quoted 42 lines

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

---

 fs/autofs4/autofs_i.h |    3 +++
 fs/autofs4/inode.c    |    2 ++
 fs/autofs4/waitq.c    |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 0 deletions(-)

fs/autofs4/autofs_i.h +3 -0

index ea024d8..fa76d18 100644
@@ -63,6 +63,9 @@ struct autofs_info {
 	unsigned long last_used;
 	atomic_t count;
 
+	uid_t uid;
+	gid_t gid;
+
 	mode_t	mode;
 	size_t	size;
 

fs/autofs4/inode.c +2 -0

index 9ca2d07..9408507 100644
@@ -53,6 +53,8 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
 		atomic_set(&ino->count, 0);
 	}
 
+	ino->uid = 0;
+	ino->gid = 0;
 	ino->mode = mode;
 	ino->last_used = jiffies;
 

fs/autofs4/waitq.c +0 -0

index 6d87bb1..7c60c0b 100644

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 461

Quoted 5 lines

fs/autofs4/waitq.c +2 -0

@@ -457,6 +457,40 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 
 	status = wq->status;
 
+	/*
+	 * For direct and offset mounts we need to track the requestrer

typo which I'll fix.

Quoted 28 lines

fs/autofs4/waitq.c +28 -0

+	 * uid and gid in the dentry info struct. This is so it can be
+	 * supplied, on request, by the misc device ioctl interface.
+	 * This is needed during daemon resatart when reconnecting
+	 * to existing, active, autofs mounts. The uid and gid (and
+	 * related string values) may be used for macro substitution
+	 * in autofs mount maps.
+	 */
+	if (!status) {
+		struct autofs_info *ino;
+		struct dentry *de = NULL;
+
+		/* direct mount or browsable map */
+		ino = autofs4_dentry_ino(dentry);
+		if (!ino) {
+			/* If not lookup actual dentry used */
+			de = d_lookup(dentry->d_parent, &dentry->d_name);
+			if (de)
+				ino = autofs4_dentry_ino(de);
+		}
+
+		/* Set mount requester */
+		if (ino) {
+			spin_lock(&sbi->fs_lock);
+			ino->uid = wq->uid;
+			ino->gid = wq->gid;
+			spin_unlock(&sbi->fs_lock);
+		}
+

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 493

Quoted 4 lines

fs/autofs4/waitq.c +4 -0

+		if (de)
+			dput(de);
+	}
+

Please remind me again why autofs's use of current->uid and current->gid is not busted in the presence of PID namespaces, where these things are no longer system-wide unique?

Quoted 88 lines · Andrew Morton

Quoted 2 lines · On Thu, 07 Aug 2008 19:40:14 +0800 Ian Kent <[email protected]> wrote:

Patch to track the uid and gid of the last process to request a mount for on an autofs dentry.

pet peeve: changelog should not tell the reader that this is a "patch". Because when someone is reading the changelog in the git repository, they hopefully already know that.

Quoted 42 lines

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

---

 fs/autofs4/autofs_i.h |    3 +++
 fs/autofs4/inode.c    |    2 ++
 fs/autofs4/waitq.c    |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 0 deletions(-)

fs/autofs4/autofs_i.h +3 -0

index ea024d8..fa76d18 100644
@@ -63,6 +63,9 @@ struct autofs_info {
 	unsigned long last_used;
 	atomic_t count;
 
+	uid_t uid;
+	gid_t gid;
+
 	mode_t	mode;
 	size_t	size;
 

fs/autofs4/inode.c +2 -0

index 9ca2d07..9408507 100644
@@ -53,6 +53,8 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
 		atomic_set(&ino->count, 0);
 	}
 
+	ino->uid = 0;
+	ino->gid = 0;
 	ino->mode = mode;
 	ino->last_used = jiffies;
 

fs/autofs4/waitq.c +0 -0

index 6d87bb1..7c60c0b 100644

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 461

Quoted 5 lines

fs/autofs4/waitq.c +2 -0

@@ -457,6 +457,40 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 
 	status = wq->status;
 
+	/*
+	 * For direct and offset mounts we need to track the requestrer

typo which I'll fix.

Quoted 28 lines

fs/autofs4/waitq.c +28 -0

+	 * uid and gid in the dentry info struct. This is so it can be
+	 * supplied, on request, by the misc device ioctl interface.
+	 * This is needed during daemon resatart when reconnecting
+	 * to existing, active, autofs mounts. The uid and gid (and
+	 * related string values) may be used for macro substitution
+	 * in autofs mount maps.
+	 */
+	if (!status) {
+		struct autofs_info *ino;
+		struct dentry *de = NULL;
+
+		/* direct mount or browsable map */
+		ino = autofs4_dentry_ino(dentry);
+		if (!ino) {
+			/* If not lookup actual dentry used */
+			de = d_lookup(dentry->d_parent, &dentry->d_name);
+			if (de)
+				ino = autofs4_dentry_ino(de);
+		}
+
+		/* Set mount requester */
+		if (ino) {
+			spin_lock(&sbi->fs_lock);
+			ino->uid = wq->uid;
+			ino->gid = wq->gid;
+			spin_unlock(&sbi->fs_lock);
+		}
+

Quoted 8 lines

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 493

Quoted 4 lines

fs/autofs4/waitq.c +4 -0

+		if (de)
+			dput(de);
+	}
+

Please remind me again why autofs's use of current->uid and current->gid is not busted in the presence of PID namespaces, where these things are no longer system-wide unique?

I actually don't see what the autofs4_waitq->pid is used for. It's copied from current into wq->pid at autofs4_wait, and into a packet to send to userspace (I assume) at autofs4_notify_daemon.

So as long as a daemon can serve multiple pid namespaces (which doubtless it can), the pid could be confusing (or erroneous) for the daemon.

If I'm remotely right about how the pid is being used, then the thing to do would be to
1. store the daemon's pid namespace (would that belong in the autofs_sb_info?)

2. store the task_pid(current) in the waitqueue
3. retrieve the pid_t for the waiting task in the daemon's pid namespace, and put that into the packet at autofs4_notify_daemon.

I realize this patch was about the *uids*, but the pids seem more urgent.

Quoted 5 lines

Quoted 4 lines

Please remind me again why autofs's use of current->uid and current->gid is not busted in the presence of PID namespaces, where these things are no longer system-wide unique?

Quoted 7 lines

I actually don't see what the autofs4_waitq->pid is used for. It's copied from current into wq->pid at autofs4_wait, and into a packet to send to userspace (I assume) at autofs4_notify_daemon.

So as long as a daemon can serve multiple pid namespaces (which doubtless it can), the pid could be confusing (or erroneous) for the daemon.

Your point is well taken.

The pid is used purely for logging purposes to aid in debugging in user space. I'm not sure it is worth worrying about it too much as the daemon has no business interfering with user space processes it is not the owner of.

Quoted 5 lines

If I'm remotely right about how the pid is being used, then the thing to do would be to
1. store the daemon's pid namespace (would that belong in the autofs_sb_info?)

Yep.

Quoted 5 lines

2. store the task_pid(current) in the waitqueue
3. retrieve the pid_t for the waiting task in the daemon's pid namespace, and put that into the packet at autofs4_notify_daemon.

Quoted 2 lines

I realize this patch was about the *uids*, but the pids seem more urgent.

OK, I get it.
I'll have a go at doing this for completeness.

Ian

Quoted 33 lines · Ian Kent

Quoted 5 lines

Quoted 4 lines

Please remind me again why autofs's use of current->uid and current->gid is not busted in the presence of PID namespaces, where these things are no longer system-wide unique?

Quoted 7 lines

I actually don't see what the autofs4_waitq->pid is used for. It's copied from current into wq->pid at autofs4_wait, and into a packet to send to userspace (I assume) at autofs4_notify_daemon.

So as long as a daemon can serve multiple pid namespaces (which doubtless it can), the pid could be confusing (or erroneous) for the daemon.

Your point is well taken.

The pid is used purely for logging purposes to aid in debugging in user space. I'm not sure it is worth worrying about it too much as the daemon has no business interfering with user space processes it is not the owner of.

Quoted 5 lines

If I'm remotely right about how the pid is being used, then the thing to do would be to
1. store the daemon's pid namespace (would that belong in the autofs_sb_info?)

Yep.

Quoted 5 lines

2. store the task_pid(current) in the waitqueue
3. retrieve the pid_t for the waiting task in the daemon's pid namespace, and put that into the packet at autofs4_notify_daemon.

Quoted 5 lines

Quoted 2 lines

I realize this patch was about the *uids*, but the pids seem more urgent.

OK, I get it.
I'll have a go at doing this for completeness.

On second thoughts I'm not sure about this.

The pid that is logged needs to relate to a process in the name space of the one that caused the mount to be done.

For example, suppose a GUI user finds mounts never expiring, then we get a debug log to try and identify the culprit. So the pid should correspond to a process that the user sees (So I guess in the namespace of that user).

This is the only reason I added the pid to the request packet in the first place.

Please correct me if my understanding of this is not right.

Quoted 51 lines · Ian Kent

Quoted 33 lines · Ian Kent

Quoted 5 lines

Quoted 4 lines

Please remind me again why autofs's use of current->uid and current->gid is not busted in the presence of PID namespaces, where these things are no longer system-wide unique?

Quoted 7 lines

I actually don't see what the autofs4_waitq->pid is used for. It's copied from current into wq->pid at autofs4_wait, and into a packet to send to userspace (I assume) at autofs4_notify_daemon.

So as long as a daemon can serve multiple pid namespaces (which doubtless it can), the pid could be confusing (or erroneous) for the daemon.

Your point is well taken.

The pid is used purely for logging purposes to aid in debugging in user space. I'm not sure it is worth worrying about it too much as the daemon has no business interfering with user space processes it is not the owner of.

Quoted 5 lines

If I'm remotely right about how the pid is being used, then the thing to do would be to
1. store the daemon's pid namespace (would that belong in the autofs_sb_info?)

Yep.

Quoted 5 lines

2. store the task_pid(current) in the waitqueue
3. retrieve the pid_t for the waiting task in the daemon's pid namespace, and put that into the packet at autofs4_notify_daemon.

Quoted 5 lines

Quoted 2 lines

I realize this patch was about the *uids*, but the pids seem more urgent.

OK, I get it.
I'll have a go at doing this for completeness.

On second thoughts I'm not sure about this.

The pid that is logged needs to relate to a process in the name space of the one that caused the mount to be done.

For example, suppose a GUI user finds mounts never expiring, then we get a debug log to try and identify the culprit. So the pid should correspond to a process that the user sees (So I guess in the namespace of that user).

Quoted 4 lines

This is the only reason I added the pid to the request packet in the first place.

Please correct me if my understanding of this is not right.

It's not wrong, but we just have to think through which value is the most useful.

Any process executing clone(CLONE_NEWPID) (with CAP_SYS_ADMIN) can start an application in a new pid namespace. So imagine the user at the desktop clicking some button which runs an application in a new pid namespace. Now if the user starts an xterm and runs ps -ef, the pid values he'll see for the tasks in that new namespace will not be the same as those which the application sees for itself, and not the same as those which, right now, autofs would report.

For instance, if I start a shell in a new pid namespace, then within the new pid namespace ps -ef gives me:

sh-3.2# ps -ef

UID        PID  PPID  C STIME TTY          TIME CMD
root         1     0  0 10:54 pts/1    00:00:00 /bin/sh
root         5     1  0 10:54 pts/1    00:00:00 /bin/sleep 100
root         6     1  0 10:54 pts/1    00:00:00 ps -ef

but from another shell as the same user, partial output of ps -ef gives me:

root      2877  2876  0 10:54 pts/1    00:00:00 /bin/sh
root      2881  2877  0 10:54 pts/1    00:00:00 /bin/sleep 100

And so what we're trying to decide is whether autofs should send pid 5 or pid 2881 for a message about the "/bin/sleep 100" task.

In fact, if the user clicks that button twice, chances are both instances of the application will have the same pid values for each process in the application. So now if autofs sends a message to the user about the application, the user cannot tell which process is at fault.

Autofs will be sending the user some message about 'process 5'. The user won't know whether it means "the real" pid 5, [watchdog/0], pid 5 in the first instance of the application, or pid 5 in the second instance.

Now it's true that the user's xterm may still be in a different (descendent) pidns of the autofs daemon. But we can't expect the autofs daemon to do pid_t translation for the user, so I think what we have to aim for is making sure that the values reported are unique within the pidns of the autofs daemon. And that means sending back either the pid values in the autofs daemon's pid namespace, or using the top-level pid_ts, that is, the pid values in the init namespace, which will be unique on the whole system.

Sorry this turned out long-winded, I hope it makes sense. And if I'm just showing a misunderstanding of what you're doing, please do correct me :)

Quoted 107 lines · Serge E. Hallyn

Quoted 51 lines · Ian Kent

Quoted 33 lines · Ian Kent

Quoted 5 lines

Quoted 4 lines

Please remind me again why autofs's use of current->uid and current->gid is not busted in the presence of PID namespaces, where these things are no longer system-wide unique?

Quoted 7 lines

I actually don't see what the autofs4_waitq->pid is used for. It's copied from current into wq->pid at autofs4_wait, and into a packet to send to userspace (I assume) at autofs4_notify_daemon.

So as long as a daemon can serve multiple pid namespaces (which doubtless it can), the pid could be confusing (or erroneous) for the daemon.

Your point is well taken.

The pid is used purely for logging purposes to aid in debugging in user space. I'm not sure it is worth worrying about it too much as the daemon has no business interfering with user space processes it is not the owner of.

Quoted 5 lines

If I'm remotely right about how the pid is being used, then the thing to do would be to
1. store the daemon's pid namespace (would that belong in the autofs_sb_info?)

Yep.

Quoted 5 lines

2. store the task_pid(current) in the waitqueue
3. retrieve the pid_t for the waiting task in the daemon's pid namespace, and put that into the packet at autofs4_notify_daemon.

Quoted 5 lines

Quoted 2 lines

I realize this patch was about the *uids*, but the pids seem more urgent.

OK, I get it.
I'll have a go at doing this for completeness.

On second thoughts I'm not sure about this.

The pid that is logged needs to relate to a process in the name space of the one that caused the mount to be done.

For example, suppose a GUI user finds mounts never expiring, then we get a debug log to try and identify the culprit. So the pid should correspond to a process that the user sees (So I guess in the namespace of that user).

Quoted 4 lines

This is the only reason I added the pid to the request packet in the first place.

Please correct me if my understanding of this is not right.

It's not wrong, but we just have to think through which value is the most useful.

Any process executing clone(CLONE_NEWPID) (with CAP_SYS_ADMIN) can start an application in a new pid namespace. So imagine the user at the desktop clicking some button which runs an application in a new pid namespace. Now if the user starts an xterm and runs ps -ef, the pid values he'll see for the tasks in that new namespace will not be the same as those which the application sees for itself, and not the same as those which, right now, autofs would report.

For instance, if I start a shell in a new pid namespace, then within the new pid namespace ps -ef gives me:

sh-3.2# ps -ef

UID        PID  PPID  C STIME TTY          TIME CMD
root         1     0  0 10:54 pts/1    00:00:00 /bin/sh
root         5     1  0 10:54 pts/1    00:00:00 /bin/sleep 100
root         6     1  0 10:54 pts/1    00:00:00 ps -ef

but from another shell as the same user, partial output of ps -ef gives me:

root      2877  2876  0 10:54 pts/1    00:00:00 /bin/sh
root      2881  2877  0 10:54 pts/1    00:00:00 /bin/sleep 100

And so what we're trying to decide is whether autofs should send pid 5 or pid 2881 for a message about the "/bin/sleep 100" task.

In fact, if the user clicks that button twice, chances are both instances of the application will have the same pid values for each process in the application. So now if autofs sends a message to the user about the application, the user cannot tell which process is at fault.

Autofs will be sending the user some message about 'process 5'. The user won't know whether it means "the real" pid 5, [watchdog/0], pid 5 in the first instance of the application, or pid 5 in the second instance.

Now it's true that the user's xterm may still be in a different (descendent) pidns of the autofs daemon. But we can't expect the autofs daemon to do pid_t translation for the user, so I think what we have to aim for is making sure that the values reported are unique within the pidns of the autofs daemon. And that means sending back either the pid values in the autofs daemon's pid namespace, or using the top-level pid_ts, that is, the pid values in the init namespace, which will be unique on the whole system.

Quoted 3 lines

Sorry this turned out long-winded, I hope it makes sense. And if I'm just showing a misunderstanding of what you're doing, please do correct me :)

Yes, it's a bit tricky.

In reality this information is only logged when debug logging is enabled and generally is only used by myself or others that maintain autofs. But getting sensible logging is important so it's worth sorting this out.

I think it would be best to use the pid of the highest view namespace which I think is the gist of what you said in the beginning. Then (at some future time), if there was a user space API, the daemon could report additional pid information related to subordinate pid namespaces. I am assuming that, to be useful, an autofs daemon that is able to serve multiple namespaces would be higher up in the tree. But the forgoing description sounds like there's not a necessarily a hierarchic structure to pid namespaces?

The other issue that comes up is, after storing (a reference to) the daemon namespace in the super block info struct a "kill -9" on the daemon would render the namespace invalid. At the moment, when this happens the write on the kernel pipe fails causing the autofs mount to become catatonic, but for the namespace aware case the namespace is now invalid so we won't get that far. I could make the mount catatonic when

I detect that the namespace has become invalid but I'm not sure how to check it. Is there a way I can to do this? Would there be any issues with namespace (pid) reuse for such a check?

Sorry to seem so thick about this but the implications of namespaces for autofs are quite new to me and it is important that autofs "plays well with others" as the development progresses.

Anyway, assuming I'm on the right track I'll poke around further and see if I can work out how I to do this.

Quoted 119 lines · Ian Kent

Quoted 107 lines · Serge E. Hallyn

Quoted 51 lines · Ian Kent

Quoted 33 lines · Ian Kent

Quoted 5 lines

Quoted 4 lines

Please remind me again why autofs's use of current->uid and current->gid is not busted in the presence of PID namespaces, where these things are no longer system-wide unique?

Quoted 7 lines

I actually don't see what the autofs4_waitq->pid is used for. It's copied from current into wq->pid at autofs4_wait, and into a packet to send to userspace (I assume) at autofs4_notify_daemon.

So as long as a daemon can serve multiple pid namespaces (which doubtless it can), the pid could be confusing (or erroneous) for the daemon.

Your point is well taken.

The pid is used purely for logging purposes to aid in debugging in user space. I'm not sure it is worth worrying about it too much as the daemon has no business interfering with user space processes it is not the owner of.

Quoted 5 lines

If I'm remotely right about how the pid is being used, then the thing to do would be to
1. store the daemon's pid namespace (would that belong in the autofs_sb_info?)

Yep.

Quoted 5 lines

2. store the task_pid(current) in the waitqueue
3. retrieve the pid_t for the waiting task in the daemon's pid namespace, and put that into the packet at autofs4_notify_daemon.

Quoted 5 lines

Quoted 2 lines

I realize this patch was about the *uids*, but the pids seem more urgent.

OK, I get it.
I'll have a go at doing this for completeness.

On second thoughts I'm not sure about this.

The pid that is logged needs to relate to a process in the name space of the one that caused the mount to be done.

For example, suppose a GUI user finds mounts never expiring, then we get a debug log to try and identify the culprit. So the pid should correspond to a process that the user sees (So I guess in the namespace of that user).

Quoted 4 lines

This is the only reason I added the pid to the request packet in the first place.

Please correct me if my understanding of this is not right.

It's not wrong, but we just have to think through which value is the most useful.

Any process executing clone(CLONE_NEWPID) (with CAP_SYS_ADMIN) can start an application in a new pid namespace. So imagine the user at the desktop clicking some button which runs an application in a new pid namespace. Now if the user starts an xterm and runs ps -ef, the pid values he'll see for the tasks in that new namespace will not be the same as those which the application sees for itself, and not the same as those which, right now, autofs would report.

For instance, if I start a shell in a new pid namespace, then within the new pid namespace ps -ef gives me:

sh-3.2# ps -ef

UID        PID  PPID  C STIME TTY          TIME CMD
root         1     0  0 10:54 pts/1    00:00:00 /bin/sh
root         5     1  0 10:54 pts/1    00:00:00 /bin/sleep 100
root         6     1  0 10:54 pts/1    00:00:00 ps -ef

but from another shell as the same user, partial output of ps -ef gives me:

root      2877  2876  0 10:54 pts/1    00:00:00 /bin/sh
root      2881  2877  0 10:54 pts/1    00:00:00 /bin/sleep 100

And so what we're trying to decide is whether autofs should send pid 5 or pid 2881 for a message about the "/bin/sleep 100" task.

In fact, if the user clicks that button twice, chances are both instances of the application will have the same pid values for each process in the application. So now if autofs sends a message to the user about the application, the user cannot tell which process is at fault.

Autofs will be sending the user some message about 'process 5'. The user won't know whether it means "the real" pid 5, [watchdog/0], pid 5 in the first instance of the application, or pid 5 in the second instance.

Now it's true that the user's xterm may still be in a different (descendent) pidns of the autofs daemon. But we can't expect the autofs daemon to do pid_t translation for the user, so I think what we have to aim for is making sure that the values reported are unique within the pidns of the autofs daemon. And that means sending back either the pid values in the autofs daemon's pid namespace, or using the top-level pid_ts, that is, the pid values in the init namespace, which will be unique on the whole system.

Quoted 3 lines

Sorry this turned out long-winded, I hope it makes sense. And if I'm just showing a misunderstanding of what you're doing, please do correct me :)

Yes, it's a bit tricky.

In reality this information is only logged when debug logging is enabled and generally is only used by myself or others that maintain autofs. But getting sensible logging is important so it's worth sorting this out.

Quoted 8 lines

I think it would be best to use the pid of the highest view namespace which I think is the gist of what you said in the beginning. Then (at some future time), if there was a user space API, the daemon could report additional pid information related to subordinate pid namespaces. I am assuming that, to be useful, an autofs daemon that is able to serve multiple namespaces would be higher up in the tree. But the forgoing description sounds like there's not a necessarily a hierarchic structure to pid namespaces?

It is a simple tree, but of course if you have 3 autofs daemons in separate containers, and one on the main host, then the pid namespaces for each of the 3 container autofs daemons will be siblings, so they won't have meaningful translations for each others' pids, while pids in each container will have meaningful translations in the host system's pid namespace (the init_pid_ns).

Quoted 6 lines

The other issue that comes up is, after storing (a reference to) the daemon namespace in the super block info struct a "kill -9" on the daemon would render the namespace invalid. At the moment, when this happens the write on the kernel pipe fails causing the autofs mount to become catatonic, but for the namespace aware case the namespace is now invalid so we won't get that far. I could make the mount catatonic when

I figured you would grab a reference to the pid namespace, so it wouldn't go away until you released the superblock or registered a new daemon for it.

Quoted 3 lines

I detect that the namespace has become invalid but I'm not sure how to check it. Is there a way I can to do this? Would there be any issues with namespace (pid) reuse for such a check?

I'm not sure what you mean - but struct pid is never reused so long as you have a reference to one. So you could store get_pid(task_pid(autofs_daemon)) and check whether the autofs daemon's pid has changed that way, I suppose. But grabbing a struct pid reference does not pin the pid_ns iirc.

Maybe you're right about just getting the top-level pid_nr.

Quoted 2 lines · On Thu, 07 Aug 2008 19:40:14 +0800 Ian Kent <[email protected]> wrote:

Patch to track the uid and gid of the last process to request a mount for on an autofs dentry.

pet peeve: changelog should not tell the reader that this is a "patch". Because when someone is reading the changelog in the git repository, they hopefully already know that.

Quoted 42 lines

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

---

 fs/autofs4/autofs_i.h |    3 +++
 fs/autofs4/inode.c    |    2 ++
 fs/autofs4/waitq.c    |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 0 deletions(-)

fs/autofs4/autofs_i.h +3 -0

index ea024d8..fa76d18 100644
@@ -63,6 +63,9 @@ struct autofs_info {
 	unsigned long last_used;
 	atomic_t count;
 
+	uid_t uid;
+	gid_t gid;
+
 	mode_t	mode;
 	size_t	size;
 

fs/autofs4/inode.c +2 -0

index 9ca2d07..9408507 100644
@@ -53,6 +53,8 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
 		atomic_set(&ino->count, 0);
 	}
 
+	ino->uid = 0;
+	ino->gid = 0;
 	ino->mode = mode;
 	ino->last_used = jiffies;
 

fs/autofs4/waitq.c +0 -0

index 6d87bb1..7c60c0b 100644

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 461

Quoted 5 lines

fs/autofs4/waitq.c +2 -0

@@ -457,6 +457,40 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 
 	status = wq->status;
 
+	/*
+	 * For direct and offset mounts we need to track the requestrer

typo which I'll fix.

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 467

Quoted 6 lines

fs/autofs4/waitq.c +6 -0

+	 * uid and gid in the dentry info struct. This is so it can be
+	 * supplied, on request, by the misc device ioctl interface.
+	 * This is needed during daemon resatart when reconnecting
+	 * to existing, active, autofs mounts. The uid and gid (and
+	 * related string values) may be used for macro substitution
+	 * in autofs mount maps.

Quoted 16 lines

fs/autofs4/waitq.c +16 -0

+	 */
+	if (!status) {
+		struct autofs_info *ino;
+		struct dentry *de = NULL;
+
+		/* direct mount or browsable map */
+		ino = autofs4_dentry_ino(dentry);
+		if (!ino) {
+			/* If not lookup actual dentry used */
+			de = d_lookup(dentry->d_parent, &dentry->d_name);
+			if (de)
+				ino = autofs4_dentry_ino(de);
+		}
+
+		/* Set mount requester */
+		if (ino) {

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 493

Quoted 10 lines

fs/autofs4/waitq.c +10 -0

+			spin_lock(&sbi->fs_lock);
+			ino->uid = wq->uid;
+			ino->gid = wq->gid;
+			spin_unlock(&sbi->fs_lock);
+		}
+
+		if (de)
+			dput(de);
+	}
+

Please remind me again why autofs's use of current->uid and current->gid is not busted in the presence of PID namespaces, where these things are no longer system-wide unique?
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 60 lines · Andrew Morton

Quoted 2 lines · On Thu, 07 Aug 2008 19:40:14 +0800 Ian Kent <[email protected]> wrote:

Patch to track the uid and gid of the last process to request a mount for on an autofs dentry.

pet peeve: changelog should not tell the reader that this is a "patch". Because when someone is reading the changelog in the git repository, they hopefully already know that.

Quoted 42 lines

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

---

 fs/autofs4/autofs_i.h |    3 +++
 fs/autofs4/inode.c    |    2 ++
 fs/autofs4/waitq.c    |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 0 deletions(-)

fs/autofs4/autofs_i.h +3 -0

index ea024d8..fa76d18 100644
@@ -63,6 +63,9 @@ struct autofs_info {
 	unsigned long last_used;
 	atomic_t count;
 
+	uid_t uid;
+	gid_t gid;
+
 	mode_t	mode;
 	size_t	size;
 

fs/autofs4/inode.c +2 -0

index 9ca2d07..9408507 100644
@@ -53,6 +53,8 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
 		atomic_set(&ino->count, 0);
 	}
 
+	ino->uid = 0;
+	ino->gid = 0;
 	ino->mode = mode;
 	ino->last_used = jiffies;
 

fs/autofs4/waitq.c +0 -0

index 6d87bb1..7c60c0b 100644

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 461

Quoted 5 lines

fs/autofs4/waitq.c +2 -0

@@ -457,6 +457,40 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 
 	status = wq->status;
 
+	/*
+	 * For direct and offset mounts we need to track the requestrer

typo which I'll fix.

Quoted 6 lines

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 467

Quoted 6 lines

fs/autofs4/waitq.c +6 -0

+	 * uid and gid in the dentry info struct. This is so it can be
+	 * supplied, on request, by the misc device ioctl interface.
+	 * This is needed during daemon resatart when reconnecting
+	 * to existing, active, autofs mounts. The uid and gid (and
+	 * related string values) may be used for macro substitution
+	 * in autofs mount maps.

Hi Ian,

could you say just a few more words on these macro substitution?

I think your use of uids can completely ignore user namespaces, but it depends on who does the macro substitutions and how...

thanks,
-serge

Quoted 34 lines

Quoted 16 lines

fs/autofs4/waitq.c +16 -0

+	 */
+	if (!status) {
+		struct autofs_info *ino;
+		struct dentry *de = NULL;
+
+		/* direct mount or browsable map */
+		ino = autofs4_dentry_ino(dentry);
+		if (!ino) {
+			/* If not lookup actual dentry used */
+			de = d_lookup(dentry->d_parent, &dentry->d_name);
+			if (de)
+				ino = autofs4_dentry_ino(de);
+		}
+
+		/* Set mount requester */
+		if (ino) {

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 493

Quoted 10 lines

fs/autofs4/waitq.c +10 -0

+			spin_lock(&sbi->fs_lock);
+			ino->uid = wq->uid;
+			ino->gid = wq->gid;
+			spin_unlock(&sbi->fs_lock);
+		}
+
+		if (de)
+			dput(de);
+	}
+

Please remind me again why autofs's use of current->uid and current->gid is not busted in the presence of PID namespaces, where these things are no longer system-wide unique?
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 70 lines · Serge E. Hallyn

Quoted 60 lines · Andrew Morton

Quoted 2 lines · On Thu, 07 Aug 2008 19:40:14 +0800 Ian Kent <[email protected]> wrote:

Patch to track the uid and gid of the last process to request a mount for on an autofs dentry.

pet peeve: changelog should not tell the reader that this is a "patch". Because when someone is reading the changelog in the git repository, they hopefully already know that.

Quoted 42 lines

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

---

 fs/autofs4/autofs_i.h |    3 +++
 fs/autofs4/inode.c    |    2 ++
 fs/autofs4/waitq.c    |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 0 deletions(-)

fs/autofs4/autofs_i.h +3 -0

index ea024d8..fa76d18 100644
@@ -63,6 +63,9 @@ struct autofs_info {
 	unsigned long last_used;
 	atomic_t count;
 
+	uid_t uid;
+	gid_t gid;
+
 	mode_t	mode;
 	size_t	size;
 

fs/autofs4/inode.c +2 -0

index 9ca2d07..9408507 100644
@@ -53,6 +53,8 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
 		atomic_set(&ino->count, 0);
 	}
 
+	ino->uid = 0;
+	ino->gid = 0;
 	ino->mode = mode;
 	ino->last_used = jiffies;
 

fs/autofs4/waitq.c +0 -0

index 6d87bb1..7c60c0b 100644

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 461

Quoted 5 lines

fs/autofs4/waitq.c +2 -0

@@ -457,6 +457,40 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 
 	status = wq->status;
 
+	/*
+	 * For direct and offset mounts we need to track the requestrer

typo which I'll fix.

Quoted 6 lines

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 467

Quoted 6 lines

fs/autofs4/waitq.c +6 -0

+	 * uid and gid in the dentry info struct. This is so it can be
+	 * supplied, on request, by the misc device ioctl interface.
+	 * This is needed during daemon resatart when reconnecting
+	 * to existing, active, autofs mounts. The uid and gid (and
+	 * related string values) may be used for macro substitution
+	 * in autofs mount maps.

Hi Ian,

Quoted 4 lines

could you say just a few more words on these macro substitution?

I think your use of uids can completely ignore user namespaces, but it depends on who does the macro substitutions and how...

Suppose we have an autofs map entry in /etc/auto.master:
/test /etc/auto.test

and in /etc/auto.test

im1  /om1 server:/dir1 \
     /om2/$USER server2:/dir2

Then if this is automounted and the daemon is sent a SIGKILL and started again we need the uid number that was used when this was mounted to re-construct the offsets (/om2/$USER in this case needs the substitution). The uid is used to lookup the user name.

Can you point me in the right direction wrt. to namespaces on this please Serge? We didn't really get to the bottom of it last time I think.

Quoted 2 lines · On Thu, 07 Aug 2008 19:40:14 +0800 Ian Kent <[email protected]> wrote:

Patch to track the uid and gid of the last process to request a mount for on an autofs dentry.

pet peeve: changelog should not tell the reader that this is a "patch". Because when someone is reading the changelog in the git repository, they hopefully already know that.

Quoted 42 lines

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

---

 fs/autofs4/autofs_i.h |    3 +++
 fs/autofs4/inode.c    |    2 ++
 fs/autofs4/waitq.c    |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 0 deletions(-)

fs/autofs4/autofs_i.h +3 -0

index ea024d8..fa76d18 100644
@@ -63,6 +63,9 @@ struct autofs_info {
 	unsigned long last_used;
 	atomic_t count;
 
+	uid_t uid;
+	gid_t gid;
+
 	mode_t	mode;
 	size_t	size;
 

fs/autofs4/inode.c +2 -0

index 9ca2d07..9408507 100644
@@ -53,6 +53,8 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
 		atomic_set(&ino->count, 0);
 	}
 
+	ino->uid = 0;
+	ino->gid = 0;
 	ino->mode = mode;
 	ino->last_used = jiffies;
 

fs/autofs4/waitq.c +0 -0

index 6d87bb1..7c60c0b 100644

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 461

Quoted 5 lines

fs/autofs4/waitq.c +2 -0

@@ -457,6 +457,40 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 
 	status = wq->status;
 
+	/*
+	 * For direct and offset mounts we need to track the requestrer

typo which I'll fix.

Quoted 28 lines

fs/autofs4/waitq.c +28 -0

+	 * uid and gid in the dentry info struct. This is so it can be
+	 * supplied, on request, by the misc device ioctl interface.
+	 * This is needed during daemon resatart when reconnecting
+	 * to existing, active, autofs mounts. The uid and gid (and
+	 * related string values) may be used for macro substitution
+	 * in autofs mount maps.
+	 */
+	if (!status) {
+		struct autofs_info *ino;
+		struct dentry *de = NULL;
+
+		/* direct mount or browsable map */
+		ino = autofs4_dentry_ino(dentry);
+		if (!ino) {
+			/* If not lookup actual dentry used */
+			de = d_lookup(dentry->d_parent, &dentry->d_name);
+			if (de)
+				ino = autofs4_dentry_ino(de);
+		}
+
+		/* Set mount requester */
+		if (ino) {
+			spin_lock(&sbi->fs_lock);
+			ino->uid = wq->uid;
+			ino->gid = wq->gid;
+			spin_unlock(&sbi->fs_lock);
+		}
+

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 493

Quoted 4 lines

fs/autofs4/waitq.c +4 -0

+		if (de)
+			dput(de);
+	}
+

Please remind me again why autofs's use of current->uid and current->gid is not busted in the presence of PID namespaces, where these things are no longer system-wide unique?

Quoted 88 lines · Andrew Morton

Quoted 2 lines · On Thu, 07 Aug 2008 19:40:14 +0800 Ian Kent <[email protected]> wrote:

Patch to track the uid and gid of the last process to request a mount for on an autofs dentry.

pet peeve: changelog should not tell the reader that this is a "patch". Because when someone is reading the changelog in the git repository, they hopefully already know that.

Quoted 42 lines

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

---

 fs/autofs4/autofs_i.h |    3 +++
 fs/autofs4/inode.c    |    2 ++
 fs/autofs4/waitq.c    |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 0 deletions(-)

fs/autofs4/autofs_i.h +3 -0

index ea024d8..fa76d18 100644
@@ -63,6 +63,9 @@ struct autofs_info {
 	unsigned long last_used;
 	atomic_t count;
 
+	uid_t uid;
+	gid_t gid;
+
 	mode_t	mode;
 	size_t	size;
 

fs/autofs4/inode.c +2 -0

index 9ca2d07..9408507 100644
@@ -53,6 +53,8 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
 		atomic_set(&ino->count, 0);
 	}
 
+	ino->uid = 0;
+	ino->gid = 0;
 	ino->mode = mode;
 	ino->last_used = jiffies;
 

fs/autofs4/waitq.c +0 -0

index 6d87bb1..7c60c0b 100644

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 461

Quoted 5 lines

fs/autofs4/waitq.c +2 -0

@@ -457,6 +457,40 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 
 	status = wq->status;
 
+	/*
+	 * For direct and offset mounts we need to track the requestrer

typo which I'll fix.

Quoted 28 lines

fs/autofs4/waitq.c +28 -0

+	 * uid and gid in the dentry info struct. This is so it can be
+	 * supplied, on request, by the misc device ioctl interface.
+	 * This is needed during daemon resatart when reconnecting
+	 * to existing, active, autofs mounts. The uid and gid (and
+	 * related string values) may be used for macro substitution
+	 * in autofs mount maps.
+	 */
+	if (!status) {
+		struct autofs_info *ino;
+		struct dentry *de = NULL;
+
+		/* direct mount or browsable map */
+		ino = autofs4_dentry_ino(dentry);
+		if (!ino) {
+			/* If not lookup actual dentry used */
+			de = d_lookup(dentry->d_parent, &dentry->d_name);
+			if (de)
+				ino = autofs4_dentry_ino(de);
+		}
+
+		/* Set mount requester */
+		if (ino) {
+			spin_lock(&sbi->fs_lock);
+			ino->uid = wq->uid;
+			ino->gid = wq->gid;
+			spin_unlock(&sbi->fs_lock);
+		}
+

Quoted 8 lines

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 493

Quoted 4 lines

fs/autofs4/waitq.c +4 -0

+		if (de)
+			dput(de);
+	}
+

Please remind me again why autofs's use of current->uid and current->gid is not busted in the presence of PID namespaces, where these things are no longer system-wide unique?

We didn't really get to the bottom of that last time I posted this but hopefully Serge will get right on this and we can sort out what, idf anything I need to do.

Patch to add documentation for the miscellaneous device module of autofs4.

Documentation/filesystems/autofs4-mount-control.txt new file +8 -0

+AUTOFS_DEV_IOCTL_SETPIPEFD_CMD
+------------------------------
+
+Set the pipe file descriptor used for kernel communication to the daemon.
+...In order to protect mounts against
+incorrectly setting the pipe descriptor we also require that the autofs
+mount be catatonic (see next call).
+

Quoted 11 lines · Ian Kent

Patch to add documentation for the miscellaneous device module of autofs4.

Documentation/filesystems/autofs4-mount-control.txt new file +8 -0

+AUTOFS_DEV_IOCTL_SETPIPEFD_CMD
+------------------------------
+
+Set the pipe file descriptor used for kernel communication to the daemon.
+...In order to protect mounts against
+incorrectly setting the pipe descriptor we also require that the autofs
+mount be catatonic (see next call).
+

Documentation/filesystems/autofs4-mount-control.txt line 306

Quoted 10 lines

Documentation/filesystems/autofs4-mount-control.txt new file +9 -0

+The call requires an initialized struct autofs_dev_ioctl with the
+ioctlfd field set to the descriptor obtained from the open call...
 
+AUTOFS_DEV_IOCTL_CATATONIC_CMD
+------------------------------
+
+Make the autofs mount point catatonic...
+
+The call requires an initialized struct autofs_dev_ioctl with the
+ioctlfd field set to the descriptor obtained from the open call.

I probably have it mixed up, which file descriptor is involved. Suppose the former incarnation of the userspace daemon just dies (kill -9, etc.) It will not set any mount points catatonic, so the new incarnation needs to do it, like this:

    const char* mtpt = "/net/whatever";
    int len = strlen(mtpt);
    if (len >= PATH_MAX) len = PATH_MAX - 1;
    int fdlist[2];
    int autoFD = open("/dev/autofs", flags);
    struct autofs_dev_ioctl* parms = 
	calloc(1, sizeof(struct autofs_dev_ioctl) + PATH_MAX);
    parms->size = sizeof(struct autofs_dev_ioctl) + len + 1;
    memcpy(parms->path, mtpt, len+1);
    parms->ioctlfd = ?????;
    ioctl(autoFD, AUTOFS_DEV_IOCTL_CATATONIC_CMD, parms);
    socketpair(AF_UNIX, SOCK_DGRAM, 0, fdlist); 
    parms>ioctlfd = fdlist[1];
    ioctl(autoFD, AUTOFS_DEV_IOCTL_SETPIPEFD_CMD, parms);

I don't see how userspace can unilaterally set the mount point catatonic, because it needs to set the mount point's file descriptor first, which can only be done if the mount point is already catatonic.

Perhaps the documentation could be changed to head off the kind of misunderstanding(s) I think I have here.

James F. Carter Voice 310 825 2897 FAX 310 206 6673
UCLA-Mathnet; 6115 MSA; 520 Portola Plaza; Los Angeles, CA, USA 90095-1555
Email: [email protected] http://www.math.ucla.edu/~jimc (q.v. for PGP key)

Patch to add documentation for the miscellaneous device module of autofs4.

Subject: [PATCH 4/4] autofs4 - add miscelaneous device for ioctls

Patch to add a miscellaneous device to the autofs4 module for routing ioctls. This provides the ability to obtain an ioctl file handle for an autofs mount point that is possibly covered by another mount.

The actual problem with autofs is that it can't reconnect to existing mounts. Immediately one things of just adding the ability to remount autofs file systems would solve it, but alas, that can't work. This is because autofs direct mounts and the implementation of "on demand mount and expire" of nested mount trees have the file system mounted on top of the mount trigger dentry.

To resolve this a miscellaneous device node for routing ioctl commands to these mount points has been implemented in the autofs4 kernel module and a library added to autofs. This provides the ability to open a file descriptor for these over mounted autofs mount points.

Please refer to Documentation/filesystems/autofs4-mount-control.txt for a discussion of the problem, implementation alternatives considered and a description of the interface.

...

fs/autofs4/dev-ioctl.c new file +8 -0

+static int check_name(const char *name)
+{
+	if (!strchr(name, '/'))
+		return -EINVAL;
+	return 0;
+}
+
+/*

fs/autofs4/dev-ioctl.c new file +25 -0

+/*
+ * Check that the user compiled against correct version of autofs
+ * misc device code.
+ *
+ * As well as checking the version compatibility this always copies
+ * the kernel interface version out.
+ */
+static int check_dev_ioctl_version(int cmd, struct autofs_dev_ioctl *param)
+{
+	int err = 0;
+
+	if ((AUTOFS_DEV_IOCTL_VERSION_MAJOR != param->ver_major) ||
+	    (AUTOFS_DEV_IOCTL_VERSION_MINOR < param->ver_minor)) {
+		AUTOFS_WARN("ioctl control interface version mismatch: "
+		     "kernel(%u.%u), user(%u.%u), cmd(%d)",
+		     AUTOFS_DEV_IOCTL_VERSION_MAJOR,
+		     AUTOFS_DEV_IOCTL_VERSION_MINOR,
+		     param->ver_major, param->ver_minor, cmd);
+		err = -EINVAL;
+	}
+
+	/* Fill in the kernel version. */
+	param->ver_major = AUTOFS_DEV_IOCTL_VERSION_MAJOR;
+	param->ver_minor = AUTOFS_DEV_IOCTL_VERSION_MINOR;
+

fs/autofs4/dev-ioctl.c new file +26 -0

+	if (copy_from_user(&tmp, in, sizeof(tmp)))
+		return ERR_PTR(-EFAULT);
+
+	if (tmp.size < sizeof(tmp))
+		return ERR_PTR(-EINVAL);
+
+	ads = kmalloc(tmp.size, GFP_KERNEL);
+	if (!ads)
+		return ERR_PTR(-ENOMEM);
+
+	if (copy_from_user(ads, in, tmp.size)) {
+		kfree(ads);
+		return ERR_PTR(-EFAULT);
+	}
+
+	return ads;
+}
+
+static inline void free_dev_ioctl(struct autofs_dev_ioctl *param)
+{
+	kfree(param);
+	return;
+}
+
+/*
+ * Check sanity of parameter control fields and if a path is present

fs/autofs4/dev-ioctl.c new file +26 -0

+	}
+
+	if (param->size > sizeof(*param)) {
+		err = check_name(param->path);
+		if (err) {
+			AUTOFS_WARN("invalid path supplied for cmd(0x%08x)",
+				    cmd);
+			goto out;
+		}
+
+		err = invalid_str(param->path,
+				 (void *) ((size_t) param + param->size));
+		if (err) {
+			AUTOFS_WARN("invalid path supplied for cmd(0x%08x)",
+				    cmd);
+			goto out;
+		}
+	}
+
+	err = 0;
+out:
+	return err;
+}
+
 
...
 
+static void autofs_dev_ioctl_fd_install(unsigned int fd, struct file *file)
+{

Quoted 2 lines · On Thu, 07 Aug 2008 19:40:31 +0800 Ian Kent <[email protected]> wrote:

Subject: [PATCH 4/4] autofs4 - add miscelaneous device for ioctls

I fixed that spello

Quoted 23 lines

Patch to add a miscellaneous device to the autofs4 module for routing ioctls. This provides the ability to obtain an ioctl file handle for an autofs mount point that is possibly covered by another mount.

The actual problem with autofs is that it can't reconnect to existing mounts. Immediately one things of just adding the ability to remount autofs file systems would solve it, but alas, that can't work. This is because autofs direct mounts and the implementation of "on demand mount and expire" of nested mount trees have the file system mounted on top of the mount trigger dentry.

To resolve this a miscellaneous device node for routing ioctl commands to these mount points has been implemented in the autofs4 kernel module and a library added to autofs. This provides the ability to open a file descriptor for these over mounted autofs mount points.

Please refer to Documentation/filesystems/autofs4-mount-control.txt for a discussion of the problem, implementation alternatives considered and a description of the interface.

...

fs/autofs4/dev-ioctl.c line 43

Quoted 5 lines

fs/autofs4/dev-ioctl.c new file +5 -0

+
+#define AUTOFS_DEV_IOCTL_SIZE	sizeof(struct autofs_dev_ioctl)
+
+typedef int (*ioctl_fn)(struct file *,
+struct autofs_sb_info *, struct autofs_dev_ioctl *);

Weird layout, which I fixed.

Quoted 14 lines

fs/autofs4/dev-ioctl.c new file +13 -0

+static int check_name(const char *name)
+{
+	if (!strchr(name, '/'))
+		return -EINVAL;
+	return 0;
+}
+
+/*
+ * Check a string doesn't overrun the chunk of
+ * memory we copied from user land.
+ */
+static int invalid_str(char *str, void *end)
+{

fs/autofs4/dev-ioctl.c line 62

Quoted 5 lines

fs/autofs4/dev-ioctl.c new file +5 -0

+	while ((void *) str <= end)
+		if (!*str++)
+			return 0;
+	return -EINVAL;
+}

What is this? gWwe copy strings in from userspace in 10000 different places without needing checks such as this?

Quoted 26 lines

fs/autofs4/dev-ioctl.c new file +25 -0

+/*
+ * Check that the user compiled against correct version of autofs
+ * misc device code.
+ *
+ * As well as checking the version compatibility this always copies
+ * the kernel interface version out.
+ */
+static int check_dev_ioctl_version(int cmd, struct autofs_dev_ioctl *param)
+{
+	int err = 0;
+
+	if ((AUTOFS_DEV_IOCTL_VERSION_MAJOR != param->ver_major) ||
+	    (AUTOFS_DEV_IOCTL_VERSION_MINOR < param->ver_minor)) {
+		AUTOFS_WARN("ioctl control interface version mismatch: "
+		     "kernel(%u.%u), user(%u.%u), cmd(%d)",
+		     AUTOFS_DEV_IOCTL_VERSION_MAJOR,
+		     AUTOFS_DEV_IOCTL_VERSION_MINOR,
+		     param->ver_major, param->ver_minor, cmd);
+		err = -EINVAL;
+	}
+
+	/* Fill in the kernel version. */
+	param->ver_major = AUTOFS_DEV_IOCTL_VERSION_MAJOR;
+	param->ver_minor = AUTOFS_DEV_IOCTL_VERSION_MINOR;
+

fs/autofs4/dev-ioctl.c line 98

Quoted 10 lines

fs/autofs4/dev-ioctl.c new file +10 -0

+	return err;
+}
+
+/*
+ * Copy parameter control struct, including a possible path allocated
+ * at the end of the struct.
+ */
+static struct autofs_dev_ioctl *copy_dev_ioctl(struct autofs_dev_ioctl __user *in)
+{
+	struct autofs_dev_ioctl tmp, *ads;

Variables called `tmp' get me upset, but it seems appropriate here.

Quoted 31 lines

fs/autofs4/dev-ioctl.c new file +31 -0

+	if (copy_from_user(&tmp, in, sizeof(tmp)))
+		return ERR_PTR(-EFAULT);
+
+	if (tmp.size < sizeof(tmp))
+		return ERR_PTR(-EINVAL);
+
+	ads = kmalloc(tmp.size, GFP_KERNEL);
+	if (!ads)
+		return ERR_PTR(-ENOMEM);
+
+	if (copy_from_user(ads, in, tmp.size)) {
+		kfree(ads);
+		return ERR_PTR(-EFAULT);
+	}
+
+	return ads;
+}
+
+static inline void free_dev_ioctl(struct autofs_dev_ioctl *param)
+{
+	kfree(param);
+	return;
+}
+
+/*
+ * Check sanity of parameter control fields and if a path is present
+ * check that it has a "/" and is terminated.
+ */
+static int validate_dev_ioctl(int cmd, struct autofs_dev_ioctl *param)
+{
+	int err = -EINVAL;

fs/autofs4/dev-ioctl.c line 135

Quoted 5 lines

fs/autofs4/dev-ioctl.c new file +5 -0

+
+	if (check_dev_ioctl_version(cmd, param)) {
+		AUTOFS_WARN("invalid device control module version "
+		     "supplied for cmd(0x%08x)", cmd);
+		goto out;

check_dev_ioctl_version() carefully returned a -EFOO value, but this caller dropped it on the floor.

Quoted 30 lines

fs/autofs4/dev-ioctl.c new file +26 -0

+	}
+
+	if (param->size > sizeof(*param)) {
+		err = check_name(param->path);
+		if (err) {
+			AUTOFS_WARN("invalid path supplied for cmd(0x%08x)",
+				    cmd);
+			goto out;
+		}
+
+		err = invalid_str(param->path,
+				 (void *) ((size_t) param + param->size));
+		if (err) {
+			AUTOFS_WARN("invalid path supplied for cmd(0x%08x)",
+				    cmd);
+			goto out;
+		}
+	}
+
+	err = 0;
+out:
+	return err;
+}
+
 
...
 
+static void autofs_dev_ioctl_fd_install(unsigned int fd, struct file *file)
+{

fs/autofs4/dev-ioctl.c line 172

Quoted 10 lines

fs/autofs4/dev-ioctl.c new file +10 -0

+	struct files_struct *files = current->files;
+	struct fdtable *fdt;
+
+	spin_lock(&files->file_lock);
+	fdt = files_fdtable(files);
+	BUG_ON(fdt->fd[fd] != NULL);
+	rcu_assign_pointer(fdt->fd[fd], file);
+	FD_SET(fd, fdt->close_on_exec);
+	spin_unlock(&files->file_lock);
+}

urgh, it's bad to have done a copy-n-paste on fd_install() here. It means that if we go and change the locking in core kernel (quite possible) then people won't udpate this code.

Do we have alternative here? Can we set close_on_exec outside the lock and just call fd_install()? Probably not.

Can we export set_close_on_exec() then call that after having called fd_install()? I think so.

But not this, please.

Quoted 30 lines · Andrew Morton

Quoted 2 lines · On Thu, 07 Aug 2008 19:40:31 +0800 Ian Kent <[email protected]> wrote:

Subject: [PATCH 4/4] autofs4 - add miscelaneous device for ioctls

I fixed that spello

Quoted 23 lines

Patch to add a miscellaneous device to the autofs4 module for routing ioctls. This provides the ability to obtain an ioctl file handle for an autofs mount point that is possibly covered by another mount.

The actual problem with autofs is that it can't reconnect to existing mounts. Immediately one things of just adding the ability to remount autofs file systems would solve it, but alas, that can't work. This is because autofs direct mounts and the implementation of "on demand mount and expire" of nested mount trees have the file system mounted on top of the mount trigger dentry.

To resolve this a miscellaneous device node for routing ioctl commands to these mount points has been implemented in the autofs4 kernel module and a library added to autofs. This provides the ability to open a file descriptor for these over mounted autofs mount points.

Please refer to Documentation/filesystems/autofs4-mount-control.txt for a discussion of the problem, implementation alternatives considered and a description of the interface.

...

Quoted 7 lines

fs/autofs4/dev-ioctl.c line 43

Quoted 5 lines

fs/autofs4/dev-ioctl.c new file +5 -0

+
+#define AUTOFS_DEV_IOCTL_SIZE	sizeof(struct autofs_dev_ioctl)
+
+typedef int (*ioctl_fn)(struct file *,
+struct autofs_sb_info *, struct autofs_dev_ioctl *);

Weird layout, which I fixed.

Auuuhh .. didn't want to do this. I personally like declarations like this to be on a single line but checkpatch.pl complained about it.

Quoted 16 lines

fs/autofs4/dev-ioctl.c line 278

Quoted 11 lines

fs/autofs4/dev-ioctl.c new file +11 -0

+static void autofs_dev_ioctl_fd_install(unsigned int fd, struct file *file)
+{
+	struct files_struct *files = current->files;
+	struct fdtable *fdt;
+
+	spin_lock(&files->file_lock);
+	fdt = files_fdtable(files);
+	rcu_assign_pointer(fdt->fd[fd], file);
+	FD_SET(fd, fdt->close_on_exec);
+	spin_unlock(&files->file_lock);
+}

urgh, it's bad to have done a copy-n-paste on fd_install() here. It means that if we go and change the locking in core kernel (quite possible) then people won't udpate this code.

Quoted 7 lines

Do we have alternative here? Can we set close_on_exec outside the lock and just call fd_install()? Probably not.

Can we export set_close_on_exec() then call that after having called fd_install()? I think so.

But not this, please.

No problem.
You mentioned this last time as well.

Since there are a couple of possible approaches and I wasn't sure which way to go I thought I'd post it as is and get feedback then make it a followup patch.

Could the pthreads user space daemon exec something between fd_install() and set_close_on_exec()?

Perhaps there are some other alternative approaches to this.

Suggestions?

Quoted 40 lines · On Fri, 08 Aug 2008 11:39:19 +0800 Ian Kent <[email protected]> wrote:

Quoted 30 lines · Andrew Morton

Quoted 2 lines · On Thu, 07 Aug 2008 19:40:31 +0800 Ian Kent <[email protected]> wrote:

Subject: [PATCH 4/4] autofs4 - add miscelaneous device for ioctls

I fixed that spello

Quoted 23 lines

Patch to add a miscellaneous device to the autofs4 module for routing ioctls. This provides the ability to obtain an ioctl file handle for an autofs mount point that is possibly covered by another mount.

The actual problem with autofs is that it can't reconnect to existing mounts. Immediately one things of just adding the ability to remount autofs file systems would solve it, but alas, that can't work. This is because autofs direct mounts and the implementation of "on demand mount and expire" of nested mount trees have the file system mounted on top of the mount trigger dentry.

To resolve this a miscellaneous device node for routing ioctl commands to these mount points has been implemented in the autofs4 kernel module and a library added to autofs. This provides the ability to open a file descriptor for these over mounted autofs mount points.

Please refer to Documentation/filesystems/autofs4-mount-control.txt for a discussion of the problem, implementation alternatives considered and a description of the interface.

...

Quoted 7 lines

fs/autofs4/dev-ioctl.c line 43

Quoted 5 lines

fs/autofs4/dev-ioctl.c new file +5 -0

+
+#define AUTOFS_DEV_IOCTL_SIZE	sizeof(struct autofs_dev_ioctl)
+
+typedef int (*ioctl_fn)(struct file *,
+struct autofs_sb_info *, struct autofs_dev_ioctl *);

Weird layout, which I fixed.

Quoted 2 lines

Auuuhh .. didn't want to do this. I personally like declarations like this to be on a single line but checkpatch.pl complained about it.

Well, that's why it's a checkpatch warning, not an error. The way I look at checkpatch is that it is for detecting possible mistakes. Did you _really_ mean to do that? Usually the answer is "nope, and I wouldn't have noticed unless you told me". But sometimes the answer is "yes, I really did mean that".

I routinely ignore checkpatch 80-col warnings, unless they are flagging something which is just egregiously revolting.

Anyway, that's an aside. Given that you decided to fit that typedef into 80 cols, the way you did it was weird ;)

Quoted 31 lines

Quoted 16 lines

fs/autofs4/dev-ioctl.c line 278

Quoted 11 lines

fs/autofs4/dev-ioctl.c new file +11 -0

+static void autofs_dev_ioctl_fd_install(unsigned int fd, struct file *file)
+{
+	struct files_struct *files = current->files;
+	struct fdtable *fdt;
+
+	spin_lock(&files->file_lock);
+	fdt = files_fdtable(files);
+	rcu_assign_pointer(fdt->fd[fd], file);
+	FD_SET(fd, fdt->close_on_exec);
+	spin_unlock(&files->file_lock);
+}

urgh, it's bad to have done a copy-n-paste on fd_install() here. It means that if we go and change the locking in core kernel (quite possible) then people won't udpate this code.

Quoted 7 lines

Do we have alternative here? Can we set close_on_exec outside the lock and just call fd_install()? Probably not.

Can we export set_close_on_exec() then call that after having called fd_install()? I think so.

But not this, please.

No problem.
You mentioned this last time as well.

Since there are a couple of possible approaches and I wasn't sure which way to go I thought I'd post it as is and get feedback then make it a followup patch.

Quoted 2 lines

Could the pthreads user space daemon exec something between fd_install() and set_close_on_exec()?

Gee, I don't know, it would depend on the context.

Is that a private file*? Was it just created, and is there no possibility that any other thread can be sharing it anyway? If so, there's no problem.

Quoted 4 lines

Perhaps there are some other alternative approaches to this.

Suggestions?

I don't know enough about autofs nor about what problem you're attacking here to be able to say, sorry. I don't even know why close_on_exec is being set?

Quoted 7 lines

Quoted 7 lines

No problem.
You mentioned this last time as well.

Since there are a couple of possible approaches and I wasn't sure which way to go I thought I'd post it as is and get feedback then make it a followup patch.

Quoted 8 lines

Quoted 2 lines

Could the pthreads user space daemon exec something between fd_install() and set_close_on_exec()?

Gee, I don't know, it would depend on the context.

Is that a private file*? Was it just created, and is there no possibility that any other thread can be sharing it anyway? If so, there's no problem.

The problem is that autofs threads can exec mount or umount at any time and we see annoying selinux file descriptor leak security violation messages. So the point of this is to set the bit at the same time as the file is inserted into the table.

Quoted 5 lines

Quoted 4 lines

Perhaps there are some other alternative approaches to this.

Suggestions?

Quoted 3 lines

I don't know enough about autofs nor about what problem you're attacking here to be able to say, sorry. I don't even know why close_on_exec is being set?

OK, sorry.

What I'm really after is:
Should I do this at all, given the above?

If this is sensible, should a parameter be added to fd_insall() to allow it to be requested at descriptor install or should a new function, say, fd_install_close_on_exec() be added?

The actual problem with autofs is that it can't reconnect to existing mounts. Immediately one things of just adding the ability to remount autofs file systems would solve it, but alas, that can't work. This is because autofs direct mounts and the implementation of "on demand mount and expire" of nested mount trees have the file system mounted on top of the mount trigger dentry.

Quoted 6 lines · Ian Kent

The actual problem with autofs is that it can't reconnect to existing mounts. Immediately one things of just adding the ability to remount autofs file systems would solve it, but alas, that can't work. This is because autofs direct mounts and the implementation of "on demand mount and expire" of nested mount trees have the file system mounted on top of the mount trigger dentry.

So what you really need instead of all the ioctl cruft is to get access to the sb of the hidden autofs4 mount. One way to do that that I can think of right now is to change from using get_sb_nodev as ->get_sb to a variant that can find an existing superblock using some mount options.

Quoted 8 lines · Christoph Hellwig

Quoted 6 lines · Ian Kent

The actual problem with autofs is that it can't reconnect to existing mounts. Immediately one things of just adding the ability to remount autofs file systems would solve it, but alas, that can't work. This is because autofs direct mounts and the implementation of "on demand mount and expire" of nested mount trees have the file system mounted on top of the mount trigger dentry.

Quoted 5 lines

So what you really need instead of all the ioctl cruft is to get access to the sb of the hidden autofs4 mount. One way to do that that I can think of right now is to change from using get_sb_nodev as ->get_sb to a variant that can find an existing superblock using some mount options.

I wished I'd spoken to you about this a long time ago since you seem to know how I can do this using a remount option but this isn't enough of an explanation of how it can be done.

AFAICT, mount(8) will never be able to get a "struct path" or "struct nameidata" to a mount point that is covered using the path lookup in fs/namespace.c:do_*_mount() so we'll never be called back via any VFS methods. So tell me more about how this could work please!

Quoted 16 lines · Ian Kent

Quoted 8 lines · Christoph Hellwig

Quoted 6 lines · Ian Kent

The actual problem with autofs is that it can't reconnect to existing mounts. Immediately one things of just adding the ability to remount autofs file systems would solve it, but alas, that can't work. This is because autofs direct mounts and the implementation of "on demand mount and expire" of nested mount trees have the file system mounted on top of the mount trigger dentry.

Quoted 5 lines

So what you really need instead of all the ioctl cruft is to get access to the sb of the hidden autofs4 mount. One way to do that that I can think of right now is to change from using get_sb_nodev as ->get_sb to a variant that can find an existing superblock using some mount options.

Quoted 8 lines

I wished I'd spoken to you about this a long time ago since you seem to know how I can do this using a remount option but this isn't enough of an explanation of how it can be done.

AFAICT, mount(8) will never be able to get a "struct path" or "struct nameidata" to a mount point that is covered using the path lookup in fs/namespace.c:do_*_mount() so we'll never be called back via any VFS methods. So tell me more about how this could work please!

Your introduction tells what you want to do is a remount, for which finding the superblock is enough. If that's not the actual requirement please explain the requirement. And don't point me to the gazillion ioctls you added, that seems to mostly not have much to do with that..

Usage of the AUTOFS_TYPE_* defines is a little confusing and appears inconsistent.

Quoted 2 lines · Ian Kent

Usage of the AUTOFS_TYPE_* defines is a little confusing and appears inconsistent.

Not enough of an explanation for this patch.

It does:

- move AUTOFS_TYPE_INDIRECT/AUTOFS_TYPE_DIRECT/AUTOFS_TYPE_OFFSET to include/linux/auto_fs4.h. Obviously okay but should mentioned in the changelog.

- add a new, unused AUTOFS_TYPE_ANY constant. Why?

- add a new AUTOFS_TYPE_TRIGGER and use it instead of AUTOFS_TYPE_DIRECT or in one single case (AUTOFS_TYPE_DIRECT|AUTOFS_TYPE_OFFSET). Definitively needs a good explanation.

- replaces one use of sbi->type = 0 with AUTOFS_TYPE_INDIRECT (0x0001). Why?