From owner-freebsd-hackers@freebsd.org Mon Apr 16 20:07:26 2018 Return-Path: Delivered-To: freebsd-hackers@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id B1FAEFA1EA5 for ; Mon, 16 Apr 2018 20:07:26 +0000 (UTC) (envelope-from kib@freebsd.org) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 2100175DD7 for ; Mon, 16 Apr 2018 20:07:26 +0000 (UTC) (envelope-from kib@freebsd.org) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id w3GK7DoV056495 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Mon, 16 Apr 2018 23:07:16 +0300 (EEST) (envelope-from kib@freebsd.org) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua w3GK7DoV056495 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id w3GK7DNA056494; Mon, 16 Apr 2018 23:07:13 +0300 (EEST) (envelope-from kib@freebsd.org) X-Authentication-Warning: tom.home: kostik set sender to kib@freebsd.org using -f Date: Mon, 16 Apr 2018 23:07:13 +0300 From: Konstantin Belousov To: Thomas Munro Cc: freebsd-hackers@freebsd.org Subject: Re: PROC_SET_PDEATHSIG to get a signal when your parent exits Message-ID: <20180416200713.GL1774@kib.kiev.ua> References: <20180414180415.GC1774@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=ham autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Apr 2018 20:07:26 -0000 On Tue, Apr 17, 2018 at 01:06:57AM +1200, Thomas Munro wrote: > Hi Konstantin, > > Thanks for the review! > > On 15 April 2018 at 06:04, Konstantin Belousov wrote: > > On Sat, Apr 14, 2018 at 04:44:51PM +1200, Thomas Munro wrote: > >> [...] Should this somehow share > >> something with the Linux compat support for prctl(PR_SET_PDEATHSIG, > >> ...)? Would people want this? > > > > Do we support prctl(2) in linuxolator ? Even if yes, I doubt that > > there is PR_SET_PDEATHSIG feature. > > In sys/compat/linux/linux_misc.c there is a linux_prctl() function and > it stores the value in em->pdeath_signal, but I don't see any code in > the tree that actually does anything with it. > > >> That tests a few interesting cases I thought of, but note that I > >> haven't yet tested 32 bit support or the setuid/getuid logic. > > > > Compile the test program using "cc -m32 -o test-deathsig-32 ...." > > to easily get binary for compat32 testing. > > It turned out that I needed to modify freebsd32_procctl too. > > > We have test framework, and several procctl(2) tests already. > > Please look at the tests/sys/kern/reaper.c. You might consider > > converting your test to the framework and also adding it to the > > base system. > > Done. > > > Other comments inline. You may create the account at > > https://reviews.freebsd.org and putting patch there. > > Ok. Here is my -v2 patch: > > https://reviews.freebsd.org/differential/diff/41511/ You did not created a review. You only uploaded the diff, but there are more steps after the 'create' button. In particular, you put the description of the patch there, and assign reviewers. Put kib@freebsd.org for the start. Also, your diff lacks context information, so the review is even less useful than the patch in the mail. Please use 'svn diff -x -U999999' command when generating the diff for upload. > > >> +.It Dv PROC_SET_PDEATHSIG > >> +Request the delivery of a signal when the parent of the calling > >> +process exits. > >> +Use id type P_PID and id 0. > > > > Use > > .Fa id > > type > > .Dv P_PID > > and > > .Fa id > > 0. > > Fixed. > > > In fact, '0' is ok as an alias for the current pid, but not allowing current > > pid as the argument is strange. > > Ok, now it also accepts the caller's PID. > > > I can see why you only allow the process to set the parent signal only > > on itself, but also can see that it does not cost anything to allow > > arbitrary pid. > > I wondered about that, but it seemed a bit strange to allow you to > request that *some other* PID should get a signal when *its* parent > exits. I think a different interface for "signal me when process X" > (not just my parent) might be useful, but that's a different feature. Ok. > > > But I do not see how would pfind(0) return curproc. Don't you need to add > > some code for this in kern_procctl() ? > > No, I just use td->td_proc. If we only support with the current > process we don't need to call pfind() at all. Ok. > > >> +The value is cleared for the children > >> +of fork() and when executing set-user-ID or set-group-ID binaries. > > .Xr fork 2 > >> +.Fa arg > >> +must point to a value of type int indicating the signal > >> +that should be delivered to the caller. Use zero to cancel the > > > > Start new sentence from the new line. > > Done. > > >> +delivery of signals. > >> +.It Dv PROC_GET_PDEATHSIG > >> +Query the current signal number that will be delivered when the parent > >> +of the calling process exits. > >> +Use id type P_PID and id 0. > > > > Same markup. > > Done. > > >> + /* Don't inherit PROC_SET_PDEATHSIG value if setuid/setgid. */ > >> + if (credential_changing) > >> + imgp->proc->p_pdeathsig = 0; > >> + > > > > What is the Linux behaviour on exec ? It seems of negative value to > > receive a signal which disposition is reset to default. In other words, > > for the non-suid image, you typicall get the process terminated and > > perhaps core dumped when parent is exiting. > > This is the documented Linux behaviour. I'm guessing it's disabled > for setuid/setgid because you wouldn't normally be allowed to signal > that process so you shouldn't be allowed to request a signal on parent > exit (once exec changes the credentials). That seems to make sense. > > I think in the case of a non-setuid image the point is probably to be > able to make it die if the parent dies... It can't really be relying > on the exec'd binary installing a signal handler, because the signal > might arrive before the binary manages to do that. It is still strange, but if this is the Linux behaviour, I would close eyes and accept it to not make consumers handle the difference. > > >> if (credential_changing && > >> #ifdef CAPABILITY_MODE > >> ((oldcred->cr_flags & CRED_FLAG_CAPMODE) == 0) && > >> diff --git a/sys/kern/kern_exit.c b/sys/kern/kern_exit.c > >> index f672a2c7358..55f08a004eb 100644 > >> --- a/sys/kern/kern_exit.c > >> +++ b/sys/kern/kern_exit.c > >> @@ -480,6 +480,12 @@ exit1(struct thread *td, int rval, int signo) > >> PROC_LOCK(q->p_reaper); > >> pksignal(q->p_reaper, SIGCHLD, ksi1); > >> PROC_UNLOCK(q->p_reaper); > >> + } else if (q->p_pdeathsig > 0) { > >> + /* > >> + * The child asked to received a signal > >> + * when we exit. > >> + */ > >> + kern_psignal(q, q->p_pdeathsig); > > > > Don't you need to lock q around kern_psignal() ? Test your > > patch with WITNESS and INVARIANTS kernel options enabled. > > PROC_LOCK(q) appears above the whole if/else block. This is what I do not see in the patch with limited context, unless I start reading by applying. > > Those options were enabled for my testing (I used GENERIC out of the > source tree, and they're enabled in there). > > >> + case PROC_GET_PDEATHSIG: > >> + if (idtype != P_PID || id != 0) > >> + return (EINVAL); > >> + PROC_LOCK(td->td_proc); > >> + signum = td->td_proc->p_pdeathsig; > >> + PROC_UNLOCK(td->td_proc); > >> + *(int *)data = signum; > > > > Why do you need to mediate assignment through the signum ? > > Ok, assigned directly to *(int *)data. > > > Note that the locking around the lone integer assignment is excessive, > > but it does not hurt. > > Ok, I'll leave the locking. > > >> +#define PROC_SET_PDEATHSIG 11 /* set parent death signal */ > >> +#define PROC_GET_PDEATHSIG 12 /* get parent death signal */ > > > > Note that other commands names follow the pattern > > PROC__, > > in your case it would be PROC_PDEATHSIG_SET/GET. > > Changed. > > > Please use tab after #define. > > Fixed.