Re: [PATCH 0/2] struct pid-ify autofs4

[Eric W. Biederman]

Ian Kent <raven@xxxxxxxxxx> writes:

> On Mon, 2012-09-24 at 15:34 +0200, Miklos Szeredi wrote:
>> Ian Kent <raven@xxxxxxxxxx> writes:
>> 
>> > On Fri, 2012-09-21 at 17:44 +0200, Miklos Szeredi wrote:
>> >> Miklos Szeredi <miklos@xxxxxxxxxx> writes:
>> >> 
>> >> > These two patches change autofs4 to store struct pid pointers instead of pid_t
>> >> > values.
>> >> >
>> >> > Fixed various issues with the previous post.  Not tested, handle with
>> >> > care!
>> >> 
>> >> Customer gave positive test results.
>> >
>> > For what exactly, there's no problem description in these patches?
>> 
>> From what I understand (and I'm not an expert by any means) is that
>> autofs doesn't work if containers are used.  The first patch fixes this.
>
> Yeah, the problem with that is that "autofs doesn't work if containers
> are used" is ill defined since there are use cases where it does, I
> believe. At the very least, ill defined in my view of things.

An easy complaint is that task->pid and task->tgid are deprecated fields
in the task structure.  Things that use pids should in use struct pid
values instead.

The trick part of using struct pid values is that there are times when
you need to interact with userspace.  And the question is which pid
namespace is your userspace process in so that you can convert
to and from the proper pid namespace.

The pgrp option on the mount of autofs is buggy because the pid
namespace of the process group is not captured at the time of mount
and so userspace could think it is talking about one process group
while autofs is talking about another process group.  This is a
practical problem if the process that mounts autofs is not running
in the initial pid namespace.

There is a second question.  What happens if the oz_pgrp exists
and then pids wrap around and another process uses the same process
group number.  Currently the autofs code will treat the new proces
group like the old one leading to unexpected behavior.  Which I believe
will be autofs mounts not happening when desired.

Another problem is what happens when a process triggers an automount.
Today we will report the pid and the tgid in the initial pid namespace
of the process that triggered the mount.

So what I can see is that today if the process that mounts autofs
(aka the process at the other end of the autofs pipe) is not in
the initial pid namespace things will go awry, as autofs will report
pid values that make no sense to anyone.

I would like to say the patches fix that problem (and they come close)
however they still translate everything into the initial pid namespace.

> But I can't even sensibly discuss it because of the lack of specified
> use cases and requirements for each. So, there's a chance this will
> break another case that does work.

That seems to be a reasonable concern.  Education so that the
differences in the code are comprehensible.

I am also curious about which case people were seeing problems with as
these patches were reported to be tested and to have fixed a customer
problem.  The only case that is particularly clear to me that these
patches will fix is the case of process group wrap around, but proces
group wrap around should be exceedingly rare.

To handle mounts made outside of the initial pid namespace it appears
that all that is needed is to caputure the pid namespace of the process
that mounts autofs and uses pid_nr_ns to format pids into that
namespace, inside of autofs

It is my feeling that in addition to capture oz_pgrp on mount the
automount and autofs_dev_ioctl_set_pipefd should capture a pid namespace
for formating pids delivered into the pipe.  That would allow the
automount process itself to live outside of the initial pid namespace.

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/