Re: [PATCH v2 3/3] Make core_pattern support namespace

[Eric W. Biederman]

Zhao Lei <zhaolei@xxxxxxxxxxxxxx> writes:

> Hi, Eric
>
>> -----Original Message-----
>> From: Eric W. Biederman [mailto:ebiederm@xxxxxxxxxxxx]
>> Sent: Wednesday, March 23, 2016 6:43 AM
>> To: Zhao Lei <zhaolei@xxxxxxxxxxxxxx>
>> Cc: linux-kernel@xxxxxxxxxxxxxxx; containers@xxxxxxxxxxxxxxxxxxxxxxxxxx;
>> 'Mateusz Guzik' <mguzik@xxxxxxxxxx>; 'Kamezawa Hiroyuki'
>> <kamezawa.hiroyu@xxxxxxxxxxxxxx>
>> Subject: Re: [PATCH v2 3/3] Make core_pattern support namespace
>> 
>> Zhao Lei <zhaolei@xxxxxxxxxxxxxx> writes:
>> 
>> > Hi, Eric
>> >
>> >> -----Original Message-----
>> >> From: Eric W. Biederman [mailto:ebiederm@xxxxxxxxxxxx]
>> >> Sent: Tuesday, March 22, 2016 5:25 AM
>> >> To: Zhao Lei <zhaolei@xxxxxxxxxxxxxx>
>> >> Cc: linux-kernel@xxxxxxxxxxxxxxx; containers@xxxxxxxxxxxxxxxxxxxxxxxxxx;
>> >> 'Mateusz Guzik' <mguzik@xxxxxxxxxx>; 'Kamezawa Hiroyuki'
>> >> <kamezawa.hiroyu@xxxxxxxxxxxxxx>
>> >> Subject: Re: [PATCH v2 3/3] Make core_pattern support namespace
>> >>
>> >> Zhao Lei <zhaolei@xxxxxxxxxxxxxx> writes:
>> >>
>> >> > Hi, Eric
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: Eric W. Biederman [mailto:ebiederm@xxxxxxxxxxxx]
>> >>
>> >> > Let me make a summarize:
>> >> > You think this way is not acceptable, because the pipe program is running
>> >> > in the panic-process's namespace context.
>> >>
>> >> Actually my view is that your patchset is not acceptable because it
>> >> is implemented in a way that is not backwards compatible (AKA it can
>> >> break existing configurations that remain unchanged) and your
>> >> implementation does not appear in the least safe from malicious users.
>> >>
>> >> There is also a problem that your patchset is simply buggy for what it
>> >> tries to implement, as using pid_ns_for_children and the multiple kbuild
>> >> robot emails testifies.
>> >>
>> >> > And in my view, a pipe program in the host's top level namespace is also
>> >> > a problem.
>> >> >
>> >> > Let us think a container, to make it act as a real machine, when a program
>> >> > panic, linux kernel should dump it into the container's filesystem.
>> >> >
>> >> > For the kernel, to keep the current way of forking pipe program by kthread,
>> >> > just let the pipe thread running in the container's namespace, instead the
>> >> host,
>> >> > may solve the problem in current kernel.
>> >> >
>> >> > What is your opinion?
>> >> >
>> >> > Btw, this patch is trying to solve the problem descripted in thread named:
>> >> > "piping core dump to a program escapes container" in
>> >> >
>> >>
>> http://lists.linuxfoundation.org/pipermail/containers/2015-December/036476.
>> >> html
>> >> > Maybe using a userspace tool can make container dump to anywhere,
>> >> > but for kernel ifself, it is better to solve above problem if we can.
>> >>
>> >> I think it would be great to find a way to run a core dump helper and
>> >> otherwise allow setting the core dump pattern in a container in a way
>> >> that is safe from malicious users and does not break existing setups.
>> >>
>> > So, there is following problem:
>> > 1: safe from malicious users
>> >   We can try to find a way to fork process which have no relationship
>> >   with the panic process.
>> > 2: Bug in patch
>> >   It can be fixed, but I'd rather get a conclusion of this discussion
>> >   before fix.
>> > 3: Backwards compatible
>> >   It maybe the biggest problem in discussion, this patch is used to let
>> >   container dump files into container, it is different with current action.
>> >   Before patch:
>> >     File type dump_pattern: dump to container
>> >     Pipe type dump_pattern: dump to host
>> >   After patch:
>> >     File type dump_pattern: dump to container
>> >     Pipe type dump_pattern: dump to container
>> >   The second design seems better but not compatible with current kernel,
>> >   but this patch can not fix to keep compatible because it is the patch's
>> >   function.
>> >   Maybe we can make some workagound, as:
>> >   a. Add a kernel config to let the old style as default.
>> >   b. keep old style, and add "||" for core_pattern, as
>> >     echo "|| /root/container_dumper" >/proc/sys/kernel/core_pattern
>> >     to dump to container.
>> >
>> >   What is your opinion about it?
>> 
>
> Thanks for detailed answer.
>
>> There are two parts to backwards compatibility.
>> 
>> 1) How should core patterns that are set outside of any container be
>>    treated?
>> 
>>    The patchset under discussion imported core patterns set outside of
>>    containers into containers completely trivially changing their
>>    behavior and breaking backwards compatibility.  That is just not
>>    acceptable.
>> 
> Before this patch, setting pattern outside container will change setting
> in container.
> And after this patch, host and container are separated, they have respective
> setting, and no relationship except the container will copy host's setting
> in create.
>
> If we don't think compatibility, it may be a good idea, the container is only
> allowed to change the core_pattern by itself.
> It is strange that the core_pattern in container was changed but user/process
> in container do nothing.
>
> But just as you said, it have compatibility problem, someone maybe using
> this feature to modify core_pattern in container in host.
>
> To keep the compatibility and allow custom core_pattern in continer,
> maybe we can:
> 1: If no process setting core_pattern in container, the container's
>   core_pattern keep same copy with host.
>   And if core_pattern in host changed this time, the container's pattern
>   changed with host.
> 2: If core_pattern in container changed by some one/process in container,
>   it is separated with host, and modification in host will not affect container.
>
> What is your opinion about it?

That sounds correct.  Use the core_pattern for the container if it
exists otherwise use a core_pattern for an outer container/host.

>> 2) How should core patterns inside of containers be treated?
>> 
>>    There are corner cases that I am not thinking of that will cause
>>    regressions but I think we can reasonably assume that core patterns
>>    are not set inside of containers today.  Assuming that is true,
>>    then setting a core pattern inside of a container is the only thing
>>    that the kernel needs to detect to handle working inside of a
>>    container.
>> 
>>    The practical question I see for this case is which parent process
>>    needs to be used for your core dump helper, and which set of
>>    namespaces that parent helper has.
>> 
>>    I will also remind people thinking about these issues that containers
>>    can be run recursisvely.
>> 
> Got it.
> I'll try to find a way.

Eric