Re: [PATCH 2/4] autofs4 - track uid and gid of last mount requester

Re: [PATCH 2/4] autofs4 - track uid and gid of last mount requester

[Andrew Morton]

On Thu, 07 Aug 2008 19:40:14 +0800
Ian Kent <raven@themaw.net> wrote:

> Patch to track the uid and gid of the last process to request a mount
> for on an autofs dentry.

pet peeve: changelog should not tell the reader that this is a "patch".
Because when someone is reading the changelog in the git repository,
they hopefully already know that.

> Signed-off-by: Ian Kent <raven@themaw.net>
> 
> ---
> 
>  fs/autofs4/autofs_i.h |    3 +++
>  fs/autofs4/inode.c    |    2 ++
>  fs/autofs4/waitq.c    |   34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 39 insertions(+), 0 deletions(-)
> 
> 
> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> index ea024d8..fa76d18 100644
> --- a/fs/autofs4/autofs_i.h
> +++ b/fs/autofs4/autofs_i.h
> @@ -63,6 +63,9 @@ struct autofs_info {
>  	unsigned long last_used;
>  	atomic_t count;
>  
> +	uid_t uid;
> +	gid_t gid;
> +
>  	mode_t	mode;
>  	size_t	size;
>  
> diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
> index 9ca2d07..9408507 100644
> --- a/fs/autofs4/inode.c
> +++ b/fs/autofs4/inode.c
> @@ -53,6 +53,8 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
>  		atomic_set(&ino->count, 0);
>  	}
>  
> +	ino->uid = 0;
> +	ino->gid = 0;
>  	ino->mode = mode;
>  	ino->last_used = jiffies;
>  
> diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> index 6d87bb1..7c60c0b 100644
> --- a/fs/autofs4/waitq.c
> +++ b/fs/autofs4/waitq.c
> @@ -457,6 +457,40 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
>  
>  	status = wq->status;
>  
> +	/*
> +	 * For direct and offset mounts we need to track the requestrer

typo which I'll fix.

> +	 * uid and gid in the dentry info struct. This is so it can be
> +	 * supplied, on request, by the misc device ioctl interface.
> +	 * This is needed during daemon resatart when reconnecting
> +	 * to existing, active, autofs mounts. The uid and gid (and
> +	 * related string values) may be used for macro substitution
> +	 * in autofs mount maps.
> +	 */
> +	if (!status) {
> +		struct autofs_info *ino;
> +		struct dentry *de = NULL;
> +
> +		/* direct mount or browsable map */
> +		ino = autofs4_dentry_ino(dentry);
> +		if (!ino) {
> +			/* If not lookup actual dentry used */
> +			de = d_lookup(dentry->d_parent, &dentry->d_name);
> +			if (de)
> +				ino = autofs4_dentry_ino(de);
> +		}
> +
> +		/* Set mount requester */
> +		if (ino) {
> +			spin_lock(&sbi->fs_lock);
> +			ino->uid = wq->uid;
> +			ino->gid = wq->gid;
> +			spin_unlock(&sbi->fs_lock);
> +		}
> +
> +		if (de)
> +			dput(de);
> +	}
> +

Please remind me again why autofs's use of current->uid and
current->gid is not busted in the presence of PID namespaces, where
these things are no longer system-wide unique?

Re: [PATCH 2/4] autofs4 - track uid and gid of last mount requester

[Serge E. Hallyn]

Quoting Andrew Morton (akpm@linux-foundation.org):
> On Thu, 07 Aug 2008 19:40:14 +0800
> Ian Kent <raven@themaw.net> wrote:
> 
> > Patch to track the uid and gid of the last process to request a mount
> > for on an autofs dentry.
> 
> pet peeve: changelog should not tell the reader that this is a "patch".
> Because when someone is reading the changelog in the git repository,
> they hopefully already know that.
> 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > 
> > ---
> > 
> >  fs/autofs4/autofs_i.h |    3 +++
> >  fs/autofs4/inode.c    |    2 ++
> >  fs/autofs4/waitq.c    |   34 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 39 insertions(+), 0 deletions(-)
> > 
> > 
> > diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> > index ea024d8..fa76d18 100644
> > --- a/fs/autofs4/autofs_i.h
> > +++ b/fs/autofs4/autofs_i.h
> > @@ -63,6 +63,9 @@ struct autofs_info {
> >  	unsigned long last_used;
> >  	atomic_t count;
> >  
> > +	uid_t uid;
> > +	gid_t gid;
> > +
> >  	mode_t	mode;
> >  	size_t	size;
> >  
> > diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
> > index 9ca2d07..9408507 100644
> > --- a/fs/autofs4/inode.c
> > +++ b/fs/autofs4/inode.c
> > @@ -53,6 +53,8 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
> >  		atomic_set(&ino->count, 0);
> >  	}
> >  
> > +	ino->uid = 0;
> > +	ino->gid = 0;
> >  	ino->mode = mode;
> >  	ino->last_used = jiffies;
> >  
> > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> > index 6d87bb1..7c60c0b 100644
> > --- a/fs/autofs4/waitq.c
> > +++ b/fs/autofs4/waitq.c
> > @@ -457,6 +457,40 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
> >  
> >  	status = wq->status;
> >  
> > +	/*
> > +	 * For direct and offset mounts we need to track the requestrer
> 
> typo which I'll fix.
> 
> > +	 * uid and gid in the dentry info struct. This is so it can be
> > +	 * supplied, on request, by the misc device ioctl interface.
> > +	 * This is needed during daemon resatart when reconnecting
> > +	 * to existing, active, autofs mounts. The uid and gid (and
> > +	 * related string values) may be used for macro substitution
> > +	 * in autofs mount maps.
> > +	 */
> > +	if (!status) {
> > +		struct autofs_info *ino;
> > +		struct dentry *de = NULL;
> > +
> > +		/* direct mount or browsable map */
> > +		ino = autofs4_dentry_ino(dentry);
> > +		if (!ino) {
> > +			/* If not lookup actual dentry used */
> > +			de = d_lookup(dentry->d_parent, &dentry->d_name);
> > +			if (de)
> > +				ino = autofs4_dentry_ino(de);
> > +		}
> > +
> > +		/* Set mount requester */
> > +		if (ino) {
> > +			spin_lock(&sbi->fs_lock);
> > +			ino->uid = wq->uid;
> > +			ino->gid = wq->gid;
> > +			spin_unlock(&sbi->fs_lock);
> > +		}
> > +
> > +		if (de)
> > +			dput(de);
> > +	}
> > +
> 
> Please remind me again why autofs's use of current->uid and
> current->gid is not busted in the presence of PID namespaces, where
> these things are no longer system-wide unique?

I actually don't see what the autofs4_waitq->pid is used for.  It's
copied from current into wq->pid at autofs4_wait, and into a packet to
send to userspace (I assume) at autofs4_notify_daemon.

So as long as a daemon can serve multiple pid namespaces (which
doubtless it can), the pid could be confusing (or erroneous) for the
daemon.

If I'm remotely right about how the pid is being used, then the thing to
do would be to 
	1. store the daemon's pid namespace  (would that belong in
	the autofs_sb_info?)
	2. store the task_pid(current) in the waitqueue
	3. retrieve the pid_t for the waiting task in the daemon's
	pid namespace, and put that into the packet at
	autofs4_notify_daemon.

I realize this patch was about the *uids*, but the pids seem more
urgent.

-serge

Re: [PATCH 2/4] autofs4 - track uid and gid of last mount requester

[Serge E. Hallyn]

Quoting Andrew Morton (akpm@linux-foundation.org):
> On Thu, 07 Aug 2008 19:40:14 +0800
> Ian Kent <raven@themaw.net> wrote:
> 
> > Patch to track the uid and gid of the last process to request a mount
> > for on an autofs dentry.
> 
> pet peeve: changelog should not tell the reader that this is a "patch".
> Because when someone is reading the changelog in the git repository,
> they hopefully already know that.
> 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > 
> > ---
> > 
> >  fs/autofs4/autofs_i.h |    3 +++
> >  fs/autofs4/inode.c    |    2 ++
> >  fs/autofs4/waitq.c    |   34 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 39 insertions(+), 0 deletions(-)
> > 
> > 
> > diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> > index ea024d8..fa76d18 100644
> > --- a/fs/autofs4/autofs_i.h
> > +++ b/fs/autofs4/autofs_i.h
> > @@ -63,6 +63,9 @@ struct autofs_info {
> >  	unsigned long last_used;
> >  	atomic_t count;
> >  
> > +	uid_t uid;
> > +	gid_t gid;
> > +
> >  	mode_t	mode;
> >  	size_t	size;
> >  
> > diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
> > index 9ca2d07..9408507 100644
> > --- a/fs/autofs4/inode.c
> > +++ b/fs/autofs4/inode.c
> > @@ -53,6 +53,8 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
> >  		atomic_set(&ino->count, 0);
> >  	}
> >  
> > +	ino->uid = 0;
> > +	ino->gid = 0;
> >  	ino->mode = mode;
> >  	ino->last_used = jiffies;
> >  
> > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> > index 6d87bb1..7c60c0b 100644
> > --- a/fs/autofs4/waitq.c
> > +++ b/fs/autofs4/waitq.c
> > @@ -457,6 +457,40 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
> >  
> >  	status = wq->status;
> >  
> > +	/*
> > +	 * For direct and offset mounts we need to track the requestrer
> 
> typo which I'll fix.
> 
> > +	 * uid and gid in the dentry info struct. This is so it can be
> > +	 * supplied, on request, by the misc device ioctl interface.
> > +	 * This is needed during daemon resatart when reconnecting
> > +	 * to existing, active, autofs mounts. The uid and gid (and
> > +	 * related string values) may be used for macro substitution
> > +	 * in autofs mount maps.

Hi Ian,

could you say just a few more words on these macro substitution?

I think your use of uids can completely ignore user namespaces, but it
depends on who does the macro substitutions and how...

thanks,
-serge

> > +	 */
> > +	if (!status) {
> > +		struct autofs_info *ino;
> > +		struct dentry *de = NULL;
> > +
> > +		/* direct mount or browsable map */
> > +		ino = autofs4_dentry_ino(dentry);
> > +		if (!ino) {
> > +			/* If not lookup actual dentry used */
> > +			de = d_lookup(dentry->d_parent, &dentry->d_name);
> > +			if (de)
> > +				ino = autofs4_dentry_ino(de);
> > +		}
> > +
> > +		/* Set mount requester */
> > +		if (ino) {
> > +			spin_lock(&sbi->fs_lock);
> > +			ino->uid = wq->uid;
> > +			ino->gid = wq->gid;
> > +			spin_unlock(&sbi->fs_lock);
> > +		}
> > +
> > +		if (de)
> > +			dput(de);
> > +	}
> > +
> 
> Please remind me again why autofs's use of current->uid and
> current->gid is not busted in the presence of PID namespaces, where
> these things are no longer system-wide unique?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] autofs4 - track uid and gid of last mount requester

[Ian Kent]

On Thu, 2008-08-07 at 17:15 -0500, Serge E. Hallyn wrote:
> Quoting Andrew Morton (akpm@linux-foundation.org):
> > On Thu, 07 Aug 2008 19:40:14 +0800
> > Ian Kent <raven@themaw.net> wrote:
> > 
> > > Patch to track the uid and gid of the last process to request a mount
> > > for on an autofs dentry.
> > 
> > pet peeve: changelog should not tell the reader that this is a "patch".
> > Because when someone is reading the changelog in the git repository,
> > they hopefully already know that.
> > 
> > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > 
> > > ---
> > > 
> > >  fs/autofs4/autofs_i.h |    3 +++
> > >  fs/autofs4/inode.c    |    2 ++
> > >  fs/autofs4/waitq.c    |   34 ++++++++++++++++++++++++++++++++++
> > >  3 files changed, 39 insertions(+), 0 deletions(-)
> > > 
> > > 
> > > diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> > > index ea024d8..fa76d18 100644
> > > --- a/fs/autofs4/autofs_i.h
> > > +++ b/fs/autofs4/autofs_i.h
> > > @@ -63,6 +63,9 @@ struct autofs_info {
> > >  	unsigned long last_used;
> > >  	atomic_t count;
> > >  
> > > +	uid_t uid;
> > > +	gid_t gid;
> > > +
> > >  	mode_t	mode;
> > >  	size_t	size;
> > >  
> > > diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
> > > index 9ca2d07..9408507 100644
> > > --- a/fs/autofs4/inode.c
> > > +++ b/fs/autofs4/inode.c
> > > @@ -53,6 +53,8 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
> > >  		atomic_set(&ino->count, 0);
> > >  	}
> > >  
> > > +	ino->uid = 0;
> > > +	ino->gid = 0;
> > >  	ino->mode = mode;
> > >  	ino->last_used = jiffies;
> > >  
> > > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> > > index 6d87bb1..7c60c0b 100644
> > > --- a/fs/autofs4/waitq.c
> > > +++ b/fs/autofs4/waitq.c
> > > @@ -457,6 +457,40 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
> > >  
> > >  	status = wq->status;
> > >  
> > > +	/*
> > > +	 * For direct and offset mounts we need to track the requestrer
> > 
> > typo which I'll fix.
> > 
> > > +	 * uid and gid in the dentry info struct. This is so it can be
> > > +	 * supplied, on request, by the misc device ioctl interface.
> > > +	 * This is needed during daemon resatart when reconnecting
> > > +	 * to existing, active, autofs mounts. The uid and gid (and
> > > +	 * related string values) may be used for macro substitution
> > > +	 * in autofs mount maps.
> 
> Hi Ian,
> 
> could you say just a few more words on these macro substitution?
> 
> I think your use of uids can completely ignore user namespaces, but it
> depends on who does the macro substitutions and how...

Suppose we have an autofs map entry in /etc/auto.master:
/test   /etc/auto.test

and in /etc/auto.test

im1  /om1 server:/dir1 \
     /om2/$USER server2:/dir2

Then if this is automounted and the daemon is sent a SIGKILL and started
again we need the uid number that was used when this was mounted to
re-construct the offsets (/om2/$USER in this case needs the
substitution). The uid is used to lookup the user name.

Can you point me in the right direction wrt. to namespaces on this
please Serge? We didn't really get to the bottom of it last time I
think.

> 
> thanks,
> -serge
> 
> > > +	 */
> > > +	if (!status) {
> > > +		struct autofs_info *ino;
> > > +		struct dentry *de = NULL;
> > > +
> > > +		/* direct mount or browsable map */
> > > +		ino = autofs4_dentry_ino(dentry);
> > > +		if (!ino) {
> > > +			/* If not lookup actual dentry used */
> > > +			de = d_lookup(dentry->d_parent, &dentry->d_name);
> > > +			if (de)
> > > +				ino = autofs4_dentry_ino(de);
> > > +		}
> > > +
> > > +		/* Set mount requester */
> > > +		if (ino) {
> > > +			spin_lock(&sbi->fs_lock);
> > > +			ino->uid = wq->uid;
> > > +			ino->gid = wq->gid;
> > > +			spin_unlock(&sbi->fs_lock);
> > > +		}
> > > +
> > > +		if (de)
> > > +			dput(de);
> > > +	}
> > > +
> > 
> > Please remind me again why autofs's use of current->uid and
> > current->gid is not busted in the presence of PID namespaces, where
> > these things are no longer system-wide unique?
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] autofs4 - track uid and gid of last mount requester

[Ian Kent]

On Thu, 2008-08-07 at 13:46 -0700, Andrew Morton wrote:
> On Thu, 07 Aug 2008 19:40:14 +0800
> Ian Kent <raven@themaw.net> wrote:
> 
> > Patch to track the uid and gid of the last process to request a mount
> > for on an autofs dentry.
> 
> pet peeve: changelog should not tell the reader that this is a "patch".
> Because when someone is reading the changelog in the git repository,
> they hopefully already know that.
> 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > 
> > ---
> > 
> >  fs/autofs4/autofs_i.h |    3 +++
> >  fs/autofs4/inode.c    |    2 ++
> >  fs/autofs4/waitq.c    |   34 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 39 insertions(+), 0 deletions(-)
> > 
> > 
> > diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> > index ea024d8..fa76d18 100644
> > --- a/fs/autofs4/autofs_i.h
> > +++ b/fs/autofs4/autofs_i.h
> > @@ -63,6 +63,9 @@ struct autofs_info {
> >  	unsigned long last_used;
> >  	atomic_t count;
> >  
> > +	uid_t uid;
> > +	gid_t gid;
> > +
> >  	mode_t	mode;
> >  	size_t	size;
> >  
> > diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
> > index 9ca2d07..9408507 100644
> > --- a/fs/autofs4/inode.c
> > +++ b/fs/autofs4/inode.c
> > @@ -53,6 +53,8 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
> >  		atomic_set(&ino->count, 0);
> >  	}
> >  
> > +	ino->uid = 0;
> > +	ino->gid = 0;
> >  	ino->mode = mode;
> >  	ino->last_used = jiffies;
> >  
> > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> > index 6d87bb1..7c60c0b 100644
> > --- a/fs/autofs4/waitq.c
> > +++ b/fs/autofs4/waitq.c
> > @@ -457,6 +457,40 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
> >  
> >  	status = wq->status;
> >  
> > +	/*
> > +	 * For direct and offset mounts we need to track the requestrer
> 
> typo which I'll fix.
> 
> > +	 * uid and gid in the dentry info struct. This is so it can be
> > +	 * supplied, on request, by the misc device ioctl interface.
> > +	 * This is needed during daemon resatart when reconnecting
> > +	 * to existing, active, autofs mounts. The uid and gid (and
> > +	 * related string values) may be used for macro substitution
> > +	 * in autofs mount maps.
> > +	 */
> > +	if (!status) {
> > +		struct autofs_info *ino;
> > +		struct dentry *de = NULL;
> > +
> > +		/* direct mount or browsable map */
> > +		ino = autofs4_dentry_ino(dentry);
> > +		if (!ino) {
> > +			/* If not lookup actual dentry used */
> > +			de = d_lookup(dentry->d_parent, &dentry->d_name);
> > +			if (de)
> > +				ino = autofs4_dentry_ino(de);
> > +		}
> > +
> > +		/* Set mount requester */
> > +		if (ino) {
> > +			spin_lock(&sbi->fs_lock);
> > +			ino->uid = wq->uid;
> > +			ino->gid = wq->gid;
> > +			spin_unlock(&sbi->fs_lock);
> > +		}
> > +
> > +		if (de)
> > +			dput(de);
> > +	}
> > +
> 
> Please remind me again why autofs's use of current->uid and
> current->gid is not busted in the presence of PID namespaces, where
> these things are no longer system-wide unique?

We didn't really get to the bottom of that last time I posted this but
hopefully Serge will get right on this and we can sort out what, idf
anything I need to do.

Ian

Re: [PATCH 2/4] autofs4 - track uid and gid of last mount requester

[Ian Kent]

On Thu, 2008-08-07 at 17:12 -0500, Serge E. Hallyn wrote:
> Quoting Andrew Morton (akpm@linux-foundation.org):
> > On Thu, 07 Aug 2008 19:40:14 +0800
> > Ian Kent <raven@themaw.net> wrote:
> > 
> > > Patch to track the uid and gid of the last process to request a mount
> > > for on an autofs dentry.
> > 
> > pet peeve: changelog should not tell the reader that this is a "patch".
> > Because when someone is reading the changelog in the git repository,
> > they hopefully already know that.
> > 
> > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > 
> > > ---
> > > 
> > >  fs/autofs4/autofs_i.h |    3 +++
> > >  fs/autofs4/inode.c    |    2 ++
> > >  fs/autofs4/waitq.c    |   34 ++++++++++++++++++++++++++++++++++
> > >  3 files changed, 39 insertions(+), 0 deletions(-)
> > > 
> > > 
> > > diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> > > index ea024d8..fa76d18 100644
> > > --- a/fs/autofs4/autofs_i.h
> > > +++ b/fs/autofs4/autofs_i.h
> > > @@ -63,6 +63,9 @@ struct autofs_info {
> > >  	unsigned long last_used;
> > >  	atomic_t count;
> > >  
> > > +	uid_t uid;
> > > +	gid_t gid;
> > > +
> > >  	mode_t	mode;
> > >  	size_t	size;
> > >  
> > > diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
> > > index 9ca2d07..9408507 100644
> > > --- a/fs/autofs4/inode.c
> > > +++ b/fs/autofs4/inode.c
> > > @@ -53,6 +53,8 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
> > >  		atomic_set(&ino->count, 0);
> > >  	}
> > >  
> > > +	ino->uid = 0;
> > > +	ino->gid = 0;
> > >  	ino->mode = mode;
> > >  	ino->last_used = jiffies;
> > >  
> > > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> > > index 6d87bb1..7c60c0b 100644
> > > --- a/fs/autofs4/waitq.c
> > > +++ b/fs/autofs4/waitq.c
> > > @@ -457,6 +457,40 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
> > >  
> > >  	status = wq->status;
> > >  
> > > +	/*
> > > +	 * For direct and offset mounts we need to track the requestrer
> > 
> > typo which I'll fix.
> > 
> > > +	 * uid and gid in the dentry info struct. This is so it can be
> > > +	 * supplied, on request, by the misc device ioctl interface.
> > > +	 * This is needed during daemon resatart when reconnecting
> > > +	 * to existing, active, autofs mounts. The uid and gid (and
> > > +	 * related string values) may be used for macro substitution
> > > +	 * in autofs mount maps.
> > > +	 */
> > > +	if (!status) {
> > > +		struct autofs_info *ino;
> > > +		struct dentry *de = NULL;
> > > +
> > > +		/* direct mount or browsable map */
> > > +		ino = autofs4_dentry_ino(dentry);
> > > +		if (!ino) {
> > > +			/* If not lookup actual dentry used */
> > > +			de = d_lookup(dentry->d_parent, &dentry->d_name);
> > > +			if (de)
> > > +				ino = autofs4_dentry_ino(de);
> > > +		}
> > > +
> > > +		/* Set mount requester */
> > > +		if (ino) {
> > > +			spin_lock(&sbi->fs_lock);
> > > +			ino->uid = wq->uid;
> > > +			ino->gid = wq->gid;
> > > +			spin_unlock(&sbi->fs_lock);
> > > +		}
> > > +
> > > +		if (de)
> > > +			dput(de);
> > > +	}
> > > +
> > 
> > Please remind me again why autofs's use of current->uid and
> > current->gid is not busted in the presence of PID namespaces, where
> > these things are no longer system-wide unique?
> 
> I actually don't see what the autofs4_waitq->pid is used for.  It's
> copied from current into wq->pid at autofs4_wait, and into a packet to
> send to userspace (I assume) at autofs4_notify_daemon.
> 
> So as long as a daemon can serve multiple pid namespaces (which
> doubtless it can), the pid could be confusing (or erroneous) for the
> daemon.

Your point is well taken.

The pid is used purely for logging purposes to aid in debugging in user
space. I'm not sure it is worth worrying about it too much as the daemon
has no business interfering with user space processes it is not the
owner of.

> 
> If I'm remotely right about how the pid is being used, then the thing to
> do would be to 
> 	1. store the daemon's pid namespace  (would that belong in
> 	the autofs_sb_info?)
 
Yep.

> 	2. store the task_pid(current) in the waitqueue
> 	3. retrieve the pid_t for the waiting task in the daemon's
> 	pid namespace, and put that into the packet at
> 	autofs4_notify_daemon.
> 
> I realize this patch was about the *uids*, but the pids seem more
> urgent.

OK, I get it.
I'll have a go at doing this for completeness.

Ian

Re: [PATCH 2/4] autofs4 - track uid and gid of last mount requester

[Ian Kent]

On Fri, 2008-08-08 at 11:48 +0800, Ian Kent wrote:
> > > 
> > > Please remind me again why autofs's use of current->uid and
> > > current->gid is not busted in the presence of PID namespaces, where
> > > these things are no longer system-wide unique?
> > 
> > I actually don't see what the autofs4_waitq->pid is used for.  It's
> > copied from current into wq->pid at autofs4_wait, and into a packet to
> > send to userspace (I assume) at autofs4_notify_daemon.
> > 
> > So as long as a daemon can serve multiple pid namespaces (which
> > doubtless it can), the pid could be confusing (or erroneous) for the
> > daemon.
> 
> Your point is well taken.
> 
> The pid is used purely for logging purposes to aid in debugging in user
> space. I'm not sure it is worth worrying about it too much as the daemon
> has no business interfering with user space processes it is not the
> owner of.
> 
> > 
> > If I'm remotely right about how the pid is being used, then the thing to
> > do would be to 
> > 	1. store the daemon's pid namespace  (would that belong in
> > 	the autofs_sb_info?)
>  
> Yep.
> 
> > 	2. store the task_pid(current) in the waitqueue
> > 	3. retrieve the pid_t for the waiting task in the daemon's
> > 	pid namespace, and put that into the packet at
> > 	autofs4_notify_daemon.
> > 
> > I realize this patch was about the *uids*, but the pids seem more
> > urgent.
> 
> OK, I get it.
> I'll have a go at doing this for completeness.

On second thoughts I'm not sure about this.

The pid that is logged needs to relate to a process in the name space of
the one that caused the mount to be done.

For example, suppose a GUI user finds mounts never expiring, then we get
a debug log to try and identify the culprit. So the pid should
correspond to a process that the user sees (So I guess in the namespace
of that user).

This is the only reason I added the pid to the request packet in the
first place.

Please correct me if my understanding of this is not right.

Ian


> 
> Ian
> 

Re: [PATCH 2/4] autofs4 - track uid and gid of last mount requester

[Ian Kent]

On Fri, 2008-08-08 at 11:25 +0800, Ian Kent wrote:
> On Thu, 2008-08-07 at 13:46 -0700, Andrew Morton wrote:
> > On Thu, 07 Aug 2008 19:40:14 +0800
> > Ian Kent <raven@themaw.net> wrote:
> > 
> > > Patch to track the uid and gid of the last process to request a mount
> > > for on an autofs dentry.
> > 
> > pet peeve: changelog should not tell the reader that this is a "patch".
> > Because when someone is reading the changelog in the git repository,
> > they hopefully already know that.
> > 
> > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > 
> > > ---
> > > 
> > >  fs/autofs4/autofs_i.h |    3 +++
> > >  fs/autofs4/inode.c    |    2 ++
> > >  fs/autofs4/waitq.c    |   34 ++++++++++++++++++++++++++++++++++
> > >  3 files changed, 39 insertions(+), 0 deletions(-)
> > > 
> > > 
> > > diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> > > index ea024d8..fa76d18 100644
> > > --- a/fs/autofs4/autofs_i.h
> > > +++ b/fs/autofs4/autofs_i.h
> > > @@ -63,6 +63,9 @@ struct autofs_info {
> > >  	unsigned long last_used;
> > >  	atomic_t count;
> > >  
> > > +	uid_t uid;
> > > +	gid_t gid;
> > > +
> > >  	mode_t	mode;
> > >  	size_t	size;
> > >  
> > > diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
> > > index 9ca2d07..9408507 100644
> > > --- a/fs/autofs4/inode.c
> > > +++ b/fs/autofs4/inode.c
> > > @@ -53,6 +53,8 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
> > >  		atomic_set(&ino->count, 0);
> > >  	}
> > >  
> > > +	ino->uid = 0;
> > > +	ino->gid = 0;
> > >  	ino->mode = mode;
> > >  	ino->last_used = jiffies;
> > >  
> > > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> > > index 6d87bb1..7c60c0b 100644
> > > --- a/fs/autofs4/waitq.c
> > > +++ b/fs/autofs4/waitq.c
> > > @@ -457,6 +457,40 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
> > >  
> > >  	status = wq->status;
> > >  
> > > +	/*
> > > +	 * For direct and offset mounts we need to track the requestrer
> > 
> > typo which I'll fix.
> > 
> > > +	 * uid and gid in the dentry info struct. This is so it can be
> > > +	 * supplied, on request, by the misc device ioctl interface.
> > > +	 * This is needed during daemon resatart when reconnecting
> > > +	 * to existing, active, autofs mounts. The uid and gid (and
> > > +	 * related string values) may be used for macro substitution
> > > +	 * in autofs mount maps.
> > > +	 */
> > > +	if (!status) {
> > > +		struct autofs_info *ino;
> > > +		struct dentry *de = NULL;
> > > +
> > > +		/* direct mount or browsable map */
> > > +		ino = autofs4_dentry_ino(dentry);
> > > +		if (!ino) {
> > > +			/* If not lookup actual dentry used */
> > > +			de = d_lookup(dentry->d_parent, &dentry->d_name);
> > > +			if (de)
> > > +				ino = autofs4_dentry_ino(de);
> > > +		}
> > > +
> > > +		/* Set mount requester */
> > > +		if (ino) {
> > > +			spin_lock(&sbi->fs_lock);
> > > +			ino->uid = wq->uid;
> > > +			ino->gid = wq->gid;
> > > +			spin_unlock(&sbi->fs_lock);
> > > +		}
> > > +
> > > +		if (de)
> > > +			dput(de);
> > > +	}
> > > +
> > 
> > Please remind me again why autofs's use of current->uid and
> > current->gid is not busted in the presence of PID namespaces, where
> > these things are no longer system-wide unique?
> 
> We didn't really get to the bottom of that last time I posted this but
> hopefully Serge will get right on this and we can sort out what, idf
> anything I need to do.

But, at this stage, I believe that if namespaces are being used then
there would also need to be a daemon within the container which would
have the same namespace view as other processes in the container. I
expect that the situation is not nearly as straight forward as I think
so I'll need to wait for further comments.

Ian

Re: [PATCH 2/4] autofs4 - track uid and gid of last mount requester

[Serge E. Hallyn]

Quoting Ian Kent (raven@themaw.net):
> 
> On Fri, 2008-08-08 at 11:48 +0800, Ian Kent wrote:
> > > > 
> > > > Please remind me again why autofs's use of current->uid and
> > > > current->gid is not busted in the presence of PID namespaces, where
> > > > these things are no longer system-wide unique?
> > > 
> > > I actually don't see what the autofs4_waitq->pid is used for.  It's
> > > copied from current into wq->pid at autofs4_wait, and into a packet to
> > > send to userspace (I assume) at autofs4_notify_daemon.
> > > 
> > > So as long as a daemon can serve multiple pid namespaces (which
> > > doubtless it can), the pid could be confusing (or erroneous) for the
> > > daemon.
> > 
> > Your point is well taken.
> > 
> > The pid is used purely for logging purposes to aid in debugging in user
> > space. I'm not sure it is worth worrying about it too much as the daemon
> > has no business interfering with user space processes it is not the
> > owner of.
> > 
> > > 
> > > If I'm remotely right about how the pid is being used, then the thing to
> > > do would be to 
> > > 	1. store the daemon's pid namespace  (would that belong in
> > > 	the autofs_sb_info?)
> >  
> > Yep.
> > 
> > > 	2. store the task_pid(current) in the waitqueue
> > > 	3. retrieve the pid_t for the waiting task in the daemon's
> > > 	pid namespace, and put that into the packet at
> > > 	autofs4_notify_daemon.
> > > 
> > > I realize this patch was about the *uids*, but the pids seem more
> > > urgent.
> > 
> > OK, I get it.
> > I'll have a go at doing this for completeness.
> 
> On second thoughts I'm not sure about this.
> 
> The pid that is logged needs to relate to a process in the name space of
> the one that caused the mount to be done.
> 
> For example, suppose a GUI user finds mounts never expiring, then we get
> a debug log to try and identify the culprit. So the pid should
> correspond to a process that the user sees (So I guess in the namespace
> of that user).
> 
> This is the only reason I added the pid to the request packet in the
> first place.
> 
> Please correct me if my understanding of this is not right.

It's not wrong, but we just have to think through which value is the
most useful.

Any process executing clone(CLONE_NEWPID) (with CAP_SYS_ADMIN) can start
an application in a new pid namespace.  So imagine the user at the
desktop clicking some button which runs an application in a new pid
namespace.  Now if the user starts an xterm and runs ps -ef, the pid
values he'll see for the tasks in that new namespace will not be the
same as those which the application sees for itself, and not the same as
those which, right now, autofs would report.

For instance, if I start a shell in a new pid namespace, then within the
new pid namespace ps -ef gives me:

sh-3.2# ps -ef
UID        PID  PPID  C STIME TTY          TIME CMD
root         1     0  0 10:54 pts/1    00:00:00 /bin/sh
root         5     1  0 10:54 pts/1    00:00:00 /bin/sleep 100
root         6     1  0 10:54 pts/1    00:00:00 ps -ef

but from another shell as the same user, partial output of ps -ef
gives me:

root      2877  2876  0 10:54 pts/1    00:00:00 /bin/sh
root      2881  2877  0 10:54 pts/1    00:00:00 /bin/sleep 100

And so what we're trying to decide is whether autofs should send
pid 5 or pid 2881 for a message about the "/bin/sleep 100" task.

In fact, if the user clicks that button twice, chances are both
instances of the application will have the same pid values for each
process in the application.  So now if autofs sends a message to the
user about the application, the user cannot tell which process is at
fault.

Autofs will be sending the user some message about 'process 5'.  The
user won't know whether it means "the real" pid 5, [watchdog/0],
pid 5 in the first instance of the application, or pid 5 in the
second instance.

Now it's true that the user's xterm may still be in a different
(descendent) pidns of the autofs daemon.  But we can't expect
the autofs daemon to do pid_t translation for the user, so I
think what we have to aim for is making sure that the values
reported are unique within the pidns of the autofs daemon.  And
that means sending back either the pid values in the autofs
daemon's pid namespace, or using the top-level pid_ts, that is,
the pid values in the init namespace, which will be unique
on the whole system.

Sorry this turned out long-winded, I hope it makes sense.
And if I'm just showing a misunderstanding of what you're doing,
please do correct me :)

thanks,
-serge

Re: [PATCH 2/4] autofs4 - track uid and gid of last mount requester

[Serge E. Hallyn]

Quoting Ian Kent (raven@themaw.net):
> 
> On Thu, 2008-08-07 at 17:15 -0500, Serge E. Hallyn wrote:
> > Quoting Andrew Morton (akpm@linux-foundation.org):
> > > On Thu, 07 Aug 2008 19:40:14 +0800
> > > Ian Kent <raven@themaw.net> wrote:
> > > 
> > > > Patch to track the uid and gid of the last process to request a mount
> > > > for on an autofs dentry.
> > > 
> > > pet peeve: changelog should not tell the reader that this is a "patch".
> > > Because when someone is reading the changelog in the git repository,
> > > they hopefully already know that.
> > > 
> > > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > > 
> > > > ---
> > > > 
> > > >  fs/autofs4/autofs_i.h |    3 +++
> > > >  fs/autofs4/inode.c    |    2 ++
> > > >  fs/autofs4/waitq.c    |   34 ++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 39 insertions(+), 0 deletions(-)
> > > > 
> > > > 
> > > > diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> > > > index ea024d8..fa76d18 100644
> > > > --- a/fs/autofs4/autofs_i.h
> > > > +++ b/fs/autofs4/autofs_i.h
> > > > @@ -63,6 +63,9 @@ struct autofs_info {
> > > >  	unsigned long last_used;
> > > >  	atomic_t count;
> > > >  
> > > > +	uid_t uid;
> > > > +	gid_t gid;
> > > > +
> > > >  	mode_t	mode;
> > > >  	size_t	size;
> > > >  
> > > > diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
> > > > index 9ca2d07..9408507 100644
> > > > --- a/fs/autofs4/inode.c
> > > > +++ b/fs/autofs4/inode.c
> > > > @@ -53,6 +53,8 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
> > > >  		atomic_set(&ino->count, 0);
> > > >  	}
> > > >  
> > > > +	ino->uid = 0;
> > > > +	ino->gid = 0;
> > > >  	ino->mode = mode;
> > > >  	ino->last_used = jiffies;
> > > >  
> > > > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> > > > index 6d87bb1..7c60c0b 100644
> > > > --- a/fs/autofs4/waitq.c
> > > > +++ b/fs/autofs4/waitq.c
> > > > @@ -457,6 +457,40 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
> > > >  
> > > >  	status = wq->status;
> > > >  
> > > > +	/*
> > > > +	 * For direct and offset mounts we need to track the requestrer
> > > 
> > > typo which I'll fix.
> > > 
> > > > +	 * uid and gid in the dentry info struct. This is so it can be
> > > > +	 * supplied, on request, by the misc device ioctl interface.
> > > > +	 * This is needed during daemon resatart when reconnecting
> > > > +	 * to existing, active, autofs mounts. The uid and gid (and
> > > > +	 * related string values) may be used for macro substitution
> > > > +	 * in autofs mount maps.
> > 
> > Hi Ian,
> > 
> > could you say just a few more words on these macro substitution?
> > 
> > I think your use of uids can completely ignore user namespaces, but it
> > depends on who does the macro substitutions and how...
> 
> Suppose we have an autofs map entry in /etc/auto.master:
> /test   /etc/auto.test
> 
> and in /etc/auto.test
> 
> im1  /om1 server:/dir1 \
>      /om2/$USER server2:/dir2
> 
> Then if this is automounted and the daemon is sent a SIGKILL and started
> again we need the uid number that was used when this was mounted to
> re-construct the offsets (/om2/$USER in this case needs the
> substitution). The uid is used to lookup the user name.

Based on that I can't quite tell whether there is any security property
that could be violated by uid 500 'tricking' the autofs daemon into
doing something for uid 0.  It sounds like it could be annoying but
not a security violation?

> Can you point me in the right direction wrt. to namespaces on this
> please Serge? We didn't really get to the bottom of it last time I
> think.

Well user namespaces are still being implemented, in fact at some level
still being designed.  So it doesn't hurt to consider the right thing
to do right off the bat, but unlike the pid namespaces this really shouldn't
be hurting anyone right now.

User namespaces will be hierarchical - like pid namespaces, except
somewhat differently.  If user 500 in userns 0 creates a new user
namespace, then each userid in the new user namespace is also
'owned' by (500,0).  Our hope is that we can make the userns
core such that unprivileged users can safely unshare a new pid
namespace.  So for that reason, I suspect we'll want to handle
user namespace much like I just suggested handling pid namespaces.

1. For each 'system container', that is, a process set with its
own network stack, its own devices, etc, we'll want separate
autofs daemons.  That's simple enough - apart from determining
what qualifies as a 'system container'.  Here I think that will
depend on how autofs talks to the userspace daemon, and what
needs to be isolated in order to prevent a surreptitious admin
in one system container from having its autofs daemon talk to
another.

2. Within a system container, I think we'll want the autofs
daemon to store a pointer to its user namespace.  When an
autofs4_wait happens, the wq uses the full current->user
to look up the task's uid within the autofs daemon's own user
namespace, and it uses for macro expansions.

That prevents unprivileged user hallyn from creating a new
user namespace and trying to fool autofs into expanding
macros for uid 0.

Note that certainly for now and probably a long time coming,
you do need privilege to clone(CLONE_NEWUSER) so this isn't
urgent.

thanks,
-serge

Re: [PATCH 2/4] autofs4 - track uid and gid of last mount requester

[Ian Kent]

On Fri, 2008-08-08 at 09:58 -0500, Serge E. Hallyn wrote:
> Quoting Ian Kent (raven@themaw.net):
> > 
> > On Fri, 2008-08-08 at 11:48 +0800, Ian Kent wrote:
> > > > > 
> > > > > Please remind me again why autofs's use of current->uid and
> > > > > current->gid is not busted in the presence of PID namespaces, where
> > > > > these things are no longer system-wide unique?
> > > > 
> > > > I actually don't see what the autofs4_waitq->pid is used for.  It's
> > > > copied from current into wq->pid at autofs4_wait, and into a packet to
> > > > send to userspace (I assume) at autofs4_notify_daemon.
> > > > 
> > > > So as long as a daemon can serve multiple pid namespaces (which
> > > > doubtless it can), the pid could be confusing (or erroneous) for the
> > > > daemon.
> > > 
> > > Your point is well taken.
> > > 
> > > The pid is used purely for logging purposes to aid in debugging in user
> > > space. I'm not sure it is worth worrying about it too much as the daemon
> > > has no business interfering with user space processes it is not the
> > > owner of.
> > > 
> > > > 
> > > > If I'm remotely right about how the pid is being used, then the thing to
> > > > do would be to 
> > > > 	1. store the daemon's pid namespace  (would that belong in
> > > > 	the autofs_sb_info?)
> > >  
> > > Yep.
> > > 
> > > > 	2. store the task_pid(current) in the waitqueue
> > > > 	3. retrieve the pid_t for the waiting task in the daemon's
> > > > 	pid namespace, and put that into the packet at
> > > > 	autofs4_notify_daemon.
> > > > 
> > > > I realize this patch was about the *uids*, but the pids seem more
> > > > urgent.
> > > 
> > > OK, I get it.
> > > I'll have a go at doing this for completeness.
> > 
> > On second thoughts I'm not sure about this.
> > 
> > The pid that is logged needs to relate to a process in the name space of
> > the one that caused the mount to be done.
> > 
> > For example, suppose a GUI user finds mounts never expiring, then we get
> > a debug log to try and identify the culprit. So the pid should
> > correspond to a process that the user sees (So I guess in the namespace
> > of that user).
> > 
> > This is the only reason I added the pid to the request packet in the
> > first place.
> > 
> > Please correct me if my understanding of this is not right.
> 
> It's not wrong, but we just have to think through which value is the
> most useful.
> 
> Any process executing clone(CLONE_NEWPID) (with CAP_SYS_ADMIN) can start
> an application in a new pid namespace.  So imagine the user at the
> desktop clicking some button which runs an application in a new pid
> namespace.  Now if the user starts an xterm and runs ps -ef, the pid
> values he'll see for the tasks in that new namespace will not be the
> same as those which the application sees for itself, and not the same as
> those which, right now, autofs would report.
> 
> For instance, if I start a shell in a new pid namespace, then within the
> new pid namespace ps -ef gives me:
> 
> sh-3.2# ps -ef
> UID        PID  PPID  C STIME TTY          TIME CMD
> root         1     0  0 10:54 pts/1    00:00:00 /bin/sh
> root         5     1  0 10:54 pts/1    00:00:00 /bin/sleep 100
> root         6     1  0 10:54 pts/1    00:00:00 ps -ef
> 
> but from another shell as the same user, partial output of ps -ef
> gives me:
> 
> root      2877  2876  0 10:54 pts/1    00:00:00 /bin/sh
> root      2881  2877  0 10:54 pts/1    00:00:00 /bin/sleep 100
> 
> And so what we're trying to decide is whether autofs should send
> pid 5 or pid 2881 for a message about the "/bin/sleep 100" task.
> 
> In fact, if the user clicks that button twice, chances are both
> instances of the application will have the same pid values for each
> process in the application.  So now if autofs sends a message to the
> user about the application, the user cannot tell which process is at
> fault.
> 
> Autofs will be sending the user some message about 'process 5'.  The
> user won't know whether it means "the real" pid 5, [watchdog/0],
> pid 5 in the first instance of the application, or pid 5 in the
> second instance.
> 
> Now it's true that the user's xterm may still be in a different
> (descendent) pidns of the autofs daemon.  But we can't expect
> the autofs daemon to do pid_t translation for the user, so I
> think what we have to aim for is making sure that the values
> reported are unique within the pidns of the autofs daemon.  And
> that means sending back either the pid values in the autofs
> daemon's pid namespace, or using the top-level pid_ts, that is,
> the pid values in the init namespace, which will be unique
> on the whole system.
> 
> Sorry this turned out long-winded, I hope it makes sense.
> And if I'm just showing a misunderstanding of what you're doing,
> please do correct me :)

Yes, it's a bit tricky.

In reality this information is only logged when debug logging is enabled
and generally is only used by myself or others that maintain autofs. But
getting sensible logging is important so it's worth sorting this out.

I think it would be best to use the pid of the highest view namespace
which I think is the gist of what you said in the beginning. Then (at
some future time), if there was a user space API, the daemon could
report additional pid information related to subordinate pid namespaces.
I am assuming that, to be useful, an autofs daemon that is able to serve
multiple namespaces would be higher up in the tree. But the forgoing
description sounds like there's not a necessarily a hierarchic structure
to pid namespaces?

The other issue that comes up is, after storing (a reference to) the
daemon namespace in the super block info struct a "kill -9" on the
daemon would render the namespace invalid. At the moment, when this
happens the write on the kernel pipe fails causing the autofs mount to
become catatonic, but for the namespace aware case the namespace is now
invalid so we won't get that far. I could make the mount catatonic when
I detect that the namespace has become invalid but I'm not sure how to
check it. Is there a way I can to do this? Would there be any issues
with namespace (pid) reuse for such a check?

Sorry to seem so thick about this but the implications of namespaces for
autofs are quite new to me and it is important that autofs "plays well
with others" as the development progresses.

Anyway, assuming I'm on the right track I'll poke around further and see
if I can work out how I to do this.

Ian

Re: [PATCH 2/4] autofs4 - track uid and gid of last mount requester

[Serge E. Hallyn]

Quoting Ian Kent (raven@themaw.net):
> 
> On Fri, 2008-08-08 at 09:58 -0500, Serge E. Hallyn wrote:
> > Quoting Ian Kent (raven@themaw.net):
> > > 
> > > On Fri, 2008-08-08 at 11:48 +0800, Ian Kent wrote:
> > > > > > 
> > > > > > Please remind me again why autofs's use of current->uid and
> > > > > > current->gid is not busted in the presence of PID namespaces, where
> > > > > > these things are no longer system-wide unique?
> > > > > 
> > > > > I actually don't see what the autofs4_waitq->pid is used for.  It's
> > > > > copied from current into wq->pid at autofs4_wait, and into a packet to
> > > > > send to userspace (I assume) at autofs4_notify_daemon.
> > > > > 
> > > > > So as long as a daemon can serve multiple pid namespaces (which
> > > > > doubtless it can), the pid could be confusing (or erroneous) for the
> > > > > daemon.
> > > > 
> > > > Your point is well taken.
> > > > 
> > > > The pid is used purely for logging purposes to aid in debugging in user
> > > > space. I'm not sure it is worth worrying about it too much as the daemon
> > > > has no business interfering with user space processes it is not the
> > > > owner of.
> > > > 
> > > > > 
> > > > > If I'm remotely right about how the pid is being used, then the thing to
> > > > > do would be to 
> > > > > 	1. store the daemon's pid namespace  (would that belong in
> > > > > 	the autofs_sb_info?)
> > > >  
> > > > Yep.
> > > > 
> > > > > 	2. store the task_pid(current) in the waitqueue
> > > > > 	3. retrieve the pid_t for the waiting task in the daemon's
> > > > > 	pid namespace, and put that into the packet at
> > > > > 	autofs4_notify_daemon.
> > > > > 
> > > > > I realize this patch was about the *uids*, but the pids seem more
> > > > > urgent.
> > > > 
> > > > OK, I get it.
> > > > I'll have a go at doing this for completeness.
> > > 
> > > On second thoughts I'm not sure about this.
> > > 
> > > The pid that is logged needs to relate to a process in the name space of
> > > the one that caused the mount to be done.
> > > 
> > > For example, suppose a GUI user finds mounts never expiring, then we get
> > > a debug log to try and identify the culprit. So the pid should
> > > correspond to a process that the user sees (So I guess in the namespace
> > > of that user).
> > > 
> > > This is the only reason I added the pid to the request packet in the
> > > first place.
> > > 
> > > Please correct me if my understanding of this is not right.
> > 
> > It's not wrong, but we just have to think through which value is the
> > most useful.
> > 
> > Any process executing clone(CLONE_NEWPID) (with CAP_SYS_ADMIN) can start
> > an application in a new pid namespace.  So imagine the user at the
> > desktop clicking some button which runs an application in a new pid
> > namespace.  Now if the user starts an xterm and runs ps -ef, the pid
> > values he'll see for the tasks in that new namespace will not be the
> > same as those which the application sees for itself, and not the same as
> > those which, right now, autofs would report.
> > 
> > For instance, if I start a shell in a new pid namespace, then within the
> > new pid namespace ps -ef gives me:
> > 
> > sh-3.2# ps -ef
> > UID        PID  PPID  C STIME TTY          TIME CMD
> > root         1     0  0 10:54 pts/1    00:00:00 /bin/sh
> > root         5     1  0 10:54 pts/1    00:00:00 /bin/sleep 100
> > root         6     1  0 10:54 pts/1    00:00:00 ps -ef
> > 
> > but from another shell as the same user, partial output of ps -ef
> > gives me:
> > 
> > root      2877  2876  0 10:54 pts/1    00:00:00 /bin/sh
> > root      2881  2877  0 10:54 pts/1    00:00:00 /bin/sleep 100
> > 
> > And so what we're trying to decide is whether autofs should send
> > pid 5 or pid 2881 for a message about the "/bin/sleep 100" task.
> > 
> > In fact, if the user clicks that button twice, chances are both
> > instances of the application will have the same pid values for each
> > process in the application.  So now if autofs sends a message to the
> > user about the application, the user cannot tell which process is at
> > fault.
> > 
> > Autofs will be sending the user some message about 'process 5'.  The
> > user won't know whether it means "the real" pid 5, [watchdog/0],
> > pid 5 in the first instance of the application, or pid 5 in the
> > second instance.
> > 
> > Now it's true that the user's xterm may still be in a different
> > (descendent) pidns of the autofs daemon.  But we can't expect
> > the autofs daemon to do pid_t translation for the user, so I
> > think what we have to aim for is making sure that the values
> > reported are unique within the pidns of the autofs daemon.  And
> > that means sending back either the pid values in the autofs
> > daemon's pid namespace, or using the top-level pid_ts, that is,
> > the pid values in the init namespace, which will be unique
> > on the whole system.
> > 
> > Sorry this turned out long-winded, I hope it makes sense.
> > And if I'm just showing a misunderstanding of what you're doing,
> > please do correct me :)
> 
> Yes, it's a bit tricky.
> 
> In reality this information is only logged when debug logging is enabled
> and generally is only used by myself or others that maintain autofs. But
> getting sensible logging is important so it's worth sorting this out.
> 
> I think it would be best to use the pid of the highest view namespace
> which I think is the gist of what you said in the beginning. Then (at
> some future time), if there was a user space API, the daemon could
> report additional pid information related to subordinate pid namespaces.
> I am assuming that, to be useful, an autofs daemon that is able to serve
> multiple namespaces would be higher up in the tree. But the forgoing
> description sounds like there's not a necessarily a hierarchic structure
> to pid namespaces?

It is a simple tree, but of course if you have 3 autofs daemons in
separate containers, and one on the main host, then the pid namespaces for
each of the 3 container autofs daemons will be siblings, so they won't
have meaningful translations for each others' pids, while pids in
each container will have meaningful translations in the host system's
pid namespace (the init_pid_ns).

> The other issue that comes up is, after storing (a reference to) the
> daemon namespace in the super block info struct a "kill -9" on the
> daemon would render the namespace invalid. At the moment, when this
> happens the write on the kernel pipe fails causing the autofs mount to
> become catatonic, but for the namespace aware case the namespace is now
> invalid so we won't get that far. I could make the mount catatonic when

I figured you would grab a reference to the pid namespace, so it
wouldn't go away until you released the superblock or registered a new
daemon for it.

> I detect that the namespace has become invalid but I'm not sure how to
> check it. Is there a way I can to do this? Would there be any issues
> with namespace (pid) reuse for such a check?

I'm not sure what you mean - but struct pid is never reused so long
as you have a reference to one.  So you could store
get_pid(task_pid(autofs_daemon)) and check whether the autofs daemon's
pid has changed that way, I suppose.  But grabbing a struct pid reference
does not pin the pid_ns iirc.

Maybe you're right about just getting the top-level pid_nr.

> Sorry to seem so thick about this but the implications of namespaces for
> autofs are quite new to me and it is important that autofs "plays well
> with others" as the development progresses.
>
> Anyway, assuming I'm on the right track I'll poke around further and see
> if I can work out how I to do this.

Yes I think you're on the right track.  And registering the top-level
pid may be the simplest thing to do.  You'd get that by grabbing
pid_nr(task_pid(task)).  Then you don't have to do any sort of pid
namespace storing at all.

The other approach has its appeal, but maybe it's not worth the
complications at this point.

thanks,
-serge

Re: [PATCH 2/4] autofs4 - track uid and gid of last mount requester

[Serge E. Hallyn]

Quoting Serge E. Hallyn (serue@us.ibm.com):
> Quoting Ian Kent (raven@themaw.net):
> > 
> > On Fri, 2008-08-08 at 09:58 -0500, Serge E. Hallyn wrote:
> > > Quoting Ian Kent (raven@themaw.net):
> > > > 
> > > > On Fri, 2008-08-08 at 11:48 +0800, Ian Kent wrote:
> > > > > > > 
> > > > > > > Please remind me again why autofs's use of current->uid and
> > > > > > > current->gid is not busted in the presence of PID namespaces, where
> > > > > > > these things are no longer system-wide unique?
> > > > > > 
> > > > > > I actually don't see what the autofs4_waitq->pid is used for.  It's
> > > > > > copied from current into wq->pid at autofs4_wait, and into a packet to
> > > > > > send to userspace (I assume) at autofs4_notify_daemon.
> > > > > > 
> > > > > > So as long as a daemon can serve multiple pid namespaces (which
> > > > > > doubtless it can), the pid could be confusing (or erroneous) for the
> > > > > > daemon.
> > > > > 
> > > > > Your point is well taken.
> > > > > 
> > > > > The pid is used purely for logging purposes to aid in debugging in user
> > > > > space. I'm not sure it is worth worrying about it too much as the daemon
> > > > > has no business interfering with user space processes it is not the
> > > > > owner of.
> > > > > 
> > > > > > 
> > > > > > If I'm remotely right about how the pid is being used, then the thing to
> > > > > > do would be to 
> > > > > > 	1. store the daemon's pid namespace  (would that belong in
> > > > > > 	the autofs_sb_info?)
> > > > >  
> > > > > Yep.
> > > > > 
> > > > > > 	2. store the task_pid(current) in the waitqueue
> > > > > > 	3. retrieve the pid_t for the waiting task in the daemon's
> > > > > > 	pid namespace, and put that into the packet at
> > > > > > 	autofs4_notify_daemon.
> > > > > > 
> > > > > > I realize this patch was about the *uids*, but the pids seem more
> > > > > > urgent.
> > > > > 
> > > > > OK, I get it.
> > > > > I'll have a go at doing this for completeness.
> > > > 
> > > > On second thoughts I'm not sure about this.
> > > > 
> > > > The pid that is logged needs to relate to a process in the name space of
> > > > the one that caused the mount to be done.
> > > > 
> > > > For example, suppose a GUI user finds mounts never expiring, then we get
> > > > a debug log to try and identify the culprit. So the pid should
> > > > correspond to a process that the user sees (So I guess in the namespace
> > > > of that user).
> > > > 
> > > > This is the only reason I added the pid to the request packet in the
> > > > first place.
> > > > 
> > > > Please correct me if my understanding of this is not right.
> > > 
> > > It's not wrong, but we just have to think through which value is the
> > > most useful.
> > > 
> > > Any process executing clone(CLONE_NEWPID) (with CAP_SYS_ADMIN) can start
> > > an application in a new pid namespace.  So imagine the user at the
> > > desktop clicking some button which runs an application in a new pid
> > > namespace.  Now if the user starts an xterm and runs ps -ef, the pid
> > > values he'll see for the tasks in that new namespace will not be the
> > > same as those which the application sees for itself, and not the same as
> > > those which, right now, autofs would report.
> > > 
> > > For instance, if I start a shell in a new pid namespace, then within the
> > > new pid namespace ps -ef gives me:
> > > 
> > > sh-3.2# ps -ef
> > > UID        PID  PPID  C STIME TTY          TIME CMD
> > > root         1     0  0 10:54 pts/1    00:00:00 /bin/sh
> > > root         5     1  0 10:54 pts/1    00:00:00 /bin/sleep 100
> > > root         6     1  0 10:54 pts/1    00:00:00 ps -ef
> > > 
> > > but from another shell as the same user, partial output of ps -ef
> > > gives me:
> > > 
> > > root      2877  2876  0 10:54 pts/1    00:00:00 /bin/sh
> > > root      2881  2877  0 10:54 pts/1    00:00:00 /bin/sleep 100
> > > 
> > > And so what we're trying to decide is whether autofs should send
> > > pid 5 or pid 2881 for a message about the "/bin/sleep 100" task.
> > > 
> > > In fact, if the user clicks that button twice, chances are both
> > > instances of the application will have the same pid values for each
> > > process in the application.  So now if autofs sends a message to the
> > > user about the application, the user cannot tell which process is at
> > > fault.
> > > 
> > > Autofs will be sending the user some message about 'process 5'.  The
> > > user won't know whether it means "the real" pid 5, [watchdog/0],
> > > pid 5 in the first instance of the application, or pid 5 in the
> > > second instance.
> > > 
> > > Now it's true that the user's xterm may still be in a different
> > > (descendent) pidns of the autofs daemon.  But we can't expect
> > > the autofs daemon to do pid_t translation for the user, so I
> > > think what we have to aim for is making sure that the values
> > > reported are unique within the pidns of the autofs daemon.  And
> > > that means sending back either the pid values in the autofs
> > > daemon's pid namespace, or using the top-level pid_ts, that is,
> > > the pid values in the init namespace, which will be unique
> > > on the whole system.
> > > 
> > > Sorry this turned out long-winded, I hope it makes sense.
> > > And if I'm just showing a misunderstanding of what you're doing,
> > > please do correct me :)
> > 
> > Yes, it's a bit tricky.
> > 
> > In reality this information is only logged when debug logging is enabled
> > and generally is only used by myself or others that maintain autofs. But
> > getting sensible logging is important so it's worth sorting this out.
> > 
> > I think it would be best to use the pid of the highest view namespace
> > which I think is the gist of what you said in the beginning. Then (at
> > some future time), if there was a user space API, the daemon could
> > report additional pid information related to subordinate pid namespaces.
> > I am assuming that, to be useful, an autofs daemon that is able to serve
> > multiple namespaces would be higher up in the tree. But the forgoing
> > description sounds like there's not a necessarily a hierarchic structure
> > to pid namespaces?
> 
> It is a simple tree, but of course if you have 3 autofs daemons in
> separate containers, and one on the main host, then the pid namespaces for
> each of the 3 container autofs daemons will be siblings, so they won't
> have meaningful translations for each others' pids, while pids in
> each container will have meaningful translations in the host system's
> pid namespace (the init_pid_ns).
> 
> > The other issue that comes up is, after storing (a reference to) the
> > daemon namespace in the super block info struct a "kill -9" on the
> > daemon would render the namespace invalid. At the moment, when this
> > happens the write on the kernel pipe fails causing the autofs mount to
> > become catatonic, but for the namespace aware case the namespace is now
> > invalid so we won't get that far. I could make the mount catatonic when
> 
> I figured you would grab a reference to the pid namespace, so it
> wouldn't go away until you released the superblock or registered a new
> daemon for it.
> 
> > I detect that the namespace has become invalid but I'm not sure how to
> > check it. Is there a way I can to do this? Would there be any issues
> > with namespace (pid) reuse for such a check?
> 
> I'm not sure what you mean - but struct pid is never reused so long
> as you have a reference to one.  So you could store
> get_pid(task_pid(autofs_daemon)) and check whether the autofs daemon's
> pid has changed that way, I suppose.  But grabbing a struct pid reference
> does not pin the pid_ns iirc.
> 
> Maybe you're right about just getting the top-level pid_nr.

After doing a quick test with your git tree, I find that I was wrong,
and task->pid is the global pid of the process, not the pid in its own
pid namespace.

So currently autofs sends a process id in the init_pid_ns.  This may
be meaningless in the autofs daemon's pid namespace, but since the
purpose is just for logging/debugging, having a global pid, which
uniquely identifies any task on the system, seems correct.

So in terms of pids no change is needed IMO.

thanks,
-serge