linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] struct pid-ify autofs4
@ 2012-09-19 15:29 Miklos Szeredi
  2012-09-19 15:29 ` [PATCH 1/2] Replace pid_t in autofs4 with struct pid reference Miklos Szeredi
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Miklos Szeredi @ 2012-09-19 15:29 UTC (permalink / raw)
  To: raven
  Cc: autofs, linux-fsdevel, linux-kernel, sukadev, serge.hallyn,
	ebiederm, mszeredi


These two patches change autofs4 to store struct pid pointers instead of pid_t
values.

Fixed various issues with the previous post.  Not tested, handle with care!

Thanks,
Miklos
---


Serge Hallyn (1):
      autofs4: store struct pids in autofs_waitqs

Sukadev Bhattiprolu (1):
      Replace pid_t in autofs4 with struct pid reference.

---
 fs/autofs4/autofs_i.h  |    8 ++++----
 fs/autofs4/dev-ioctl.c |    3 ++-
 fs/autofs4/inode.c     |   22 +++++++++++++++-------
 fs/autofs4/waitq.c     |   19 +++++++++++++------
 4 files changed, 34 insertions(+), 18 deletions(-)



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] Replace pid_t in autofs4 with struct pid reference.
  2012-09-19 15:29 [PATCH 0/2] struct pid-ify autofs4 Miklos Szeredi
@ 2012-09-19 15:29 ` Miklos Szeredi
  2012-09-19 15:29 ` [PATCH 2/2] autofs4: store struct pids in autofs_waitqs Miklos Szeredi
  2012-09-21 15:44 ` [PATCH 0/2] struct pid-ify autofs4 Miklos Szeredi
  2 siblings, 0 replies; 11+ messages in thread
From: Miklos Szeredi @ 2012-09-19 15:29 UTC (permalink / raw)
  To: raven
  Cc: autofs, linux-fsdevel, linux-kernel, sukadev, serge.hallyn,
	ebiederm, mszeredi, Serge Hallyn

From: Sukadev Bhattiprolu <sukadev@us.ibm.com>

Make autofs4 container-friendly by caching struct pid reference rather
than pid.

Signed-off-by: Sukadev Bhattiprolu <sukadev@us.ibm.com>
Cc: Serge Hallyn <serue@us.ibm.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/autofs4/autofs_i.h  |    4 ++--
 fs/autofs4/dev-ioctl.c |    3 ++-
 fs/autofs4/inode.c     |   22 +++++++++++++++-------
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index 908e184..8457a1f 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -104,7 +104,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;
@@ -139,7 +139,7 @@ static inline struct autofs_info *autofs4_dentry_ino(struct dentry *dentry)
    filesystem without "magic".) */
 
 static inline int autofs4_oz_mode(struct autofs_sb_info *sbi) {
-	return sbi->catatonic || task_pgrp_nr(current) == sbi->oz_pgrp;
+	return sbi->catatonic || task_pgrp(current) == sbi->oz_pgrp;
 }
 
 /* Does a dentry have some pending activity? */
diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index abf645c..423346d 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -381,7 +381,8 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
 			fput(pipe);
 			goto out;
 		}
-		sbi->oz_pgrp = task_pgrp_nr(current);
+		put_pid(sbi->oz_pgrp);
+		sbi->oz_pgrp = get_pid(task_pgrp(current));
 		sbi->pipefd = pipefd;
 		sbi->pipe = pipe;
 		sbi->catatonic = 0;
diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index 8a4fed8..a2b4f4d 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -62,6 +62,8 @@ void autofs4_kill_sb(struct super_block *sb)
 	/* Free wait queues, close pipe */
 	autofs4_catatonic_mode(sbi);
 
+	put_pid(sbi->oz_pgrp);
+
 	sb->s_fs_info = NULL;
 	kfree(sbi);
 
@@ -83,7 +85,7 @@ static int autofs4_show_options(struct seq_file *m, struct dentry *root)
 		seq_printf(m, ",uid=%u", root_inode->i_uid);
 	if (root_inode->i_gid != 0)
 		seq_printf(m, ",gid=%u", root_inode->i_gid);
-	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);
@@ -127,7 +129,8 @@ static const match_table_t tokens = {
 };
 
 static int parse_options(char *options, int *pipefd, uid_t *uid, gid_t *gid,
-		pid_t *pgrp, unsigned int *type, int *minproto, int *maxproto)
+			 struct pid **pgrp, unsigned int *type, int *minproto,
+			 int *maxproto)
 {
 	char *p;
 	substring_t args[MAX_OPT_ARGS];
@@ -135,7 +138,6 @@ static int parse_options(char *options, int *pipefd, uid_t *uid, gid_t *gid,
 
 	*uid = current_uid();
 	*gid = current_gid();
-	*pgrp = task_pgrp_nr(current);
 
 	*minproto = AUTOFS_MIN_PROTO_VERSION;
 	*maxproto = AUTOFS_MAX_PROTO_VERSION;
@@ -169,7 +171,12 @@ static int parse_options(char *options, int *pipefd, uid_t *uid, gid_t *gid,
 		case Opt_pgrp:
 			if (match_int(args, &option))
 				return 1;
-			*pgrp = option;
+			put_pid(*pgrp);
+			*pgrp = find_get_pid(option);
+			if (!*pgrp) {
+				pr_warn("autofs: could not find process group %d\n", option);
+				return 1;
+			}
 			break;
 		case Opt_minproto:
 			if (match_int(args, &option))
@@ -217,7 +224,7 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 	sbi->pipe = NULL;
 	sbi->catatonic = 1;
 	sbi->exp_timeout = 0;
-	sbi->oz_pgrp = task_pgrp_nr(current);
+	sbi->oz_pgrp = get_pid(task_pgrp(current));
 	sbi->sb = s;
 	sbi->version = 0;
 	sbi->sub_version = 0;
@@ -283,9 +290,9 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 		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, pid_nr(sbi->oz_pgrp));
 	pipe = fget(pipefd);
-	
+
 	if (!pipe) {
 		printk("autofs: could not open pipe file descriptor\n");
 		goto fail_dput;
@@ -315,6 +322,7 @@ fail_dput:
 fail_ino:
 	kfree(ino);
 fail_free:
+	put_pid(sbi->oz_pgrp);
 	kfree(sbi);
 	s->s_fs_info = NULL;
 fail_unlock:
-- 
1.7.7


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] autofs4: store struct pids in autofs_waitqs
  2012-09-19 15:29 [PATCH 0/2] struct pid-ify autofs4 Miklos Szeredi
  2012-09-19 15:29 ` [PATCH 1/2] Replace pid_t in autofs4 with struct pid reference Miklos Szeredi
@ 2012-09-19 15:29 ` Miklos Szeredi
  2012-09-21 15:44 ` [PATCH 0/2] struct pid-ify autofs4 Miklos Szeredi
  2 siblings, 0 replies; 11+ messages in thread
From: Miklos Szeredi @ 2012-09-19 15:29 UTC (permalink / raw)
  To: raven
  Cc: autofs, linux-fsdevel, linux-kernel, sukadev, serge.hallyn,
	ebiederm, mszeredi

From: Serge Hallyn <serue@us.ibm.com>

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>
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
Cc: Eric Biederman <ebiederm@xmission.com>
---
 fs/autofs4/autofs_i.h |    4 ++--
 fs/autofs4/waitq.c    |   19 +++++++++++++------
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index 8457a1f..c3b3302 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -91,8 +91,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;
 	unsigned int wait_ctr;
diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index da8876d..27b18ae 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -165,8 +165,8 @@ static void autofs4_notify_daemon(struct autofs_sb_info *sbi,
 		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:
@@ -348,6 +348,13 @@ static int validate_request(struct autofs_wait_queue **wait,
 	return 1;
 }
 
+static void autofs_free_wait_queue(struct autofs_wait_queue *wq)
+{
+	put_pid(wq->pid);
+	put_pid(wq->tgid);
+	kfree(wq);
+}
+
 int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 		enum autofs_notify notify)
 {
@@ -425,8 +432,8 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 		wq->ino = autofs4_get_ino(sbi);
 		wq->uid = current_uid();
 		wq->gid = current_gid();
-		wq->pid = current->pid;
-		wq->tgid = current->tgid;
+		wq->pid = get_pid(task_pid(current));
+		wq->tgid = get_pid(task_tgid(current));
 		wq->status = -EINTR; /* Status return if interrupted */
 		wq->wait_ctr = 2;
 		mutex_unlock(&sbi->wq_mutex);
@@ -526,7 +533,7 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 	/* Are we the last process to need status? */
 	mutex_lock(&sbi->wq_mutex);
 	if (!--wq->wait_ctr)
-		kfree(wq);
+		autofs_free_wait_queue(wq);
 	mutex_unlock(&sbi->wq_mutex);
 
 	return status;
@@ -554,7 +561,7 @@ int autofs4_wait_release(struct autofs_sb_info *sbi, autofs_wqt_t wait_queue_tok
 	wq->status = status;
 	wake_up_interruptible(&wq->queue);
 	if (!--wq->wait_ctr)
-		kfree(wq);
+		autofs_free_wait_queue(wq);
 	mutex_unlock(&sbi->wq_mutex);
 
 	return 0;
-- 
1.7.7


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] struct pid-ify autofs4
  2012-09-19 15:29 [PATCH 0/2] struct pid-ify autofs4 Miklos Szeredi
  2012-09-19 15:29 ` [PATCH 1/2] Replace pid_t in autofs4 with struct pid reference Miklos Szeredi
  2012-09-19 15:29 ` [PATCH 2/2] autofs4: store struct pids in autofs_waitqs Miklos Szeredi
@ 2012-09-21 15:44 ` Miklos Szeredi
  2012-09-24  0:38   ` Ian Kent
  2 siblings, 1 reply; 11+ messages in thread
From: Miklos Szeredi @ 2012-09-21 15:44 UTC (permalink / raw)
  To: raven
  Cc: autofs, linux-fsdevel, linux-kernel, sukadev, serge.hallyn, ebiederm

Miklos Szeredi <miklos@szeredi.hu> writes:

> These two patches change autofs4 to store struct pid pointers instead of pid_t
> values.
>
> Fixed various issues with the previous post.  Not tested, handle with
> care!

Customer gave positive test results.

Thanks,
Miklos

>
> Thanks,
> Miklos
> ---
>
>
> Serge Hallyn (1):
>       autofs4: store struct pids in autofs_waitqs
>
> Sukadev Bhattiprolu (1):
>       Replace pid_t in autofs4 with struct pid reference.
>
> ---
>  fs/autofs4/autofs_i.h  |    8 ++++----
>  fs/autofs4/dev-ioctl.c |    3 ++-
>  fs/autofs4/inode.c     |   22 +++++++++++++++-------
>  fs/autofs4/waitq.c     |   19 +++++++++++++------
>  4 files changed, 34 insertions(+), 18 deletions(-)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] struct pid-ify autofs4
  2012-09-21 15:44 ` [PATCH 0/2] struct pid-ify autofs4 Miklos Szeredi
@ 2012-09-24  0:38   ` Ian Kent
  2012-09-24 13:34     ` Miklos Szeredi
  0 siblings, 1 reply; 11+ messages in thread
From: Ian Kent @ 2012-09-24  0:38 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: autofs, linux-fsdevel, linux-kernel, sukadev, serge.hallyn, ebiederm

On Fri, 2012-09-21 at 17:44 +0200, Miklos Szeredi wrote:
> Miklos Szeredi <miklos@szeredi.hu> writes:
> 
> > These two patches change autofs4 to store struct pid pointers instead of pid_t
> > values.
> >
> > Fixed various issues with the previous post.  Not tested, handle with
> > care!
> 
> Customer gave positive test results.

For what exactly, there's no problem description in these patches?

> 
> Thanks,
> Miklos
> 
> >
> > Thanks,
> > Miklos
> > ---
> >
> >
> > Serge Hallyn (1):
> >       autofs4: store struct pids in autofs_waitqs
> >
> > Sukadev Bhattiprolu (1):
> >       Replace pid_t in autofs4 with struct pid reference.
> >
> > ---
> >  fs/autofs4/autofs_i.h  |    8 ++++----
> >  fs/autofs4/dev-ioctl.c |    3 ++-
> >  fs/autofs4/inode.c     |   22 +++++++++++++++-------
> >  fs/autofs4/waitq.c     |   19 +++++++++++++------
> >  4 files changed, 34 insertions(+), 18 deletions(-)



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] struct pid-ify autofs4
  2012-09-24  0:38   ` Ian Kent
@ 2012-09-24 13:34     ` Miklos Szeredi
  2012-09-25  1:46       ` Ian Kent
  0 siblings, 1 reply; 11+ messages in thread
From: Miklos Szeredi @ 2012-09-24 13:34 UTC (permalink / raw)
  To: Ian Kent
  Cc: autofs, linux-fsdevel, linux-kernel, sukadev, serge.hallyn, ebiederm

Ian Kent <raven@themaw.net> writes:

> On Fri, 2012-09-21 at 17:44 +0200, Miklos Szeredi wrote:
>> Miklos Szeredi <miklos@szeredi.hu> writes:
>> 
>> > These two patches change autofs4 to store struct pid pointers instead of pid_t
>> > values.
>> >
>> > Fixed various issues with the previous post.  Not tested, handle with
>> > care!
>> 
>> Customer gave positive test results.
>
> For what exactly, there's no problem description in these patches?

>From what I understand (and I'm not an expert by any means) is that
autofs doesn't work if containers are used.  The first patch fixes this.

Both the patches replace pid_t with a refcounted struct pid object,
which has better lifetime properties: you don't know whether a pid_t is
valid, because pid numbers are reused, while pid objects remain valid
until there are no more references to them.

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] struct pid-ify autofs4
  2012-09-24 13:34     ` Miklos Szeredi
@ 2012-09-25  1:46       ` Ian Kent
  2012-09-25  2:56         ` Eric W. Biederman
  0 siblings, 1 reply; 11+ messages in thread
From: Ian Kent @ 2012-09-25  1:46 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: autofs, linux-fsdevel, linux-kernel, sukadev, serge.hallyn, ebiederm

On Mon, 2012-09-24 at 15:34 +0200, Miklos Szeredi wrote:
> Ian Kent <raven@themaw.net> writes:
> 
> > On Fri, 2012-09-21 at 17:44 +0200, Miklos Szeredi wrote:
> >> Miklos Szeredi <miklos@szeredi.hu> writes:
> >> 
> >> > These two patches change autofs4 to store struct pid pointers instead of pid_t
> >> > values.
> >> >
> >> > Fixed various issues with the previous post.  Not tested, handle with
> >> > care!
> >> 
> >> Customer gave positive test results.
> >
> > For what exactly, there's no problem description in these patches?
> 
> From what I understand (and I'm not an expert by any means) is that
> autofs doesn't work if containers are used.  The first patch fixes this.

Yeah, the problem with that is that "autofs doesn't work if containers
are used" is ill defined since there are use cases where it does, I
believe. At the very least, ill defined in my view of things.

But I can't even sensibly discuss it because of the lack of specified
use cases and requirements for each. So, there's a chance this will
break another case that does work.

All I can do is ask annoying questions each time time I see related
patches.

> 
> Both the patches replace pid_t with a refcounted struct pid object,
> which has better lifetime properties: you don't know whether a pid_t is
> valid, because pid numbers are reused, while pid objects remain valid
> until there are no more references to them.

Yep, at least I got that.

> 
> Thanks,
> Miklos



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] struct pid-ify autofs4
  2012-09-25  1:46       ` Ian Kent
@ 2012-09-25  2:56         ` Eric W. Biederman
  2012-10-11  3:34           ` Ian Kent
  0 siblings, 1 reply; 11+ messages in thread
From: Eric W. Biederman @ 2012-09-25  2:56 UTC (permalink / raw)
  To: Ian Kent
  Cc: Miklos Szeredi, autofs, linux-fsdevel, linux-kernel, sukadev,
	serge.hallyn

Ian Kent <raven@themaw.net> writes:

> On Mon, 2012-09-24 at 15:34 +0200, Miklos Szeredi wrote:
>> Ian Kent <raven@themaw.net> writes:
>> 
>> > On Fri, 2012-09-21 at 17:44 +0200, Miklos Szeredi wrote:
>> >> Miklos Szeredi <miklos@szeredi.hu> writes:
>> >> 
>> >> > These two patches change autofs4 to store struct pid pointers instead of pid_t
>> >> > values.
>> >> >
>> >> > Fixed various issues with the previous post.  Not tested, handle with
>> >> > care!
>> >> 
>> >> Customer gave positive test results.
>> >
>> > For what exactly, there's no problem description in these patches?
>> 
>> From what I understand (and I'm not an expert by any means) is that
>> autofs doesn't work if containers are used.  The first patch fixes this.
>
> Yeah, the problem with that is that "autofs doesn't work if containers
> are used" is ill defined since there are use cases where it does, I
> believe. At the very least, ill defined in my view of things.

An easy complaint is that task->pid and task->tgid are deprecated fields
in the task structure.  Things that use pids should in use struct pid
values instead.

The trick part of using struct pid values is that there are times when
you need to interact with userspace.  And the question is which pid
namespace is your userspace process in so that you can convert
to and from the proper pid namespace.

The pgrp option on the mount of autofs is buggy because the pid
namespace of the process group is not captured at the time of mount
and so userspace could think it is talking about one process group
while autofs is talking about another process group.  This is a
practical problem if the process that mounts autofs is not running
in the initial pid namespace.

There is a second question.  What happens if the oz_pgrp exists
and then pids wrap around and another process uses the same process
group number.  Currently the autofs code will treat the new proces
group like the old one leading to unexpected behavior.  Which I believe
will be autofs mounts not happening when desired.

Another problem is what happens when a process triggers an automount.
Today we will report the pid and the tgid in the initial pid namespace
of the process that triggered the mount.

So what I can see is that today if the process that mounts autofs
(aka the process at the other end of the autofs pipe) is not in
the initial pid namespace things will go awry, as autofs will report
pid values that make no sense to anyone.

I would like to say the patches fix that problem (and they come close)
however they still translate everything into the initial pid namespace.

> But I can't even sensibly discuss it because of the lack of specified
> use cases and requirements for each. So, there's a chance this will
> break another case that does work.

That seems to be a reasonable concern.  Education so that the
differences in the code are comprehensible.

I am also curious about which case people were seeing problems with as
these patches were reported to be tested and to have fixed a customer
problem.  The only case that is particularly clear to me that these
patches will fix is the case of process group wrap around, but proces
group wrap around should be exceedingly rare.

To handle mounts made outside of the initial pid namespace it appears
that all that is needed is to caputure the pid namespace of the process
that mounts autofs and uses pid_nr_ns to format pids into that
namespace, inside of autofs

It is my feeling that in addition to capture oz_pgrp on mount the
automount and autofs_dev_ioctl_set_pipefd should capture a pid namespace
for formating pids delivered into the pipe.  That would allow the
automount process itself to live outside of the initial pid namespace.

Eric


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] struct pid-ify autofs4
  2012-09-25  2:56         ` Eric W. Biederman
@ 2012-10-11  3:34           ` Ian Kent
  2012-10-24 14:59             ` Miklos Szeredi
  0 siblings, 1 reply; 11+ messages in thread
From: Ian Kent @ 2012-10-11  3:34 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Miklos Szeredi, autofs, linux-fsdevel, linux-kernel, sukadev,
	serge.hallyn

On Mon, 2012-09-24 at 19:56 -0700, Eric W. Biederman wrote:
> Ian Kent <raven@themaw.net> writes:
> 
> > On Mon, 2012-09-24 at 15:34 +0200, Miklos Szeredi wrote:
> >> Ian Kent <raven@themaw.net> writes:
> >> 
> >> > On Fri, 2012-09-21 at 17:44 +0200, Miklos Szeredi wrote:
> >> >> Miklos Szeredi <miklos@szeredi.hu> writes:
> >> >> 
> >> >> > These two patches change autofs4 to store struct pid pointers instead of pid_t
> >> >> > values.
> >> >> >
> >> >> > Fixed various issues with the previous post.  Not tested, handle with
> >> >> > care!
> >> >> 
> >> >> Customer gave positive test results.
> >> >
> >> > For what exactly, there's no problem description in these patches?
> >> 
> >> From what I understand (and I'm not an expert by any means) is that
> >> autofs doesn't work if containers are used.  The first patch fixes this.
> >
> > Yeah, the problem with that is that "autofs doesn't work if containers
> > are used" is ill defined since there are use cases where it does, I
> > believe. At the very least, ill defined in my view of things.
> 
> An easy complaint is that task->pid and task->tgid are deprecated fields
> in the task structure.  Things that use pids should in use struct pid
> values instead.

Yes, we do need to fix that.

> 
> The trick part of using struct pid values is that there are times when
> you need to interact with userspace.  And the question is which pid
> namespace is your userspace process in so that you can convert
> to and from the proper pid namespace.
> 
> The pgrp option on the mount of autofs is buggy because the pid
> namespace of the process group is not captured at the time of mount
> and so userspace could think it is talking about one process group
> while autofs is talking about another process group.  This is a
> practical problem if the process that mounts autofs is not running
> in the initial pid namespace.

Yep.

> 
> There is a second question.  What happens if the oz_pgrp exists
> and then pids wrap around and another process uses the same process
> group number.  Currently the autofs code will treat the new proces
> group like the old one leading to unexpected behavior.  Which I believe
> will be autofs mounts not happening when desired.

OK, but I think your saying a process in another namespace could then
use that process group number or the daemon would need to be SIGKILLed
in the initial namespace.

> 
> Another problem is what happens when a process triggers an automount.
> Today we will report the pid and the tgid in the initial pid namespace
> of the process that triggered the mount.
> 
> So what I can see is that today if the process that mounts autofs
> (aka the process at the other end of the autofs pipe) is not in
> the initial pid namespace things will go awry, as autofs will report
> pid values that make no sense to anyone.

Yep, I get that too.

> 
> I would like to say the patches fix that problem (and they come close)
> however they still translate everything into the initial pid namespace.

I think it's a little more difficult to do than it appears since
multiple namespace behavior is not defined for autofs.

For example, suppose we had the situation where the correct namespace
was always used and another instance of automount was run within a
namespace using different maps. When the namespace is created we could
get autofs mounts from the initial namespace which the daemon won't know
how to handle so they would need to be umounted before the new daemon is
started. Since that isn't automated as part of namespace creation it
almost certainly would lead to complains and lots of confusion.

At the moment it seems best to restrict interactions to a single daemon
in the initial namespace until specific behavioral requirements are
clear.

> 
> > But I can't even sensibly discuss it because of the lack of specified
> > use cases and requirements for each. So, there's a chance this will
> > break another case that does work.
> 
> That seems to be a reasonable concern.  Education so that the
> differences in the code are comprehensible.
> 
> I am also curious about which case people were seeing problems with as
> these patches were reported to be tested and to have fixed a customer
> problem.  The only case that is particularly clear to me that these
> patches will fix is the case of process group wrap around, but proces
> group wrap around should be exceedingly rare.

This comment arose because of a bug report regarding lxc not working
with automount. It had an accompanying patch that had a couple of hunks
from an unrelated patch and I think most of what is in the patches here.
My initial reaction was caution since I don't want to break what now
functions OK with what was a suspect patch.

If that change is done in RHEL it would have to be a back port of these
patches since it is clearer what they are trying to achieve and I
believe they would not break existing function.

> 
> To handle mounts made outside of the initial pid namespace it appears
> that all that is needed is to caputure the pid namespace of the process
> that mounts autofs and uses pid_nr_ns to format pids into that
> namespace, inside of autofs

Unfortunately, I think that is just the start of what's needed.

I think the example above should be enough to show that there's a need
to define semantic behavior and usage restrictions before trying to code
a solution and that's not entirely straight forward.
 
> 
> It is my feeling that in addition to capture oz_pgrp on mount the
> automount and autofs_dev_ioctl_set_pipefd should capture a pid namespace
> for formating pids delivered into the pipe.  That would allow the
> automount process itself to live outside of the initial pid namespace.

I think this should be safe enough and is a good idea to do by itself.

We would need to be careful not to "sell" it as a multiple namespace use
fix though, as it would probably look like that to others.

Ian


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] struct pid-ify autofs4
  2012-10-11  3:34           ` Ian Kent
@ 2012-10-24 14:59             ` Miklos Szeredi
  2012-10-25  0:25               ` Ian Kent
  0 siblings, 1 reply; 11+ messages in thread
From: Miklos Szeredi @ 2012-10-24 14:59 UTC (permalink / raw)
  To: Ian Kent
  Cc: Eric W. Biederman, autofs, linux-fsdevel, linux-kernel, sukadev,
	serge.hallyn

> Ian Kent <raven@themaw.net> writes:
> 
> > Yeah, the problem with that is that "autofs doesn't work if containers
> > are used" is ill defined since there are use cases where it does, I
> > believe. At the very least, ill defined in my view of things.

Customer says:

 "There is no interaction between host and the conatainer.  The host use
  only his own automount and each containers used automount in their
  container."

I think it's a pretty clearly defined use case.  And one which automount
could easily support since the only requirement is that all namespaces
are treated equally.

But I agree that adding safeguards against cases which don't have such
easily defined semantics (such as triggers from several different
namespaces).

I'll post updated patches.

Thanks,
Miklos


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] struct pid-ify autofs4
  2012-10-24 14:59             ` Miklos Szeredi
@ 2012-10-25  0:25               ` Ian Kent
  0 siblings, 0 replies; 11+ messages in thread
From: Ian Kent @ 2012-10-25  0:25 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: Eric W. Biederman, autofs, linux-fsdevel, linux-kernel, sukadev,
	serge.hallyn

On Wed, 2012-10-24 at 16:59 +0200, Miklos Szeredi wrote:
> > Ian Kent <raven@themaw.net> writes:
> > 
> > > Yeah, the problem with that is that "autofs doesn't work if containers
> > > are used" is ill defined since there are use cases where it does, I
> > > believe. At the very least, ill defined in my view of things.
> 
> Customer says:
> 
>  "There is no interaction between host and the conatainer.  The host use
>   only his own automount and each containers used automount in their
>   container."

That sounds like a sensible requirement to me.

> 
> I think it's a pretty clearly defined use case.  And one which automount
> could easily support since the only requirement is that all namespaces
> are treated equally.

Yep. A problem might be dealing with mounts cloned from the parent
namespace at container creation. Ideally they wouldn't be duplicated so
they wouldn't need to be cleaned up (perhaps that's justified given the
requirement above).

Another thought is, what would happen on just cloning a namespace, not
necessarily as a container (is that even a sensible question)? The user
may actually want the mounts in this case, and can we even tell the
difference at namespace creation?

> 
> But I agree that adding safeguards against cases which don't have such
> easily defined semantics (such as triggers from several different
> namespaces).

Look forward to it.

> 
> I'll post updated patches.
> 
> Thanks,
> Miklos
> 



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2012-10-25  0:32 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-19 15:29 [PATCH 0/2] struct pid-ify autofs4 Miklos Szeredi
2012-09-19 15:29 ` [PATCH 1/2] Replace pid_t in autofs4 with struct pid reference Miklos Szeredi
2012-09-19 15:29 ` [PATCH 2/2] autofs4: store struct pids in autofs_waitqs Miklos Szeredi
2012-09-21 15:44 ` [PATCH 0/2] struct pid-ify autofs4 Miklos Szeredi
2012-09-24  0:38   ` Ian Kent
2012-09-24 13:34     ` Miklos Szeredi
2012-09-25  1:46       ` Ian Kent
2012-09-25  2:56         ` Eric W. Biederman
2012-10-11  3:34           ` Ian Kent
2012-10-24 14:59             ` Miklos Szeredi
2012-10-25  0:25               ` Ian Kent

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).