Replace pid_t in autofs with struct pid reference

From: Sukadev Bhattiprolu <[email protected]>
Subject: [PATCH 2/2] Replace pid_t in autofs with struct pid reference.

Make autofs container-friendly by caching struct pid reference rather than pid_t and using pid_nr() to retreive a task's pid_t.

ChangeLog:
- Fix Eric Biederman's comments - Use find_get_pid() to hold a reference to oz_pgrp and release while unmounting; separate out changes to autofs and autofs4.

- Fix Cedric's comments: retain old prototype of parse_options() and move necessary change to its caller.

Signed-off-by Sukadev Bhattiprolu <[email protected]>

Cc Cedric Le Goater <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Serge Hallyn <[email protected]>

Cc Eric Biederman <[email protected]>

Cc [email protected]

Acked-by Eric W. Biederman <[email protected]>

---
 fs/autofs/autofs_i.h |    4 ++--
 fs/autofs/inode.c    |   20 ++++++++++++++++----
 fs/autofs/root.c     |    6 ++++--
 3 files changed, 22 insertions(+), 8 deletions(-)

Index: lx26-21-rc3-mm2/fs/autofs/autofs_i.h
===================================================================

fs/autofs/autofs_i.h +2 -2

@@ -101,7 +101,7 @@ struct autofs_symlink {
 struct autofs_sb_info {
 	u32 magic;
 	struct file *pipe;
-	pid_t oz_pgrp;
+	struct pid *oz_pgrp;
 	int catatonic;
 	struct super_block *sb;
 	unsigned long exp_timeout;

@@ -122,7 +122,7 @@ static inline struct autofs_sb_info *aut
    filesystem without "magic".) */
 
 static inline int autofs_oz_mode(struct autofs_sb_info *sbi) {
-	return sbi->catatonic || process_group(current) == sbi->oz_pgrp;
+	return sbi->catatonic || task_pgrp(current) == sbi->oz_pgrp;
 }
 
 /* Hash operations */

Index: lx26-21-rc3-mm2/fs/autofs/inode.c
===================================================================

fs/autofs/inode.c +16 -4

@@ -37,6 +37,8 @@ void autofs_kill_sb(struct super_block *
 	if (!sbi->catatonic)
 		autofs_catatonic_mode(sbi); /* Free wait queues, close pipe */
 
+	put_pid(sbi->oz_pgrp);
+
 	autofs_hash_nuke(sbi);
 	for (n = 0 ; n < AUTOFS_MAX_SYMLINKS ; n++) {
 		if (test_bit(n, sbi->symlink_bitmap))

@@ -139,6 +141,7 @@ int autofs_fill_super(struct super_block
 	int pipefd;
 	struct autofs_sb_info *sbi;
 	int minproto, maxproto;
+	pid_t pgid;
 
 	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
 	if (!sbi)

@@ -150,7 +153,6 @@ int autofs_fill_super(struct super_block
 	sbi->pipe = NULL;
 	sbi->catatonic = 1;
 	sbi->exp_timeout = 0;
-	sbi->oz_pgrp = process_group(current);
 	autofs_initialize_hash(&sbi->dirhash);
 	sbi->queues = NULL;
 	memset(sbi->symlink_bitmap, 0, sizeof(long)*AUTOFS_SYMLINK_BITMAP_LEN);

@@ -171,7 +173,7 @@ int autofs_fill_super(struct super_block
 
 	/* Can this call block?  - WTF cares? s is locked. */
 	if (parse_options(data, &pipefd, &root_inode->i_uid,
-				&root_inode->i_gid, &sbi->oz_pgrp, &minproto,
+				&root_inode->i_gid, &pgid, &minproto,
 				&maxproto)) {
 		printk("autofs: called with bogus options\n");
 		goto fail_dput;

@@ -184,13 +186,21 @@ int autofs_fill_super(struct super_block
 		goto fail_dput;
 	}
 
-	DPRINTK(("autofs: pipe fd = %d, pgrp = %u\n", pipefd, sbi->oz_pgrp));
+	DPRINTK(("autofs: pipe fd = %d, pgrp = %u\n", pipefd, pgid));
+	sbi->oz_pgrp = find_get_pid(pgid);
+
+	if (!sbi->oz_pgrp) {
+		printk("autofs: could not find process group %d\n", pgid);
+		goto fail_dput;
+	}
+
 	pipe = fget(pipefd);
 
 	if (!pipe) {
 		printk("autofs: could not open pipe file descriptor\n");
-		goto fail_dput;
+		goto fail_put_pid;
 	}
+
 	if (!pipe->f_op || !pipe->f_op->write)
 		goto fail_fput;
 	sbi->pipe = pipe;

@@ -205,6 +215,8 @@ int autofs_fill_super(struct super_block
 fail_fput:
 	printk("autofs: pipe file descriptor does not contain proper ops\n");
 	fput(pipe);
+fail_put_pid:
+	put_pid(sbi->oz_pgrp);
 fail_dput:
 	dput(root);
 	goto fail_free;

Index: lx26-21-rc3-mm2/fs/autofs/root.c
===================================================================

fs/autofs/root.c +4 -3

@@ -213,8 +213,10 @@ static struct dentry *autofs_root_lookup
 	sbi = autofs_sbi(dir->i_sb);
 
 	oz_mode = autofs_oz_mode(sbi);
-	DPRINTK(("autofs_lookup: pid = %u, pgrp = %u, catatonic = %d, oz_mode = %d\n",
-		 current->pid, process_group(current), sbi->catatonic, oz_mode));
+	DPRINTK(("autofs_lookup: pid = %u, pgrp = %u, catatonic = %d, "
+				"oz_mode = %d\n", pid_nr(task_pid(current)),
+				process_group(current), sbi->catatonic,
+				oz_mode));
 
 	/*
 	 * Mark the dentry incomplete, but add it. This is needed so
-

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 11 lines · On Mon, 12 Mar 2007, [email protected] wrote:

From: Sukadev Bhattiprolu <[email protected]>
Subject: [PATCH 2/2] Replace pid_t in autofs with struct pid reference.

Make autofs container-friendly by caching struct pid reference rather than pid_t and using pid_nr() to retreive a task's pid_t.

ChangeLog:
- Fix Eric Biederman's comments - Use find_get_pid() to hold a reference to oz_pgrp and release while unmounting; separate out changes to autofs and autofs4.

What changes to autofs4?
Do you intend this change to be made for autofs4 also? Perhaps you expected me to do them, in which case you probably should ask me to do the patch.

Quoted 14 lines · Ian Kent

Quoted 11 lines · On Mon, 12 Mar 2007, [email protected] wrote:

From: Sukadev Bhattiprolu <[email protected]>
Subject: [PATCH 2/2] Replace pid_t in autofs with struct pid reference.

Make autofs container-friendly by caching struct pid reference rather than pid_t and using pid_nr() to retreive a task's pid_t.

ChangeLog:
- Fix Eric Biederman's comments - Use find_get_pid() to hold a reference to oz_pgrp and release while unmounting; separate out changes to autofs and autofs4.

Quoted 4 lines

What changes to autofs4?
Do you intend this change to be made for autofs4 also? Perhaps you expected me to do them, in which case you probably should ask me to do the patch.

The review history goes something like this.
- That's a big patch why are you touching autofs and autofs4 at the same time?
<split>
- Hmm. That change in the autofs4 patch looks fishy. <patch postponed until the issue was addressed>

autofs4 uses pids more extensively than autofs and so the change is correspondingly larger.

If you would like to look at what it would take to get autofs4 to only store values as struct pid * instead of storing pid_t that would be great. But for the most part I this is a massive global change that those of us pushing it are responsible for changing as fixing up as much of the code as we can, as is usual kernel practice.

How about you send over the autofs4 bit and I'll have a look (the autofs patch looked fine). That would save me a bit of time and if there are any changes needed I can send an updated patch for you guys to review. I don't think autofs4 uses pids differently, in principle, than autofs so it "should" be straight forward.

Quoted 5 lines

How about you send over the autofs4 bit and I'll have a look (the autofs patch looked fine). That would save me a bit of time and if there are any changes needed I can send an updated patch for you guys to review. I don't think autofs4 uses pids differently, in principle, than autofs so it "should" be straight forward.

Here's the latest.

thanks,

C.

From: Sukadev Bhattiprolu <[email protected]>

Subject: Replace pid_t in autofs4 with struct pid reference.

Make autofs4 container-friendly by caching struct pid reference rather than pid_t and using pid_nr() to retreive a task's pid_t.

ChangeLog:
- Fix Eric Biederman's comments - Use find_get_pid() to hold a reference to oz_pgrp and release while unmounting; separate out changes to autofs and autofs4.
- Also rollback my earlier change to autofs_wait_queue (pid and tgid in the wait queue are just used to write to a userspace daemon's pipe).
- Fix Cedric's comments: retain old prototype of parse_options() and move necessary change to its caller.

Signed-off-by Sukadev Bhattiprolu <[email protected]>

Cc Cedric Le Goater <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Serge Hallyn <[email protected]>

Cc Eric Biederman <[email protected]>

Cc [email protected]

---
 fs/autofs4/autofs_i.h |    6 +++---
 fs/autofs4/inode.c    |   21 ++++++++++++++++-----
 fs/autofs4/root.c     |    3 ++-
 fs/autofs4/waitq.c    |    4 ++--
 4 files changed, 23 insertions(+), 11 deletions(-)

Index: 2.6.20/fs/autofs4/autofs_i.h
===================================================================

fs/autofs4/autofs_i.h +3 -3

@@ -35,7 +35,7 @@
 /* #define DEBUG */
 
 #ifdef DEBUG
-#define DPRINTK(fmt,args...) do { printk(KERN_DEBUG "pid %d: %s: " fmt "\n" , current->pid , __FUNCTION__ , ##args); } while(0)
+#define DPRINTK(fmt,args...) do { printk(KERN_DEBUG "pid %d: %s: " fmt "\n" , pid_nr(task_pid(current)), __FUNCTION__ , ##args); } while(0)
 #else
 #define DPRINTK(fmt,args...) do {} while(0)
 #endif

@@ -96,7 +96,7 @@ struct autofs_sb_info {
 	u32 magic;
 	int pipefd;
 	struct file *pipe;
-	pid_t oz_pgrp;
+	struct pid *oz_pgrp;
 	int catatonic;
 	int version;
 	int sub_version;

@@ -127,7 +127,7 @@ static inline struct autofs_info *autofs
    filesystem without "magic".) */
 
 static inline int autofs4_oz_mode(struct autofs_sb_info *sbi) {
-	return sbi->catatonic || process_group(current) == sbi->oz_pgrp;
+	return sbi->catatonic || task_pgrp(current) == sbi->oz_pgrp;
 }
 
 /* Does a dentry have some pending activity? */

Index: 2.6.20/fs/autofs4/inode.c
===================================================================

fs/autofs4/inode.c +16 -5

@@ -163,6 +163,8 @@ void autofs4_kill_sb(struct super_block 
 	if ( !sbi->catatonic )
 		autofs4_catatonic_mode(sbi); /* Free wait queues, close pipe */
 
+	put_pid(sbi->oz_pgrp);
+
 	/* Clean up and release dangling references */
 	autofs4_force_release(sbi);
 

@@ -181,7 +183,7 @@ static int autofs4_show_options(struct s
 		return 0;
 
 	seq_printf(m, ",fd=%d", sbi->pipefd);
-	seq_printf(m, ",pgrp=%d", sbi->oz_pgrp);
+	seq_printf(m, ",pgrp=%d", pid_nr(sbi->oz_pgrp));
 	seq_printf(m, ",timeout=%lu", sbi->exp_timeout/HZ);
 	seq_printf(m, ",minproto=%d", sbi->min_proto);
 	seq_printf(m, ",maxproto=%d", sbi->max_proto);

@@ -311,6 +313,7 @@ int autofs4_fill_super(struct super_bloc
 	int pipefd;
 	struct autofs_sb_info *sbi;
 	struct autofs_info *ino;
+	pid_t pgid;
 
 	sbi = kmalloc(sizeof(*sbi), GFP_KERNEL);
 	if (!sbi)

@@ -325,7 +328,6 @@ int autofs4_fill_super(struct super_bloc
 	sbi->pipe = NULL;
 	sbi->catatonic = 1;
 	sbi->exp_timeout = 0;
-	sbi->oz_pgrp = process_group(current);
 	sbi->sb = s;
 	sbi->version = 0;
 	sbi->sub_version = 0;

@@ -361,7 +363,7 @@ int autofs4_fill_super(struct super_bloc
 
 	/* Can this call block? */
 	if (parse_options(data, &pipefd, &root_inode->i_uid, &root_inode->i_gid,
-				&sbi->oz_pgrp, &sbi->type, &sbi->min_proto,
+				&pgid, &sbi->type, &sbi->min_proto,
 				&sbi->max_proto)) {
 		printk("autofs: called with bogus options\n");
 		goto fail_dput;

@@ -389,12 +391,19 @@ int autofs4_fill_super(struct super_bloc
 		sbi->version = sbi->max_proto;
 	sbi->sub_version = AUTOFS_PROTO_SUBVERSION;
 
-	DPRINTK("pipe fd = %d, pgrp = %u", pipefd, sbi->oz_pgrp);
+	DPRINTK("pipe fd = %d, pgrp = %u", pipefd, pgid);
+
+	sbi->oz_pgrp = find_get_pid(pgid);
+	if (!sbi->oz_pgrp) {
+		printk("autofs: could not find process group %d\n", pgid);
+		goto fail_dput;
+	}
+
 	pipe = fget(pipefd);
 
 	if (!pipe) {
 		printk("autofs: could not open pipe file descriptor\n");
-		goto fail_dput;
+		goto fail_put_pid;
 	}
 	if (!pipe->f_op || !pipe->f_op->write)
 		goto fail_fput;

@@ -415,6 +424,8 @@ fail_fput:
 	printk("autofs: pipe file descriptor does not contain proper ops\n");
 	fput(pipe);
 	/* fall through */
+fail_put_pid:
+	put_pid(sbi->oz_pgrp);
 fail_dput:
 	dput(root);
 	goto fail_free;

Index: 2.6.20/fs/autofs4/root.c
===================================================================

fs/autofs4/root.c +2 -1

@@ -495,7 +495,8 @@ static struct dentry *autofs4_lookup(str
 	oz_mode = autofs4_oz_mode(sbi);
 
 	DPRINTK("pid = %u, pgrp = %u, catatonic = %d, oz_mode = %d",
-		 current->pid, process_group(current), sbi->catatonic, oz_mode);
+		 pid_nr(task_pid(current)), process_group(current),
+		 sbi->catatonic, oz_mode);
 
 	/*
 	 * Mark the dentry incomplete, but add it. This is needed so

Index: 2.6.20/fs/autofs4/waitq.c
===================================================================

fs/autofs4/waitq.c +2 -2

@@ -292,8 +292,8 @@ int autofs4_wait(struct autofs_sb_info *
 		wq->ino = autofs4_get_ino(sbi);
 		wq->uid = current->uid;
 		wq->gid = current->gid;
-		wq->pid = current->pid;
-		wq->tgid = current->tgid;
+		wq->pid = pid_nr(task_pid(current));
+		wq->tgid = pid_nr(task_tgid(current));
 		wq->status = -EINTR; /* Status return if interrupted */
 		atomic_set(&wq->wait_ctr, 2);
 		mutex_unlock(&sbi->wq_mutex);

Quoted 6 lines · Cedric Le Goater

Quoted 5 lines

How about you send over the autofs4 bit and I'll have a look (the autofs patch looked fine). That would save me a bit of time and if there are any changes needed I can send an updated patch for you guys to review. I don't think autofs4 uses pids differently, in principle, than autofs so it "should" be straight forward.

Quoted 1 line

Here's the latest.

That looks OK to me, assuming the "find_get_pid" and friends do what they suggest, but I'll give it a closer look tomorrow.

A ref count is used here, what affect does that have on a thread (or process) that may go away (or be summarily killed) without umounting the mount?

Quoted 7 lines

Index: 2.6.20/fs/autofs4/waitq.c
===================================================================

fs/autofs4/waitq.c +0 -0

@@ -292,8 +292,8 @@ int autofs4_wait(struct autofs_sb_info *
 		wq->ino = autofs4_get_ino(sbi);
 		wq->uid = current->uid;

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info * · line 299

Quoted 8 lines

fs/autofs4/waitq.c +2 -2

 		wq->gid = current->gid;
-		wq->pid = current->pid;
-		wq->tgid = current->tgid;
+		wq->pid = pid_nr(task_pid(current));
+		wq->tgid = pid_nr(task_tgid(current));
 		wq->status = -EINTR; /* Status return if interrupted */
 		atomic_set(&wq->wait_ctr, 2);
 		mutex_unlock(&sbi->wq_mutex);

Quoted 7 lines · Ian Kent

Quoted 6 lines · Cedric Le Goater

Quoted 5 lines

How about you send over the autofs4 bit and I'll have a look (the autofs patch looked fine). That would save me a bit of time and if there are any changes needed I can send an updated patch for you guys to review. I don't think autofs4 uses pids differently, in principle, than autofs so it "should" be straight forward.

Quoted 8 lines

Quoted 1 line

Here's the latest.

That looks OK to me, assuming the "find_get_pid" and friends do what they suggest, but I'll give it a closer look tomorrow.

A ref count is used here, what affect does that have on a thread (or process) that may go away (or be summarily killed) without umounting the mount?

Nothing.

The primary advantage is that you are pid wrap around safe as the struct pid will never point to another process after one of those events occurs.

struct pid is a very small structure so not freeing it when the process it originally referred to goes away is no big deal. Although not leaking when you stop using it is still important.

The other big use of struct pid is that to get the user space pid value you call pid_nr(). Depending on the pid namespace of the caller the return value of pid_nr() can be different. So when you store a pid or pass a pid between processes that should be done by passing a struct pid because those processes could be in different pid namespaces.

Quoted 12 lines

Quoted 12 lines

Index: 2.6.20/fs/autofs4/waitq.c
===================================================================

fs/autofs4/waitq.c +2 -2

@@ -292,8 +292,8 @@ int autofs4_wait(struct autofs_sb_info *
 		wq->ino = autofs4_get_ino(sbi);
 		wq->uid = current->uid;
 		wq->gid = current->gid;
-		wq->pid = current->pid;
-		wq->tgid = current->tgid;
+		wq->pid = pid_nr(task_pid(current));
+		wq->tgid = pid_nr(task_tgid(current));

Quoted 3 lines

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info * · line 299

Quoted 3 lines

fs/autofs4/waitq.c +0 -0

 		wq->status = -EINTR; /* Status return if interrupted */
 		atomic_set(&wq->wait_ctr, 2);
 		mutex_unlock(&sbi->wq_mutex);

I have a concern with this bit as I my quick review said the wait queue persists, and if so we should be cache the struct pid pointer, not the pid_t value. Heck the whol pid_nr(task_xxx(current)) idiom I find very suspicious.

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Index: 2.6.20/fs/autofs4/waitq.c
===================================================================

fs/autofs4/waitq.c +2 -2

@@ -292,8 +292,8 @@ int autofs4_wait(struct autofs_sb_info *
 		wq->ino = autofs4_get_ino(sbi);
 		wq->uid = current->uid;
 		wq->gid = current->gid;
-		wq->pid = current->pid;
-		wq->tgid = current->tgid;
+		wq->pid = pid_nr(task_pid(current));
+		wq->tgid = pid_nr(task_tgid(current));

Quoted 8 lines

Quoted 3 lines

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info * · line 299

Quoted 3 lines

fs/autofs4/waitq.c +0 -0

 		wq->status = -EINTR; /* Status return if interrupted */
 		atomic_set(&wq->wait_ctr, 2);
 		mutex_unlock(&sbi->wq_mutex);

I have a concern with this bit as I my quick review said the wait queue persists, and if so we should be cache the struct pid pointer, not the pid_t value. Heck the whol pid_nr(task_xxx(current)) idiom I find very suspicious.

Based just on what I see right here I agree it seems like we would want to store a ref to the pid, not store the pid_nr(pid) output, so in this context it is suspicious.

OTOH if you're saying that using pid_nr(task_pid(current)) anywhere should always be 'wrong', then please explain why, as I think we have a disagreement on the meanings of the structs involved. In other words, at some point I expect the only way to get a "pid number" out of a task would be using this exact idiom, "pid_nr(task_pid(current))".

Quoted 22 lines · Serge E. Hallyn

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Index: 2.6.20/fs/autofs4/waitq.c
===================================================================

fs/autofs4/waitq.c +2 -2

@@ -292,8 +292,8 @@ int autofs4_wait(struct autofs_sb_info *
 		wq->ino = autofs4_get_ino(sbi);
 		wq->uid = current->uid;
 		wq->gid = current->gid;
-		wq->pid = current->pid;
-		wq->tgid = current->tgid;
+		wq->pid = pid_nr(task_pid(current));
+		wq->tgid = pid_nr(task_tgid(current));

Quoted 8 lines

Quoted 3 lines

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info * · line 299

Quoted 3 lines

fs/autofs4/waitq.c +0 -0

 		wq->status = -EINTR; /* Status return if interrupted */
 		atomic_set(&wq->wait_ctr, 2);
 		mutex_unlock(&sbi->wq_mutex);

I have a concern with this bit as I my quick review said the wait queue persists, and if so we should be cache the struct pid pointer, not the pid_t value. Heck the whol pid_nr(task_xxx(current)) idiom I find very suspicious.

Quoted 3 lines

Based just on what I see right here I agree it seems like we would want to store a ref to the pid, not store the pid_nr(pid) output, so in this context it is suspicious.

So that far we are in agreement.

Quoted 5 lines

OTOH if you're saying that using pid_nr(task_pid(current)) anywhere should always be 'wrong', then please explain why, as I think we have a disagreement on the meanings of the structs involved. In other words, at some point I expect the only way to get a "pid number" out of a task would be using this exact idiom, "pid_nr(task_pid(current))".

Dealing with the current process is very common, and "pid_nr(task_pid(current)" is very long winded. Therefore I think it makes sense to have a specialized helper for that case.

I don't think "current->pid" and "current->tgid" are necessarily wrong.

For "process_session(current)", and "process_group(current)" I think they are fine but we might optimize them to something like:
"current_session()" and "current_group()".

The important part is that we have clearly detectable idioms for finding the pid values. So we can find the users and audit the code. Having a little more change so that the problem cases don't compile when they comes from a patch that hasn't caught up yet with the changes is also useful.

The only advantage I see in making everything go through something
like: pid_nr(task_pid(current)) is that we don't have the problem of storing the pid value twice. However if we have short hand helper functions for that case it will still work and we won't be horribly wordy.

Further I don't know how expensive pid_nr is going to be, I don't think it will be very expensive. But I still think it may be reasonable to cache the answers for the current process on the task_struct. Fewer cache lines and all of that jazz.

Mostly I just think pid_nr(task_pid(xxx)) looks ugly is rarely needed and is frequently associated with a bad conversion.

Eric

True, current->pid can probably always be legitimately taken as the pid number in the current task's cloning namespace. But task->pid is wrong.

So if as you say it's worth caching (not saying I doubt you, just that I haven't verified), then ideally we could cache current->pid but only access it using current_pid(). Does that seem worth doing?

In any case, certainly adding a task_pid_nr() helper which for starters returns pid_nr(task_pid(task)) seems reasonable. Note that Suka's about ready to send a new iteration of the pidns patchset, so I'd like this to be considered something to clean up on top of that patchset.

Quoted 22 lines · Serge E. Hallyn

Quoted 21 lines

Quoted 16 lines

Quoted 6 lines

Index: 2.6.20/fs/autofs4/waitq.c
===================================================================

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info * · line 299

Quoted 10 lines

fs/autofs4/waitq.c +2 -2

@@ -292,8 +292,8 @@ int autofs4_wait(struct autofs_sb_info *
 		wq->ino = autofs4_get_ino(sbi);
 		wq->uid = current->uid;
 		wq->gid = current->gid;
-		wq->pid = current->pid;
-		wq->tgid = current->tgid;
+		wq->pid = pid_nr(task_pid(current));
+		wq->tgid = pid_nr(task_tgid(current));
 		wq->status = -EINTR; /* Status return if interrupted */
 		atomic_set(&wq->wait_ctr, 2);
 		mutex_unlock(&sbi->wq_mutex);

I have a concern with this bit as I my quick review said the wait queue persists, and if so we should be cache the struct pid pointer, not the pid_t value. Heck the whol pid_nr(task_xxx(current)) idiom I find very suspicious.

Quoted 3 lines

Based just on what I see right here I agree it seems like we would want to store a ref to the pid, not store the pid_nr(pid) output, so in this context it is suspicious.

So that far we are in agreement.

So how about the following on top of the patch Cedric sent out?

Subject: [PATCH] autofs4: store struct pids in autofs_waitqs
From: Serge Hallyn <[email protected]>
Date: 1174412305 -0500

Store struct pids in autofs_waitqs in place of pidnrs to prevent pid overflow problems.

Signed-off-by Serge E. Hallyn <[email protected]>

---

 fs/autofs4/autofs_i.h |   13 +++++++++++--
 fs/autofs4/waitq.c    |   15 ++++++++-------
 2 files changed, 19 insertions(+), 9 deletions(-)

86a5866c7672b88d48380504977496d5637642ab

fs/autofs4/autofs_i.h +11 -2

index 3ccec0a..5d119e3 100644
@@ -79,8 +79,8 @@ struct autofs_wait_queue {
 	u64 ino;
 	uid_t uid;
 	gid_t gid;
-	pid_t pid;
-	pid_t tgid;
+	struct pid *pid;
+	struct pid *tgid;
 	/* This is for status reporting upon return */
 	int status;
 	atomic_t wait_ctr;

@@ -228,5 +228,14 @@ out:
 	return ret;
 }
 
+static void autofs_free_wait_queue(struct autofs_wait_queue *wq)
+{
+	if (wq->pid)
+		put_pid(wq->pid);
+	if (wq->tgid)
+		put_pid(wq->tgid);
+	kfree(wq);
+}
+
 void autofs4_dentry_release(struct dentry *);
 extern void autofs4_kill_sb(struct super_block *);

fs/autofs4/waitq.c +0 -0

index 9857543..4a9ad9b 100644

fs/autofs4/waitq.c +2 -2

 	}
 	default:

@@ -292,8 +292,8 @@ int autofs4_wait(struct autofs_sb_info *
 		wq->ino = autofs4_get_ino(sbi);
 		wq->uid = current->uid;
 		wq->gid = current->gid;
-		wq->pid = pid_nr(task_pid(current));
-		wq->tgid = pid_nr(task_tgid(current));
+		wq->pid = get_pid(task_pid(current));
+		wq->tgid = get_pid(task_tgid(current));
 		wq->status = -EINTR; /* Status return if interrupted */
 		atomic_set(&wq->wait_ctr, 2);
 		mutex_unlock(&sbi->wq_mutex);

fs/autofs4/waitq.c +0 -0

 	return status;
 }

@@ -390,7 +391,7 @@ int autofs4_wait_release(struct autofs_s
 	wq->status = status;
 
 	if (atomic_dec_and_test(&wq->wait_ctr)) /* Is anyone still waiting for

this guy? */

fs/autofs4/waitq.c +1 -1

-		kfree(wq);
+		autofs_free_wait_queue(wq);
 	else
 		wake_up_interruptible(&wq->queue);

Signature · 2 lines

-- 
1.1.6

Quoted 8 lines · Serge E. Hallyn

So how about the following on top of the patch Cedric sent out?

Subject: [PATCH] autofs4: store struct pids in autofs_waitqs
From: Serge Hallyn <[email protected]>
Date: 1174412305 -0500

Store struct pids in autofs_waitqs in place of pidnrs to prevent pid overflow problems.

At first skip it doesn't looks ok.

Quoted 47 lines

Signed-off-by Serge E. Hallyn <[email protected]>

---

 fs/autofs4/autofs_i.h |   13 +++++++++++--
 fs/autofs4/waitq.c    |   15 ++++++++-------
 2 files changed, 19 insertions(+), 9 deletions(-)

86a5866c7672b88d48380504977496d5637642ab

fs/autofs4/autofs_i.h +11 -2

index 3ccec0a..5d119e3 100644
@@ -79,8 +79,8 @@ struct autofs_wait_queue {
 	u64 ino;
 	uid_t uid;
 	gid_t gid;
-	pid_t pid;
-	pid_t tgid;
+	struct pid *pid;
+	struct pid *tgid;
 	/* This is for status reporting upon return */
 	int status;
 	atomic_t wait_ctr;

@@ -228,5 +228,14 @@ out:
 	return ret;
 }
 
+static void autofs_free_wait_queue(struct autofs_wait_queue *wq)
+{
+	if (wq->pid)
+		put_pid(wq->pid);
+	if (wq->tgid)
+		put_pid(wq->tgid);
+	kfree(wq);
+}
+
 void autofs4_dentry_release(struct dentry *);
 extern void autofs4_kill_sb(struct super_block *);

fs/autofs4/waitq.c +0 -0

index 9857543..4a9ad9b 100644
@@ -141,8 +141,8 @@ static void autofs4_notify_daemon(struct
 		packet->ino = wq->ino;
 		packet->uid = wq->uid;
 		packet->gid = wq->gid;

fs/autofs4/waitq.c static void autofs4_notify_daemon(struct · line 146

Quoted 5 lines

fs/autofs4/waitq.c +2 -2

-		packet->pid = wq->pid;
-		packet->tgid = wq->tgid;
+		packet->pid = pid_nr(wq->pid);
+		packet->tgid = pid_nr(wq->tgid);
 		break;

I'm assuming we build the packet in the process context of the daemon we are sending it to. If not we have a problem here.

Quoted 16 lines

fs/autofs4/waitq.c +2 -2

 	}
 	default:

@@ -292,8 +292,8 @@ int autofs4_wait(struct autofs_sb_info *
 		wq->ino = autofs4_get_ino(sbi);
 		wq->uid = current->uid;
 		wq->gid = current->gid;
-		wq->pid = pid_nr(task_pid(current));
-		wq->tgid = pid_nr(task_tgid(current));
+		wq->pid = get_pid(task_pid(current));
+		wq->tgid = get_pid(task_tgid(current));
 		wq->status = -EINTR; /* Status return if interrupted */
 		atomic_set(&wq->wait_ctr, 2);
 		mutex_unlock(&sbi->wq_mutex);

@@ -360,8 +360,9 @@ int autofs4_wait(struct autofs_sb_info *
 	status = wq->status;
 

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info * · line 365

Quoted 6 lines

fs/autofs4/waitq.c +3 -2

 	/* Are we the last process to need status? */
-	if (atomic_dec_and_test(&wq->wait_ctr))
-		kfree(wq);
+	if (atomic_dec_and_test(&wq->wait_ctr)) {
+		autofs_free_wait_queue(wq);
+	}

Unnecessary braces....

Quoted 10 lines

Quoted 10 lines

void autofs4_dentry_release(struct dentry *); extern void autofs4_kill_sb(struct super_block *);

fs/autofs4/waitq.c +0 -0

index 9857543..4a9ad9b 100644
@@ -141,8 +141,8 @@ static void autofs4_notify_daemon(struct
 		packet->ino = wq->ino;
 		packet->uid = wq->uid;
 		packet->gid = wq->gid;

Quoted 8 lines

fs/autofs4/waitq.c static void autofs4_notify_daemon(struct · line 146

Quoted 5 lines

fs/autofs4/waitq.c +2 -2

-		packet->pid = wq->pid;
-		packet->tgid = wq->tgid;
+		packet->pid = pid_nr(wq->pid);
+		packet->tgid = pid_nr(wq->tgid);
 		break;

I'm assuming we build the packet in the process context of the daemon we are sending it to. If not we have a problem here.

Yes this is data being sent to a userspace daemon (Ian pls correct me if I'm wrong) so the pid_nr is the only thing we can send.

Quoted 19 lines · Serge E. Hallyn

Quoted 10 lines

Quoted 10 lines

void autofs4_dentry_release(struct dentry *); extern void autofs4_kill_sb(struct super_block *);

fs/autofs4/waitq.c +0 -0

index 9857543..4a9ad9b 100644
@@ -141,8 +141,8 @@ static void autofs4_notify_daemon(struct
 		packet->ino = wq->ino;
 		packet->uid = wq->uid;
 		packet->gid = wq->gid;

Quoted 8 lines

fs/autofs4/waitq.c static void autofs4_notify_daemon(struct · line 146

Quoted 5 lines

fs/autofs4/waitq.c +2 -2

-		packet->pid = wq->pid;
-		packet->tgid = wq->tgid;
+		packet->pid = pid_nr(wq->pid);
+		packet->tgid = pid_nr(wq->tgid);
 		break;

I'm assuming we build the packet in the process context of the daemon we are sending it to. If not we have a problem here.

Quoted 2 lines

Yes this is data being sent to a userspace daemon (Ian pls correct me if I'm wrong) so the pid_nr is the only thing we can send.

Agreed. The question is are we in the user space daemon's process when we generate the pid_nr. Or do we stuff this in some kind of socket, and the socket switch locations of the packet.

Basically I'm just trying to be certain we are calling pid_nr in the proper context. Otherwise we could get the wrong pid when we have multiple pid namespaces in play.

Quoted 19 lines · Serge E. Hallyn

Quoted 18 lines

Quoted 7 lines

void autofs4_dentry_release(struct dentry *); extern void autofs4_kill_sb(struct super_block *);

fs/autofs4/waitq.c +0 -0

index 9857543..4a9ad9b 100644

fs/autofs4/waitq.c static void autofs4_notify_daemon(struct · line 146

Quoted 8 lines

fs/autofs4/waitq.c +2 -2

@@ -141,8 +141,8 @@ static void autofs4_notify_daemon(struct
 		packet->ino = wq->ino;
 		packet->uid = wq->uid;
 		packet->gid = wq->gid;
-		packet->pid = wq->pid;
-		packet->tgid = wq->tgid;
+		packet->pid = pid_nr(wq->pid);
+		packet->tgid = pid_nr(wq->tgid);
 		break;

I'm assuming we build the packet in the process context of the daemon we are sending it to. If not we have a problem here.

Quoted 2 lines

Yes this is data being sent to a userspace daemon (Ian pls correct me if I'm wrong) so the pid_nr is the only thing we can send.

Agreed. The question is are we in the user space daemon's process when we generate the pid_nr. Or do we stuff this in some kind of socket, and the socket switch locations of the packet.

Quoted 21 lines · Eric W. Biederman

Quoted 19 lines · Serge E. Hallyn

Quoted 18 lines

Quoted 7 lines

void autofs4_dentry_release(struct dentry *); extern void autofs4_kill_sb(struct super_block *);

fs/autofs4/waitq.c +0 -0

index 9857543..4a9ad9b 100644

fs/autofs4/waitq.c static void autofs4_notify_daemon(struct · line 146

Quoted 8 lines

fs/autofs4/waitq.c +2 -2

@@ -141,8 +141,8 @@ static void autofs4_notify_daemon(struct
 		packet->ino = wq->ino;
 		packet->uid = wq->uid;
 		packet->gid = wq->gid;
-		packet->pid = wq->pid;
-		packet->tgid = wq->tgid;
+		packet->pid = pid_nr(wq->pid);
+		packet->tgid = pid_nr(wq->tgid);
 		break;

I'm assuming we build the packet in the process context of the daemon we are sending it to. If not we have a problem here.

Quoted 6 lines

Quoted 2 lines

Yes this is data being sent to a userspace daemon (Ian pls correct me if I'm wrong) so the pid_nr is the only thing we can send.

Agreed. The question is are we in the user space daemon's process when we generate the pid_nr. Or do we stuff this in some kind of socket, and the socket switch locations of the packet.

The context here is the automount daemon only for expire runs.

Mount request packets are triggered by user processes walking over an autofs mount point directory. So "current" in this case isn't the autofs daemon.

Requests are sent via a pipe to the daemon.

Quoted 28 lines · Eric W. Biederman

Quoted 19 lines · Serge E. Hallyn

Quoted 10 lines

Quoted 10 lines

void autofs4_dentry_release(struct dentry *); extern void autofs4_kill_sb(struct super_block *);

fs/autofs4/waitq.c +0 -0

index 9857543..4a9ad9b 100644
@@ -141,8 +141,8 @@ static void autofs4_notify_daemon(struct
 		packet->ino = wq->ino;
 		packet->uid = wq->uid;
 		packet->gid = wq->gid;

Quoted 8 lines

fs/autofs4/waitq.c static void autofs4_notify_daemon(struct · line 146

Quoted 5 lines

fs/autofs4/waitq.c +2 -2

-		packet->pid = wq->pid;
-		packet->tgid = wq->tgid;
+		packet->pid = pid_nr(wq->pid);
+		packet->tgid = pid_nr(wq->tgid);
 		break;

I'm assuming we build the packet in the process context of the daemon we are sending it to. If not we have a problem here.

Quoted 2 lines

Yes this is data being sent to a userspace daemon (Ian pls correct me if I'm wrong) so the pid_nr is the only thing we can send.

Agreed. The question is are we in the user space daemon's process when we generate the pid_nr. Or do we stuff this in some kind of socket, and the socket switch locations of the packet.

Quoted 3 lines

Basically I'm just trying to be certain we are calling pid_nr in the proper context. Otherwise we could get the wrong pid when we have multiple pid namespaces in play.

We need to know what the userspace daemon being written to is doing with autofs_ptype_{missing,expire}_{in,}direct() messages.

If I understand correctly, the pid being sent is of a process which tried to automount some directory. The message is being sent to the autofs daemon, which should be running in the root pid namespace.

So as it is, the pid_nr(wq->pid) should be done under the init pid_namespace, since it's a kthread. So as long as the userspace automount daemon is started in the root pid namespace, the pid it gets will be the right one.

Ian, does what I'm saying make sense, or am I wrong about how things work for autofs?

thanks,
-serge

PS
Note that if I'm right, but some machine starts autofs in a child pid_namespace, the pid_nr() the way I have it is wrong. I'm not sure in that case how we go about fixing that. Somehow we need to store the autofs userspace daemon's pid namespace pointer to help us find the proper pid_nr.

Quoted 29 lines · Ian Kent

Quoted 21 lines · Eric W. Biederman

Quoted 19 lines · Serge E. Hallyn

Quoted 18 lines

Quoted 7 lines

void autofs4_dentry_release(struct dentry *); extern void autofs4_kill_sb(struct super_block *);

fs/autofs4/waitq.c +0 -0

index 9857543..4a9ad9b 100644

fs/autofs4/waitq.c static void autofs4_notify_daemon(struct · line 146

Quoted 8 lines

fs/autofs4/waitq.c +2 -2

@@ -141,8 +141,8 @@ static void autofs4_notify_daemon(struct
 		packet->ino = wq->ino;
 		packet->uid = wq->uid;
 		packet->gid = wq->gid;
-		packet->pid = wq->pid;
-		packet->tgid = wq->tgid;
+		packet->pid = pid_nr(wq->pid);
+		packet->tgid = pid_nr(wq->tgid);
 		break;

I'm assuming we build the packet in the process context of the daemon we are sending it to. If not we have a problem here.

Quoted 6 lines

Quoted 2 lines

Yes this is data being sent to a userspace daemon (Ian pls correct me if I'm wrong) so the pid_nr is the only thing we can send.

Agreed. The question is are we in the user space daemon's process when we generate the pid_nr. Or do we stuff this in some kind of socket, and the socket switch locations of the packet.

Quoted 7 lines

The context here is the automount daemon only for expire runs.

Mount request packets are triggered by user processes walking over an autofs mount point directory. So "current" in this case isn't the autofs daemon.

Requests are sent via a pipe to the daemon.

So is the pid used for anything other than debugging?

In any case, here is a replacement patch which sends the pid number in the pid_namespace of the process which did the autofs4 mount.

Still not sure whether that is actually what makes sense...

From: "Serge E. Hallyn" <[email protected]>
Subject: [PATCH] autofs: prevent pid wraparound in waitqs

Instead of storing pid numbers for waitqs, store references to struct pids. Also store a reference to the mounter's pid namespace in the autofs4 sb info so that pid numbers for mount miss and expiry msgs can send the pid# in the mounter's pidns.

Signed-off-by Serge E. Hallyn <[email protected]>

---

 fs/autofs4/autofs_i.h |   14 ++++++++++++--
 fs/autofs4/inode.c    |    5 +++++
 fs/autofs4/waitq.c    |   12 ++++++------
 include/linux/pid.h   |    1 +
 kernel/pid.c          |   13 ++++++++++---
 5 files changed, 34 insertions(+), 11 deletions(-)

e4184e6923f811f8a025b831ea33541fa820fd62

fs/autofs4/autofs_i.h +12 -2

index 3ccec0a..55026dd 100644
@@ -79,8 +79,8 @@ struct autofs_wait_queue {
 	u64 ino;
 	uid_t uid;
 	gid_t gid;
-	pid_t pid;
-	pid_t tgid;
+	struct pid *pid;
+	struct pid *tgid;
 	/* This is for status reporting upon return */
 	int status;
 	atomic_t wait_ctr;

@@ -97,6 +97,7 @@ struct autofs_sb_info {
 	int pipefd;
 	struct file *pipe;
 	struct pid *oz_pgrp;
+	struct pid_namespace *pidns;
 	int catatonic;
 	int version;
 	int sub_version;

@@ -228,5 +229,14 @@ out:
 	return ret;
 }
 
+static inline void autofs_free_wait_queue(struct autofs_wait_queue *wq)
+{
+	if (wq->pid)
+		put_pid(wq->pid);
+	if (wq->tgid)
+		put_pid(wq->tgid);
+	kfree(wq);
+}
+
 void autofs4_dentry_release(struct dentry *);
 extern void autofs4_kill_sb(struct super_block *);

fs/autofs4/inode.c +5 -0

index c34131a..294efd8 100644
@@ -20,6 +20,7 @@
 #include <linux/bitops.h>
 #include <linux/smp_lock.h>
 #include <linux/magic.h>
+#include <linux/pid_namespace.h>
 #include "autofs_i.h"
 #include <linux/module.h>
 

@@ -164,6 +165,7 @@ void autofs4_kill_sb(struct super_block 
 		autofs4_catatonic_mode(sbi); /* Free wait queues, close pipe */
 
 	put_pid(sbi->oz_pgrp);
+	put_pid_ns(sbi->pidns);
 
 	/* Clean up and release dangling references */
 	autofs4_force_release(sbi);

@@ -334,6 +336,8 @@ int autofs4_fill_super(struct super_bloc
 	sbi->type = 0;
 	sbi->min_proto = 0;
 	sbi->max_proto = 0;
+	sbi->pidns = task_pid_ns(current);
+	get_pid_ns(sbi->pidns);
 	mutex_init(&sbi->wq_mutex);
 	spin_lock_init(&sbi->fs_lock);
 	sbi->queues = NULL;

@@ -435,6 +439,7 @@ fail_iput:
 fail_ino:
 	kfree(ino);
 fail_free:
+	put_pid_ns(sbi->pidns);
 	kfree(sbi);
 	s->s_fs_info = NULL;
 fail_unlock:

fs/autofs4/waitq.c +6 -6

index 9857543..30e2a90 100644
@@ -141,8 +141,8 @@ static void autofs4_notify_daemon(struct
 		packet->ino = wq->ino;
 		packet->uid = wq->uid;
 		packet->gid = wq->gid;
-		packet->pid = wq->pid;
-		packet->tgid = wq->tgid;
+		packet->pid = __pid_nr(sbi->pidns, wq->pid);
+		packet->tgid = __pid_nr(sbi->pidns, wq->tgid);
 		break;
 	}
 	default:

@@ -292,8 +292,8 @@ int autofs4_wait(struct autofs_sb_info *
 		wq->ino = autofs4_get_ino(sbi);
 		wq->uid = current->uid;
 		wq->gid = current->gid;
-		wq->pid = pid_nr(task_pid(current));
-		wq->tgid = pid_nr(task_tgid(current));
+		wq->pid = get_pid(task_pid(current));
+		wq->tgid = get_pid(task_tgid(current));
 		wq->status = -EINTR; /* Status return if interrupted */
 		atomic_set(&wq->wait_ctr, 2);
 		mutex_unlock(&sbi->wq_mutex);

@@ -361,7 +361,7 @@ int autofs4_wait(struct autofs_sb_info *
 
 	/* Are we the last process to need status? */
 	if (atomic_dec_and_test(&wq->wait_ctr))
-		kfree(wq);
+		autofs_free_wait_queue(wq);
 
 	return status;
 }

@@ -390,7 +390,7 @@ int autofs4_wait_release(struct autofs_s
 	wq->status = status;
 
 	if (atomic_dec_and_test(&wq->wait_ctr))	/* Is anyone still waiting for this guy? */
-		kfree(wq);
+		autofs_free_wait_queue(wq);
 	else
 		wake_up_interruptible(&wq->queue);
 

include/linux/pid.h +1 -0

index d399679..4f043bf 100644
@@ -120,6 +120,7 @@ extern void free_pid_nr(struct pid_nr *p
 extern struct pid_nr *alloc_pid_nr(struct pid_namespace *pid_ns);
 extern struct pid *alloc_pid(int clone_flags);
 extern void FASTCALL(free_pid(struct pid *pid));
+extern pid_t __pid_nr(struct pid_namespace *ns, struct pid *pid);
 extern pid_t pid_nr(struct pid *pid);
 
 

kernel/pid.c +10 -3

index b040e19..aa61b7e 100644
@@ -299,23 +299,30 @@ int attach_pid_nr(struct pid *pid, struc
 	return 0;
 }
 
-pid_t pid_nr(struct pid *pid)
+pid_t __pid_nr(struct pid_namespace *ns, struct pid *pid)
 {
 	struct pid_nr* pid_nr;
 	struct hlist_node *pos;
 
 	if (!pid)
 		return 0;
-
 	rcu_read_lock();
 	hlist_for_each_entry_rcu(pid_nr, pos, &pid->pid_nrs, node)
-		if (pid_nr->pid_ns == task_pid_ns(current)) {
+		if (pid_nr->pid_ns == ns) {
 			rcu_read_unlock();
 			return pid_nr->nr;
 		}
 
 	rcu_read_unlock();
 	return 0;
+
+}
+
+EXPORT_SYMBOL_GPL(__pid_nr);
+
+pid_t pid_nr(struct pid *pid)
+{
+	return __pid_nr(task_pid_ns(current), pid);
 }
 
 EXPORT_SYMBOL_GPL(pid_nr);

So is the pid used for anything other than debugging?

In any case, here is a replacement patch which sends the pid number in the pid_namespace of the process which did the autofs4 mount.

Still not sure whether that is actually what makes sense...

From: "Serge E. Hallyn" <[email protected]>
Subject: [PATCH] autofs: prevent pid wraparound in waitqs

Instead of storing pid numbers for waitqs, store references to struct pids. Also store a reference to the mounter's pid namespace in the autofs4 sb info so that pid numbers for mount miss and expiry msgs can send the pid# in the mounter's pidns.

Quoted 8 lines · Serge E. Hallyn

So is the pid used for anything other than debugging?

In any case, here is a replacement patch which sends the pid number in the pid_namespace of the process which did the autofs4 mount.

Still not sure whether that is actually what makes sense...

Quoted 8 lines

From: "Serge E. Hallyn" <[email protected]>
Subject: [PATCH] autofs: prevent pid wraparound in waitqs

Instead of storing pid numbers for waitqs, store references to struct pids. Also store a reference to the mounter's pid namespace in the autofs4 sb info so that pid numbers for mount miss and expiry msgs can send the pid# in the mounter's pidns.

Hmm. Not quite what I would have expected but given that we are sending data over a pipe that sounds reasonable.

If it wasn't a pipe we would really want to do this in the context of the process receiving the message, but since a pipe can receive a message, and then be passed to another process we clearly can't know the pid namespace of the process receiving the message.

Therefore just caching the pid namespace either on pipe open or on mount makes sense. pipe open might be better.

Serge we really need to introduce __pid_nr in a separate patch.

And we really seem to be confusing Ian.

Plus we have some pid namespace ref counting issues we need to handle carefully.

Let's stop working on autofs4 for a bit, fix the pid namespace infrastructure so there is enough of it to handle autofs4 and then come back.

Either that or take autofs4 in two passes. Pass one we do what we can with the current infrastructure. Pass two after we fix up the infrastructure including introducing __pid_nr we come back and update autofs4 to handle multiple pid namespaces properly.

Quoted 45 lines · Serge E. Hallyn

Quoted 29 lines · Ian Kent

Quoted 21 lines · Eric W. Biederman

Quoted 19 lines · Serge E. Hallyn

Quoted 18 lines

Quoted 7 lines

void autofs4_dentry_release(struct dentry *); extern void autofs4_kill_sb(struct super_block *);

fs/autofs4/waitq.c +0 -0

index 9857543..4a9ad9b 100644

fs/autofs4/waitq.c static void autofs4_notify_daemon(struct · line 146

Quoted 8 lines

fs/autofs4/waitq.c +2 -2

@@ -141,8 +141,8 @@ static void autofs4_notify_daemon(struct
 		packet->ino = wq->ino;
 		packet->uid = wq->uid;
 		packet->gid = wq->gid;
-		packet->pid = wq->pid;
-		packet->tgid = wq->tgid;
+		packet->pid = pid_nr(wq->pid);
+		packet->tgid = pid_nr(wq->tgid);
 		break;

I'm assuming we build the packet in the process context of the daemon we are sending it to. If not we have a problem here.

Quoted 6 lines

Quoted 2 lines

Yes this is data being sent to a userspace daemon (Ian pls correct me if I'm wrong) so the pid_nr is the only thing we can send.

Agreed. The question is are we in the user space daemon's process when we generate the pid_nr. Or do we stuff this in some kind of socket, and the socket switch locations of the packet.

Quoted 7 lines

The context here is the automount daemon only for expire runs.

Mount request packets are triggered by user processes walking over an autofs mount point directory. So "current" in this case isn't the autofs daemon.

Requests are sent via a pipe to the daemon.

So is the pid used for anything other than debugging?

In any case, here is a replacement patch which sends the pid number in the pid_namespace of the process which did the autofs4 mount.

Still not sure whether that is actually what makes sense...

Quoted 8 lines

From: "Serge E. Hallyn" <[email protected]>
Subject: [PATCH] autofs: prevent pid wraparound in waitqs

Instead of storing pid numbers for waitqs, store references to struct pids. Also store a reference to the mounter's pid namespace in the autofs4 sb info so that pid numbers for mount miss and expiry msgs can send the pid# in the mounter's pidns.

I think this amounts to what I suggested in my previous replies. Hopefully my comments are enough to clear up any questions on correctness of this approach.

Sorry to be a pain but I'm having a little trouble reviewing the patch because I'm not clear on where the code to handle the automount process group (so called oz_pgrp), from the first patch, fits in with this.

Is this patch in addition to the original?

If so are the references to pid_nr still OK?

Quoted 34 lines · Serge E. Hallyn

Quoted 32 lines · Eric W. Biederman

Quoted 19 lines · Serge E. Hallyn

Quoted 10 lines

Quoted 10 lines

void autofs4_dentry_release(struct dentry *); extern void autofs4_kill_sb(struct super_block *);

fs/autofs4/waitq.c +0 -0

index 9857543..4a9ad9b 100644
@@ -141,8 +141,8 @@ static void autofs4_notify_daemon(struct
 		packet->ino = wq->ino;
 		packet->uid = wq->uid;
 		packet->gid = wq->gid;

Quoted 8 lines

fs/autofs4/waitq.c static void autofs4_notify_daemon(struct · line 146

Quoted 5 lines

fs/autofs4/waitq.c +2 -2

-		packet->pid = wq->pid;
-		packet->tgid = wq->tgid;
+		packet->pid = pid_nr(wq->pid);
+		packet->tgid = pid_nr(wq->tgid);
 		break;

I'm assuming we build the packet in the process context of the daemon we are sending it to. If not we have a problem here.

Quoted 3 lines

Yes this is data being sent to a userspace daemon (Ian pls correct me if I'm wrong) so the pid_nr is the only thing we can send.

Agreed. The question is are we in the user space daemon's process when we generate the pid_nr. Or do we stuff this in some kind of socket, and the socket switch locations of the packet.

Basically I'm just trying to be certain we are calling pid_nr in the proper context. Otherwise we could get the wrong pid when we have multiple pid namespaces in play.

Quoted 2 lines

We need to know what the userspace daemon being written to is doing with autofs_ptype_{missing,expire}_{in,}direct() messages.

At the moment autofs only uses the packet->pid for logging purposes. This solves an age old problem of not knowing who is causing mount requests.

I'm not aware of any other applications that use version 5 yet but that of course could change. So we can't really know what will be done with these ids at some point in the future.

Quoted 3 lines

If I understand correctly, the pid being sent is of a process which tried to automount some directory. The message is being sent to the autofs daemon, which should be running in the root pid namespace.

Yes, but it could be the autofs daemon itself in the expire case.

Usually it doesn't make sense to run an automounting application as other than "root" but I'm not familiar with other possible userspace applications. Perhaps User Mode Linux could be an issue?

Quoted 6 lines

So as it is, the pid_nr(wq->pid) should be done under the init pid_namespace, since it's a kthread. So as long as the userspace automount daemon is started in the root pid namespace, the pid it gets will be the right one.

Quoted 2 lines

Ian, does what I'm saying make sense, or am I wrong about how things work for autofs?

Yep. That's the way it is.

Quoted 9 lines

thanks,
-serge

PS
Note that if I'm right, but some machine starts autofs in a child pid_namespace, the pid_nr() the way I have it is wrong. I'm not sure in that case how we go about fixing that. Somehow we need to store the autofs userspace daemon's pid namespace pointer to help us find the proper pid_nr.

In order for any user space application to use the module it must mount the autofs file system, passing a file handle for the pipe to use for communication. This must always be done. Can we grab the process pid namespace at that time and store it in the superblock info structure?

How does this affect getting ids for waitq request packets of other user space processes triggering mounts? I'm guessing that they would need to belong to the appropriate namespace for this mechanism to funtion correctly.

Ian

Quoted 55 lines · Ian Kent

Quoted 45 lines · Serge E. Hallyn

Quoted 29 lines · Ian Kent

Quoted 21 lines · Eric W. Biederman

Quoted 19 lines · Serge E. Hallyn

Quoted 18 lines

Quoted 7 lines

void autofs4_dentry_release(struct dentry *); extern void autofs4_kill_sb(struct super_block *);

fs/autofs4/waitq.c +0 -0

index 9857543..4a9ad9b 100644

fs/autofs4/waitq.c static void autofs4_notify_daemon(struct · line 146

Quoted 8 lines

fs/autofs4/waitq.c +2 -2

@@ -141,8 +141,8 @@ static void autofs4_notify_daemon(struct
 		packet->ino = wq->ino;
 		packet->uid = wq->uid;
 		packet->gid = wq->gid;
-		packet->pid = wq->pid;
-		packet->tgid = wq->tgid;
+		packet->pid = pid_nr(wq->pid);
+		packet->tgid = pid_nr(wq->tgid);
 		break;

I'm assuming we build the packet in the process context of the daemon we are sending it to. If not we have a problem here.

Quoted 6 lines

Quoted 2 lines

Yes this is data being sent to a userspace daemon (Ian pls correct me if I'm wrong) so the pid_nr is the only thing we can send.

Agreed. The question is are we in the user space daemon's process when we generate the pid_nr. Or do we stuff this in some kind of socket, and the socket switch locations of the packet.

Quoted 7 lines

The context here is the automount daemon only for expire runs.

Mount request packets are triggered by user processes walking over an autofs mount point directory. So "current" in this case isn't the autofs daemon.

Requests are sent via a pipe to the daemon.

So is the pid used for anything other than debugging?

In any case, here is a replacement patch which sends the pid number in the pid_namespace of the process which did the autofs4 mount.

Still not sure whether that is actually what makes sense...

Quoted 8 lines

From: "Serge E. Hallyn" <[email protected]>
Subject: [PATCH] autofs: prevent pid wraparound in waitqs

Instead of storing pid numbers for waitqs, store references to struct pids. Also store a reference to the mounter's pid namespace in the autofs4 sb info so that pid numbers for mount miss and expiry msgs can send the pid# in the mounter's pidns.

Quoted 7 lines

I think this amounts to what I suggested in my previous replies. Hopefully my comments are enough to clear up any questions on correctness of this approach.

Sorry to be a pain but I'm having a little trouble reviewing the patch because I'm not clear on where the code to handle the automount process group (so called oz_pgrp), from the first patch, fits in with this.

It also has pidspace infrastructure code in it, so I think we will just hold off on this until we have that infrastructure merged into the pidspace code and into -mm. Then we can send you a single, more concise patch.

Quoted 1 line

Is this patch in addition to the original?

Yes.

Quoted 1 line

If so are the references to pid_nr still OK?

I think so, because AIUI the rest are all executed in a context where current is both the actor and recipient.

Thanks for your help.

Quoted 38 lines · Ian Kent

Quoted 36 lines · Serge E. Hallyn

Quoted 25 lines · Eric W. Biederman

Quoted 19 lines · Serge E. Hallyn

Quoted 10 lines

Quoted 10 lines

void autofs4_dentry_release(struct dentry *); extern void autofs4_kill_sb(struct super_block *);

fs/autofs4/waitq.c +0 -0

index 9857543..4a9ad9b 100644
@@ -141,8 +141,8 @@ static void autofs4_notify_daemon(struct
 		packet->ino = wq->ino;
 		packet->uid = wq->uid;
 		packet->gid = wq->gid;

Quoted 8 lines

fs/autofs4/waitq.c static void autofs4_notify_daemon(struct · line 146

Quoted 5 lines

fs/autofs4/waitq.c +2 -2

-		packet->pid = wq->pid;
-		packet->tgid = wq->tgid;
+		packet->pid = pid_nr(wq->pid);
+		packet->tgid = pid_nr(wq->tgid);
 		break;

I'm assuming we build the packet in the process context of the daemon we are sending it to. If not we have a problem here.

Quoted 3 lines

Yes this is data being sent to a userspace daemon (Ian pls correct me if I'm wrong) so the pid_nr is the only thing we can send.

Quoted 7 lines

Agreed. The question is are we in the user space daemon's process when we generate the pid_nr. Or do we stuff this in some kind of socket, and the socket switch locations of the packet.

Basically I'm just trying to be certain we are calling pid_nr in the proper context. Otherwise we could get the wrong pid when we have multiple pid namespaces in play.

We need to know what the userspace daemon being written to is doing with autofs_ptype_{missing,expire}_{in,}direct() messages.

Quoted 3 lines

At the moment autofs only uses the packet->pid for logging purposes. This solves an age old problem of not knowing who is causing mount requests.

probably I'm wrong, but that sounds like the packet->pid is supposed to be the pid of the process _causing_ the mount, not the user space daemon communicating with the kernel ...

Quoted 21 lines

I'm not aware of any other applications that use version 5 yet but that of course could change. So we can't really know what will be done with these ids at some point in the future.

Quoted 3 lines

If I understand correctly, the pid being sent is of a process which tried to automount some directory. The message is being sent to the autofs daemon, which should be running in the root pid namespace.

Yes, but it could be the autofs daemon itself in the expire case.

Usually it doesn't make sense to run an automounting application as other than "root" but I'm not familiar with other possible userspace applications. Perhaps User Mode Linux could be an issue?

Quoted 6 lines

So as it is, the pid_nr(wq->pid) should be done under the init pid_namespace, since it's a kthread. So as long as the userspace automount daemon is started in the root pid namespace, the pid it gets will be the right one.

Quoted 4 lines

Quoted 2 lines

Ian, does what I'm saying make sense, or am I wrong about how things work for autofs?

Yep. That's the way it is.

assumed we allow auto mounter mounts inside a context (I see no immediate reason not to do that) we want to know the name/pid space the userspace daemon is running in as well as the name/pid space of the trigger task

Quoted 10 lines

Quoted 9 lines

thanks,
-serge

PS
Note that if I'm right, but some machine starts autofs in a child pid_namespace, the pid_nr() the way I have it is wrong. I'm not sure in that case how we go about fixing that. Somehow we need to store the autofs userspace daemon's pid namespace pointer to help us find the proper pid_nr.

Quoted 4 lines

In order for any user space application to use the module it must mount the autofs file system, passing a file handle for the pipe to use for communication. This must always be done. Can we grab the process pid namespace at that time and store it in the superblock info structure?

probably yes, but if my assumption above is correct, that isn't necessarily the pid/space for the trigger process (although it makes sense that it _should_ be)

Quoted 14 lines

Quoted 9 lines

From: "Serge E. Hallyn" <[email protected]>
Subject: [PATCH] autofs: prevent pid wraparound in waitqs

Instead of storing pid numbers for waitqs, store references to struct pids. Also store a reference to the mounter's pid namespace in the autofs4 sb info so that pid numbers for mount miss and expiry msgs can send the pid# in the mounter's pidns.

I think this amounts to what I suggested in my previous replies. Hopefully my comments are enough to clear up any questions on correctness of this approach.

Quoted 3 lines

Sorry to be a pain but I'm having a little trouble reviewing the patch because I'm not clear on where the code to handle the automount process group (so called oz_pgrp), from the first patch, fits in with this.

It also has pidspace infrastructure code in it, so I think we will just hold off on this until we have that infrastructure merged into the pidspace code and into -mm. Then we can send you a single, more concise patch.

Quoted 6 lines · Serge E. Hallyn

PS
Note that if I'm right, but some machine starts autofs in a child pid_namespace, the pid_nr() the way I have it is wrong. I'm not sure in that case how we go about fixing that. Somehow we need to store the autofs userspace daemon's pid namespace pointer to help us find the proper pid_nr.

In order for any user space application to use the module it must mount the autofs file system, passing a file handle for the pipe to use for communication. This must always be done. Can we grab the process pid namespace at that time and store it in the superblock info structure?

How does this affect getting ids for waitq request packets of other user space processes triggering mounts? I'm guessing that they would need to belong to the appropriate namespace for this mechanism to funtion correctly.

btw please don't be confused by my use of "user space application". In my comments I'm talking about applications that are similar in function to autofs itself and not processes that might trigger mounts. For example the "autodir" package uses the autofs filesystem to provide dynamic home directory creation. The point being that we can't know what an application may do with the info in a request packet so we need to be sure we cover all the cases.

How does this affect getting ids for waitq request packets of other user space processes triggering mounts? I'm guessing that they would need to belong to the appropriate namespace for this mechanism to funtion correctly.