Date: Tue, 17 Apr 2018 01:06:57 +1200 From: Thomas Munro <munro@ip9.org> To: Konstantin Belousov <kib@freebsd.org> Cc: freebsd-hackers@freebsd.org Subject: Re: PROC_SET_PDEATHSIG to get a signal when your parent exits Message-ID: <CADLWmXUXPWpfB1nkY6mTvJvHrwPsbhVv1GAXuE5N4gj30oGz0A@mail.gmail.com> In-Reply-To: <20180414180415.GC1774@kib.kiev.ua> References: <CADLWmXWGReyZn9ZoM9q8gs=%2Bbqw7e4-Ti1HNEPxYoJJ=6gZHhA@mail.gmail.com> <20180414180415.GC1774@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Konstantin, Thanks for the review! On 15 April 2018 at 06:04, Konstantin Belousov <kib@freebsd.org> 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/ >> +.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. > 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. >> +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. >> 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. 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_<FACILITY>_<CMD>, > in your case it would be PROC_PDEATHSIG_SET/GET. Changed. > Please use tab after #define. Fixed.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CADLWmXUXPWpfB1nkY6mTvJvHrwPsbhVv1GAXuE5N4gj30oGz0A>