Date: Mon, 16 Apr 2018 23:07:13 +0300 From: Konstantin Belousov <kib@freebsd.org> To: Thomas Munro <munro@ip9.org> 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> In-Reply-To: <CADLWmXUXPWpfB1nkY6mTvJvHrwPsbhVv1GAXuE5N4gj30oGz0A@mail.gmail.com> References: <CADLWmXWGReyZn9ZoM9q8gs=%2Bbqw7e4-Ti1HNEPxYoJJ=6gZHhA@mail.gmail.com> <20180414180415.GC1774@kib.kiev.ua> <CADLWmXUXPWpfB1nkY6mTvJvHrwPsbhVv1GAXuE5N4gj30oGz0A@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <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/ 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_<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?20180416200713.GL1774>