Re: [PATCH] autofs: fix the wrong usage of the deprecated task_pgrp_nr()

[Ian Kent <raven@themaw.net>]

On Mon, 2009-01-19 at 13:42 +0100, Oleg Nesterov wrote:
> On 01/19, Ian Kent wrote:
> >
> > On Mon, 2009-01-19 at 09:32 +0100, Oleg Nesterov wrote:
> > >
> > > No, I am trying to say that the current code is wrong ;)
> > >
> > > > > task_pgrp_vnr() reporst the pid_t in the global namespace, but
> > > > > find_get_pid() searches "struct pid" in the current namespace.
> > > > > We can get the wrong pid. I tried to document this in changelog.
> > > >
> > > > We don't know whether it's the wrong pid because the environments were
> > > > this is used haven't been defined. Depending on expected usage of pid
> > > > namespaces the global pid may or may not be the correct one. This was
> > > > not determined the last time this came up.
> > >
> > > Confused. The current code can't be right.
> > >
> > > Lets consider the simplest case, there is no "pgrp=" option during mount.
> >
> > No, the pgrp is required at mount time and must be the pid of the
> > process group leader. But it isn't enforced in the code so that "is" a
> > bug.
> 
> I see, but I didn't mean _this_ is bug. Please see below.

Right, I see the second of the two patches would cover this case since
the process doing the mount is always automount as it doesn't make sense
otherwise.

> 
> > > In that case the current code does:
> > >
> > > 	pid_t pgrp = task_pgrp_nr(current);
> > > 	sbi->oz_pgrp = find_get_pid(pgid);
> > >
> > > But this means that sbi->oz_pgrp != task_prgp(current), unless of
> > > course we are from the global namespace. ->oz_pgrp is a "random"
> > > pid or NULL.
> > >
> > > What I am missed?
> >
> > What your missing is that all I'm asking for is a little background
> > information on what the change is about so that I can understand it.
> >
> > I think you are making assumptions that just aren't true about my
> > understanding of the pid namespace work.
> >
> > The current situation is that pgrp corresponds to the session leader of
> > the automount(8) process and that process is started at boot so I guess
> > it is within the global namespace.
> 
> In that case the patch doesn't make the difference, because if the
> task runs in the global namespace then
> 
> 	task_pgrp_nr(current) == task_pgrp_vnr(current);

And this pretty much answers the question I had, this should be fine
then.

> 
> > All we need to do now (since the
> > issue will be much more complex if we consider multiple instances of
> > automount(8) started within pid namespaces) is verify that changes we
> > make to obtain the pgrp will correspond to the pid of automount(8) in
> > the global namespace.
> 
> And now we have a problem afaics, because without this patch ->oz_pgrp
> does not necessary match the pid of automount.
> 
> Let's look at
> 
> 	static inline int autofs_oz_mode(struct autofs_sb_info *sbi) {
> 		return sbi->catatonic || task_pgrp(current) == sbi->oz_pgrp;
> 	}
> 
> Please note that task_pgrp(current) is "struct pid *", not pid_t.
> It does not belong to any particular namespace, it "represents" all
> namespaces this task is visible in, up to global.
> 
> Let's suppose automount starts in the level 2 namespace.
> It's pgrp == 10 in the global namespace, and at the same time
> it is == 20 from automount->nsproxy->pid_ns pov.
> 
> We need sbi->oz_pgrp == task_pgrp(automount).
> 
> But, by default parse_options() sets pgrp == 10, this is what
> task_pgrp_nr(current) == task_pgrp_nr_ns(current, init_pid_ns)
> returns.
> 
> Now autofs_fill_super() does sbi->oz_pgrp = find_get_pid(10).
> This means: find the pid which has the numeric value == 10
> in our namespace == automount->nsproxy->pid_ns.
> 
> But we should use 20, not 10. Otherwise we get the wrong pid or
> NULL.
> 
> In short. Let's suppose automount(8) startes within the
> pid namespace, and there is no "pgrp=" parameter. Then,
> 
> Before the patch
> 
> 	sbi->oz_pgrp != task_pgrp(automount)
> 
> After the patch
> 
> 	sbi->oz_pgrp == task_pgrp(automount)
> 
> And please note that these "!="/"==" apply to any namespace. I mean,
> when we call autofs_oz_mode() it does not matter in which namespace
> autofs_oz_mode() is executed, we compare "struct pid*", not pid_t.

I think your saying that the option pgrp= is broken and should be
deprecated and that we should always set sbi->oz_pgrp the same way as
you have done in the case where it isn't given as an option and in
autofs_dev_ioctl_setpipefd(). Basically, just silently overriding it and
setting the correct value to maintain backward option compatibility.

> 
> 
> Ian, please let me know if I am answering the wrong question,
> I am not sure I understand you.

It's interesting that your description appears to assume the the same
process may appear in different pid namespaces (am I right?) whereas in
the descriptions I have been writing I'm thinking of pid namspeces as
having there own set of proceses and not being able to see processes
outside the space. But then to be useful I guess both possibilities must
exist.

Ian