sigabort(), p_sigmask & p_siglist

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

sigabort(), p_sigmask & p_siglist

Martin Pieuchot
Diff below introduces an helper for sending an uncatchable SIGABRT and
annotate that `p_siglist' and `p_sigmask' are updated using atomic
operations.

As a result setsigvec() becomes local to kern/kern_sig.c.

Note that other places in the kernel use sigexit(p, SIGABRT) for the
same purpose and aren't converted by this change.

Ik?

Index: kern/kern_pledge.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_pledge.c,v
retrieving revision 1.263
diff -u -p -r1.263 kern_pledge.c
--- kern/kern_pledge.c 17 Jul 2020 16:28:19 -0000 1.263
+++ kern/kern_pledge.c 15 Sep 2020 08:30:19 -0000
@@ -529,7 +529,6 @@ pledge_fail(struct proc *p, int error, u
 {
  const char *codes = "";
  int i;
- struct sigaction sa;
 
  /* Print first matching pledge */
  for (i = 0; code && pledgenames[i].bits != 0; i++)
@@ -550,11 +549,7 @@ pledge_fail(struct proc *p, int error, u
  p->p_p->ps_acflag |= APLEDGE;
 
  /* Send uncatchable SIGABRT for coredump */
- memset(&sa, 0, sizeof sa);
- sa.sa_handler = SIG_DFL;
- setsigvec(p, SIGABRT, &sa);
- atomic_clearbits_int(&p->p_sigmask, sigmask(SIGABRT));
- psignal(p, SIGABRT);
+ sigabort(p);
 
  p->p_p->ps_pledge = 0; /* Disable all PLEDGE_ flags */
  KERNEL_UNLOCK();
Index: kern/kern_proc.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_proc.c,v
retrieving revision 1.86
diff -u -p -r1.86 kern_proc.c
--- kern/kern_proc.c 30 Jan 2020 08:51:27 -0000 1.86
+++ kern/kern_proc.c 15 Sep 2020 08:52:57 -0000
@@ -494,7 +494,6 @@ void
 db_kill_cmd(db_expr_t addr, int have_addr, db_expr_t count, char *modif)
 {
  struct process *pr;
- struct sigaction sa;
  struct proc *p;
 
  pr = prfind(addr);
@@ -506,11 +505,7 @@ db_kill_cmd(db_expr_t addr, int have_add
  p = TAILQ_FIRST(&pr->ps_threads);
 
  /* Send uncatchable SIGABRT for coredump */
- memset(&sa, 0, sizeof sa);
- sa.sa_handler = SIG_DFL;
- setsigvec(p, SIGABRT, &sa);
- atomic_clearbits_int(&p->p_sigmask, sigmask(SIGABRT));
- psignal(p, SIGABRT);
+ sigabort(p);
 }
 
 void
Index: kern/kern_sig.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_sig.c,v
retrieving revision 1.262
diff -u -p -r1.262 kern_sig.c
--- kern/kern_sig.c 13 Sep 2020 13:33:37 -0000 1.262
+++ kern/kern_sig.c 15 Sep 2020 08:33:04 -0000
@@ -122,6 +122,8 @@ const int sigprop[NSIG + 1] = {
 #define stopsigmask (sigmask(SIGSTOP) | sigmask(SIGTSTP) | \
     sigmask(SIGTTIN) | sigmask(SIGTTOU))
 
+void setsigvec(struct proc *, int, struct sigaction *);
+
 void proc_stop(struct proc *p, int);
 void proc_stop_sweep(void *);
 void *proc_stop_si;
@@ -1483,6 +1493,21 @@ sigexit(struct proc *p, int signum)
  }
  exit1(p, 0, signum, EXIT_NORMAL);
  /* NOTREACHED */
+}
+
+/*
+ * Send uncatchable SIGABRT for coredump.
+ */
+void
+sigabort(struct proc *p)
+{
+ struct sigaction sa;
+
+ memset(&sa, 0, sizeof sa);
+ sa.sa_handler = SIG_DFL;
+ setsigvec(p, SIGABRT, &sa);
+ atomic_clearbits_int(&p->p_sigmask, sigmask(SIGABRT));
+ psignal(p, SIGABRT);
 }
 
 /*
Index: sys/signalvar.h
===================================================================
RCS file: /cvs/src/sys/sys/signalvar.h,v
retrieving revision 1.43
diff -u -p -r1.43 signalvar.h
--- sys/signalvar.h 13 Sep 2020 13:33:37 -0000 1.43
+++ sys/signalvar.h 15 Sep 2020 08:34:15 -0000
@@ -126,9 +126,9 @@ void siginit(struct sigacts *);
 void trapsignal(struct proc *p, int sig, u_long code, int type,
     union sigval val);
 void sigexit(struct proc *, int);
+void sigabort(struct proc *);
 int sigismasked(struct proc *, int);
 int sigonstack(size_t);
-void setsigvec(struct proc *, int, struct sigaction *);
 int killpg1(struct proc *, int, int, int);
 
 void signal_init(void);
Index: sys/proc.h
===================================================================
RCS file: /cvs/src/sys/sys/proc.h,v
retrieving revision 1.299
diff -u -p -r1.299 proc.h
--- sys/proc.h 26 Aug 2020 03:19:09 -0000 1.299
+++ sys/proc.h 15 Sep 2020 10:15:36 -0000
@@ -383,14 +383,14 @@ struct proc {
  struct kcov_dev *p_kd; /* kcov device handle */
  struct lock_list_entry *p_sleeplocks; /* WITNESS lock tracking */
 
- int p_siglist; /* Signals arrived but not delivered. */
+ int p_siglist; /* [a] Signals arrived & not delivered*/
 
 /* End area that is zeroed on creation. */
 #define p_endzero p_startcopy
 
 /* The following fields are all copied upon creation in fork. */
 #define p_startcopy p_sigmask
- sigset_t p_sigmask; /* Current signal mask. */
+ sigset_t p_sigmask; /* [a] Current signal mask */
 
  u_char p_slppri; /* [S] Sleeping priority */
  u_char p_usrpri; /* [S] Priority based on p_estcpu & ps_nice */

Reply | Threaded
Open this post in threaded view
|

Re: sigabort(), p_sigmask & p_siglist

Claudio Jeker
On Tue, Sep 15, 2020 at 12:52:40PM +0200, Martin Pieuchot wrote:

> Diff below introduces an helper for sending an uncatchable SIGABRT and
> annotate that `p_siglist' and `p_sigmask' are updated using atomic
> operations.
>
> As a result setsigvec() becomes local to kern/kern_sig.c.
>
> Note that other places in the kernel use sigexit(p, SIGABRT) for the
> same purpose and aren't converted by this change.
>
> Ik?

OK claudio@ but I would split the proc.h into its own commit since it is
unrelated to the sigabort() introduction.
 

> Index: kern/kern_pledge.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_pledge.c,v
> retrieving revision 1.263
> diff -u -p -r1.263 kern_pledge.c
> --- kern/kern_pledge.c 17 Jul 2020 16:28:19 -0000 1.263
> +++ kern/kern_pledge.c 15 Sep 2020 08:30:19 -0000
> @@ -529,7 +529,6 @@ pledge_fail(struct proc *p, int error, u
>  {
>   const char *codes = "";
>   int i;
> - struct sigaction sa;
>  
>   /* Print first matching pledge */
>   for (i = 0; code && pledgenames[i].bits != 0; i++)
> @@ -550,11 +549,7 @@ pledge_fail(struct proc *p, int error, u
>   p->p_p->ps_acflag |= APLEDGE;
>  
>   /* Send uncatchable SIGABRT for coredump */
> - memset(&sa, 0, sizeof sa);
> - sa.sa_handler = SIG_DFL;
> - setsigvec(p, SIGABRT, &sa);
> - atomic_clearbits_int(&p->p_sigmask, sigmask(SIGABRT));
> - psignal(p, SIGABRT);
> + sigabort(p);
>  
>   p->p_p->ps_pledge = 0; /* Disable all PLEDGE_ flags */
>   KERNEL_UNLOCK();
> Index: kern/kern_proc.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_proc.c,v
> retrieving revision 1.86
> diff -u -p -r1.86 kern_proc.c
> --- kern/kern_proc.c 30 Jan 2020 08:51:27 -0000 1.86
> +++ kern/kern_proc.c 15 Sep 2020 08:52:57 -0000
> @@ -494,7 +494,6 @@ void
>  db_kill_cmd(db_expr_t addr, int have_addr, db_expr_t count, char *modif)
>  {
>   struct process *pr;
> - struct sigaction sa;
>   struct proc *p;
>  
>   pr = prfind(addr);
> @@ -506,11 +505,7 @@ db_kill_cmd(db_expr_t addr, int have_add
>   p = TAILQ_FIRST(&pr->ps_threads);
>  
>   /* Send uncatchable SIGABRT for coredump */
> - memset(&sa, 0, sizeof sa);
> - sa.sa_handler = SIG_DFL;
> - setsigvec(p, SIGABRT, &sa);
> - atomic_clearbits_int(&p->p_sigmask, sigmask(SIGABRT));
> - psignal(p, SIGABRT);
> + sigabort(p);
>  }
>  
>  void
> Index: kern/kern_sig.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_sig.c,v
> retrieving revision 1.262
> diff -u -p -r1.262 kern_sig.c
> --- kern/kern_sig.c 13 Sep 2020 13:33:37 -0000 1.262
> +++ kern/kern_sig.c 15 Sep 2020 08:33:04 -0000
> @@ -122,6 +122,8 @@ const int sigprop[NSIG + 1] = {
>  #define stopsigmask (sigmask(SIGSTOP) | sigmask(SIGTSTP) | \
>      sigmask(SIGTTIN) | sigmask(SIGTTOU))
>  
> +void setsigvec(struct proc *, int, struct sigaction *);
> +
>  void proc_stop(struct proc *p, int);
>  void proc_stop_sweep(void *);
>  void *proc_stop_si;
> @@ -1483,6 +1493,21 @@ sigexit(struct proc *p, int signum)
>   }
>   exit1(p, 0, signum, EXIT_NORMAL);
>   /* NOTREACHED */
> +}
> +
> +/*
> + * Send uncatchable SIGABRT for coredump.
> + */
> +void
> +sigabort(struct proc *p)
> +{
> + struct sigaction sa;
> +
> + memset(&sa, 0, sizeof sa);
> + sa.sa_handler = SIG_DFL;
> + setsigvec(p, SIGABRT, &sa);
> + atomic_clearbits_int(&p->p_sigmask, sigmask(SIGABRT));
> + psignal(p, SIGABRT);
>  }
>  
>  /*
> Index: sys/signalvar.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/signalvar.h,v
> retrieving revision 1.43
> diff -u -p -r1.43 signalvar.h
> --- sys/signalvar.h 13 Sep 2020 13:33:37 -0000 1.43
> +++ sys/signalvar.h 15 Sep 2020 08:34:15 -0000
> @@ -126,9 +126,9 @@ void siginit(struct sigacts *);
>  void trapsignal(struct proc *p, int sig, u_long code, int type,
>      union sigval val);
>  void sigexit(struct proc *, int);
> +void sigabort(struct proc *);
>  int sigismasked(struct proc *, int);
>  int sigonstack(size_t);
> -void setsigvec(struct proc *, int, struct sigaction *);
>  int killpg1(struct proc *, int, int, int);
>  
>  void signal_init(void);
> Index: sys/proc.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/proc.h,v
> retrieving revision 1.299
> diff -u -p -r1.299 proc.h
> --- sys/proc.h 26 Aug 2020 03:19:09 -0000 1.299
> +++ sys/proc.h 15 Sep 2020 10:15:36 -0000
> @@ -383,14 +383,14 @@ struct proc {
>   struct kcov_dev *p_kd; /* kcov device handle */
>   struct lock_list_entry *p_sleeplocks; /* WITNESS lock tracking */
>  
> - int p_siglist; /* Signals arrived but not delivered. */
> + int p_siglist; /* [a] Signals arrived & not delivered*/
>  
>  /* End area that is zeroed on creation. */
>  #define p_endzero p_startcopy
>  
>  /* The following fields are all copied upon creation in fork. */
>  #define p_startcopy p_sigmask
> - sigset_t p_sigmask; /* Current signal mask. */
> + sigset_t p_sigmask; /* [a] Current signal mask */
>  
>   u_char p_slppri; /* [S] Sleeping priority */
>   u_char p_usrpri; /* [S] Priority based on p_estcpu & ps_nice */
>

--
:wq Claudio

Reply | Threaded
Open this post in threaded view
|

Re: sigabort(), p_sigmask & p_siglist

Miod Vallat
In reply to this post by Martin Pieuchot

> Diff below introduces an helper for sending an uncatchable SIGABRT and
> annotate that `p_siglist' and `p_sigmask' are updated using atomic
> operations.

Why not use sigexit(p, SIGABRT); for that purpose?

Reply | Threaded
Open this post in threaded view
|

Re: sigabort(), p_sigmask & p_siglist

Martin Pieuchot
On 16/09/20(Wed) 06:09, Miod Vallat wrote:
>
> > Diff below introduces an helper for sending an uncatchable SIGABRT and
> > annotate that `p_siglist' and `p_sigmask' are updated using atomic
> > operations.
>
> Why not use sigexit(p, SIGABRT); for that purpose?

That's a better solution indeed.  deraadt@ pointed something that goes
in this direction as well because sigexit() parks siblings earlier and
that reduces the amount of noise between the detection of corruption
and the content of the coredump.

Updated diff below, ok?

Index: kern/kern_sig.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_sig.c,v
retrieving revision 1.262
diff -u -p -r1.262 kern_sig.c
--- kern/kern_sig.c 13 Sep 2020 13:33:37 -0000 1.262
+++ kern/kern_sig.c 16 Sep 2020 07:45:26 -0000
@@ -122,6 +122,8 @@ const int sigprop[NSIG + 1] = {
 #define stopsigmask (sigmask(SIGSTOP) | sigmask(SIGTSTP) | \
     sigmask(SIGTTIN) | sigmask(SIGTTOU))
 
+void setsigvec(struct proc *, int, struct sigaction *);
+
 void proc_stop(struct proc *p, int);
 void proc_stop_sweep(void *);
 void *proc_stop_si;
Index: kern/kern_pledge.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_pledge.c,v
retrieving revision 1.263
diff -u -p -r1.263 kern_pledge.c
--- kern/kern_pledge.c 17 Jul 2020 16:28:19 -0000 1.263
+++ kern/kern_pledge.c 16 Sep 2020 07:45:28 -0000
@@ -529,7 +529,6 @@ pledge_fail(struct proc *p, int error, u
 {
  const char *codes = "";
  int i;
- struct sigaction sa;
 
  /* Print first matching pledge */
  for (i = 0; code && pledgenames[i].bits != 0; i++)
@@ -550,11 +549,7 @@ pledge_fail(struct proc *p, int error, u
  p->p_p->ps_acflag |= APLEDGE;
 
  /* Send uncatchable SIGABRT for coredump */
- memset(&sa, 0, sizeof sa);
- sa.sa_handler = SIG_DFL;
- setsigvec(p, SIGABRT, &sa);
- atomic_clearbits_int(&p->p_sigmask, sigmask(SIGABRT));
- psignal(p, SIGABRT);
+ sigexit(p, SIGABRT);
 
  p->p_p->ps_pledge = 0; /* Disable all PLEDGE_ flags */
  KERNEL_UNLOCK();
Index: kern/kern_proc.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_proc.c,v
retrieving revision 1.86
diff -u -p -r1.86 kern_proc.c
--- kern/kern_proc.c 30 Jan 2020 08:51:27 -0000 1.86
+++ kern/kern_proc.c 16 Sep 2020 07:45:26 -0000
@@ -494,7 +494,6 @@ void
 db_kill_cmd(db_expr_t addr, int have_addr, db_expr_t count, char *modif)
 {
  struct process *pr;
- struct sigaction sa;
  struct proc *p;
 
  pr = prfind(addr);
@@ -506,11 +505,7 @@ db_kill_cmd(db_expr_t addr, int have_add
  p = TAILQ_FIRST(&pr->ps_threads);
 
  /* Send uncatchable SIGABRT for coredump */
- memset(&sa, 0, sizeof sa);
- sa.sa_handler = SIG_DFL;
- setsigvec(p, SIGABRT, &sa);
- atomic_clearbits_int(&p->p_sigmask, sigmask(SIGABRT));
- psignal(p, SIGABRT);
+ sigexit(p, SIGABRT);
 }
 
 void
Index: sys/signalvar.h
===================================================================
RCS file: /cvs/src/sys/sys/signalvar.h,v
retrieving revision 1.43
diff -u -p -r1.43 signalvar.h
--- sys/signalvar.h 13 Sep 2020 13:33:37 -0000 1.43
+++ sys/signalvar.h 16 Sep 2020 07:45:22 -0000
@@ -128,7 +128,6 @@ void trapsignal(struct proc *p, int sig,
 void sigexit(struct proc *, int);
 int sigismasked(struct proc *, int);
 int sigonstack(size_t);
-void setsigvec(struct proc *, int, struct sigaction *);
 int killpg1(struct proc *, int, int, int);
 
 void signal_init(void);

Reply | Threaded
Open this post in threaded view
|

Re: sigabort(), p_sigmask & p_siglist

Theo de Raadt-2
Something doesn't feel right.

db_kill_cmd finds a process, called p, then kills it.  In your new diff
calling sigexit, take note of the comment at the top:

 * Force the current process to exit with the specified signal, dumping core

current process?  Doesn't look like it, it looks like it kills p.  But then
it calls exit1?

Is this actually behaving the same for the db_kill_cmd() call?

 

Reply | Threaded
Open this post in threaded view
|

Re: sigabort(), p_sigmask & p_siglist

Martin Pieuchot
On 16/09/20(Wed) 02:08, Theo de Raadt wrote:

> Something doesn't feel right.
>
> db_kill_cmd finds a process, called p, then kills it.  In your new diff
> calling sigexit, take note of the comment at the top:
>
>  * Force the current process to exit with the specified signal, dumping core
>
> current process?  Doesn't look like it, it looks like it kills p.  But then
> it calls exit1?
>
> Is this actually behaving the same for the db_kill_cmd() call?

Indeed, I messed up, in sigexit() the "struct proc *" argument means
`curproc'.

Reply | Threaded
Open this post in threaded view
|

Re: sigabort(), p_sigmask & p_siglist

Miod Vallat
In reply to this post by Theo de Raadt-2
> Something doesn't feel right.
>
> db_kill_cmd finds a process, called p, then kills it.  In your new diff
> calling sigexit, take note of the comment at the top:
>
>  * Force the current process to exit with the specified signal, dumping core
>
> current process?  Doesn't look like it, it looks like it kills p.  But then
> it calls exit1?

Ah, yes, sigexit() ends up with a context switch. This is not a good
idea for the places where Martin wants to use it, his first diff was
better. Sorry for the noise.