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

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

[Oleg Nesterov]

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.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- CUR/fs/autofs/inode.c~1_AUTOFS	2009-01-12 23:07:46.000000000 +0100
+++ CUR/fs/autofs/inode.c	2009-01-18 06:18:49.000000000 +0100
@@ -78,7 +78,7 @@ static int parse_options(char *options, 
 
 	*uid = current_uid();
 	*gid = current_gid();
-	*pgrp = task_pgrp_nr(current);
+	*pgrp = task_pgrp_vnr(current);
 
 	*minproto = *maxproto = AUTOFS_PROTO_VERSION;
 
--- CUR/fs/autofs/root.c~1_AUTOFS	2008-10-10 00:13:53.000000000 +0200
+++ CUR/fs/autofs/root.c	2009-01-18 06:15:42.000000000 +0100
@@ -215,7 +215,7 @@ static struct dentry *autofs_root_lookup
 	oz_mode = autofs_oz_mode(sbi);
 	DPRINTK(("autofs_lookup: pid = %u, pgrp = %u, catatonic = %d, "
 				"oz_mode = %d\n", task_pid_nr(current),
-				task_pgrp_nr(current), sbi->catatonic,
+				task_pgrp_vnr(current), sbi->catatonic,
 				oz_mode));
 
 	/*
@@ -550,7 +550,8 @@ static int autofs_root_ioctl(struct inod
 	struct autofs_sb_info *sbi = autofs_sbi(inode->i_sb);
 	void __user *argp = (void __user *)arg;
 
-	DPRINTK(("autofs_ioctl: cmd = 0x%08x, arg = 0x%08lx, sbi = %p, pgrp = %u\n",cmd,arg,sbi,task_pgrp_nr(current)));
+	DPRINTK(("autofs_ioctl: cmd = 0x%08x, arg = 0x%08lx, sbi = %p, pgrp = %u\n",
+		cmd, arg, sbi, task_pgrp_vnr(current)));
 
 	if (_IOC_TYPE(cmd) != _IOC_TYPE(AUTOFS_IOC_FIRST) ||
 	     _IOC_NR(cmd) - _IOC_NR(AUTOFS_IOC_FIRST) >= AUTOFS_IOC_COUNT)

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

[Ian Kent]

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.
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().
3) Why you believe this change won't introduce a regression.

> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> 
> --- CUR/fs/autofs/inode.c~1_AUTOFS	2009-01-12 23:07:46.000000000 +0100
> +++ CUR/fs/autofs/inode.c	2009-01-18 06:18:49.000000000 +0100
> @@ -78,7 +78,7 @@ static int parse_options(char *options, 
>  
>  	*uid = current_uid();
>  	*gid = current_gid();
> -	*pgrp = task_pgrp_nr(current);
> +	*pgrp = task_pgrp_vnr(current);
>  
>  	*minproto = *maxproto = AUTOFS_PROTO_VERSION;
>  
> --- CUR/fs/autofs/root.c~1_AUTOFS	2008-10-10 00:13:53.000000000 +0200
> +++ CUR/fs/autofs/root.c	2009-01-18 06:15:42.000000000 +0100
> @@ -215,7 +215,7 @@ static struct dentry *autofs_root_lookup
>  	oz_mode = autofs_oz_mode(sbi);
>  	DPRINTK(("autofs_lookup: pid = %u, pgrp = %u, catatonic = %d, "
>  				"oz_mode = %d\n", task_pid_nr(current),
> -				task_pgrp_nr(current), sbi->catatonic,
> +				task_pgrp_vnr(current), sbi->catatonic,
>  				oz_mode));
>  
>  	/*
> @@ -550,7 +550,8 @@ static int autofs_root_ioctl(struct inod
>  	struct autofs_sb_info *sbi = autofs_sbi(inode->i_sb);
>  	void __user *argp = (void __user *)arg;
>  
> -	DPRINTK(("autofs_ioctl: cmd = 0x%08x, arg = 0x%08lx, sbi = %p, pgrp = %u\n",cmd,arg,sbi,task_pgrp_nr(current)));
> +	DPRINTK(("autofs_ioctl: cmd = 0x%08x, arg = 0x%08lx, sbi = %p, pgrp = %u\n",
> +		cmd, arg, sbi, task_pgrp_vnr(current)));
>  
>  	if (_IOC_TYPE(cmd) != _IOC_TYPE(AUTOFS_IOC_FIRST) ||
>  	     _IOC_NR(cmd) - _IOC_NR(AUTOFS_IOC_FIRST) >= AUTOFS_IOC_COUNT)
> 

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

[H. Peter Anvin]

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.
> 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().
> 3) Why you believe this change won't introduce a regression.
> 

The other thing is also: isn't it high time to remove autofs 3?  It has
been unmaintained for at least 10 years now.  I should know ;)

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

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

[Ian Kent]

On Sun, 2009-01-18 at 22:35 -0800, H. Peter Anvin wrote:
> 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.
> > 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().
> > 3) Why you believe this change won't introduce a regression.
> > 
> 
> The other thing is also: isn't it high time to remove autofs 3?  It has
> been unmaintained for at least 10 years now.  I should know ;)

That's a good idea.
Couldn't we rename autofs4 to autofs and add a MODULE_ALIAS("autofs4")
or something?

Ian

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

[H. Peter Anvin]

Ian Kent wrote:
>>>
>> The other thing is also: isn't it high time to remove autofs 3?  It has
>> been unmaintained for at least 10 years now.  I should know ;)
> 
> That's a good idea.
> Couldn't we rename autofs4 to autofs and add a MODULE_ALIAS("autofs4")
> or something?
> 

Works for me.  Now, this really isn't an -rc2 kind of thing, but I would
ACK such a change for the next merge window.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

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

[Ian Kent]

On Mon, 2009-01-19 at 09:09 -0800, H. Peter Anvin wrote:
> Ian Kent wrote:
> >>>
> >> The other thing is also: isn't it high time to remove autofs 3?  It has
> >> been unmaintained for at least 10 years now.  I should know ;)
> > 
> > That's a good idea.
> > Couldn't we rename autofs4 to autofs and add a MODULE_ALIAS("autofs4")
> > or something?
> > 
> 
> Works for me.  Now, this really isn't an -rc2 kind of thing, but I would
> ACK such a change for the next merge window.

Sounds good to me.
I'll try for the next merge window.

Ian

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

[Oleg Nesterov]

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.

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.

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.

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

[Ian Kent]

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

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

[Oleg Nesterov]

On 01/19, Ian Kent wrote:
>
> 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.

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

Oleg.

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

[Ian Kent]

On Mon, 2009-01-19 at 09:32 +0100, Oleg Nesterov wrote:
> On 01/19, Ian Kent wrote:
> >
> > 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.
> 
> 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.

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

I suspect the main issue that is looming for autofs wrt. pid namespaces
is that file system namespaces are not tied to pid namespaces. That will
probably lead to the situation were don't know whether we want the pgrp
of an automount(8) within the pid namespace or we actually need it for
the process in the global namespace, assuming the file system namespace
might be shared between multiple pid namespaces.

But, I'm not up with this at all so I could be very mistaken.
Hopefully this little rant will clarify what I'm asking, but then maybe
I've just got it all wrong and made the situation worse, so help me out!

Ian

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

[Oleg Nesterov]

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.

> > 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);

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


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

Oleg.

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

[Ian Kent]

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

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

[Oleg Nesterov]

On 01/19, Ian Kent wrote:
>
> On Mon, 2009-01-19 at 13:42 +0100, Oleg Nesterov wrote:
> >
> > 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

No, no, sorry if I confused you.

If the "pgrp=" option was specified, the patch has no effect, and the
code is correct with or without the patch.

> It's interesting that your description appears to assume the the same
> process may appear in different pid namespaces (am I right?)

Yes. It is visible in its own namespace, in the ->parent namespace,
->parent->parent, and so on until the global init_pid_ns.

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

Yes, we can not see the tasks outside the space (but of course we can
see the tasks from the child namespaces).

IOW. Let's suppose we are in the global namespace, and some task
forks the child C via clone(CLONE_NEWPID).

Now, in the global namespace it has, say, pid == 100. But when
C does sys_getpid() it gets 1.

C can not see the tasks from the global namespace, but any task
from the global ns can see C with pid == 100.

And please note that C has the single "struct pid" wchich represents
all pid_t's. The same for pgrp/session. task_pgrp() returns the same
pid (pointer to the struct, not pid_t), no matter in which namespace
the code runs. But the result of pid_vnr(pid) or find_pid(pid_nr) do
differ depending on current->nsproxy->pid_ns.

Oleg.

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

[Serge E. Hallyn]

Quoting Oleg Nesterov (oleg@redhat.com):
> On 01/19, Ian Kent wrote:
> >
> > On Mon, 2009-01-19 at 13:42 +0100, Oleg Nesterov wrote:
> > >
> > > 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
> 
> No, no, sorry if I confused you.
> 
> If the "pgrp=" option was specified, the patch has no effect, and the
> code is correct with or without the patch.

But so there does still need to be a patch modifying parse_options()
to return an error if pgrp= was not specified, right?

-serge

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

[Oleg Nesterov]

On 01/19, Serge E. Hallyn wrote:
>
> Quoting Oleg Nesterov (oleg@redhat.com):
> > On 01/19, Ian Kent wrote:
> > >
> > > On Mon, 2009-01-19 at 13:42 +0100, Oleg Nesterov wrote:
> > > >
> > > > 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
> >
> > No, no, sorry if I confused you.
> > 
> > If the "pgrp=" option was specified, the patch has no effect, and the
> > code is correct with or without the patch.
>
> But so there does still need to be a patch modifying parse_options()
> to return an error if pgrp= was not specified, right?

Why? In that case we should use the caller's pgrp. This is what the
current tries to do, why should the patch change this behaviour?

Oleg.

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

[Serge E. Hallyn]

Quoting Oleg Nesterov (oleg@redhat.com):
> On 01/19, Serge E. Hallyn wrote:
> >
> > Quoting Oleg Nesterov (oleg@redhat.com):
> > > On 01/19, Ian Kent wrote:
> > > >
> > > > On Mon, 2009-01-19 at 13:42 +0100, Oleg Nesterov wrote:
> > > > >
> > > > > 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
> > >
> > > No, no, sorry if I confused you.
> > > 
> > > If the "pgrp=" option was specified, the patch has no effect, and the
> > > code is correct with or without the patch.
> >
> > But so there does still need to be a patch modifying parse_options()
> > to return an error if pgrp= was not specified, right?
> 
> Why? In that case we should use the caller's pgrp. This is what the
> current tries to do, why should the patch change this behaviour?

Well, because Ian said that not specifying it is supposed to
be an error :)  I didn't quite understand why, so am fishing
for more info...

-serge

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

[Oleg Nesterov]

On 01/19, Serge E. Hallyn wrote:
>
> Quoting Oleg Nesterov (oleg@redhat.com):
> > On 01/19, Serge E. Hallyn wrote:
> > >
> > > But so there does still need to be a patch modifying parse_options()
> > > to return an error if pgrp= was not specified, right?
> >
> > Why? In that case we should use the caller's pgrp. This is what the
> > current tries to do, why should the patch change this behaviour?
>
> Well, because Ian said that not specifying it is supposed to
> be an error :)  I didn't quite understand why, so am fishing
> for more info...

I think you misunderstood him. Or I am totally confused ;)

In any case. Both autofs and autofs4 use current's pgrp if this
option was not specified, and these patches doesn't change this
behaviour.


Actually, I am very much surprized this one-liner patch has so
many questions. Isn't it "obiously correct" ?

Oleg.

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

[H. Peter Anvin]

Oleg Nesterov wrote:
> 
> Actually, I am very much surprized this one-liner patch has so
> many questions. Isn't it "obiously correct" ?
> 

Obviously not.

	-hpa

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

[Oleg Nesterov]

On 01/19, H. Peter Anvin wrote:
>
> Oleg Nesterov wrote:
>>
>> Actually, I am very much surprized this one-liner patch has so
>> many questions. Isn't it "obiously correct" ?
>>
>
> Obviously not.

Do you agree the current code is not right?

What do you not like in this change?

Oleg.

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

[Serge E. Hallyn]

Quoting Oleg Nesterov (oleg@redhat.com):
> On 01/19, Serge E. Hallyn wrote:
> >
> > Quoting Oleg Nesterov (oleg@redhat.com):
> > > On 01/19, Serge E. Hallyn wrote:
> > > >
> > > > But so there does still need to be a patch modifying parse_options()
> > > > to return an error if pgrp= was not specified, right?
> > >
> > > Why? In that case we should use the caller's pgrp. This is what the
> > > current tries to do, why should the patch change this behaviour?
> >
> > Well, because Ian said that not specifying it is supposed to
> > be an error :)  I didn't quite understand why, so am fishing
> > for more info...
> 
> I think you misunderstood him. Or I am totally confused ;)
> 
> In any case. Both autofs and autofs4 use current's pgrp if this
> option was not specified, and these patches doesn't change this
> behaviour.
> 
> 
> Actually, I am very much surprized this one-liner patch has so
> many questions. Isn't it "obiously correct" ?

I'm not sure which one-liner you're talking about.  In fact,
the patch I'm looking at right now isn't the one i looked at
before my last response.  Dangit.

The patch turning the cached pid_t into a struct pid is
certainly mostly right.  It shouldn't store a pid_t.

But as for passing pid_t's in from userspace and especially
printing them out in error messages, I believe what Ian was
trying to do before, which seemed sensible, was to always
use values in the init_pid_ns.  After all, if you do a DPRINTK
with pid_vnr(somepid), then by the time a human reads the logs
the subjective pidns might no longer exist.  So for logs I'd
tend to agree with printing out the pid_t in the init_pid_ns.

-serge

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

[Oleg Nesterov]

On 01/19, Serge E. Hallyn wrote:
>
> Quoting Oleg Nesterov (oleg@redhat.com):
> > 
> > Actually, I am very much surprized this one-liner patch has so
> > many questions. Isn't it "obiously correct" ?
> 
> I'm not sure which one-liner you're talking about.  In fact,
> the patch I'm looking at right now isn't the one i looked at
> before my last response.  Dangit.
> 
> The patch turning the cached pid_t into a struct pid is
> certainly mostly right.  It shouldn't store a pid_t.

This is the next patch. This one does

	--- CUR/fs/autofs/inode.c~1_AUTOFS	2009-01-12 23:07:46.000000000 +0100
	+++ CUR/fs/autofs/inode.c	2009-01-18 06:18:49.000000000 +0100
	@@ -78,7 +78,7 @@ static int parse_options(char *options, 
	 
		*uid = current_uid();
		*gid = current_gid();
	-	*pgrp = task_pgrp_nr(current);
	+	*pgrp = task_pgrp_vnr(current);
	 
		*minproto = *maxproto = AUTOFS_PROTO_VERSION;

> But as for passing pid_t's in from userspace

passing pid_t's in from userspace uses current namespace, with
or without the patch.

> and especially
> printing them out in error messages, I believe what Ian was
> trying to do before, which seemed sensible, was to always
> use values in the init_pid_ns.  After all, if you do a DPRINTK
> with pid_vnr(somepid), then by the time a human reads the logs
> the subjective pidns might no longer exist.  So for logs I'd
> tend to agree with printing out the pid_t in the init_pid_ns.

OK, may be this makes sense, this was not discussed yet. This
can be changed. But otoh, if this code runs within the sub
namespace, then it is not easy to see why oz_mode == true
if we print the value in init_pid_ns.

Oleg.

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

[Serge E. Hallyn]

Quoting Oleg Nesterov (oleg@redhat.com):
> On 01/19, Serge E. Hallyn wrote:
> >
> > Quoting Oleg Nesterov (oleg@redhat.com):
> > > 
> > > Actually, I am very much surprized this one-liner patch has so
> > > many questions. Isn't it "obiously correct" ?
> > 
> > I'm not sure which one-liner you're talking about.  In fact,
> > the patch I'm looking at right now isn't the one i looked at
> > before my last response.  Dangit.
> > 
> > The patch turning the cached pid_t into a struct pid is
> > certainly mostly right.  It shouldn't store a pid_t.
> 
> This is the next patch. This one does
> 
> 	--- CUR/fs/autofs/inode.c~1_AUTOFS	2009-01-12 23:07:46.000000000 +0100
> 	+++ CUR/fs/autofs/inode.c	2009-01-18 06:18:49.000000000 +0100
> 	@@ -78,7 +78,7 @@ static int parse_options(char *options, 
> 	 
> 		*uid = current_uid();
> 		*gid = current_gid();
> 	-	*pgrp = task_pgrp_nr(current);
> 	+	*pgrp = task_pgrp_vnr(current);

Ok, that was the one I had looked at earlier (though now I can't find
it).  But that just seems wrong to me.  We should certainly not be
caching a pid_vnr in the kernel.  That is imo incomparably worse than
storing a pid_nr.

Can we just jump straight to caching the struct pid?

> 		*minproto = *maxproto = AUTOFS_PROTO_VERSION;
> 
> > But as for passing pid_t's in from userspace
> 
> passing pid_t's in from userspace uses current namespace, with
> or without the patch.

Which makes sense on the one hand, but OTOH could be confusing
if as I requested we print out init_pid_ns values.  (sigh)

> > and especially
> > printing them out in error messages, I believe what Ian was
> > trying to do before, which seemed sensible, was to always
> > use values in the init_pid_ns.  After all, if you do a DPRINTK
> > with pid_vnr(somepid), then by the time a human reads the logs
> > the subjective pidns might no longer exist.  So for logs I'd
> > tend to agree with printing out the pid_t in the init_pid_ns.
> 
> OK, may be this makes sense, this was not discussed yet. This
> can be changed. But otoh, if this code runs within the sub
> namespace, then it is not easy to see why oz_mode == true
> if we print the value in init_pid_ns.
> 
> Oleg.

Yes... would it be overkill to just print both?

Or maybe I'm being silly.  Every pidns goes away eventually
including the init_pid_ns :)

-serge

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

[Oleg Nesterov]

On 01/19, Serge E. Hallyn wrote:
>
> Quoting Oleg Nesterov (oleg@redhat.com):
> > 
> > This is the next patch. This one does
> > 
> > 	--- CUR/fs/autofs/inode.c~1_AUTOFS	2009-01-12 23:07:46.000000000 +0100
> > 	+++ CUR/fs/autofs/inode.c	2009-01-18 06:18:49.000000000 +0100
> > 	@@ -78,7 +78,7 @@ static int parse_options(char *options, 
> > 	 
> > 		*uid = current_uid();
> > 		*gid = current_gid();
> > 	-	*pgrp = task_pgrp_nr(current);
> > 	+	*pgrp = task_pgrp_vnr(current);
> 
> Ok, that was the one I had looked at earlier (though now I can't find
> it).  But that just seems wrong to me.  We should certainly not be
> caching a pid_vnr in the kernel.  That is imo incomparably worse than
> storing a pid_nr.

We do not cache it. We use this pgrp as an argument for find_pid()
right after return from parse_options(). And find_pid() uses
current->nsproxy->pid_ns. That is why this is bugfix.

> Can we just jump straight to caching the struct pid?

Of course it is ugly to store pid_t and then call find_pid(),
I don't understand why the code was written this way. But I
am not going to cleanup this code ;)

(note also that the 2nd patch I sent for autofs4 does not use
 pid_t at all).

> > passing pid_t's in from userspace uses current namespace, with
> > or without the patch.
>
> Which makes sense on the one hand, but OTOH could be confusing
> if as I requested we print out init_pid_ns values.  (sigh)

But it is not possible to pass the global pid_t from within
the subnamespace via "pgrp=" option, automount (or whatever)
just can't know it when it runs in the subnamespace.

> Yes... would it be overkill to just print both?

perharps, I don't know...

But this is imho a bit off-topic, we can change the debugging
output later any way we like.

Oleg.

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

[Serge E. Hallyn]

Quoting Oleg Nesterov (oleg@redhat.com):
> On 01/19, Serge E. Hallyn wrote:
> >
> > Quoting Oleg Nesterov (oleg@redhat.com):
> > > 
> > > This is the next patch. This one does
> > > 
> > > 	--- CUR/fs/autofs/inode.c~1_AUTOFS	2009-01-12 23:07:46.000000000 +0100
> > > 	+++ CUR/fs/autofs/inode.c	2009-01-18 06:18:49.000000000 +0100
> > > 	@@ -78,7 +78,7 @@ static int parse_options(char *options, 
> > > 	 
> > > 		*uid = current_uid();
> > > 		*gid = current_gid();
> > > 	-	*pgrp = task_pgrp_nr(current);
> > > 	+	*pgrp = task_pgrp_vnr(current);
> > 
> > Ok, that was the one I had looked at earlier (though now I can't find
> > it).  But that just seems wrong to me.  We should certainly not be
> > caching a pid_vnr in the kernel.  That is imo incomparably worse than
> > storing a pid_nr.
> 
> We do not cache it. We use this pgrp as an argument for find_pid()
> right after return from parse_options(). And find_pid() uses
> current->nsproxy->pid_ns. That is why this is bugfix.

So it does.  And so it is.

thanks,
-serge

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

[Ian Kent]

On Mon, 2009-01-19 at 22:31 +0100, Oleg Nesterov wrote:
> On 01/19, Serge E. Hallyn wrote:
> >
> > Quoting Oleg Nesterov (oleg@redhat.com):
> > > 
> > > This is the next patch. This one does
> > > 
> > > 	--- CUR/fs/autofs/inode.c~1_AUTOFS	2009-01-12 23:07:46.000000000 +0100
> > > 	+++ CUR/fs/autofs/inode.c	2009-01-18 06:18:49.000000000 +0100
> > > 	@@ -78,7 +78,7 @@ static int parse_options(char *options, 
> > > 	 
> > > 		*uid = current_uid();
> > > 		*gid = current_gid();
> > > 	-	*pgrp = task_pgrp_nr(current);
> > > 	+	*pgrp = task_pgrp_vnr(current);
> > 
> > Ok, that was the one I had looked at earlier (though now I can't find
> > it).  But that just seems wrong to me.  We should certainly not be
> > caching a pid_vnr in the kernel.  That is imo incomparably worse than
> > storing a pid_nr.
> 
> We do not cache it. We use this pgrp as an argument for find_pid()
> right after return from parse_options(). And find_pid() uses
> current->nsproxy->pid_ns. That is why this is bugfix.
> 
> > Can we just jump straight to caching the struct pid?
> 
> Of course it is ugly to store pid_t and then call find_pid(),
> I don't understand why the code was written this way. But I
> am not going to cleanup this code ;)
> 
> (note also that the 2nd patch I sent for autofs4 does not use
>  pid_t at all).
> 
> > > passing pid_t's in from userspace uses current namespace, with
> > > or without the patch.
> >
> > Which makes sense on the one hand, but OTOH could be confusing
> > if as I requested we print out init_pid_ns values.  (sigh)
> 
> But it is not possible to pass the global pid_t from within
> the subnamespace via "pgrp=" option, automount (or whatever)
> just can't know it when it runs in the subnamespace.
> 
> > Yes... would it be overkill to just print both?
> 
> perharps, I don't know...
> 
> But this is imho a bit off-topic, we can change the debugging
> output later any way we like.

The use of pid_ts in debug statements could be confusing, but generally,
the debug output will only be used when the person gathering it is aware
of the environment it is collected in. We can change these as needed as
time passes.

We also send the pid_t via autofs request packets and it is used for
user space debug prints. It is open to the same confusion and I'm still
not sure how to deal with that but it isn't important to sort that out
now either, for the same reason as above.

Ian

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

[Ian Kent]

On Mon, 2009-01-19 at 20:17 +0100, Oleg Nesterov wrote:
> On 01/19, Serge E. Hallyn wrote:
> >
> > Quoting Oleg Nesterov (oleg@redhat.com):
> > > On 01/19, Serge E. Hallyn wrote:
> > > >
> > > > But so there does still need to be a patch modifying parse_options()
> > > > to return an error if pgrp= was not specified, right?
> > >
> > > Why? In that case we should use the caller's pgrp. This is what the
> > > current tries to do, why should the patch change this behaviour?
> >
> > Well, because Ian said that not specifying it is supposed to
> > be an error :)  I didn't quite understand why, so am fishing
> > for more info...
> 
> I think you misunderstood him. Or I am totally confused ;)
> 
> In any case. Both autofs and autofs4 use current's pgrp if this
> option was not specified, and these patches doesn't change this
> behaviour.
> 
> 
> Actually, I am very much surprized this one-liner patch has so
> many questions. Isn't it "obiously correct" ?

Maybe it is.

Sorry Serge, this was actually the first of two patches from Oleg and I
only copied you on the first (since that is where I started the
discussion), oops!

The reason this has become so difficult is largely my fault and it is
entirely due to my lack of understanding of how automount will play
within pid namespaces.

But your last reply to me was very helpful in this regard, thanks for
your patience. So this painful discussion has been useful, at least to
me.

I see you've already backed out part of your original change, which may
have been a little premature, as I can likely go back and review the
original patches now.

Ian

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

[H. Peter Anvin]

Ian Kent wrote:
>>
>> Actually, I am very much surprized this one-liner patch has so
>> many questions. Isn't it "obiously correct" ?
> 
> Maybe it is.
> 

The fact that something may not be *obviously* correct doesn't prevent 
it from be correct, of course.

	-hpa

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

[Oleg Nesterov]

On 01/20, Ian Kent wrote:
>
> I see you've already backed out part of your original change, which may
> have been a little premature,

I think this is fine. When/if we remove task_pgrp_nr() from fs/, we
can do the trivial patch which just removes this helper from sched.h.

And thanks for discussion. Somehow I forgot that the untested changes
is a pain for maintainers, and it was the bad idea to make the next
patch which depend on these changes.

Oleg.

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

[Ian Kent]

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.

Following the lengthy discussion, largely for my benefit, I'm now
looking at the patch with some idea of sensibility.

It seems to me that when setting sbi->oz_pgrp we should use
task_pgrp_vnr(). As Oleg says, and I agree, this should always give
correct results when checking if current is the session leader.

But I think that for debug logging we should always report numbers that
correspond to the object as it is seen in the global namespace as that
is the most likely reference point someone examining debug logging will
be working from.

Oleg, what should be the recommended call to report that, instead of the
task_pgrp_nr() call?

> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> 
> --- CUR/fs/autofs/inode.c~1_AUTOFS	2009-01-12 23:07:46.000000000 +0100
> +++ CUR/fs/autofs/inode.c	2009-01-18 06:18:49.000000000 +0100
> @@ -78,7 +78,7 @@ static int parse_options(char *options, 
>  
>  	*uid = current_uid();
>  	*gid = current_gid();
> -	*pgrp = task_pgrp_nr(current);
> +	*pgrp = task_pgrp_vnr(current);
>  
>  	*minproto = *maxproto = AUTOFS_PROTO_VERSION;
>  
> --- CUR/fs/autofs/root.c~1_AUTOFS	2008-10-10 00:13:53.000000000 +0200
> +++ CUR/fs/autofs/root.c	2009-01-18 06:15:42.000000000 +0100
> @@ -215,7 +215,7 @@ static struct dentry *autofs_root_lookup
>  	oz_mode = autofs_oz_mode(sbi);
>  	DPRINTK(("autofs_lookup: pid = %u, pgrp = %u, catatonic = %d, "
>  				"oz_mode = %d\n", task_pid_nr(current),
> -				task_pgrp_nr(current), sbi->catatonic,
> +				task_pgrp_vnr(current), sbi->catatonic,
>  				oz_mode));
>  
>  	/*
> @@ -550,7 +550,8 @@ static int autofs_root_ioctl(struct inod
>  	struct autofs_sb_info *sbi = autofs_sbi(inode->i_sb);
>  	void __user *argp = (void __user *)arg;
>  
> -	DPRINTK(("autofs_ioctl: cmd = 0x%08x, arg = 0x%08lx, sbi = %p, pgrp = %u\n",cmd,arg,sbi,task_pgrp_nr(current)));
> +	DPRINTK(("autofs_ioctl: cmd = 0x%08x, arg = 0x%08lx, sbi = %p, pgrp = %u\n",
> +		cmd, arg, sbi, task_pgrp_vnr(current)));
>  
>  	if (_IOC_TYPE(cmd) != _IOC_TYPE(AUTOFS_IOC_FIRST) ||
>  	     _IOC_NR(cmd) - _IOC_NR(AUTOFS_IOC_FIRST) >= AUTOFS_IOC_COUNT)
> 

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

[Oleg Nesterov]

On 01/23, Ian Kent wrote:
>
> But I think that for debug logging we should always report numbers that
> correspond to the object as it is seen in the global namespace as that
> is the most likely reference point someone examining debug logging will
> be working from.
>
> Oleg, what should be the recommended call to report that, instead of the
> task_pgrp_nr() call?

task_pgrp_nr_ns(tsk, &init_pid_ns) gives the same result. Except, if the
tsk has already exited it returns 0.


But, Ian, if you prefer to use task_pgrp_nr() - just use it. Personally,
I'd like to kill task_pgrp_nr(), but I can't "prove" it should die.

Oleg.

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

[Ian Kent]

On Fri, 2009-01-23 at 09:13 +0100, Oleg Nesterov wrote:
> On 01/23, Ian Kent wrote:
> >
> > But I think that for debug logging we should always report numbers that
> > correspond to the object as it is seen in the global namespace as that
> > is the most likely reference point someone examining debug logging will
> > be working from.
> >
> > Oleg, what should be the recommended call to report that, instead of the
> > task_pgrp_nr() call?
> 
> task_pgrp_nr_ns(tsk, &init_pid_ns) gives the same result. Except, if the
> tsk has already exited it returns 0.
> 
> 
> But, Ian, if you prefer to use task_pgrp_nr() - just use it. Personally,
> I'd like to kill task_pgrp_nr(), but I can't "prove" it should die.

I'm not concerned with what's used so much as fitting in with the pid
namespace effort and maintaining sensible debug output. This stuff has
been in autofs4 since long before the pid namespace work began and the
changes to it weren't done by me anyway.

Ian