Date:   Thu, 25 Oct 2018 14:56:18 -0700
From:   Enke Chen <enkechen@...co.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Arnd Bergmann <arnd@...db.de>,
        Khalid Aziz <khalid.aziz@...cle.com>,
        Kate Stewart <kstewart@...uxfoundation.org>,
        Helge Deller <deller@....de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Christian Brauner <christian@...uner.io>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Dave Martin <Dave.Martin@....com>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        Michal Hocko <mhocko@...nel.org>,
        Rik van Riel <riel@...riel.com>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Roman Gushchin <guro@...com>,
        Marcos Paulo de Souza <marcos.souza.org@...il.com>,
        Oleg Nesterov <oleg@...hat.com>,
        Dominik Brodowski <linux@...inikbrodowski.net>,
        Cyrill Gorcunov <gorcunov@...nvz.org>,
        Yang Shi <yang.shi@...ux.alibaba.com>,
        Jann Horn <jannh@...gle.com>,
        Kees Cook <keescook@...omium.org>, x86@...nel.org,
        linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
        "Victor Kamensky (kamensky)" <kamensky@...co.com>,
        xe-linux-external@...co.com, Stefan Strogin <sstrogin@...co.com>,
        Enke Chen <enkechen@...co.com>
Subject: Re: [PATCH v2] kernel/signal: Signal-based pre-coredump notification

Hi, Eric:

Please see my replied inline.

On 10/25/18 5:23 AM, Eric W. Biederman wrote:
> Enke Chen <enkechen@...co.com> writes:
> 
>> Hi, Eric:
>>
>> Thanks for your comments. Please see my replies inline.
>>
>> On 10/24/18 6:29 AM, Eric W. Biederman wrote:
>>> Enke Chen <enkechen@...co.com> writes:
>>>
>>>> For simplicity and consistency, this patch provides an implementation
>>>> for signal-based fault notification prior to the coredump of a child
>>>> process. A new prctl command, PR_SET_PREDUMP_SIG, is defined that can
>>>> be used by an application to express its interest and to specify the
>>>> signal (SIGCHLD or SIGUSR1 or SIGUSR2) for such a notification. A new
>>>> signal code (si_code), CLD_PREDUMP, is also defined for SIGCHLD.
>>>>
>>>> Changes to prctl(2):
>>>>
>>>>    PR_SET_PREDUMP_SIG (since Linux 4.20.x)
>>>>           Set the child pre-coredump signal of the calling process to
>>>>           arg2 (either SIGUSR1, or SIUSR2, or SIGCHLD, or 0 to clear).
>>>>           This is the signal that the calling process will get prior to
>>>>           the coredump of a child process. This value is cleared across
>>>>           execve(2), or for the child of a fork(2).
>>>>
>>>>           When SIGCHLD is specified, the signal code will be set to
>>>>           CLD_PREDUMP in such an SIGCHLD signal.
>>>
>>> Your signal handling is still not right.  Please read and comprehend
>>> siginfo_layout.
>>>
>>> You have not filled in all of the required fields for the SIGCHLD case.
>>> For the non SIGCHLD case you are using si_code == 0 == SI_USER which is
>>> very wrong.  This is not a user generated signal.
>>>
>>> Let me say this slowly.  The pair si_signo si_code determines the union
>>> member of struct siginfo.  That needs to be handled consistently. You
>>> aren't.  I just finished fixing this up in the entire kernel and now you
>>> are trying to add a usage that is worst than most of the bugs I have
>>> fixed.  I really don't appreciate having to deal with no bugs.
>>>
>>
>> My apologies. I will investigate and make them consistent.
>>
>>>
>>>
>>> Further siginfo can be dropped.  Multiple signals with the same signal
>>> number can be consolidated.  What is your plan for dealing with that?
>>
>> The primary application for the early notification involves a process
>> manager which is responsible for re-spawning processes or initiating
>> the control-plane fail-over. There are two models:
>>
>> One model is to have 1:1 relationship between a process manager and
>> application process. There can only be one predump-signal (say, SIGUSR1)
>> from the child to the parent, and will unlikely be dropped or consolidated.
>>
>> Another model is to have 1:N where there is only one process manager with
>> multiple application processes. One of the RT signal can be used to help
>> make it more reliable.
> 
> Which suggests you want one of the negative si_codes, and to use the _rt
> siginfo member like sigqueue.

It seems that we do not need to touch the si_codes. A dedicated signal
for the pre-coredump notification is simpler and more robust. There are
enough RT signal numbers available.

> 
>>> Other code paths pair with wait to get the information out.  There
>>> is no equivalent of wait in your code.
>>
>> I was not aware of that before.  Let me investigate.
>>
>>>
>>> Signals can be delayed by quite a bit, scheduling delays etc.  They can
>>> not provide any meaningful kind of real time notification.
>>>
>>
>> The timing requirement is about 50-100 msecs for BFD.  Not sure if that
>> qualifies as "real time".  This mechanism has worked well in deployment
>> over the years.
> 
> It would help if those numbers were put into the patch description so
> people can tell if the mechanism is quick enough.

I will do as suggested, but at the risk of making the patch description
longer than the patch itself :-)

> 
>>> So between delays and loss of information signals appear to be a very
>>> poor fit for this usecase.
>>>
>>> I am concerned about code that does not fit the usecase well because
>>> such code winds up as code that no one cares about that must be
>>> maintained indefinitely, because somewhere out there there is one use
>>> that would break if the interface was removed.  This does not feel like
>>> an interface people will want to use and maintain in proper working
>>> order forever.
>>>
>>> Ugh.  Your test case is even using signalfd.  So you don't even want
>>> this signal to be delivered as a signal.
>>
>> I actually tested sigaction()/waitpid() as well. If there is a preference,
>> I can check in the sigaction()/waitpid() version instead.
>>
>>>
>>> You add an interface that takes a pointer and you don't add a compat
>>> interface.  See Oleg's point of just returning the signal number in the
>>> return code.
>>
>> This is what Oleg said "but I won't insist, this is subjective and cosmetic".
>>
>> It is no big deal either way. It just seems less work if we do not keep
>> adding exceptions to the prctl(2) manpage:
>>  
>> prctl(2):
>>
>>        On success, PR_GET_DUMPABLE,   PR_GET_KEEPCAPS,   PR_GET_NO_NEW_PRIVS,   PR_CAPBSET_READ,    PR_GET_TIMING,    PR_GET_SECUREBITS,
>>        PR_MCE_KILL_GET,  PR_CAP_AMBIENT+PR_CAP_AMBIENT_IS_SET,  and  (if  it returns) PR_GET_SECCOMP return the nonnegative values described
>>        above.  All other option values return 0 on success.  On error, -1 is returned, and errno is set appropriately.
> 
> More work in the man page versus less work in the kernel, and less code
> to maintain.  I will vote for more work in the man page.

Oleg has given me a pass on this one. It is one line. But I still
prefer not to change back unless there is strong opinion... 

> 
>>> Now I am wondering how well prctl works from a 32bit process on a 64bit
>>> kernel.  At first glance it looks like it probably does not work.
>>>
>>
>> I am not sure which part would be problematic.
> 
> 32bit pointers need to be translated into 64bit pointers.  If the system
> call does not zero extend them.  Plus structure sizes.
> 
> I think prctl is just inside the line where problems happen but it is so
> close to the line of structure size differences that it makes me
> nervous.  Typically pointers in structures are what cause system calls
> to cross that line.
> 
>>> Consistency with PDEATHSIG is not a good argument for anything.
>>> PDEATHSIG at the present time is unusable in the real world by most
>>> applications that want something like it.
>>
>> Agreed, PDEATHSIG seems to have a few issues ...
>>
>>>
>>> So far I see an interface that even you don't want to use as designed,
>>> that is implemented incorrectly.
>>>
>>> The concern is real and deserves to be addressed.  I don't think signals
>>> are the right way to handle it, and certainly not this patch as it
>>> stands.
>>
>> I will address your concerns on the patch. Regarding the requirement and the
>> overall solution, if there are specific questions that I have not answered,
>> please let me know.
> 
> So far so good.
> 

Thanks. Reviews from folks on the list have certainly made the code shorter,
simpler and cleaner.

-- Enke

Powered by blists - more mailing lists