PROC_SET_PDEATHSIG to get a signal when your parent exits

[Konstantin Belousov]

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 at 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 at 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.