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

[Ian Kent <raven@themaw.net>]

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