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

[Ian Kent <raven@themaw.net>]

On Mon, 2009-01-19 at 08:08 +0100, Oleg Nesterov wrote:
> On 01/19, Ian Kent wrote:
> >
> > On Sun, 2009-01-18 at 08:34 +0100, Oleg Nesterov wrote:
> > > parse_options(&pgid) sets pgid = task_pgrp_nr() which uses the global
> > > namespace. This is wrong, we use this pgid to find "struct pid" in the
> > > current's namespace. Change parse_options() to use task_pgrp_vnr().
> > >
> > > Also do s/task_pgrp_nr/task_pgrp_vnr/ in the debugging printks.
> > > checkpatch.pl complains about "line over 80 characters", but it should
> > > blame the cuurent code, not the patch.
> >
> > This changelog entry doesn't really have anything that I can use to work
> > out if this change might introduce regressions.
> >
> > It would be helpful to me if you could include:
> > 1) A brief statement about what your trying to achieve and why.
> 
> First of all, I think this patch fixes a bug.
> 
> What we are doing in autofs_fill_super()->parse_options() path
> is find_get_pid(task_pgrp_vnr(current)), this is wrong.

So your saying that your patch is wrong?
I'm not following this at all.

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

> 
> Another reason is that task_pgrp_nr() is deprecated. We are wasting
> 2 words in signal_struct without good reason. Please look at
> 
> 	http://marc.info/?l=linux-kernel&m=123228947019281
> 
> > 2) The reason why task_pgrp_nr() has changed to task_pgrp_vnr() since
> > you made the change (that is someone working on pid namespaces) to
> > task_pgrp_nr().
> 
> see above.
> 
> > 3) Why you believe this change won't introduce a regression.
> 
> Ah, sorry. I forgot to mention this patch is only compile tested
> (like the next one, but at least it has the note in changelog).
> 
> But again, I think this is bugfix.
> 
> Oleg.
>