From mboxrd@z Thu Jan 1 00:00:00 1970 From: Enke Chen Subject: Re: [PATCH v2] kernel/signal: Signal-based pre-coredump notification Date: Thu, 25 Oct 2018 14:24:19 -0700 Message-ID: <3da93f7c-972d-14e0-dae2-f0217ba03dc1@cisco.com> References: <458c04d8-d189-4a26-729a-bb1d1d751534@cisco.com> <87sh0vpj5q.fsf@xmission.com> <87zhv2md04.fsf@xmission.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <87zhv2md04.fsf@xmission.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: "Eric W. Biederman" Cc: Thomas Gleixner , Ingo Molnar , Borislav Petkov , "H. Peter Anvin" , Peter Zijlstra , Arnd Bergmann , Khalid Aziz , Kate Stewart , Helge Deller , Greg Kroah-Hartman , Al Viro , Andrew Morton , Christian Brauner , Catalin Marinas , Will Deacon , Dave Martin , Mauro Carvalho Chehab , Michal Hocko , Rik van Riel , "Kirill A. Shutemov" List-Id: linux-arch.vger.kernel.org Hi, Eric: I have a couple comments inlined. >> On Wed, Oct 24, 2018 at 3:30 PM Eric W. Biederman wrote: >>> Enke Chen 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. >> [...] >>> Ugh. Your test case is even using signalfd. So you don't even want >>> this signal to be delivered as a signal. >> >> Just to make sure everyone's on the same page: You're suggesting that >> it might make sense to deliver the pre-dump notification via a new >> type of file instead (along the lines of signalfd, timerfd, eventfd >> and so on)? > > My real complaint was that the API was not being tested in the way it > is expected to be used. Which makes a test pretty much useless as some > aspect userspace could regress and the test would not notice because it > is testing something different. > > As I stated in a prior email, I have test code for both sigaction/waipid(), and signefd(). As the sigaction/waitpid is more widely used and that is what you prefer, I will change the selftest code to reflect that in the next version. Actually I should separate out the selftest code. > > I do think that a file descriptor based API might be a good alternative > to a signal based API. The proc connector and signals are not the only > API solution. > > The common solution to this problem is that distributions defailt the > rlimit core file size to 0. We do need coredumps in order to have the bugs fixed. Thanks. -- Enke From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from alln-iport-7.cisco.com ([173.37.142.94]:12509 "EHLO alln-iport-7.cisco.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726085AbeJZF6l (ORCPT ); Fri, 26 Oct 2018 01:58:41 -0400 Subject: Re: [PATCH v2] kernel/signal: Signal-based pre-coredump notification References: <458c04d8-d189-4a26-729a-bb1d1d751534@cisco.com> <87sh0vpj5q.fsf@xmission.com> <87zhv2md04.fsf@xmission.com> From: Enke Chen Message-ID: <3da93f7c-972d-14e0-dae2-f0217ba03dc1@cisco.com> Date: Thu, 25 Oct 2018 14:24:19 -0700 MIME-Version: 1.0 In-Reply-To: <87zhv2md04.fsf@xmission.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-arch-owner@vger.kernel.org List-ID: To: "Eric W. Biederman" Cc: Thomas Gleixner , Ingo Molnar , Borislav Petkov , "H. Peter Anvin" , Peter Zijlstra , Arnd Bergmann , Khalid Aziz , Kate Stewart , Helge Deller , Greg Kroah-Hartman , Al Viro , Andrew Morton , Christian Brauner , Catalin Marinas , Will Deacon , Dave Martin , Mauro Carvalho Chehab , Michal Hocko , Rik van Riel , "Kirill A. Shutemov" , Roman Gushchin , Marcos Paulo de Souza , Oleg Nesterov , Dominik Brodowski , Cyrill Gorcunov , Yang Shi , Jann Horn , Kees Cook , x86@kernel.org, linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, "Victor Kamensky (kamensky)" , xe-linux-external@cisco.com, Stefan Strogin , Enke Chen Message-ID: <20181025212419.ma8sQq4f0Q7DgR9kzMx0-q_vbbjL2YVb-gb0upwF7I4@z> Hi, Eric: I have a couple comments inlined. >> On Wed, Oct 24, 2018 at 3:30 PM Eric W. Biederman wrote: >>> Enke Chen 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. >> [...] >>> Ugh. Your test case is even using signalfd. So you don't even want >>> this signal to be delivered as a signal. >> >> Just to make sure everyone's on the same page: You're suggesting that >> it might make sense to deliver the pre-dump notification via a new >> type of file instead (along the lines of signalfd, timerfd, eventfd >> and so on)? > > My real complaint was that the API was not being tested in the way it > is expected to be used. Which makes a test pretty much useless as some > aspect userspace could regress and the test would not notice because it > is testing something different. > > As I stated in a prior email, I have test code for both sigaction/waipid(), and signefd(). As the sigaction/waitpid is more widely used and that is what you prefer, I will change the selftest code to reflect that in the next version. Actually I should separate out the selftest code. > > I do think that a file descriptor based API might be a good alternative > to a signal based API. The proc connector and signals are not the only > API solution. > > The common solution to this problem is that distributions defailt the > rlimit core file size to 0. We do need coredumps in order to have the bugs fixed. Thanks. -- Enke