Re: [patch 1/2] autofs4: allow autofs to work outside the initial PID namespace

Re: [patch 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Oleg Nesterov]

On 11/15, Andrew Morton wrote:
>
> Enable autofs4 to work in a "container".  oz_pgrp is converted from pid_t
> to struct pid and this is stored at mount time based on the "pgrp=" option
> or if the option is missing then the current pgrp.

I don't understand this code, so I am probably wrong. And this is minor
anyway, but...

> @@ -357,7 +358,17 @@ static int autofs_dev_ioctl_setpipefd(st
>  		mutex_unlock(&sbi->wq_mutex);
>  		return -EBUSY;
>  	} else {
> -		struct file *pipe = fget(pipefd);
> +		struct file *pipe;
> +
> +		new_pid = get_task_pid(current, PIDTYPE_PGID);
> +
> +		if (ns_of_pid(new_pid) != ns_of_pid(sbi->oz_pgrp)) {
> +			AUTOFS_WARN("Not allowed to change PID namespace");
> +			err = -EINVAL;
> +			goto out;
> +		}
> +
> +		pipe = fget(pipefd);
>  		if (!pipe) {
>  			err = -EBADF;
>  			goto out;
> @@ -367,12 +378,13 @@ static int autofs_dev_ioctl_setpipefd(st
>  			fput(pipe);
>  			goto out;
>  		}
> -		sbi->oz_pgrp = task_pgrp_nr(current);
> +		swap(sbi->oz_pgrp, new_pid);
>  		sbi->pipefd = pipefd;
>  		sbi->pipe = pipe;
>  		sbi->catatonic = 0;
>  	}
>  out:
> +	put_pid(new_pid);

This looks suspicious, put_pid() can actually kfree the old sbi->oz_pgrp
swapped above. IOW, this assumes we can't race with any user of ->oz_pgrp.

For example,

> @@ -80,7 +83,7 @@ static int autofs4_show_options(struct s
>  	if (!gid_eq(root_inode->i_gid, GLOBAL_ROOT_GID))
>  		seq_printf(m, ",gid=%u",
>  			from_kgid_munged(&init_user_ns, root_inode->i_gid));
> -	seq_printf(m, ",pgrp=%d", sbi->oz_pgrp);
> +	seq_printf(m, ",pgrp=%d", pid_vnr(sbi->oz_pgrp));

Can't this race with autofs_dev_ioctl_setpipefd() above?

Oleg.

Re: [patch 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Ian Kent]

On Sat, 2013-11-16 at 17:03 +0100, Oleg Nesterov wrote:
> On 11/15, Andrew Morton wrote:
> >
> > Enable autofs4 to work in a "container".  oz_pgrp is converted from pid_t
> > to struct pid and this is stored at mount time based on the "pgrp=" option
> > or if the option is missing then the current pgrp.
> 
> I don't understand this code, so I am probably wrong. And this is minor
> anyway, but...

And I don't understand the namespace code myself but I think you are
correct about the raceand I'm not sure what to do about it either just
yet.

The sbi->wq_mutex will prevent user space daemon notifications and wait
releases but it won't prevent lookups or umounts, the sbi->fs_lock would
be needed for that.

I'm not sure that matters though since there can be only one user space
daemon handling an automount point and the case below here requires
there is no current process owner of the mount the pipe fd is to be set
for (ie. the "if (!sbi->catatonic) { ... }"). That just means a new
process is attempting to "reconnect" to the mount, so umount shouldn't
be a problem. I guess there is a chance that the root user or an errant
pre-existing process might try and umount the mount ...

It might be sufficient to surround the contents of show_options() with a
lock on wq_mutex and update the check at the top to

"lock; if (!sbi || sbi->catatonic) {unlock, return 0; } ..."

but it might be necessary to use the sbi->fs_lock in both locations,
since a catatonic automount has no outstanding waits and can't create
new waits (and hence there can't be any valid wait release ioctls).

I'll think a bit more about the potential umount issue before offering a
patch.

> 
> > @@ -357,7 +358,17 @@ static int autofs_dev_ioctl_setpipefd(st
> >  		mutex_unlock(&sbi->wq_mutex);
> >  		return -EBUSY;
> >  	} else {
> > -		struct file *pipe = fget(pipefd);
> > +		struct file *pipe;
> > +
> > +		new_pid = get_task_pid(current, PIDTYPE_PGID);
> > +
> > +		if (ns_of_pid(new_pid) != ns_of_pid(sbi->oz_pgrp)) {
> > +			AUTOFS_WARN("Not allowed to change PID namespace");
> > +			err = -EINVAL;
> > +			goto out;
> > +		}
> > +
> > +		pipe = fget(pipefd);
> >  		if (!pipe) {
> >  			err = -EBADF;
> >  			goto out;
> > @@ -367,12 +378,13 @@ static int autofs_dev_ioctl_setpipefd(st
> >  			fput(pipe);
> >  			goto out;
> >  		}
> > -		sbi->oz_pgrp = task_pgrp_nr(current);
> > +		swap(sbi->oz_pgrp, new_pid);
> >  		sbi->pipefd = pipefd;
> >  		sbi->pipe = pipe;
> >  		sbi->catatonic = 0;
> >  	}
> >  out:
> > +	put_pid(new_pid);
> 
> This looks suspicious, put_pid() can actually kfree the old sbi->oz_pgrp
> swapped above. IOW, this assumes we can't race with any user of ->oz_pgrp.
> 
> For example,
> 
> > @@ -80,7 +83,7 @@ static int autofs4_show_options(struct s
> >  	if (!gid_eq(root_inode->i_gid, GLOBAL_ROOT_GID))
> >  		seq_printf(m, ",gid=%u",
> >  			from_kgid_munged(&init_user_ns, root_inode->i_gid));
> > -	seq_printf(m, ",pgrp=%d", sbi->oz_pgrp);
> > +	seq_printf(m, ",pgrp=%d", pid_vnr(sbi->oz_pgrp));
> 
> Can't this race with autofs_dev_ioctl_setpipefd() above?
> 
> Oleg.
> 

Re: [patch 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Oleg Nesterov]

On 11/18, Ian Kent wrote:
>
> It might be sufficient to surround the contents of show_options() with a
> lock on wq_mutex and update the check at the top to
>
> "lock; if (!sbi || sbi->catatonic) {unlock, return 0; } ..."

Or perhaps you can add synchronize_rcu() after swap(oz_pgrp, new_pid),
then show_options() can use rcu_read_lock().

This makes me think that we want put_pid_rcu() but I do not see a
simple way to implement it without synchronize_rcu or kmalloc...

Oleg.

[PATCH 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Miklos Szeredi]

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

This patch enables autofs4 to work in a "container".  oz_pgrp is converted from
pid_t to struct pid and this is stored at mount time based on the "pgrp=" option
or if the option is missing then the current pgrp.

The "pgrp=" option is interpreted in the PID namespace of the current process.
This option is flawed in that it doesn't carry the namespace information, so it
should be deprecated.  AFAICS the autofs daemon always sends the current pgrp,
which is the default anyway.

The oz_pgrp is also set from the AUTOFS_DEV_IOCTL_SETPIPEFD_CMD ioctl.  This
ioctl sets oz_pgrp to the current pgrp.  It is not allowed to change the pid
namespace.

oz_pgrp is used mainly to determine whether the process traversing the autofs
mount tree is the autofs daemon itself or not.  This function now compares the
pid pointers instead of the pid_t values.

One other use of oz_pgrp is in autofs4_show_options.  There is shows the
virtual pid number (i.e. the one that is valid inside the PID namespace of the
calling process)

For debugging printk convert oz_pgrp to the value in the initial pid namespace.

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 |   16 ++++++++++++++--
 fs/autofs4/inode.c     |   33 +++++++++++++++++++++++++--------
 3 files changed, 41 insertions(+), 12 deletions(-)

--- 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 *autofs
    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? */
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -62,6 +62,8 @@ void autofs4_kill_sb(struct super_block
 	/* Free wait queues, close pipe */
 	autofs4_catatonic_mode(sbi);
 
+	put_pid(sbi->oz_pgrp);
+
 	sb->s_fs_info = NULL;
 	kfree(sbi);
 
@@ -85,7 +87,7 @@ static int autofs4_show_options(struct s
 	if (!gid_eq(root_inode->i_gid, GLOBAL_ROOT_GID))
 		seq_printf(m, ",gid=%u",
 			from_kgid_munged(&init_user_ns, root_inode->i_gid));
-	seq_printf(m, ",pgrp=%d", sbi->oz_pgrp);
+	seq_printf(m, ",pgrp=%d", pid_vnr(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);
@@ -129,7 +131,8 @@ static const match_table_t tokens = {
 };
 
 static int parse_options(char *options, int *pipefd, kuid_t *uid, kgid_t *gid,
-		pid_t *pgrp, unsigned int *type, int *minproto, int *maxproto)
+			 int *pgrp, bool *pgrp_set, unsigned int *type,
+			 int *minproto, int *maxproto)
 {
 	char *p;
 	substring_t args[MAX_OPT_ARGS];
@@ -137,7 +140,6 @@ static int parse_options(char *options,
 
 	*uid = current_uid();
 	*gid = current_gid();
-	*pgrp = task_pgrp_nr(current);
 
 	*minproto = AUTOFS_MIN_PROTO_VERSION;
 	*maxproto = AUTOFS_MAX_PROTO_VERSION;
@@ -176,6 +178,7 @@ static int parse_options(char *options,
 			if (match_int(args, &option))
 				return 1;
 			*pgrp = option;
+			*pgrp_set = true;
 			break;
 		case Opt_minproto:
 			if (match_int(args, &option))
@@ -211,6 +214,8 @@ int autofs4_fill_super(struct super_bloc
 	int pipefd;
 	struct autofs_sb_info *sbi;
 	struct autofs_info *ino;
+	int pgrp;
+	bool pgrp_set = false;
 
 	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
 	if (!sbi)
@@ -223,7 +228,7 @@ int autofs4_fill_super(struct super_bloc
 	sbi->pipe = NULL;
 	sbi->catatonic = 1;
 	sbi->exp_timeout = 0;
-	sbi->oz_pgrp = task_pgrp_nr(current);
+	sbi->oz_pgrp = NULL;
 	sbi->sb = s;
 	sbi->version = 0;
 	sbi->sub_version = 0;
@@ -260,12 +265,23 @@ 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,
-				&sbi->max_proto)) {
+			  &pgrp, &pgrp_set, &sbi->type, &sbi->min_proto,
+			  &sbi->max_proto)) {
 		printk("autofs: called with bogus options\n");
 		goto fail_dput;
 	}
 
+	if (pgrp_set) {
+		sbi->oz_pgrp = find_get_pid(pgrp);
+		if (!sbi->oz_pgrp) {
+			pr_warn("autofs: could not find process group %d\n",
+				pgrp);
+			goto fail_dput;
+		}
+	} else {
+		sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
+	}
+
 	if (autofs_type_trigger(sbi->type))
 		__managed_dentry_set_managed(root);
 
@@ -289,9 +305,9 @@ 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, pid_nr(sbi->oz_pgrp));
 	pipe = fget(pipefd);
-	
+
 	if (!pipe) {
 		printk("autofs: could not open pipe file descriptor\n");
 		goto fail_dput;
@@ -321,6 +337,7 @@ int autofs4_fill_super(struct super_bloc
 fail_ino:
 	kfree(ino);
 fail_free:
+	put_pid(sbi->oz_pgrp);
 	kfree(sbi);
 	s->s_fs_info = NULL;
 fail_unlock:
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -346,6 +346,7 @@ static int autofs_dev_ioctl_setpipefd(st
 {
 	int pipefd;
 	int err = 0;
+	struct pid *new_pid = NULL;
 
 	if (param->setpipefd.pipefd == -1)
 		return -EINVAL;
@@ -357,7 +358,17 @@ static int autofs_dev_ioctl_setpipefd(st
 		mutex_unlock(&sbi->wq_mutex);
 		return -EBUSY;
 	} else {
-		struct file *pipe = fget(pipefd);
+		struct file *pipe;
+
+		new_pid = get_task_pid(current, PIDTYPE_PGID);
+
+		if (ns_of_pid(new_pid) != ns_of_pid(sbi->oz_pgrp)) {
+			AUTOFS_WARN("Not allowed to change PID namespace");
+			err = -EINVAL;
+			goto out;
+		}
+
+		pipe = fget(pipefd);
 		if (!pipe) {
 			err = -EBADF;
 			goto out;
@@ -367,12 +378,13 @@ static int autofs_dev_ioctl_setpipefd(st
 			fput(pipe);
 			goto out;
 		}
-		sbi->oz_pgrp = task_pgrp_nr(current);
+		swap(sbi->oz_pgrp, new_pid);
 		sbi->pipefd = pipefd;
 		sbi->pipe = pipe;
 		sbi->catatonic = 0;
 	}
 out:
+	put_pid(new_pid);
 	mutex_unlock(&sbi->wq_mutex);
 	return err;
 }

Re: [PATCH 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Serge E. Hallyn]

Quoting Miklos Szeredi (miklos@szeredi.hu):
> From: Sukadev Bhattiprolu <sukadev@us.ibm.com>
> 
> This patch enables autofs4 to work in a "container".  oz_pgrp is converted from
> pid_t to struct pid and this is stored at mount time based on the "pgrp=" option
> or if the option is missing then the current pgrp.
> 
> The "pgrp=" option is interpreted in the PID namespace of the current process.
> This option is flawed in that it doesn't carry the namespace information, so it
> should be deprecated.  AFAICS the autofs daemon always sends the current pgrp,
> which is the default anyway.
> 
> The oz_pgrp is also set from the AUTOFS_DEV_IOCTL_SETPIPEFD_CMD ioctl.  This
> ioctl sets oz_pgrp to the current pgrp.  It is not allowed to change the pid
> namespace.
> 
> oz_pgrp is used mainly to determine whether the process traversing the autofs
> mount tree is the autofs daemon itself or not.  This function now compares the
> pid pointers instead of the pid_t values.
> 
> One other use of oz_pgrp is in autofs4_show_options.  There is shows the
> virtual pid number (i.e. the one that is valid inside the PID namespace of the
> calling process)
> 
> For debugging printk convert oz_pgrp to the value in the initial pid namespace.
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev@us.ibm.com>
> Cc: Serge Hallyn <serue@us.ibm.com>

Haven't tested, but it looks good to me.

Acked-by: Serge Hallyn <serge.hallyn@canonical.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 |   16 ++++++++++++++--
>  fs/autofs4/inode.c     |   33 +++++++++++++++++++++++++--------
>  3 files changed, 41 insertions(+), 12 deletions(-)
> 
> --- 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 *autofs
>     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? */
> --- a/fs/autofs4/inode.c
> +++ b/fs/autofs4/inode.c
> @@ -62,6 +62,8 @@ void autofs4_kill_sb(struct super_block
>  	/* Free wait queues, close pipe */
>  	autofs4_catatonic_mode(sbi);
>  
> +	put_pid(sbi->oz_pgrp);
> +
>  	sb->s_fs_info = NULL;
>  	kfree(sbi);
>  
> @@ -85,7 +87,7 @@ static int autofs4_show_options(struct s
>  	if (!gid_eq(root_inode->i_gid, GLOBAL_ROOT_GID))
>  		seq_printf(m, ",gid=%u",
>  			from_kgid_munged(&init_user_ns, root_inode->i_gid));
> -	seq_printf(m, ",pgrp=%d", sbi->oz_pgrp);
> +	seq_printf(m, ",pgrp=%d", pid_vnr(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);
> @@ -129,7 +131,8 @@ static const match_table_t tokens = {
>  };
>  
>  static int parse_options(char *options, int *pipefd, kuid_t *uid, kgid_t *gid,
> -		pid_t *pgrp, unsigned int *type, int *minproto, int *maxproto)
> +			 int *pgrp, bool *pgrp_set, unsigned int *type,
> +			 int *minproto, int *maxproto)
>  {
>  	char *p;
>  	substring_t args[MAX_OPT_ARGS];
> @@ -137,7 +140,6 @@ static int parse_options(char *options,
>  
>  	*uid = current_uid();
>  	*gid = current_gid();
> -	*pgrp = task_pgrp_nr(current);
>  
>  	*minproto = AUTOFS_MIN_PROTO_VERSION;
>  	*maxproto = AUTOFS_MAX_PROTO_VERSION;
> @@ -176,6 +178,7 @@ static int parse_options(char *options,
>  			if (match_int(args, &option))
>  				return 1;
>  			*pgrp = option;
> +			*pgrp_set = true;
>  			break;
>  		case Opt_minproto:
>  			if (match_int(args, &option))
> @@ -211,6 +214,8 @@ int autofs4_fill_super(struct super_bloc
>  	int pipefd;
>  	struct autofs_sb_info *sbi;
>  	struct autofs_info *ino;
> +	int pgrp;
> +	bool pgrp_set = false;
>  
>  	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
>  	if (!sbi)
> @@ -223,7 +228,7 @@ int autofs4_fill_super(struct super_bloc
>  	sbi->pipe = NULL;
>  	sbi->catatonic = 1;
>  	sbi->exp_timeout = 0;
> -	sbi->oz_pgrp = task_pgrp_nr(current);
> +	sbi->oz_pgrp = NULL;
>  	sbi->sb = s;
>  	sbi->version = 0;
>  	sbi->sub_version = 0;
> @@ -260,12 +265,23 @@ 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,
> -				&sbi->max_proto)) {
> +			  &pgrp, &pgrp_set, &sbi->type, &sbi->min_proto,
> +			  &sbi->max_proto)) {
>  		printk("autofs: called with bogus options\n");
>  		goto fail_dput;
>  	}
>  
> +	if (pgrp_set) {
> +		sbi->oz_pgrp = find_get_pid(pgrp);
> +		if (!sbi->oz_pgrp) {
> +			pr_warn("autofs: could not find process group %d\n",
> +				pgrp);
> +			goto fail_dput;
> +		}
> +	} else {
> +		sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
> +	}
> +
>  	if (autofs_type_trigger(sbi->type))
>  		__managed_dentry_set_managed(root);
>  
> @@ -289,9 +305,9 @@ 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, pid_nr(sbi->oz_pgrp));
>  	pipe = fget(pipefd);
> -	
> +
>  	if (!pipe) {
>  		printk("autofs: could not open pipe file descriptor\n");
>  		goto fail_dput;
> @@ -321,6 +337,7 @@ int autofs4_fill_super(struct super_bloc
>  fail_ino:
>  	kfree(ino);
>  fail_free:
> +	put_pid(sbi->oz_pgrp);
>  	kfree(sbi);
>  	s->s_fs_info = NULL;
>  fail_unlock:
> --- a/fs/autofs4/dev-ioctl.c
> +++ b/fs/autofs4/dev-ioctl.c
> @@ -346,6 +346,7 @@ static int autofs_dev_ioctl_setpipefd(st
>  {
>  	int pipefd;
>  	int err = 0;
> +	struct pid *new_pid = NULL;
>  
>  	if (param->setpipefd.pipefd == -1)
>  		return -EINVAL;
> @@ -357,7 +358,17 @@ static int autofs_dev_ioctl_setpipefd(st
>  		mutex_unlock(&sbi->wq_mutex);
>  		return -EBUSY;
>  	} else {
> -		struct file *pipe = fget(pipefd);
> +		struct file *pipe;
> +
> +		new_pid = get_task_pid(current, PIDTYPE_PGID);
> +
> +		if (ns_of_pid(new_pid) != ns_of_pid(sbi->oz_pgrp)) {
> +			AUTOFS_WARN("Not allowed to change PID namespace");
> +			err = -EINVAL;
> +			goto out;
> +		}
> +
> +		pipe = fget(pipefd);
>  		if (!pipe) {
>  			err = -EBADF;
>  			goto out;
> @@ -367,12 +378,13 @@ static int autofs_dev_ioctl_setpipefd(st
>  			fput(pipe);
>  			goto out;
>  		}
> -		sbi->oz_pgrp = task_pgrp_nr(current);
> +		swap(sbi->oz_pgrp, new_pid);
>  		sbi->pipefd = pipefd;
>  		sbi->pipe = pipe;
>  		sbi->catatonic = 0;
>  	}
>  out:
> +	put_pid(new_pid);
>  	mutex_unlock(&sbi->wq_mutex);
>  	return err;
>  }
> --
> 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/

[PATCH 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Miklos Szeredi]

Patches were tested by the customer.

Ian, Eric, do these patches look OK?

Thanks,
Miklos
----
From: Sukadev Bhattiprolu <sukadev@us.ibm.com>

This patch enables autofs4 to work in a "container".  oz_pgrp is converted from
pid_t to struct pid and this is stored at mount time based on the "pgrp=" option
or if the option is missing then the current pgrp.

The "pgrp=" option is interpreted in the PID namespace of the current process.
This option is flawed in that it doesn't carry the namespace information, so it
should be deprecated.  AFAICS the autofs daemon always sends the current pgrp,
which is the default anyway.

The oz_pgrp is also set from the AUTOFS_DEV_IOCTL_SETPIPEFD_CMD ioctl.  This
ioctl sets oz_pgrp to the current pgrp.  It is not allowed to change the pid
namespace.

oz_pgrp is used mainly to determine whether the process traversing the autofs
mount tree is the autofs daemon itself or not.  This function now compares the
pid pointers instead of the pid_t values.

One other use of oz_pgrp is in autofs4_show_options.  There is shows the
virtual pid number (i.e. the one that is valid inside the PID namespace of the
calling process)

For debugging printk convert oz_pgrp to the value in the initial pid namespace.

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 |   16 ++++++++++++++--
 fs/autofs4/inode.c     |   33 +++++++++++++++++++++++++--------
 3 files changed, 41 insertions(+), 12 deletions(-)
Index: linux-2.6/fs/autofs4/autofs_i.h
===================================================================
--- linux-2.6.orig/fs/autofs4/autofs_i.h	2012-11-12 17:55:28.000000000 +0100
+++ linux-2.6/fs/autofs4/autofs_i.h	2012-11-12 19:09:40.000000000 +0100
@@ -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 *autofs
    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? */
Index: linux-2.6/fs/autofs4/inode.c
===================================================================
--- linux-2.6.orig/fs/autofs4/inode.c	2012-11-12 17:55:28.000000000 +0100
+++ linux-2.6/fs/autofs4/inode.c	2012-11-12 19:09:35.000000000 +0100
@@ -62,6 +62,8 @@ void autofs4_kill_sb(struct super_block
 	/* 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 s
 		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_vnr(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)
+			 int *pgrp, bool *pgrp_set, 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,
 
 	*uid = current_uid();
 	*gid = current_gid();
-	*pgrp = task_pgrp_nr(current);
 
 	*minproto = AUTOFS_MIN_PROTO_VERSION;
 	*maxproto = AUTOFS_MAX_PROTO_VERSION;
@@ -170,6 +172,7 @@ static int parse_options(char *options,
 			if (match_int(args, &option))
 				return 1;
 			*pgrp = option;
+			*pgrp_set = true;
 			break;
 		case Opt_minproto:
 			if (match_int(args, &option))
@@ -205,6 +208,8 @@ int autofs4_fill_super(struct super_bloc
 	int pipefd;
 	struct autofs_sb_info *sbi;
 	struct autofs_info *ino;
+	int pgrp;
+	bool pgrp_set = false;
 
 	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
 	if (!sbi)
@@ -217,7 +222,7 @@ int autofs4_fill_super(struct super_bloc
 	sbi->pipe = NULL;
 	sbi->catatonic = 1;
 	sbi->exp_timeout = 0;
-	sbi->oz_pgrp = task_pgrp_nr(current);
+	sbi->oz_pgrp = NULL;
 	sbi->sb = s;
 	sbi->version = 0;
 	sbi->sub_version = 0;
@@ -254,12 +259,23 @@ 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,
-				&sbi->max_proto)) {
+			  &pgrp, &pgrp_set, &sbi->type, &sbi->min_proto,
+			  &sbi->max_proto)) {
 		printk("autofs: called with bogus options\n");
 		goto fail_dput;
 	}
 
+	if (pgrp_set) {
+		sbi->oz_pgrp = find_get_pid(pgrp);
+		if (!sbi->oz_pgrp) {
+			pr_warn("autofs: could not find process group %d\n",
+				pgrp);
+			goto fail_dput;
+		}
+	} else {
+		sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
+	}
+
 	if (autofs_type_trigger(sbi->type))
 		__managed_dentry_set_managed(root);
 
@@ -283,9 +299,9 @@ 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, pid_nr(sbi->oz_pgrp));
 	pipe = fget(pipefd);
-	
+
 	if (!pipe) {
 		printk("autofs: could not open pipe file descriptor\n");
 		goto fail_dput;
@@ -315,6 +331,7 @@ int autofs4_fill_super(struct super_bloc
 fail_ino:
 	kfree(ino);
 fail_free:
+	put_pid(sbi->oz_pgrp);
 	kfree(sbi);
 	s->s_fs_info = NULL;
 fail_unlock:
Index: linux-2.6/fs/autofs4/dev-ioctl.c
===================================================================
--- linux-2.6.orig/fs/autofs4/dev-ioctl.c	2012-11-12 17:55:28.000000000 +0100
+++ linux-2.6/fs/autofs4/dev-ioctl.c	2012-11-12 20:41:26.000000000 +0100
@@ -346,6 +346,7 @@ static int autofs_dev_ioctl_setpipefd(st
 {
 	int pipefd;
 	int err = 0;
+	struct pid *new_pid = NULL;
 
 	if (param->setpipefd.pipefd == -1)
 		return -EINVAL;
@@ -357,7 +358,17 @@ static int autofs_dev_ioctl_setpipefd(st
 		mutex_unlock(&sbi->wq_mutex);
 		return -EBUSY;
 	} else {
-		struct file *pipe = fget(pipefd);
+		struct file *pipe;
+
+		new_pid = get_task_pid(current, PIDTYPE_PGID);
+
+		if (ns_of_pid(new_pid) != ns_of_pid(sbi->oz_pgrp)) {
+			AUTOFS_WARN("Not allowed to change PID namespace");
+			err = -EINVAL;
+			goto out;
+		}
+
+		pipe = fget(pipefd);
 		if (!pipe) {
 			err = -EBADF;
 			goto out;
@@ -367,12 +378,13 @@ static int autofs_dev_ioctl_setpipefd(st
 			fput(pipe);
 			goto out;
 		}
-		sbi->oz_pgrp = task_pgrp_nr(current);
+		swap(sbi->oz_pgrp, new_pid);
 		sbi->pipefd = pipefd;
 		sbi->pipe = pipe;
 		sbi->catatonic = 0;
 	}
 out:
+	put_pid(new_pid);
 	mutex_unlock(&sbi->wq_mutex);
 	return err;
 }

Re: [PATCH 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Ian Kent]

On Thu, 2012-11-22 at 17:24 +0100, Miklos Szeredi wrote:
> Patches were tested by the customer.
> 
> Ian, Eric, do these patches look OK?

They look OK to me but I'm still a bit concerned about changing the way
this behaves, but I also believe this is the way we want it to behave.

Give me a little bit more time to run a simple test to ensure we can at
least do what we could previously, and that's nothing more than
umounting duplicated mounts (which probably shouldn't be duplicated at
all) in the container.

> 
> Thanks,
> Miklos
> ----
> From: Sukadev Bhattiprolu <sukadev@us.ibm.com>
> 
> This patch enables autofs4 to work in a "container".  oz_pgrp is converted from
> pid_t to struct pid and this is stored at mount time based on the "pgrp=" option
> or if the option is missing then the current pgrp.
> 
> The "pgrp=" option is interpreted in the PID namespace of the current process.
> This option is flawed in that it doesn't carry the namespace information, so it
> should be deprecated.  AFAICS the autofs daemon always sends the current pgrp,
> which is the default anyway.
> 
> The oz_pgrp is also set from the AUTOFS_DEV_IOCTL_SETPIPEFD_CMD ioctl.  This
> ioctl sets oz_pgrp to the current pgrp.  It is not allowed to change the pid
> namespace.
> 
> oz_pgrp is used mainly to determine whether the process traversing the autofs
> mount tree is the autofs daemon itself or not.  This function now compares the
> pid pointers instead of the pid_t values.
> 
> One other use of oz_pgrp is in autofs4_show_options.  There is shows the
> virtual pid number (i.e. the one that is valid inside the PID namespace of the
> calling process)
> 
> For debugging printk convert oz_pgrp to the value in the initial pid namespace.
> 
> 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 |   16 ++++++++++++++--
>  fs/autofs4/inode.c     |   33 +++++++++++++++++++++++++--------
>  3 files changed, 41 insertions(+), 12 deletions(-)
> Index: linux-2.6/fs/autofs4/autofs_i.h
> ===================================================================
> --- linux-2.6.orig/fs/autofs4/autofs_i.h	2012-11-12 17:55:28.000000000 +0100
> +++ linux-2.6/fs/autofs4/autofs_i.h	2012-11-12 19:09:40.000000000 +0100
> @@ -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 *autofs
>     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? */
> Index: linux-2.6/fs/autofs4/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/autofs4/inode.c	2012-11-12 17:55:28.000000000 +0100
> +++ linux-2.6/fs/autofs4/inode.c	2012-11-12 19:09:35.000000000 +0100
> @@ -62,6 +62,8 @@ void autofs4_kill_sb(struct super_block
>  	/* 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 s
>  		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_vnr(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)
> +			 int *pgrp, bool *pgrp_set, 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,
>  
>  	*uid = current_uid();
>  	*gid = current_gid();
> -	*pgrp = task_pgrp_nr(current);
>  
>  	*minproto = AUTOFS_MIN_PROTO_VERSION;
>  	*maxproto = AUTOFS_MAX_PROTO_VERSION;
> @@ -170,6 +172,7 @@ static int parse_options(char *options,
>  			if (match_int(args, &option))
>  				return 1;
>  			*pgrp = option;
> +			*pgrp_set = true;
>  			break;
>  		case Opt_minproto:
>  			if (match_int(args, &option))
> @@ -205,6 +208,8 @@ int autofs4_fill_super(struct super_bloc
>  	int pipefd;
>  	struct autofs_sb_info *sbi;
>  	struct autofs_info *ino;
> +	int pgrp;
> +	bool pgrp_set = false;
>  
>  	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
>  	if (!sbi)
> @@ -217,7 +222,7 @@ int autofs4_fill_super(struct super_bloc
>  	sbi->pipe = NULL;
>  	sbi->catatonic = 1;
>  	sbi->exp_timeout = 0;
> -	sbi->oz_pgrp = task_pgrp_nr(current);
> +	sbi->oz_pgrp = NULL;
>  	sbi->sb = s;
>  	sbi->version = 0;
>  	sbi->sub_version = 0;
> @@ -254,12 +259,23 @@ 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,
> -				&sbi->max_proto)) {
> +			  &pgrp, &pgrp_set, &sbi->type, &sbi->min_proto,
> +			  &sbi->max_proto)) {
>  		printk("autofs: called with bogus options\n");
>  		goto fail_dput;
>  	}
>  
> +	if (pgrp_set) {
> +		sbi->oz_pgrp = find_get_pid(pgrp);
> +		if (!sbi->oz_pgrp) {
> +			pr_warn("autofs: could not find process group %d\n",
> +				pgrp);
> +			goto fail_dput;
> +		}
> +	} else {
> +		sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
> +	}
> +
>  	if (autofs_type_trigger(sbi->type))
>  		__managed_dentry_set_managed(root);
>  
> @@ -283,9 +299,9 @@ 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, pid_nr(sbi->oz_pgrp));
>  	pipe = fget(pipefd);
> -	
> +
>  	if (!pipe) {
>  		printk("autofs: could not open pipe file descriptor\n");
>  		goto fail_dput;
> @@ -315,6 +331,7 @@ int autofs4_fill_super(struct super_bloc
>  fail_ino:
>  	kfree(ino);
>  fail_free:
> +	put_pid(sbi->oz_pgrp);
>  	kfree(sbi);
>  	s->s_fs_info = NULL;
>  fail_unlock:
> Index: linux-2.6/fs/autofs4/dev-ioctl.c
> ===================================================================
> --- linux-2.6.orig/fs/autofs4/dev-ioctl.c	2012-11-12 17:55:28.000000000 +0100
> +++ linux-2.6/fs/autofs4/dev-ioctl.c	2012-11-12 20:41:26.000000000 +0100
> @@ -346,6 +346,7 @@ static int autofs_dev_ioctl_setpipefd(st
>  {
>  	int pipefd;
>  	int err = 0;
> +	struct pid *new_pid = NULL;
>  
>  	if (param->setpipefd.pipefd == -1)
>  		return -EINVAL;
> @@ -357,7 +358,17 @@ static int autofs_dev_ioctl_setpipefd(st
>  		mutex_unlock(&sbi->wq_mutex);
>  		return -EBUSY;
>  	} else {
> -		struct file *pipe = fget(pipefd);
> +		struct file *pipe;
> +
> +		new_pid = get_task_pid(current, PIDTYPE_PGID);
> +
> +		if (ns_of_pid(new_pid) != ns_of_pid(sbi->oz_pgrp)) {
> +			AUTOFS_WARN("Not allowed to change PID namespace");
> +			err = -EINVAL;
> +			goto out;
> +		}
> +
> +		pipe = fget(pipefd);
>  		if (!pipe) {
>  			err = -EBADF;
>  			goto out;
> @@ -367,12 +378,13 @@ static int autofs_dev_ioctl_setpipefd(st
>  			fput(pipe);
>  			goto out;
>  		}
> -		sbi->oz_pgrp = task_pgrp_nr(current);
> +		swap(sbi->oz_pgrp, new_pid);
>  		sbi->pipefd = pipefd;
>  		sbi->pipe = pipe;
>  		sbi->catatonic = 0;
>  	}
>  out:
> +	put_pid(new_pid);
>  	mutex_unlock(&sbi->wq_mutex);
>  	return err;
>  }

Re: [PATCH 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Ian Kent]

On Fri, 2012-11-23 at 11:45 +0800, Ian Kent wrote:
> On Thu, 2012-11-22 at 17:24 +0100, Miklos Szeredi wrote:
> > Patches were tested by the customer.
> > 
> > Ian, Eric, do these patches look OK?
> 
> They look OK to me but I'm still a bit concerned about changing the way
> this behaves, but I also believe this is the way we want it to behave.

OK, I ran the autofs Connectathon tests that I often use on a kernel
with these patches and they worked fine. So, AFAICS. the patches
shouldn't introduce regressions.

> 
> Give me a little bit more time to run a simple test to ensure we can at
> least do what we could previously, and that's nothing more than
> umounting duplicated mounts (which probably shouldn't be duplicated at
> all) in the container.

Interestingly the simple container test program I have also worked in
the same way it does on current kernels so again I didn't see a problem
adding the patches.

But I do have a couple of questions that are a little related.

Calling clone(2) with flags
CLONE_NEWPID|CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWIPC|SIGCHLD|CLONE_NEWNET
will result in a copy of the existing set of mounts. The autofs mounts
can be umounted if they are not needed.

But, on Fedora systemd sets "/" as shared at boot which prevents the
umount of these autofs mounts, unless you mark "/" as private in the
clone, after which the mounts can be umounted.

Does having "/" marked as shared in the root namespace mean that further
mounts in the root namespace will also appear in the clone and that
mounts done in the clone will appear in the root namespace?

Will mounting all autofs mounts with MS_PRIVATE prevent the autofs
mounts and any mounts under them from appearing in the root namespace?

I assume there is no way for autofs to stop the propagation from the
root namespace, is that correct?

> 
> > 
> > Thanks,
> > Miklos
> > ----
> > From: Sukadev Bhattiprolu <sukadev@us.ibm.com>
> > 
> > This patch enables autofs4 to work in a "container".  oz_pgrp is converted from
> > pid_t to struct pid and this is stored at mount time based on the "pgrp=" option
> > or if the option is missing then the current pgrp.
> > 
> > The "pgrp=" option is interpreted in the PID namespace of the current process.
> > This option is flawed in that it doesn't carry the namespace information, so it
> > should be deprecated.  AFAICS the autofs daemon always sends the current pgrp,
> > which is the default anyway.
> > 
> > The oz_pgrp is also set from the AUTOFS_DEV_IOCTL_SETPIPEFD_CMD ioctl.  This
> > ioctl sets oz_pgrp to the current pgrp.  It is not allowed to change the pid
> > namespace.
> > 
> > oz_pgrp is used mainly to determine whether the process traversing the autofs
> > mount tree is the autofs daemon itself or not.  This function now compares the
> > pid pointers instead of the pid_t values.
> > 
> > One other use of oz_pgrp is in autofs4_show_options.  There is shows the
> > virtual pid number (i.e. the one that is valid inside the PID namespace of the
> > calling process)
> > 
> > For debugging printk convert oz_pgrp to the value in the initial pid namespace.
> > 
> > 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 |   16 ++++++++++++++--
> >  fs/autofs4/inode.c     |   33 +++++++++++++++++++++++++--------
> >  3 files changed, 41 insertions(+), 12 deletions(-)
> > Index: linux-2.6/fs/autofs4/autofs_i.h
> > ===================================================================
> > --- linux-2.6.orig/fs/autofs4/autofs_i.h	2012-11-12 17:55:28.000000000 +0100
> > +++ linux-2.6/fs/autofs4/autofs_i.h	2012-11-12 19:09:40.000000000 +0100
> > @@ -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 *autofs
> >     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? */
> > Index: linux-2.6/fs/autofs4/inode.c
> > ===================================================================
> > --- linux-2.6.orig/fs/autofs4/inode.c	2012-11-12 17:55:28.000000000 +0100
> > +++ linux-2.6/fs/autofs4/inode.c	2012-11-12 19:09:35.000000000 +0100
> > @@ -62,6 +62,8 @@ void autofs4_kill_sb(struct super_block
> >  	/* 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 s
> >  		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_vnr(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)
> > +			 int *pgrp, bool *pgrp_set, 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,
> >  
> >  	*uid = current_uid();
> >  	*gid = current_gid();
> > -	*pgrp = task_pgrp_nr(current);
> >  
> >  	*minproto = AUTOFS_MIN_PROTO_VERSION;
> >  	*maxproto = AUTOFS_MAX_PROTO_VERSION;
> > @@ -170,6 +172,7 @@ static int parse_options(char *options,
> >  			if (match_int(args, &option))
> >  				return 1;
> >  			*pgrp = option;
> > +			*pgrp_set = true;
> >  			break;
> >  		case Opt_minproto:
> >  			if (match_int(args, &option))
> > @@ -205,6 +208,8 @@ int autofs4_fill_super(struct super_bloc
> >  	int pipefd;
> >  	struct autofs_sb_info *sbi;
> >  	struct autofs_info *ino;
> > +	int pgrp;
> > +	bool pgrp_set = false;
> >  
> >  	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> >  	if (!sbi)
> > @@ -217,7 +222,7 @@ int autofs4_fill_super(struct super_bloc
> >  	sbi->pipe = NULL;
> >  	sbi->catatonic = 1;
> >  	sbi->exp_timeout = 0;
> > -	sbi->oz_pgrp = task_pgrp_nr(current);
> > +	sbi->oz_pgrp = NULL;
> >  	sbi->sb = s;
> >  	sbi->version = 0;
> >  	sbi->sub_version = 0;
> > @@ -254,12 +259,23 @@ 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,
> > -				&sbi->max_proto)) {
> > +			  &pgrp, &pgrp_set, &sbi->type, &sbi->min_proto,
> > +			  &sbi->max_proto)) {
> >  		printk("autofs: called with bogus options\n");
> >  		goto fail_dput;
> >  	}
> >  
> > +	if (pgrp_set) {
> > +		sbi->oz_pgrp = find_get_pid(pgrp);
> > +		if (!sbi->oz_pgrp) {
> > +			pr_warn("autofs: could not find process group %d\n",
> > +				pgrp);
> > +			goto fail_dput;
> > +		}
> > +	} else {
> > +		sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
> > +	}
> > +
> >  	if (autofs_type_trigger(sbi->type))
> >  		__managed_dentry_set_managed(root);
> >  
> > @@ -283,9 +299,9 @@ 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, pid_nr(sbi->oz_pgrp));
> >  	pipe = fget(pipefd);
> > -	
> > +
> >  	if (!pipe) {
> >  		printk("autofs: could not open pipe file descriptor\n");
> >  		goto fail_dput;
> > @@ -315,6 +331,7 @@ int autofs4_fill_super(struct super_bloc
> >  fail_ino:
> >  	kfree(ino);
> >  fail_free:
> > +	put_pid(sbi->oz_pgrp);
> >  	kfree(sbi);
> >  	s->s_fs_info = NULL;
> >  fail_unlock:
> > Index: linux-2.6/fs/autofs4/dev-ioctl.c
> > ===================================================================
> > --- linux-2.6.orig/fs/autofs4/dev-ioctl.c	2012-11-12 17:55:28.000000000 +0100
> > +++ linux-2.6/fs/autofs4/dev-ioctl.c	2012-11-12 20:41:26.000000000 +0100
> > @@ -346,6 +346,7 @@ static int autofs_dev_ioctl_setpipefd(st
> >  {
> >  	int pipefd;
> >  	int err = 0;
> > +	struct pid *new_pid = NULL;
> >  
> >  	if (param->setpipefd.pipefd == -1)
> >  		return -EINVAL;
> > @@ -357,7 +358,17 @@ static int autofs_dev_ioctl_setpipefd(st
> >  		mutex_unlock(&sbi->wq_mutex);
> >  		return -EBUSY;
> >  	} else {
> > -		struct file *pipe = fget(pipefd);
> > +		struct file *pipe;
> > +
> > +		new_pid = get_task_pid(current, PIDTYPE_PGID);
> > +
> > +		if (ns_of_pid(new_pid) != ns_of_pid(sbi->oz_pgrp)) {
> > +			AUTOFS_WARN("Not allowed to change PID namespace");
> > +			err = -EINVAL;
> > +			goto out;
> > +		}
> > +
> > +		pipe = fget(pipefd);
> >  		if (!pipe) {
> >  			err = -EBADF;
> >  			goto out;
> > @@ -367,12 +378,13 @@ static int autofs_dev_ioctl_setpipefd(st
> >  			fput(pipe);
> >  			goto out;
> >  		}
> > -		sbi->oz_pgrp = task_pgrp_nr(current);
> > +		swap(sbi->oz_pgrp, new_pid);
> >  		sbi->pipefd = pipefd;
> >  		sbi->pipe = pipe;
> >  		sbi->catatonic = 0;
> >  	}
> >  out:
> > +	put_pid(new_pid);
> >  	mutex_unlock(&sbi->wq_mutex);
> >  	return err;
> >  }
> 

Re: [PATCH 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Miklos Szeredi]

Ian Kent <raven@themaw.net> writes:

> On Fri, 2012-11-23 at 11:45 +0800, Ian Kent wrote:
>> On Thu, 2012-11-22 at 17:24 +0100, Miklos Szeredi wrote:
>> > Patches were tested by the customer.
>> > 
>> > Ian, Eric, do these patches look OK?
>> 
>> They look OK to me but I'm still a bit concerned about changing the way
>> this behaves, but I also believe this is the way we want it to behave.
>
> OK, I ran the autofs Connectathon tests that I often use on a kernel
> with these patches and they worked fine. So, AFAICS. the patches
> shouldn't introduce regressions.

And the reason for that is the patches introduce no behavioral changes
at all if the automount daemon was started in the initial namespace.

They only change (and fix) semantics of the case when automount is
started in a cloned pid namespace.

>
>> 
>> Give me a little bit more time to run a simple test to ensure we can at
>> least do what we could previously, and that's nothing more than
>> umounting duplicated mounts (which probably shouldn't be duplicated at
>> all) in the container.
>
> Interestingly the simple container test program I have also worked in
> the same way it does on current kernels so again I didn't see a problem
> adding the patches.
>
> But I do have a couple of questions that are a little related.
>
> Calling clone(2) with flags
> CLONE_NEWPID|CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWIPC|SIGCHLD|CLONE_NEWNET
> will result in a copy of the existing set of mounts. The autofs mounts
> can be umounted if they are not needed.
>
> But, on Fedora systemd sets "/" as shared at boot which prevents the
> umount of these autofs mounts, unless you mark "/" as private in the
> clone, after which the mounts can be umounted.
>
> Does having "/" marked as shared in the root namespace mean that further
> mounts in the root namespace will also appear in the clone and that
> mounts done in the clone will appear in the root namespace?

Yes.

>
> Will mounting all autofs mounts with MS_PRIVATE prevent the autofs
> mounts and any mounts under them from appearing in the root namespace?

Changing autofs mounts to MS_PRIVATE will prevent submounts of these
from being propagated to/from the root namespace.

>
> I assume there is no way for autofs to stop the propagation from the
> root namespace, is that correct?

Not for the children of the shared mount.  For the children of the
autofs mounts, they can be prevented, which is really what you want, I
guess.

Thanks,
Miklos

Re: [PATCH 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Ian Kent]

On Fri, 2012-11-23 at 15:30 +0100, Miklos Szeredi wrote:
> Ian Kent <raven@themaw.net> writes:
> 
> > On Fri, 2012-11-23 at 11:45 +0800, Ian Kent wrote:
> >> On Thu, 2012-11-22 at 17:24 +0100, Miklos Szeredi wrote:
> >> > Patches were tested by the customer.
> >> > 
> >> > Ian, Eric, do these patches look OK?
> >> 
> >> They look OK to me but I'm still a bit concerned about changing the way
> >> this behaves, but I also believe this is the way we want it to behave.
> >
> > OK, I ran the autofs Connectathon tests that I often use on a kernel
> > with these patches and they worked fine. So, AFAICS. the patches
> > shouldn't introduce regressions.
> 
> And the reason for that is the patches introduce no behavioral changes
> at all if the automount daemon was started in the initial namespace.

Sure but I had to check.

> 
> They only change (and fix) semantics of the case when automount is
> started in a cloned pid namespace.
> 
> >
> >> 
> >> Give me a little bit more time to run a simple test to ensure we can at
> >> least do what we could previously, and that's nothing more than
> >> umounting duplicated mounts (which probably shouldn't be duplicated at
> >> all) in the container.
> >
> > Interestingly the simple container test program I have also worked in
> > the same way it does on current kernels so again I didn't see a problem
> > adding the patches.
> >
> > But I do have a couple of questions that are a little related.
> >
> > Calling clone(2) with flags
> > CLONE_NEWPID|CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWIPC|SIGCHLD|CLONE_NEWNET
> > will result in a copy of the existing set of mounts. The autofs mounts
> > can be umounted if they are not needed.
> >
> > But, on Fedora systemd sets "/" as shared at boot which prevents the
> > umount of these autofs mounts, unless you mark "/" as private in the
> > clone, after which the mounts can be umounted.
> >
> > Does having "/" marked as shared in the root namespace mean that further
> > mounts in the root namespace will also appear in the clone and that
> > mounts done in the clone will appear in the root namespace?
> 
> Yes.
> 
> >
> > Will mounting all autofs mounts with MS_PRIVATE prevent the autofs
> > mounts and any mounts under them from appearing in the root namespace?
> 
> Changing autofs mounts to MS_PRIVATE will prevent submounts of these
> from being propagated to/from the root namespace.

Right, but maybe I don't understand what you mean by the "from" here.

AFAICS autofs mounts mounted with MS_PRIVATE in the initial namespace do
propagate to the clone when it's created so I'm assuming subsequent
mounts would also. If these mounts are busy in some way they can't be
umounted in the clone unless "/" is marked private before attempting the
umount.

> 
> >
> > I assume there is no way for autofs to stop the propagation from the
> > root namespace, is that correct?
> 
> Not for the children of the shared mount.  For the children of the
> autofs mounts, they can be prevented, which is really what you want, I
> guess.

I think that's what is needed but it's only one direction.

It seems to me that people using containers and wanting to run an
instance of automount within them can't run automount in the initial
namespace if they wish to use systemd (or if "/" is marked shared for
some other reason). And from the discussion here it appears there really
isn't anything I can do within autofs to remove that restriction.

Ian

Re: [PATCH 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Ian Kent]

On Sat, 2012-11-24 at 10:23 +0800, Ian Kent wrote:
> On Fri, 2012-11-23 at 15:30 +0100, Miklos Szeredi wrote:
> > Ian Kent <raven@themaw.net> writes:
> > 
> > > On Fri, 2012-11-23 at 11:45 +0800, Ian Kent wrote:
> > >> On Thu, 2012-11-22 at 17:24 +0100, Miklos Szeredi wrote:
> > >> > Patches were tested by the customer.
> > >> > 
> > >> > Ian, Eric, do these patches look OK?
> > >> 
> > >> They look OK to me but I'm still a bit concerned about changing the way
> > >> this behaves, but I also believe this is the way we want it to behave.
> > >
> > > OK, I ran the autofs Connectathon tests that I often use on a kernel
> > > with these patches and they worked fine. So, AFAICS. the patches
> > > shouldn't introduce regressions.
> > 
> > And the reason for that is the patches introduce no behavioral changes
> > at all if the automount daemon was started in the initial namespace.
> 
> Sure but I had to check.
> 
> > 
> > They only change (and fix) semantics of the case when automount is
> > started in a cloned pid namespace.
> > 
> > >
> > >> 
> > >> Give me a little bit more time to run a simple test to ensure we can at
> > >> least do what we could previously, and that's nothing more than
> > >> umounting duplicated mounts (which probably shouldn't be duplicated at
> > >> all) in the container.
> > >
> > > Interestingly the simple container test program I have also worked in
> > > the same way it does on current kernels so again I didn't see a problem
> > > adding the patches.
> > >
> > > But I do have a couple of questions that are a little related.
> > >
> > > Calling clone(2) with flags
> > > CLONE_NEWPID|CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWIPC|SIGCHLD|CLONE_NEWNET
> > > will result in a copy of the existing set of mounts. The autofs mounts
> > > can be umounted if they are not needed.
> > >
> > > But, on Fedora systemd sets "/" as shared at boot which prevents the
> > > umount of these autofs mounts, unless you mark "/" as private in the
> > > clone, after which the mounts can be umounted.
> > >
> > > Does having "/" marked as shared in the root namespace mean that further
> > > mounts in the root namespace will also appear in the clone and that
> > > mounts done in the clone will appear in the root namespace?
> > 
> > Yes.
> > 
> > >
> > > Will mounting all autofs mounts with MS_PRIVATE prevent the autofs
> > > mounts and any mounts under them from appearing in the root namespace?
> > 
> > Changing autofs mounts to MS_PRIVATE will prevent submounts of these
> > from being propagated to/from the root namespace.
> 
> Right, but maybe I don't understand what you mean by the "from" here.
> 
> AFAICS autofs mounts mounted with MS_PRIVATE in the initial namespace do
> propagate to the clone when it's created so I'm assuming subsequent
> mounts would also. If these mounts are busy in some way they can't be
> umounted in the clone unless "/" is marked private before attempting the
> umount.

This may sound stupid but if there something like, say, MS_NOPROPAGATE
then the problem I see would pretty much just go away. No more need to
umount existing mounts and container instances would be isolated. But, I
guess, I'm not considering the possibility of cloned of processes as
well .... if that makes sense, ;)

Ian

Re: [PATCH 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Eric W. Biederman]

Ian Kent <raven@themaw.net> writes:

> On Sat, 2012-11-24 at 10:23 +0800, Ian Kent wrote:
>> On Fri, 2012-11-23 at 15:30 +0100, Miklos Szeredi wrote:
>> > Ian Kent <raven@themaw.net> writes:
>> > 
>> > > On Fri, 2012-11-23 at 11:45 +0800, Ian Kent wrote:
>> > >> On Thu, 2012-11-22 at 17:24 +0100, Miklos Szeredi wrote:
>> > >> > Patches were tested by the customer.
>> > >> > 
>> > >> > Ian, Eric, do these patches look OK?

My apologies for the delay.  I have been swamped with the holidays and
the impending 3.8 merge window.   I will take a good hard look at your
patches shortly.

>> AFAICS autofs mounts mounted with MS_PRIVATE in the initial namespace do
>> propagate to the clone when it's created so I'm assuming subsequent
>> mounts would also. If these mounts are busy in some way they can't be
>> umounted in the clone unless "/" is marked private before attempting the
>> umount.
>
> This may sound stupid but if there something like, say, MS_NOPROPAGATE
> then the problem I see would pretty much just go away. No more need to
> umount existing mounts and container instances would be isolated. But, I
> guess, I'm not considering the possibility of cloned of processes as
> well .... if that makes sense, ;)

Something is very weird is going on.  MS_PRIVATE should be the
MS_NOPROPOGATE you are looking for.  There is also MS_UNBINDABLE.
which is a stronger form of MS_PRIVATE and probably worth play with.

I would love to advertise my user namespace changes (queued for 3.8)
that reduce shared subtrees to slave subtress as the solution to this,
and that does address part of the issue but that does not really seem
like the fix.

I expect what we need to avoid unwanted mount propagation is an idiom
something like:

unshare -n
mount --private /mnt
pivot_root /mnt /
umount /mnt

Something like that is present in the startup of most containers
already.  So figuring out where to sprinkle MS_PRIVATE or MS_UNBINDABLE
so that mounts don't propogate that we want to propogate look like a
good deal.

Eric

Re: [PATCH 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Miklos Szeredi]

On Sat, Nov 24, 2012 at 1:07 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Ian Kent <raven@themaw.net> writes:
>
>> On Sat, 2012-11-24 at 10:23 +0800, Ian Kent wrote:
>>> On Fri, 2012-11-23 at 15:30 +0100, Miklos Szeredi wrote:

>>> AFAICS autofs mounts mounted with MS_PRIVATE in the initial namespace do
>>> propagate to the clone when it's created so I'm assuming subsequent
>>> mounts would also. If these mounts are busy in some way they can't be
>>> umounted in the clone unless "/" is marked private before attempting the
>>> umount.
>>
>> This may sound stupid but if there something like, say, MS_NOPROPAGATE
>> then the problem I see would pretty much just go away. No more need to
>> umount existing mounts and container instances would be isolated. But, I
>> guess, I'm not considering the possibility of cloned of processes as
>> well .... if that makes sense, ;)
>
> Something is very weird is going on.  MS_PRIVATE should be the
> MS_NOPROPOGATE you are looking for.  There is also MS_UNBINDABLE.
> which is a stronger form of MS_PRIVATE and probably worth play with.
>

MS_UNBINDABLE says:  skip this mount when copying a mount tree, such
as when the mount namespace is cloned.

If you set MS_UNBINDABLE on autofs mounts then they will simply not
appear in a cloned namespace.  Which sounds like a good idea,  no?

Thanks,
Miklos

Re: [PATCH 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Eric W. Biederman]

Miklos Szeredi <miklos@szeredi.hu> writes:

> On Sat, Nov 24, 2012 at 1:07 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>> Ian Kent <raven@themaw.net> writes:
>>
>>> On Sat, 2012-11-24 at 10:23 +0800, Ian Kent wrote:
>>>> On Fri, 2012-11-23 at 15:30 +0100, Miklos Szeredi wrote:
>
>>>> AFAICS autofs mounts mounted with MS_PRIVATE in the initial namespace do
>>>> propagate to the clone when it's created so I'm assuming subsequent
>>>> mounts would also. If these mounts are busy in some way they can't be
>>>> umounted in the clone unless "/" is marked private before attempting the
>>>> umount.

Subsequent mounts after the clone do not have a mechanism to propogate
with MS_PRIVATE.  As creating a new mount namespaces is essentially
an instance of mount --bind.  Those semantics are a little unintuitive
I have to admit.

>>> This may sound stupid but if there something like, say, MS_NOPROPAGATE
>>> then the problem I see would pretty much just go away. No more need to
>>> umount existing mounts and container instances would be isolated. But, I
>>> guess, I'm not considering the possibility of cloned of processes as
>>> well .... if that makes sense, ;)
>>
>> Something is very weird is going on.  MS_PRIVATE should be the
>> MS_NOPROPOGATE you are looking for.  There is also MS_UNBINDABLE.
>> which is a stronger form of MS_PRIVATE and probably worth play with.
>>
>
> MS_UNBINDABLE says:  skip this mount when copying a mount tree, such
> as when the mount namespace is cloned.
>
> If you set MS_UNBINDABLE on autofs mounts then they will simply not
> appear in a cloned namespace.  Which sounds like a good idea,  no?

Good point.  If the desire is for a mount to be managed by autofs
setting MS_UNBINDABLE seems required.

Eric

Re: [PATCH 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Ian Kent]

On Sat, 2012-11-24 at 14:35 -0800, Eric W. Biederman wrote:
> Miklos Szeredi <miklos@szeredi.hu> writes:
> 
> > On Sat, Nov 24, 2012 at 1:07 PM, Eric W. Biederman
> > <ebiederm@xmission.com> wrote:
> >> Ian Kent <raven@themaw.net> writes:
> >>
> >>> On Sat, 2012-11-24 at 10:23 +0800, Ian Kent wrote:
> >>>> On Fri, 2012-11-23 at 15:30 +0100, Miklos Szeredi wrote:
> >
> >>>> AFAICS autofs mounts mounted with MS_PRIVATE in the initial namespace do
> >>>> propagate to the clone when it's created so I'm assuming subsequent
> >>>> mounts would also. If these mounts are busy in some way they can't be
> >>>> umounted in the clone unless "/" is marked private before attempting the
> >>>> umount.
> 
> Subsequent mounts after the clone do not have a mechanism to propogate
> with MS_PRIVATE.  As creating a new mount namespaces is essentially
> an instance of mount --bind.  Those semantics are a little unintuitive
> I have to admit.
> 
> >>> This may sound stupid but if there something like, say, MS_NOPROPAGATE
> >>> then the problem I see would pretty much just go away. No more need to
> >>> umount existing mounts and container instances would be isolated. But, I
> >>> guess, I'm not considering the possibility of cloned of processes as
> >>> well .... if that makes sense, ;)
> >>
> >> Something is very weird is going on.  MS_PRIVATE should be the
> >> MS_NOPROPOGATE you are looking for.  There is also MS_UNBINDABLE.
> >> which is a stronger form of MS_PRIVATE and probably worth play with.
> >>
> >
> > MS_UNBINDABLE says:  skip this mount when copying a mount tree, such
> > as when the mount namespace is cloned.
> >
> > If you set MS_UNBINDABLE on autofs mounts then they will simply not
> > appear in a cloned namespace.  Which sounds like a good idea,  no?
> 
> Good point.  If the desire is for a mount to be managed by autofs
> setting MS_UNBINDABLE seems required.

Arrgh, I know that's something I should have looked into long ago.
The fact is that autofs mounts are directly related to a specific path
defined by automount maps that are associated with the daemon so bind
mounting them elsewhere makes no sense.

Is it necessary (or sensible) to use MS_PRIVATE with MS_UNBINDABLE?

> 
> Eric
> 

Re: [PATCH 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Ian Kent]

On Mon, 2012-11-26 at 07:25 +0800, Ian Kent wrote:
> On Sat, 2012-11-24 at 14:35 -0800, Eric W. Biederman wrote:
> > Miklos Szeredi <miklos@szeredi.hu> writes:
> > 
> > > On Sat, Nov 24, 2012 at 1:07 PM, Eric W. Biederman
> > > <ebiederm@xmission.com> wrote:
> > >> Ian Kent <raven@themaw.net> writes:
> > >>
> > >>> On Sat, 2012-11-24 at 10:23 +0800, Ian Kent wrote:
> > >>>> On Fri, 2012-11-23 at 15:30 +0100, Miklos Szeredi wrote:
> > >
> > >>>> AFAICS autofs mounts mounted with MS_PRIVATE in the initial namespace do
> > >>>> propagate to the clone when it's created so I'm assuming subsequent
> > >>>> mounts would also. If these mounts are busy in some way they can't be
> > >>>> umounted in the clone unless "/" is marked private before attempting the
> > >>>> umount.
> > 
> > Subsequent mounts after the clone do not have a mechanism to propogate
> > with MS_PRIVATE.  As creating a new mount namespaces is essentially
> > an instance of mount --bind.  Those semantics are a little unintuitive
> > I have to admit.
> > 
> > >>> This may sound stupid but if there something like, say, MS_NOPROPAGATE
> > >>> then the problem I see would pretty much just go away. No more need to
> > >>> umount existing mounts and container instances would be isolated. But, I
> > >>> guess, I'm not considering the possibility of cloned of processes as
> > >>> well .... if that makes sense, ;)
> > >>
> > >> Something is very weird is going on.  MS_PRIVATE should be the
> > >> MS_NOPROPOGATE you are looking for.  There is also MS_UNBINDABLE.
> > >> which is a stronger form of MS_PRIVATE and probably worth play with.
> > >>
> > >
> > > MS_UNBINDABLE says:  skip this mount when copying a mount tree, such
> > > as when the mount namespace is cloned.
> > >
> > > If you set MS_UNBINDABLE on autofs mounts then they will simply not
> > > appear in a cloned namespace.  Which sounds like a good idea,  no?
> > 
> > Good point.  If the desire is for a mount to be managed by autofs
> > setting MS_UNBINDABLE seems required.
> 
> Arrgh, I know that's something I should have looked into long ago.
> The fact is that autofs mounts are directly related to a specific path
> defined by automount maps that are associated with the daemon so bind
> mounting them elsewhere makes no sense.

Except, AFAICS, they do appear in the clone.

> 
> Is it necessary (or sensible) to use MS_PRIVATE with MS_UNBINDABLE?

and specifying MS_PRIVATE as well gives an EINVAL return.

> 
> > 
> > Eric
> > 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe autofs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Miklos Szeredi]

On Mon, Nov 26, 2012 at 3:29 AM, Ian Kent <raven@themaw.net> wrote:
>> > > MS_UNBINDABLE says:  skip this mount when copying a mount tree, such
>> > > as when the mount namespace is cloned.
>> > >
>> > > If you set MS_UNBINDABLE on autofs mounts then they will simply not
>> > > appear in a cloned namespace.  Which sounds like a good idea,  no?
>> >
>> > Good point.  If the desire is for a mount to be managed by autofs
>> > setting MS_UNBINDABLE seems required.
>>
>> Arrgh, I know that's something I should have looked into long ago.
>> The fact is that autofs mounts are directly related to a specific path
>> defined by automount maps that are associated with the daemon so bind
>> mounting them elsewhere makes no sense.
>
> Except, AFAICS, they do appear in the clone.

Hmm, yes, apparently the semantics of MS_UNBINDABLE only apply to
actual bind mounts not to namespace cloning. Even though the two
operations are closely related.  Not sure why this is so, but it is
probably not a good idea to change the semantics at this point.

Thanks,
Miklos

Re: [PATCH 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Eric W. Biederman]

Miklos Szeredi <miklos@szeredi.hu> writes:

> On Mon, Nov 26, 2012 at 3:29 AM, Ian Kent <raven@themaw.net> wrote:
>>> > > MS_UNBINDABLE says:  skip this mount when copying a mount tree, such
>>> > > as when the mount namespace is cloned.
>>> > >
>>> > > If you set MS_UNBINDABLE on autofs mounts then they will simply not
>>> > > appear in a cloned namespace.  Which sounds like a good idea,  no?
>>> >
>>> > Good point.  If the desire is for a mount to be managed by autofs
>>> > setting MS_UNBINDABLE seems required.
>>>
>>> Arrgh, I know that's something I should have looked into long ago.
>>> The fact is that autofs mounts are directly related to a specific path
>>> defined by automount maps that are associated with the daemon so bind
>>> mounting them elsewhere makes no sense.
>>
>> Except, AFAICS, they do appear in the clone.
>
> Hmm, yes, apparently the semantics of MS_UNBINDABLE only apply to
> actual bind mounts not to namespace cloning. Even though the two
> operations are closely related.  Not sure why this is so, but it is
> probably not a good idea to change the semantics at this point.

And for whatever reason this appears deliberate.

CL_COPY_ALL in copy_tree allows the copy.

The selected semantics of namespace sharing tend to mystify me.

So I don't know how much MS_UNBINDABLE helps over MS_PRIVATE.  Both
prevent propogation of changes to other namespaces.  I don't know how
much using MS_UNBINDABLE to also prevent bind mounts helps.

Eric

Re: [PATCH 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Miklos Szeredi]

On Mon, Nov 26, 2012 at 3:38 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> So I don't know how much MS_UNBINDABLE helps over MS_PRIVATE.  Both
> prevent propogation of changes to other namespaces.  I don't know how
> much using MS_UNBINDABLE to also prevent bind mounts helps.


MS_PRIVATE says ops on children (mount, unmount) won't be propageted,
MS_UNBINDABLE says the mount itself won't be copied.

I think it may make sense to introduce a new flag similar to
MS_UNBINDABLE that applies to namespace cloning as well as bind
mounting.  It says: this mount is managed by some userspace entity
(autofs) and copying it to the new namespace or to some other part of
the tree makes no sense since a new instance of the  userspace entity
responsible for managing those mounts will need to be started in the
new namespace as well.  I can't think of any other sane solution to
this issue.

Thanks,
Miklos

>
> Eric
>

[PATCH 1/2] autofs4: allow autofs to work outside the initial PID namespace

[Miklos Szeredi]

Gone over this issue once again, with a deeper understanding of what's
actually needed and updated the patches.

And I think these patches clearly fix the requirement of the customer:
autofs4 can be started in a "container" and it will be able to serve
mounts in that container.

If second patch makes sure that triggers from unrelated pid namespaces
are not honoured (because they can't be honoured properly).

Untested, for review only at this point.

Thanks,
Miklos


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

This patch enables autofs4 to work in a "container".  oz_pgrp is converted from
pid_t to struct pid and this is stored at mount time based on the "pgrp=" option
or if the option is missing then the current pgrp.

The "pgrp=" option is interpreted in the PID namespace of the current process.
This option is flawed in that it doesn't carry the namespace information, so it
should be deprecated.  AFAICS the autofs daemon always sends the current pgrp,
which is the default anyway.

The oz_pgrp is also set from the AUTOFS_DEV_IOCTL_SETPIPEFD_CMD ioctl.  This
ioctl sets oz_pgrp to the current pgrp.  It is not allowed to change the pid
namespace.

oz_pgrp is used mainly to determine whether the process traversing the autofs
mount tree is the autofs daemon itself or not.  This function now compares the
pid pointers instead of the pid_t values.

One other use of oz_pgrp is in autofs4_show_options.  There is shows the
virtual pid number (i.e. the one that is valid inside the PID namespace of the
calling process)

For debugging printk convert oz_pgrp to the value in the initial pid namespace.

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 |   16 ++++++++++++++--
 fs/autofs4/inode.c     |   33 +++++++++++++++++++++++++--------
 3 files changed, 41 insertions(+), 12 deletions(-)
Index: linux-2.6/fs/autofs4/autofs_i.h
===================================================================
--- linux-2.6.orig/fs/autofs4/autofs_i.h	2012-11-12 17:55:28.000000000 +0100
+++ linux-2.6/fs/autofs4/autofs_i.h	2012-11-12 19:09:40.000000000 +0100
@@ -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 *autofs
    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? */
Index: linux-2.6/fs/autofs4/inode.c
===================================================================
--- linux-2.6.orig/fs/autofs4/inode.c	2012-11-12 17:55:28.000000000 +0100
+++ linux-2.6/fs/autofs4/inode.c	2012-11-12 19:09:35.000000000 +0100
@@ -62,6 +62,8 @@ void autofs4_kill_sb(struct super_block
 	/* 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 s
 		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_vnr(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)
+			 int *pgrp, bool *pgrp_set, 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,
 
 	*uid = current_uid();
 	*gid = current_gid();
-	*pgrp = task_pgrp_nr(current);
 
 	*minproto = AUTOFS_MIN_PROTO_VERSION;
 	*maxproto = AUTOFS_MAX_PROTO_VERSION;
@@ -170,6 +172,7 @@ static int parse_options(char *options,
 			if (match_int(args, &option))
 				return 1;
 			*pgrp = option;
+			*pgrp_set = true;
 			break;
 		case Opt_minproto:
 			if (match_int(args, &option))
@@ -205,6 +208,8 @@ int autofs4_fill_super(struct super_bloc
 	int pipefd;
 	struct autofs_sb_info *sbi;
 	struct autofs_info *ino;
+	int pgrp;
+	bool pgrp_set = false;
 
 	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
 	if (!sbi)
@@ -217,7 +222,7 @@ int autofs4_fill_super(struct super_bloc
 	sbi->pipe = NULL;
 	sbi->catatonic = 1;
 	sbi->exp_timeout = 0;
-	sbi->oz_pgrp = task_pgrp_nr(current);
+	sbi->oz_pgrp = NULL;
 	sbi->sb = s;
 	sbi->version = 0;
 	sbi->sub_version = 0;
@@ -254,12 +259,23 @@ 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,
-				&sbi->max_proto)) {
+			  &pgrp, &pgrp_set, &sbi->type, &sbi->min_proto,
+			  &sbi->max_proto)) {
 		printk("autofs: called with bogus options\n");
 		goto fail_dput;
 	}
 
+	if (pgrp_set) {
+		sbi->oz_pgrp = find_get_pid(pgrp);
+		if (!sbi->oz_pgrp) {
+			pr_warn("autofs: could not find process group %d\n",
+				pgrp);
+			goto fail_dput;
+		}
+	} else {
+		sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
+	}
+
 	if (autofs_type_trigger(sbi->type))
 		__managed_dentry_set_managed(root);
 
@@ -283,9 +299,9 @@ 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, pid_nr(sbi->oz_pgrp));
 	pipe = fget(pipefd);
-	
+
 	if (!pipe) {
 		printk("autofs: could not open pipe file descriptor\n");
 		goto fail_dput;
@@ -315,6 +331,7 @@ int autofs4_fill_super(struct super_bloc
 fail_ino:
 	kfree(ino);
 fail_free:
+	put_pid(sbi->oz_pgrp);
 	kfree(sbi);
 	s->s_fs_info = NULL;
 fail_unlock:
Index: linux-2.6/fs/autofs4/dev-ioctl.c
===================================================================
--- linux-2.6.orig/fs/autofs4/dev-ioctl.c	2012-11-12 17:55:28.000000000 +0100
+++ linux-2.6/fs/autofs4/dev-ioctl.c	2012-11-12 20:41:26.000000000 +0100
@@ -346,6 +346,7 @@ static int autofs_dev_ioctl_setpipefd(st
 {
 	int pipefd;
 	int err = 0;
+	struct pid *new_pid = NULL;
 
 	if (param->setpipefd.pipefd == -1)
 		return -EINVAL;
@@ -357,7 +358,17 @@ static int autofs_dev_ioctl_setpipefd(st
 		mutex_unlock(&sbi->wq_mutex);
 		return -EBUSY;
 	} else {
-		struct file *pipe = fget(pipefd);
+		struct file *pipe;
+
+		new_pid = get_task_pid(current, PIDTYPE_PGID);
+
+		if (ns_of_pid(new_pid) != ns_of_pid(sbi->oz_pgrp)) {
+			AUTOFS_WARN("Not allowed to change PID namespace");
+			err = -EINVAL;
+			goto out;
+		}
+
+		pipe = fget(pipefd);
 		if (!pipe) {
 			err = -EBADF;
 			goto out;
@@ -367,12 +378,13 @@ static int autofs_dev_ioctl_setpipefd(st
 			fput(pipe);
 			goto out;
 		}
-		sbi->oz_pgrp = task_pgrp_nr(current);
+		swap(sbi->oz_pgrp, new_pid);
 		sbi->pipefd = pipefd;
 		sbi->pipe = pipe;
 		sbi->catatonic = 0;
 	}
 out:
+	put_pid(new_pid);
 	mutex_unlock(&sbi->wq_mutex);
 	return err;
 }