From: Sukadev Bhattiprolu <sukadev@us.ibm.com>
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 <sukadev@us.ibm.com>
Cc: Cedric Le Goater <clg@fr.ibm.com>
Cc: Dave Hansen <haveblue@us.ibm.com>
Cc: Serge Hallyn <serue@us.ibm.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: containers@lists.osdl.org
Acked-by: Eric W. Biederman <ebiederm@xmission.com>

---
 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
===================================================================
--- lx26-21-rc3-mm2.orig/fs/autofs/autofs_i.h	2007-03-12 17:12:05.000000000 -0700
+++ lx26-21-rc3-mm2/fs/autofs/autofs_i.h	2007-03-12 17:18:55.000000000 -0700
@@ -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
===================================================================
--- lx26-21-rc3-mm2.orig/fs/autofs/inode.c	2007-03-12 17:18:48.000000000 -0700
+++ lx26-21-rc3-mm2/fs/autofs/inode.c	2007-03-12 17:18:55.000000000 -0700
@@ -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
===================================================================
--- lx26-21-rc3-mm2.orig/fs/autofs/root.c	2007-03-12 17:18:48.000000000 -0700
+++ lx26-21-rc3-mm2/fs/autofs/root.c	2007-03-12 17:18:55.000000000 -0700
@@ -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
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Ian Kent <raven@themaw.net> writes:

> On Mon, 12 Mar 2007, sukadev@us.ibm.com wrote:
>
>> 
>> From: Sukadev Bhattiprolu <sukadev@us.ibm.com>
>> 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.

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.

Eric
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

On Mon, 12 Mar 2007, sukadev@us.ibm.com wrote:

> 
> From: Sukadev Bhattiprolu <sukadev@us.ibm.com>
> 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.

> 	- Fix Cedric's comments: retain old prototype of parse_options()
> 	  and move necessary change to its caller.
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev@us.ibm.com>
> Cc: Cedric Le Goater <clg@fr.ibm.com>
> Cc: Dave Hansen <haveblue@us.ibm.com>
> Cc: Serge Hallyn <serue@us.ibm.com>
> Cc: Eric Biederman <ebiederm@xmission.com>
> Cc: containers@lists.osdl.org
> Acked-by: Eric W. Biederman <ebiederm@xmission.com>
> 
> ---
>  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
> ===================================================================
> --- lx26-21-rc3-mm2.orig/fs/autofs/autofs_i.h	2007-03-12 17:12:05.000000000 -0700
> +++ lx26-21-rc3-mm2/fs/autofs/autofs_i.h	2007-03-12 17:18:55.000000000 -0700
> @@ -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
> ===================================================================
> --- lx26-21-rc3-mm2.orig/fs/autofs/inode.c	2007-03-12 17:18:48.000000000 -0700
> +++ lx26-21-rc3-mm2/fs/autofs/inode.c	2007-03-12 17:18:55.000000000 -0700
> @@ -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
> ===================================================================
> --- lx26-21-rc3-mm2.orig/fs/autofs/root.c	2007-03-12 17:18:48.000000000 -0700
> +++ lx26-21-rc3-mm2/fs/autofs/root.c	2007-03-12 17:18:55.000000000 -0700
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Ian Kent <raven@themaw.net> writes:

> On Fri, 2007-03-16 at 15:44 +0100, Cedric Le Goater wrote:
>> > 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.
>
> 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.

>> Index: 2.6.20/fs/autofs4/waitq.c
>> ===================================================================
>> --- 2.6.20.orig/fs/autofs4/waitq.c
>> +++ 2.6.20/fs/autofs4/waitq.c
>> @@ -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.

Eric

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

> 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 <sukadev@us.ibm.com>

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 <sukadev@us.ibm.com>
Cc: Cedric Le Goater <clg@fr.ibm.com>
Cc: Dave Hansen <haveblue@us.ibm.com>
Cc: Serge Hallyn <serue@us.ibm.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: containers@lists.osdl.org

---
 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
===================================================================
--- 2.6.20.orig/fs/autofs4/autofs_i.h
+++ 2.6.20/fs/autofs4/autofs_i.h
@@ -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
===================================================================
--- 2.6.20.orig/fs/autofs4/inode.c
+++ 2.6.20/fs/autofs4/inode.c
@@ -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
===================================================================
--- 2.6.20.orig/fs/autofs4/root.c
+++ 2.6.20/fs/autofs4/root.c
@@ -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
===================================================================
--- 2.6.20.orig/fs/autofs4/waitq.c
+++ 2.6.20/fs/autofs4/waitq.c
@@ -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);
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

On Fri, 2007-03-16 at 05:32 -0600, Eric W. Biederman wrote:
> Ian Kent <raven@themaw.net> writes:
> 
> > On Mon, 12 Mar 2007, sukadev@us.ibm.com wrote:
> >
> >> 
> >> From: Sukadev Bhattiprolu <sukadev@us.ibm.com>
> >> 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.
> 
> 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.

Ian


_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

On Fri, 2007-03-16 at 15:44 +0100, Cedric Le Goater wrote:
> > 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.

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?

Thanks
Ian

> 
> thanks,
> 
> C.
> 
> From: Sukadev Bhattiprolu <sukadev@us.ibm.com>
> 
> 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 <sukadev@us.ibm.com>
> Cc: Cedric Le Goater <clg@fr.ibm.com>
> Cc: Dave Hansen <haveblue@us.ibm.com>
> Cc: Serge Hallyn <serue@us.ibm.com>
> Cc: Eric Biederman <ebiederm@xmission.com>
> Cc: containers@lists.osdl.org
> 
> ---
>  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
> ===================================================================
> --- 2.6.20.orig/fs/autofs4/autofs_i.h
> +++ 2.6.20/fs/autofs4/autofs_i.h
> @@ -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
> ===================================================================
> --- 2.6.20.orig/fs/autofs4/inode.c
> +++ 2.6.20/fs/autofs4/inode.c
> @@ -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
> ===================================================================
> --- 2.6.20.orig/fs/autofs4/root.c
> +++ 2.6.20/fs/autofs4/root.c
> @@ -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
> ===================================================================
> --- 2.6.20.orig/fs/autofs4/waitq.c
> +++ 2.6.20/fs/autofs4/waitq.c
> @@ -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);

_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

"Serge E. Hallyn" <serue@us.ibm.com> writes:

>> 
>> >> Index: 2.6.20/fs/autofs4/waitq.c
>> >> ===================================================================
>> >> --- 2.6.20.orig/fs/autofs4/waitq.c
>> >> +++ 2.6.20/fs/autofs4/waitq.c
>> >> @@ -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.
>
> 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.

> 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
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoting Eric W. Biederman (ebiederm@xmission.com):
> Ian Kent <raven@themaw.net> writes:
> 
> > On Fri, 2007-03-16 at 15:44 +0100, Cedric Le Goater wrote:
> >> > 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.
> >
> > 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.
> 
> >> Index: 2.6.20/fs/autofs4/waitq.c
> >> ===================================================================
> >> --- 2.6.20.orig/fs/autofs4/waitq.c
> >> +++ 2.6.20/fs/autofs4/waitq.c
> >> @@ -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.

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

-serge
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

"Serge E. Hallyn" <serue@us.ibm.com> writes:

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

Agreed.

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

Doing current_pid() and friends certainly seem to be worth doing.
Mostly for the secondary effect that we can then breaking any code we
miss in our conversion.  I don't know if the caching is worth it but
it is so easy to implement I don't see a reason to avoid it.

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

I will keep it in mind.  As long a we keep patches that actually change the
code separate from patches that change the set of helper functions I think
we will be ok.  Hopefully Suka has learned enough by now that reviewing
his patches will be easier than just writing them myself.  There were
so many silly little issues to clean up in the last round of review
by the time the patch was clean enough to look at the meat I missed the
deep issues :(  At least things were clear enough that Oleg managed to
catch some of the deeper issues.

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoting Eric W. Biederman (ebiederm@xmission.com):
> "Serge E. Hallyn" <serue@us.ibm.com> writes:
> 
> >> 
> >> >> Index: 2.6.20/fs/autofs4/waitq.c
> >> >> ===================================================================
> >> >> --- 2.6.20.orig/fs/autofs4/waitq.c
> >> >> +++ 2.6.20/fs/autofs4/waitq.c
> >> >> @@ -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.
> >
> > 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.
> 
> > 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.

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.

-serge

> 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
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

"Serge E. Hallyn" <serue@us.ibm.com> writes:

> 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 <serue@us.ibm.com>
> 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.

> Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
>
> ---
>
>  fs/autofs4/autofs_i.h |   13 +++++++++++--
>  fs/autofs4/waitq.c    |   15 ++++++++-------
>  2 files changed, 19 insertions(+), 9 deletions(-)
>
> 86a5866c7672b88d48380504977496d5637642ab
> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> index 3ccec0a..5d119e3 100644
> --- a/fs/autofs4/autofs_i.h
> +++ b/fs/autofs4/autofs_i.h
> @@ -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 *);
> diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> index 9857543..4a9ad9b 100644
> --- a/fs/autofs4/waitq.c
> +++ b/fs/autofs4/waitq.c
> @@ -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.

>  	}
>  	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;
>  
>  	/* 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....
  
>  	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? */
> -		kfree(wq);
> +		autofs_free_wait_queue(wq);
>  	else
>  		wake_up_interruptible(&wq->queue);
>  
> -- 
> 1.1.6


Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoting Eric W. Biederman (ebiederm@xmission.com):
> "Serge E. Hallyn" <serue@us.ibm.com> writes:
> 
> >> 
> >> >> Index: 2.6.20/fs/autofs4/waitq.c
> >> >> ===================================================================
> >> >> --- 2.6.20.orig/fs/autofs4/waitq.c
> >> >> +++ 2.6.20/fs/autofs4/waitq.c
> >> >> @@ -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.
> >
> > 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 <serue@us.ibm.com>
Date: 1174412305 -0500

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

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

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

86a5866c7672b88d48380504977496d5637642ab
diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index 3ccec0a..5d119e3 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -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 *);
diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index 9857543..4a9ad9b 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -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;
 	}
 	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;
 
 	/* 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);
+	}
 
 	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? */
-		kfree(wq);
+		autofs_free_wait_queue(wq);
 	else
 		wake_up_interruptible(&wq->queue);
 
-- 
1.1.6
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

"Serge E. Hallyn" <serue@us.ibm.com> writes:

>> >  void autofs4_dentry_release(struct dentry *);
>> >  extern void autofs4_kill_sb(struct super_block *);
>> > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
>> > index 9857543..4a9ad9b 100644
>> > --- a/fs/autofs4/waitq.c
>> > +++ b/fs/autofs4/waitq.c
>> > @@ -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.
>
> 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.

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoting Eric W. Biederman (ebiederm@xmission.com):
> "Serge E. Hallyn" <serue@us.ibm.com> writes:
> 
> > 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 <serue@us.ibm.com>
> > 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.
> 
> > Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
> >
> > ---
> >
> >  fs/autofs4/autofs_i.h |   13 +++++++++++--
> >  fs/autofs4/waitq.c    |   15 ++++++++-------
> >  2 files changed, 19 insertions(+), 9 deletions(-)
> >
> > 86a5866c7672b88d48380504977496d5637642ab
> > diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> > index 3ccec0a..5d119e3 100644
> > --- a/fs/autofs4/autofs_i.h
> > +++ b/fs/autofs4/autofs_i.h
> > @@ -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 *);
> > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> > index 9857543..4a9ad9b 100644
> > --- a/fs/autofs4/waitq.c
> > +++ b/fs/autofs4/waitq.c
> > @@ -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.

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.

> >  	}
> >  	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;
> >  
> >  	/* 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....

Thanks.  Also needed another tweak in the .h file.  Here's a new patch.
Ian, please let me know if this (combined with the previous patch from
Cedric) is acceptable to you.

thanks,
-serge

Subject: [PATCH] autofs4: store struct pids in autofs_waitqs
From: Serge Hallyn <serue@us.ibm.com>
Date: 1174412305 -0500

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

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 fs/autofs4/autofs_i.h |   13 +++++++++++--
 fs/autofs4/waitq.c    |   12 ++++++------
 2 files changed, 17 insertions(+), 8 deletions(-)

03944aae0b9cb4a36d091f275d2f2217447b27ec
diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index 3ccec0a..8260743 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -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 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 *);
diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index 9857543..6c696ea 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -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;
 	}
 	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);
 
-- 
1.1.6
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoting Eric W. Biederman (ebiederm@xmission.com):
> "Serge E. Hallyn" <serue@us.ibm.com> writes:
> 
> >> >  void autofs4_dentry_release(struct dentry *);
> >> >  extern void autofs4_kill_sb(struct super_block *);
> >> > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> >> > index 9857543..4a9ad9b 100644
> >> > --- a/fs/autofs4/waitq.c
> >> > +++ b/fs/autofs4/waitq.c
> >> > @@ -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.
> >
> > 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.

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.
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoting Ian Kent (raven@themaw.net):
> On Tue, 2007-03-20 at 16:01 -0600, Eric W. Biederman wrote:
> > "Serge E. Hallyn" <serue@us.ibm.com> writes:
> > 
> > >> >  void autofs4_dentry_release(struct dentry *);
> > >> >  extern void autofs4_kill_sb(struct super_block *);
> > >> > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> > >> > index 9857543..4a9ad9b 100644
> > >> > --- a/fs/autofs4/waitq.c
> > >> > +++ b/fs/autofs4/waitq.c
> > >> > @@ -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.
> > >
> > > 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.

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" <serue@us.ibm.com>
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 <serue@us.ibm.com>

---

 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
diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index 3ccec0a..55026dd 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -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 *);
diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index c34131a..294efd8 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -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:
diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index 9857543..30e2a90 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -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);
 
diff --git a/include/linux/pid.h b/include/linux/pid.h
index d399679..4f043bf 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -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);
 
 
diff --git a/kernel/pid.c b/kernel/pid.c
index b040e19..aa61b7e 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -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);
-- 
1.1.6
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

"Serge E. Hallyn" <serue@us.ibm.com> writes:
>
> 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" <serue@us.ibm.com>
> 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.

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoting Eric W. Biederman (ebiederm@xmission.com):
> "Serge E. Hallyn" <serue@us.ibm.com> writes:
> >
> > 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" <serue@us.ibm.com>
> > 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.

Right, but the pipe is always opened on mount I think.  (at
autofs4_fill_super)

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

Agreed.

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

Agreed.  I just wasn't comfortable stopping until I felt we knew how
autofs4 was going to be addressed.  I think we know now, plus we've
verified another definite need for the __pid_nr(pidns, pid) helper.

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

Nah, let's hold off, and I'll sit on a patch to send out once rest of
the infrastructure goes in.

-serge
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoting Ian Kent (raven@themaw.net):
> On Wed, 2007-03-21 at 21:19 -0500, Serge E. Hallyn wrote:
> > Quoting Ian Kent (raven@themaw.net):
> > > On Tue, 2007-03-20 at 16:01 -0600, Eric W. Biederman wrote:
> > > > "Serge E. Hallyn" <serue@us.ibm.com> writes:
> > > > 
> > > > >> >  void autofs4_dentry_release(struct dentry *);
> > > > >> >  extern void autofs4_kill_sb(struct super_block *);
> > > > >> > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> > > > >> > index 9857543..4a9ad9b 100644
> > > > >> > --- a/fs/autofs4/waitq.c
> > > > >> > +++ b/fs/autofs4/waitq.c
> > > > >> > @@ -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.
> > > > >
> > > > > 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.
> > 
> > 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" <serue@us.ibm.com>
> > 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.

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.

> Is this patch in addition to the original?

Yes.

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

thanks,
-serge
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Ian Kent <raven@themaw.net> writes:

> On Wed, 2007-03-21 at 15:58 -0500, Serge E. Hallyn wrote:

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

I think this sounds like what Serge's did with his last patch, and
it sounds like the only reasonable thing we can do in this situation.

I had missed the fact the pipe is passed in at mount time.

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

Not really.  As long as the user space applications that trigger the mount
is in the pid namespace where the mount was performed or in a child of that
pid namespace it will has a process id that you can see.

If it happens that someone is entirely too creative and the process that
triggered the mount is in an entirely different pid namespace that does
not map __pid_nr will return 0.  Not perfect but at least it is well defined.
And a reasonable thing to return for a process you cannot see.

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Ian Kent <raven@themaw.net> writes:

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

A sentiment I share, and the reason why it is taking so long to come
up with a patch for autofs4 that passes review.

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Tue, 2007-03-20 at 16:01 -0600, Eric W. Biederman wrote:
> "Serge E. Hallyn" <serue@us.ibm.com> writes:
> 
> >> >  void autofs4_dentry_release(struct dentry *);
> >> >  extern void autofs4_kill_sb(struct super_block *);
> >> > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> >> > index 9857543..4a9ad9b 100644
> >> > --- a/fs/autofs4/waitq.c
> >> > +++ b/fs/autofs4/waitq.c
> >> > @@ -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.
> >
> > 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.

Ian


_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Wed, 2007-03-21 at 15:58 -0500, Serge E. Hallyn wrote:
> Quoting Eric W. Biederman (ebiederm@xmission.com):
> > "Serge E. Hallyn" <serue@us.ibm.com> writes:
> > 
> > >> >  void autofs4_dentry_release(struct dentry *);
> > >> >  extern void autofs4_kill_sb(struct super_block *);
> > >> > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> > >> > index 9857543..4a9ad9b 100644
> > >> > --- a/fs/autofs4/waitq.c
> > >> > +++ b/fs/autofs4/waitq.c
> > >> > @@ -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.
> > >
> > > 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.
> 
> 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.

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

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

Yep. That's the way it is.

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


_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Wed, 2007-03-21 at 21:19 -0500, Serge E. Hallyn wrote:
> Quoting Ian Kent (raven@themaw.net):
> > On Tue, 2007-03-20 at 16:01 -0600, Eric W. Biederman wrote:
> > > "Serge E. Hallyn" <serue@us.ibm.com> writes:
> > > 
> > > >> >  void autofs4_dentry_release(struct dentry *);
> > > >> >  extern void autofs4_kill_sb(struct super_block *);
> > > >> > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> > > >> > index 9857543..4a9ad9b 100644
> > > >> > --- a/fs/autofs4/waitq.c
> > > >> > +++ b/fs/autofs4/waitq.c
> > > >> > @@ -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.
> > > >
> > > > 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.
> 
> 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" <serue@us.ibm.com>
> 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?

Ian

> 
> Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
> 
> ---
> 
>  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
> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> index 3ccec0a..55026dd 100644
> --- a/fs/autofs4/autofs_i.h
> +++ b/fs/autofs4/autofs_i.h
> @@ -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 *);
> diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
> index c34131a..294efd8 100644
> --- a/fs/autofs4/inode.c
> +++ b/fs/autofs4/inode.c
> @@ -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:
> diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> index 9857543..30e2a90 100644
> --- a/fs/autofs4/waitq.c
> +++ b/fs/autofs4/waitq.c
> @@ -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);
>  
> diff --git a/include/linux/pid.h b/include/linux/pid.h
> index d399679..4f043bf 100644
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -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);
>  
> 
> diff --git a/kernel/pid.c b/kernel/pid.c
> index b040e19..aa61b7e 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -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);

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoting Ian Kent (raven@themaw.net):
> 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.

A feature of the pid namespaces is that any process in a cloned
namespace still has a valid pid in all ancestore pid namespaces.  So
when a process triggers a mount, either it is in the same or a decendent
pid namespace as the process which did the mounting, in which case the
pid sent to the mounter is correct;  or it is in some other namespace,
and it will get '0'.  The latter shouldn't happen in a proper setup, and
should be safe to ignore in an improper setup.

For instance, so long as any clone(CLONE_NEWPID) is always done along
with a CLONE_NEWNS, and process in the child namespace which mounts an
autofs instance, processes in the parent pid namespace won't trigger
automounts.  But if somehow they did (i.e. with shared submounts or by
not doing CLONE_NEWNS), pid 0 will be reported.

thanks,
-serge
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, Mar 22, 2007 at 11:28:43AM +0900, Ian Kent wrote:
> On Wed, 2007-03-21 at 15:58 -0500, Serge E. Hallyn wrote:
> > Quoting Eric W. Biederman (ebiederm@xmission.com):
> > > "Serge E. Hallyn" <serue@us.ibm.com> writes:
> > > 
> > > >> >  void autofs4_dentry_release(struct dentry *);
> > > >> >  extern void autofs4_kill_sb(struct super_block *);
> > > >> > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> > > >> > index 9857543..4a9ad9b 100644
> > > >> > --- a/fs/autofs4/waitq.c
> > > >> > +++ b/fs/autofs4/waitq.c
> > > >> > @@ -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.
> > > >
> > > > 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.
> > 
> > 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.

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

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

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

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)

best,
Herbert

> 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
> 
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, 2007-03-22 at 08:31 -0500, Serge E. Hallyn wrote:
> Quoting Ian Kent (raven@themaw.net):
> > On Wed, 2007-03-21 at 21:19 -0500, Serge E. Hallyn wrote:
> > > Quoting Ian Kent (raven@themaw.net):
> > > > On Tue, 2007-03-20 at 16:01 -0600, Eric W. Biederman wrote:
> > > > > "Serge E. Hallyn" <serue@us.ibm.com> writes:
> > > > > 
> > > > > >> >  void autofs4_dentry_release(struct dentry *);
> > > > > >> >  extern void autofs4_kill_sb(struct super_block *);
> > > > > >> > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> > > > > >> > index 9857543..4a9ad9b 100644
> > > > > >> > --- a/fs/autofs4/waitq.c
> > > > > >> > +++ b/fs/autofs4/waitq.c
> > > > > >> > @@ -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.
> > > > > >
> > > > > > 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.
> > > 
> > > 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" <serue@us.ibm.com>
> > > 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.
> 
> 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.
> 
> > Is this patch in addition to the original?
> 
> Yes.
> 
> > 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.

Not sure about that but lets not worry about it until we have a full
patch to look at.

> Thanks for your help.

My pleasure.

Ian


_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, 2007-03-22 at 15:33 +0100, Herbert Poetzl wrote:
> On Thu, Mar 22, 2007 at 11:28:43AM +0900, Ian Kent wrote:
> > On Wed, 2007-03-21 at 15:58 -0500, Serge E. Hallyn wrote:
> > > Quoting Eric W. Biederman (ebiederm@xmission.com):
> > > > "Serge E. Hallyn" <serue@us.ibm.com> writes:
> > > > 
> > > > >> >  void autofs4_dentry_release(struct dentry *);
> > > > >> >  extern void autofs4_kill_sb(struct super_block *);
> > > > >> > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> > > > >> > index 9857543..4a9ad9b 100644
> > > > >> > --- a/fs/autofs4/waitq.c
> > > > >> > +++ b/fs/autofs4/waitq.c
> > > > >> > @@ -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.
> > > > >
> > > > > 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.
> > > 
> > > 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.
> 
> 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 ...

That's right. The only place it's used atm is to log the pid of
processes that are triggering mounts.

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

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.

Ian


_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, 2007-03-22 at 08:31 -0500, Serge E. Hallyn wrote:
> > > 
> > > From: "Serge E. Hallyn" <serue@us.ibm.com>
> > > 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.
> 
> 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.

OK great.

I'll have a close look through the code and it's related calls when I
get hold of a complete patch. That will be much easier and mistakes will
be less likely.

Ian


_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers