Messages in this thread | | | Subject | Re: [PATCH] autofs: fix the wrong usage of the deprecated task_pgrp_nr() | From | Ian Kent <> | Date | Mon, 19 Jan 2009 22:33:36 +0900 |
| |
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
| |