PROC_SET_PDEATHSIG to get a signal when your parent exits

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

PROC_SET_PDEATHSIG to get a signal when your parent exits

Thomas Munro
Hello,

In PostgreSQL we are planning to make use of Linux's
prctl(PR_SET_PDEATHSIG) facility which tells the kernel to signal you
when your parent process exits.  This will cut down on polling and
contention on a pipe in certain busy loops, where we need to be able
to exit quickly if something kills the main supervisor process but we
aren't waiting in kevent/epoll/poll-type primitive.  We considered
SIGIO but that'd require either one pipe per child process or a common
process group, neither of which works for us.  So I decided to try
adding a new command PROC_SET_PDEATHSIG to procctl() to work exactly
like Linux.

Please see attached early draft patch.  This is my first attempt at
writing a patch for FreeBSD, so I expect that it contains newbie
mistakes and style problems and I would be grateful for any feedback.
Is this design sane?  Is that the best syscall to use for this?  Did I
get the ptrace/orphan stuff right?  Should this somehow share
something with the Linux compat support for prctl(PR_SET_PDEATHSIG,
...)?  Would people want this?

Here's a quick and dirty userspace program I came up with while
developing the patch:

https://github.com/macdice/test-deathsig/blob/master/test-deathsig.c

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.

Thanks for reading,

Thomas Munro

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"

0001-Support-PROC_SET_PDEATHSIG-as-a-command-to-procct-v1.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PROC_SET_PDEATHSIG to get a signal when your parent exits

Konstantin Belousov-3
On Sat, Apr 14, 2018 at 04:44:51PM +1200, Thomas Munro wrote:

> Hello,
>
> In PostgreSQL we are planning to make use of Linux's
> prctl(PR_SET_PDEATHSIG) facility which tells the kernel to signal you
> when your parent process exits.  This will cut down on polling and
> contention on a pipe in certain busy loops, where we need to be able
> to exit quickly if something kills the main supervisor process but we
> aren't waiting in kevent/epoll/poll-type primitive.  We considered
> SIGIO but that'd require either one pipe per child process or a common
> process group, neither of which works for us.  So I decided to try
> adding a new command PROC_SET_PDEATHSIG to procctl() to work exactly
> like Linux.
>
> Please see attached early draft patch.  This is my first attempt at
> writing a patch for FreeBSD, so I expect that it contains newbie
> mistakes and style problems and I would be grateful for any feedback.
> Is this design sane?  Is that the best syscall to use for this?  Did I
> get the ptrace/orphan stuff right?  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.

>
> Here's a quick and dirty userspace program I came up with while
> developing the patch:
>
> https://github.com/macdice/test-deathsig/blob/master/test-deathsig.c
>
> 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.

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.

Other comments inline.  You may create the account at
https://reviews.freebsd.org and putting patch there.

> From b9a6730697ba09f7ee0f81129f3f976df826d1cd Mon Sep 17 00:00:00 2001
> From: Thomas Munro <[hidden email]>
> Date: Fri, 13 Apr 2018 23:41:30 +0100
> Subject: [PATCH] Support PROC_SET_PDEATHSIG as a command to procctl.
>
> Allow processes to request the delivery of a signal upon death
> of their parent process.  This provides approximately the same
> behaviour as prctl(PR_SET_PDEATHSIG, ...) on Linux.
>
> Thomas Munro <[hidden email]>
> ---
>  lib/libc/sys/procctl.2  | 29 +++++++++++++++++++++++++++++
>  sys/kern/kern_exec.c    |  4 ++++
>  sys/kern/kern_exit.c    | 13 +++++++++++++
>  sys/kern/kern_procctl.c | 33 ++++++++++++++++++++++++++++++++-
>  sys/kern/kern_thread.c  |  4 ++--
>  sys/sys/proc.h          |  1 +
>  sys/sys/procctl.h       |  2 ++
>  7 files changed, 83 insertions(+), 3 deletions(-)
>
> diff --git a/lib/libc/sys/procctl.2 b/lib/libc/sys/procctl.2
> index af96ae6e04c..e49ef36995f 100644
> --- a/lib/libc/sys/procctl.2
> +++ b/lib/libc/sys/procctl.2
> @@ -391,6 +391,24 @@ otherwise.
>  See the note about sysctl
>  .Dv kern.trap_enotcap
>  above, which gives independent global control of signal delivery.
> +.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.

In fact, '0' is ok as an alias for the current pid, but not allowing current
pid as the argument is strange.

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.

But I do not see how would pfind(0) return curproc.  Don't you need to add
some code for this in kern_procctl() ?


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

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

> +.Fa arg
> +must point to a memory location that can hold a value of type int.
> +If signal delivery has not been requested, it will contain zero
> +on return.
>  .El
>  .Sh NOTES
>  Disabling tracing on a process should not be considered a security
> @@ -487,6 +505,12 @@ parameter for the
>  or
>  .Dv PROC_TRAPCAP_CTL
>  request is invalid.
> +.It Bq Er EINVAL
> +The
> +.Dv PROC_SET_PDEATHSIG
> +or
> +.Dv PROC_GET_PDEATHSIG
> +request referenced an unsupported id, id_type or invalid signal number.
>  .El
>  .Sh SEE ALSO
>  .Xr dtrace 1 ,
> @@ -506,3 +530,8 @@ function appeared in
>  The reaper facility is based on a similar feature of Linux and
>  DragonflyBSD, and first appeared in
>  .Fx 10.2 .
> +The
> +.Dv PROC_SET_PDEATHSIG
> +facility is based on the prctl(PR_SET_PDEATHSIG, ...) feature of Linux,
> +and first appeared in
> +.Fx 12.0 .
> diff --git a/sys/kern/kern_exec.c b/sys/kern/kern_exec.c
> index 21b25e0148c..679915a6e6e 100644
> --- a/sys/kern/kern_exec.c
> +++ b/sys/kern/kern_exec.c
> @@ -522,6 +522,10 @@ do_execve(struct thread *td, struct image_args *args, struct mac *mac_p)
>   credential_changing |= will_transition;
>  #endif
>  
> + /* 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.

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

>   }
>   } else {
>   /*
> @@ -520,6 +526,13 @@ exit1(struct thread *td, int rval, int signo)
>   */
>   while ((q = LIST_FIRST(&p->p_orphans)) != NULL) {
>   PROC_LOCK(q);
> + /*
> + * If we are the real parent of this process
> + * but it has been reparented to a debugger, then
> + * check if it asked for a signal when we exit.
> + */
> + if (q->p_pdeathsig > 0)
> + kern_psignal(q, q->p_pdeathsig);
>   CTR2(KTR_PTRACE, "exit: pid %d, clearing orphan %d", p->p_pid,
>      q->p_pid);
>   clear_orphan(q);
> diff --git a/sys/kern/kern_procctl.c b/sys/kern/kern_procctl.c
> index dddc6b9c1d3..c0adeed570e 100644
> --- a/sys/kern/kern_procctl.c
> +++ b/sys/kern/kern_procctl.c
> @@ -431,7 +431,7 @@ sys_procctl(struct thread *td, struct procctl_args *uap)
>   struct procctl_reaper_pids rp;
>   struct procctl_reaper_kill rk;
>   } x;
> - int error, error1, flags;
> + int error, error1, flags, signum;
>  
>   switch (uap->com) {
>   case PROC_SPROTECT:
> @@ -467,6 +467,15 @@ sys_procctl(struct thread *td, struct procctl_args *uap)
>   case PROC_TRAPCAP_STATUS:
>   data = &flags;
>   break;
> + case PROC_SET_PDEATHSIG:
> + error = copyin(uap->data, &signum, sizeof(signum));
> + if (error != 0)
> + return (error);
> + data = &signum;
> + break;
> + case PROC_GET_PDEATHSIG:
> + data = &signum;
> + break;
>   default:
>   return (EINVAL);
>   }
> @@ -486,6 +495,10 @@ sys_procctl(struct thread *td, struct procctl_args *uap)
>   if (error == 0)
>   error = copyout(&flags, uap->data, sizeof(flags));
>   break;
> + case PROC_GET_PDEATHSIG:
> + if (error == 0)
> + error = copyout(&signum, uap->data, sizeof(signum));
> + break;
>   }
>   return (error);
>  }
> @@ -527,6 +540,7 @@ kern_procctl(struct thread *td, idtype_t idtype, id_t id, int com, void *data)
>   struct pgrp *pg;
>   struct proc *p;
>   int error, first_error, ok;
> + int signum;
>   bool tree_locked;
>  
>   switch (com) {
> @@ -560,6 +574,23 @@ kern_procctl(struct thread *td, idtype_t idtype, id_t id, int com, void *data)
>   case PROC_TRAPCAP_STATUS:
>   tree_locked = false;
>   break;
> + case PROC_SET_PDEATHSIG:
> + signum = *(int *)data;
> + if (idtype != P_PID || id != 0 ||
> +    (signum != 0 && !_SIG_VALID(signum)))
> + return (EINVAL);
> + PROC_LOCK(td->td_proc);
> + td->td_proc->p_pdeathsig = signum;
> + PROC_UNLOCK(td->td_proc);
> + return (0);
> + 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 ?

Note that the locking around the lone integer assignment is excessive,
but it does not hurt.

> + return (0);
>   default:
>   return (EINVAL);
>   }
> diff --git a/sys/kern/kern_thread.c b/sys/kern/kern_thread.c
> index 4d2f020063e..b01cd99e597 100644
> --- a/sys/kern/kern_thread.c
> +++ b/sys/kern/kern_thread.c
> @@ -91,7 +91,7 @@ _Static_assert(offsetof(struct proc, p_pid) == 0xbc,
>      "struct proc KBI p_pid");
>  _Static_assert(offsetof(struct proc, p_filemon) == 0x3d0,
>      "struct proc KBI p_filemon");
> -_Static_assert(offsetof(struct proc, p_comm) == 0x3e0,
> +_Static_assert(offsetof(struct proc, p_comm) == 0x3e4,
>      "struct proc KBI p_comm");
>  _Static_assert(offsetof(struct proc, p_emuldata) == 0x4b8,
>      "struct proc KBI p_emuldata");
> @@ -111,7 +111,7 @@ _Static_assert(offsetof(struct proc, p_pid) == 0x74,
>      "struct proc KBI p_pid");
>  _Static_assert(offsetof(struct proc, p_filemon) == 0x27c,
>      "struct proc KBI p_filemon");
> -_Static_assert(offsetof(struct proc, p_comm) == 0x288,
> +_Static_assert(offsetof(struct proc, p_comm) == 0x28c,
>      "struct proc KBI p_comm");
>  _Static_assert(offsetof(struct proc, p_emuldata) == 0x314,
>      "struct proc KBI p_emuldata");
> diff --git a/sys/sys/proc.h b/sys/sys/proc.h
> index d63362d7232..19e99439188 100644
> --- a/sys/sys/proc.h
> +++ b/sys/sys/proc.h
> @@ -624,6 +624,7 @@ struct proc {
>   u_int p_treeflag; /* (e) P_TREE flags */
>   int p_pendingexits; /* (c) Count of pending thread exits. */
>   struct filemon *p_filemon; /* (c) filemon-specific data. */
> + int p_pdeathsig; /* (c) Signal from parent on exit. */
>  /* End area that is zeroed on creation. */
>  #define p_endzero p_magic
>  
> diff --git a/sys/sys/procctl.h b/sys/sys/procctl.h
> index 17144e9f0c6..cf0a69475e4 100644
> --- a/sys/sys/procctl.h
> +++ b/sys/sys/procctl.h
> @@ -51,6 +51,8 @@
>  #define PROC_TRACE_STATUS 8 /* query tracing status */
>  #define PROC_TRAPCAP_CTL 9 /* trap capability errors */
>  #define PROC_TRAPCAP_STATUS 10 /* query trap capability status */
> +#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.

Please use tab after #define.

>  
>  /* Operations for PROC_SPROTECT (passed in integer arg). */
>  #define PPROT_OP(x) ((x) & 0xf)
> --
> 2.16.2
>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: 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 <[hidden email]> 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.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: PROC_SET_PDEATHSIG to get a signal when your parent exits

Konstantin Belousov-3
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 <[hidden email]> 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
[hidden email] 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.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: PROC_SET_PDEATHSIG to get a signal when your parent exits

Thomas Munro
On 17 April 2018 at 08:07, Konstantin Belousov <[hidden email]> wrote:

> On Tue, Apr 17, 2018 at 01:06:57AM +1200, Thomas Munro wrote:
>> 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
> [hidden email] 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.

Ah, I see.  Done:

https://reviews.freebsd.org/D15106
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"