PROC_SET_PDEATHSIG to get a signal when your parent exits

[Thomas Munro]

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/

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