Re: proctreelk
On 20/03/18(Tue) 18:24, Martin Pieuchot wrote: > On 26/02/18(Mon) 17:52, Martin Pieuchot wrote: > > On 20/10/17(Fri) 11:50, Martin Pieuchot wrote: > > > On 14/10/17(Sat) 22:07, Philip Guenther wrote: > > > > > > > > The diff below adds proctreelk, an rwlock protecting the links of the > > > > process tree and related bits, as well as uidinfolk, an rwlock > > > > protecting > > > > the uidinfo hash table. > > > > > > > > Parts of this are based on FreeBSD's proctree_lock, particularly the > > > > reorganization of enterpgrp() into enternewpgrp() and enterthispgrp() > > > > and > > > > the splitting out of killjobc() from exit1(). > > > > > > > > This diff should address the previously reported crashes seen when > > > > using > > > > ktrace(2) while creating/exiting processes. > > > > > > > > This has been stable for quite a while under my usage; please test and > > > > report any issues. > > > > > > First of all, I'm very happy to see this diff. Thanks Philip. > > > > > > I have been running this diff on my amd64 NFS client/server build > > > machine since you posted it. So far no issue, so it is stable for > > > this usage as well. > > > > > > I'd however appreciate if you could commit the killjobc() and > > > enter*grp() refactoring first. Because in case of revert this > > > would be less pain. > > > > I've done that. > > > > > That's also for this reason that I introduced a macro for the > > > NET_LOCK(). So I could enable/disable it without having to revert > > > N files. No idea if this could be useful there two. > > > > > > I like the way you annotate the protected elements in the structure. > > > I'll try to do the same for bpf. > > > > > > I'm also suggesting you commit the `uidinfolk' bits first. This seems > > > quite safe. In this exact part, what about introducing a uid_release(), > > > a wrapper around rw_exit_write(), to be called after uid_find()? > > > This way you can keep the lock local. > > > > And that too, here's an updated/rebased diff. > > Here's an updated diff where I fixed multiple issues reported by visa@. > This is mostly for discussion / backup. Changes include: > > - Release the `proctreelk' in sys_getsid()'s error path. > > - Document that process_zap() releases the `proctreelk'. > > - Do not hold the `proctreelk' around ttywait() in killjobc() > > - All access to fields marked with [t] in sys/proc.h should now be >protected or documented with XXXPT. > > - As a result setlogin(2) and getlogin_r(2) are now marked NOLOCK. > > The bigger problem is currently csignal() and pgsignal(). Both can be > called from interrupt context. That means that `pg_members' might have > to be protected differently. Here's a rebased version of the diff. Since people started having more interests in it. diff --git sys/kern/exec_elf.c sys/kern/exec_elf.c index 3fdd8d7a31b..7054a7a5409 100644 --- sys/kern/exec_elf.c +++ sys/kern/exec_elf.c @@ -1228,12 +1228,14 @@ coredump_notes_elf(struct proc *p, void *iocookie, size_t *sizep) cpi.cpi_sigcatch = pr->ps_sigacts->ps_sigcatch; cpi.cpi_pid = pr->ps_pid; + rw_enter_read(); cpi.cpi_ppid = pr->ps_pptr->ps_pid; cpi.cpi_pgrp = pr->ps_pgid; if (pr->ps_session->s_leader) cpi.cpi_sid = pr->ps_session->s_leader->ps_pid; else cpi.cpi_sid = 0; + rw_exit_read(); cpi.cpi_ruid = p->p_ucred->cr_ruid; cpi.cpi_euid = p->p_ucred->cr_uid; diff --git sys/kern/kern_acct.c sys/kern/kern_acct.c index 3ce6831734d..aba5fec49e4 100644 --- sys/kern/kern_acct.c +++ sys/kern/kern_acct.c @@ -206,11 +206,13 @@ acct_process(struct proc *p) acct.ac_gid = pr->ps_ucred->cr_rgid; /* (7) The terminal from which the process was started */ + rw_enter_read(); if ((pr->ps_flags & PS_CONTROLT) && pr->ps_pgrp->pg_session->s_ttyp) acct.ac_tty = pr->ps_pgrp->pg_session->s_ttyp->t_dev; else acct.ac_tty = NODEV; + rw_exit_read(); /* (8) The boolean flags that tell how the process terminated, etc. */ acct.ac_flag = pr->ps_acflag; diff --git sys/kern/kern_exec.c sys/kern/kern_exec.c index 88d3b1b47f7..ba3b50df62c 100644 --- sys/kern/kern_exec.c +++ sys/kern/kern_e
Re: proctreelk
On 26/02/18(Mon) 17:52, Martin Pieuchot wrote:
> On 20/10/17(Fri) 11:50, Martin Pieuchot wrote:
> > On 14/10/17(Sat) 22:07, Philip Guenther wrote:
> > >
> > > The diff below adds proctreelk, an rwlock protecting the links of the
> > > process tree and related bits, as well as uidinfolk, an rwlock protecting
> > > the uidinfo hash table.
> > >
> > > Parts of this are based on FreeBSD's proctree_lock, particularly the
> > > reorganization of enterpgrp() into enternewpgrp() and enterthispgrp() and
> > > the splitting out of killjobc() from exit1().
> > >
> > > This diff should address the previously reported crashes seen when using
> > > ktrace(2) while creating/exiting processes.
> > >
> > > This has been stable for quite a while under my usage; please test and
> > > report any issues.
> >
> > First of all, I'm very happy to see this diff. Thanks Philip.
> >
> > I have been running this diff on my amd64 NFS client/server build
> > machine since you posted it. So far no issue, so it is stable for
> > this usage as well.
> >
> > I'd however appreciate if you could commit the killjobc() and
> > enter*grp() refactoring first. Because in case of revert this
> > would be less pain.
>
> I've done that.
>
> > That's also for this reason that I introduced a macro for the
> > NET_LOCK(). So I could enable/disable it without having to revert
> > N files. No idea if this could be useful there two.
> >
> > I like the way you annotate the protected elements in the structure.
> > I'll try to do the same for bpf.
> >
> > I'm also suggesting you commit the `uidinfolk' bits first. This seems
> > quite safe. In this exact part, what about introducing a uid_release(),
> > a wrapper around rw_exit_write(), to be called after uid_find()?
> > This way you can keep the lock local.
>
> And that too, here's an updated/rebased diff.
Here's an updated diff where I fixed multiple issues reported by visa@.
This is mostly for discussion / backup. Changes include:
- Release the `proctreelk' in sys_getsid()'s error path.
- Document that process_zap() releases the `proctreelk'.
- Do not hold the `proctreelk' around ttywait() in killjobc()
- All access to fields marked with [t] in sys/proc.h should now be
protected or documented with XXXPT.
- As a result setlogin(2) and getlogin_r(2) are now marked NOLOCK.
The bigger problem is currently csignal() and pgsignal(). Both can be
called from interrupt context. That means that `pg_members' might have
to be protected differently.
Thoughts?
diff --git sys/kern/exec_elf.c sys/kern/exec_elf.c
index 0a2fc0a9c8a..e371acf1b33 100644
--- sys/kern/exec_elf.c
+++ sys/kern/exec_elf.c
@@ -1168,12 +1168,14 @@ coredump_notes_elf(struct proc *p, void *iocookie,
size_t *sizep)
cpi.cpi_sigcatch = pr->ps_sigacts->ps_sigcatch;
cpi.cpi_pid = pr->ps_pid;
+ rw_enter_read();
cpi.cpi_ppid = pr->ps_pptr->ps_pid;
cpi.cpi_pgrp = pr->ps_pgid;
if (pr->ps_session->s_leader)
cpi.cpi_sid = pr->ps_session->s_leader->ps_pid;
else
cpi.cpi_sid = 0;
+ rw_exit_read();
cpi.cpi_ruid = p->p_ucred->cr_ruid;
cpi.cpi_euid = p->p_ucred->cr_uid;
diff --git sys/kern/kern_acct.c sys/kern/kern_acct.c
index 9951a2a2789..78c6fa6a8ac 100644
--- sys/kern/kern_acct.c
+++ sys/kern/kern_acct.c
@@ -206,11 +206,13 @@ acct_process(struct proc *p)
acct.ac_gid = pr->ps_ucred->cr_rgid;
/* (7) The terminal from which the process was started */
+ rw_enter_read();
if ((pr->ps_flags & PS_CONTROLT) &&
pr->ps_pgrp->pg_session->s_ttyp)
acct.ac_tty = pr->ps_pgrp->pg_session->s_ttyp->t_dev;
else
acct.ac_tty = NODEV;
+ rw_exit_read();
/* (8) The boolean flags that tell how the process terminated, etc. */
acct.ac_flag = pr->ps_acflag;
diff --git sys/kern/kern_exec.c sys/kern/kern_exec.c
index 7aa601f43a3..acb91590e36 100644
--- sys/kern/kern_exec.c
+++ sys/kern/kern_exec.c
@@ -509,9 +509,11 @@ sys_execve(struct proc *p, void *v, register_t *retval)
atomic_setbits_int(>ps_flags, PS_EXEC);
if (pr->ps_flags & PS_PPWAIT) {
+ rw_enter_read();
atomic_clearbits_int(>ps_flags, PS_PPWAIT);
atomic_clearbits_int(>ps_pptr->ps_flags, PS_ISPWAIT);
wakeup(pr->ps_pptr);
+ rw_exit_read();
}
/*
Re: proctreelk
On 20/10/17(Fri) 11:50, Martin Pieuchot wrote:
> On 14/10/17(Sat) 22:07, Philip Guenther wrote:
> >
> > The diff below adds proctreelk, an rwlock protecting the links of the
> > process tree and related bits, as well as uidinfolk, an rwlock protecting
> > the uidinfo hash table.
> >
> > Parts of this are based on FreeBSD's proctree_lock, particularly the
> > reorganization of enterpgrp() into enternewpgrp() and enterthispgrp() and
> > the splitting out of killjobc() from exit1().
> >
> > This diff should address the previously reported crashes seen when using
> > ktrace(2) while creating/exiting processes.
> >
> > This has been stable for quite a while under my usage; please test and
> > report any issues.
>
> First of all, I'm very happy to see this diff. Thanks Philip.
>
> I have been running this diff on my amd64 NFS client/server build
> machine since you posted it. So far no issue, so it is stable for
> this usage as well.
>
> I'd however appreciate if you could commit the killjobc() and
> enter*grp() refactoring first. Because in case of revert this
> would be less pain.
I've done that.
> That's also for this reason that I introduced a macro for the
> NET_LOCK(). So I could enable/disable it without having to revert
> N files. No idea if this could be useful there two.
>
> I like the way you annotate the protected elements in the structure.
> I'll try to do the same for bpf.
>
> I'm also suggesting you commit the `uidinfolk' bits first. This seems
> quite safe. In this exact part, what about introducing a uid_release(),
> a wrapper around rw_exit_write(), to be called after uid_find()?
> This way you can keep the lock local.
And that too, here's an updated/rebased diff.
Index: sys/proc.h
===
RCS file: /cvs/src/sys/sys/proc.h,v
retrieving revision 1.247
diff -u -p -r1.247 proc.h
--- sys/proc.h 26 Feb 2018 13:43:51 - 1.247
+++ sys/proc.h 26 Feb 2018 16:47:58 -
@@ -44,6 +44,7 @@
#include/* For struct selinfo */
#include /* For LOGIN_NAME_MAX */
#include
+#include /* For struct rwlock */
#include/* For struct timeout */
#include /* For struct klist */
#include /* For struct mutex */
@@ -55,16 +56,25 @@
#endif
/*
+ * Locks used to protect struct members in this file:
+ * I immutable after creation
+ * t proctreelk
+ *
+ * If multiple locks are listed then all are required for writes,
+ * but any one of them is sufficient for reads.
+ */
+
+/*
* One structure allocated per session.
*/
struct process;
struct session {
int s_count;/* Ref cnt; pgrps in session. */
- struct process *s_leader; /* Session leader. */
+ struct process *s_leader; /* [t] Session leader. */
struct vnode *s_ttyvp; /* Vnode of controlling terminal. */
- struct tty *s_ttyp;/* Controlling terminal. */
- chars_login[LOGIN_NAME_MAX];/* Setlogin() name. */
- pid_t s_verauthppid;
+ struct tty *s_ttyp;/* [t] Controlling terminal. */
+ chars_login[LOGIN_NAME_MAX];/* setlogin() name. */
+ pid_t s_verauthppid; /* TIOCSETVERAUTH info */
uid_t s_verauthuid;
struct timeout s_verauthto;
};
@@ -75,10 +85,10 @@ void zapverauth(/* struct session */ voi
* One structure allocated per process group.
*/
struct pgrp {
- LIST_ENTRY(pgrp) pg_hash; /* Hash chain. */
- LIST_HEAD(, process) pg_members;/* Pointer to pgrp members. */
- struct session *pg_session;/* Pointer to session. */
- pid_t pg_id; /* Pgrp id. */
+ LIST_ENTRY(pgrp) pg_hash; /* [t] Hash chain. */
+ LIST_HEAD(, process) pg_members;/* [t] Pointer to pgrp members. */
+ struct session *pg_session;/* [I] Pointer to session. */
+ pid_t pg_id; /* [I] Pgrp id. */
int pg_jobc;/* # procs qualifying pgrp for job control */
};
@@ -156,17 +166,17 @@ struct process {
LIST_ENTRY(process) ps_list;/* List of all processes. */
TAILQ_HEAD(,proc) ps_threads; /* Threads in this process. */
- LIST_ENTRY(process) ps_pglist; /* List of processes in pgrp. */
- struct process *ps_pptr; /* Pointer to parent process. */
- LIST_ENTRY(process) ps_sibling; /* List of sibling processes. */
- LIST_HEAD(, process) ps_children;/* Pointer to list of children. */
+ LIST_ENTRY(process) ps_pglist; /* [t] List of processes in pgrp. */
+ struct process *ps_pptr; /* [t] Pointer to parent process. */
+ LIST_ENTRY(process) ps_sibling; /* [t] List of sibli
Re: proctreelk
On 14/10/17(Sat) 22:07, Philip Guenther wrote:
>
> The diff below adds proctreelk, an rwlock protecting the links of the
> process tree and related bits, as well as uidinfolk, an rwlock protecting
> the uidinfo hash table.
>
> Parts of this are based on FreeBSD's proctree_lock, particularly the
> reorganization of enterpgrp() into enternewpgrp() and enterthispgrp() and
> the splitting out of killjobc() from exit1().
>
> This diff should address the previously reported crashes seen when using
> ktrace(2) while creating/exiting processes.
>
> This has been stable for quite a while under my usage; please test and
> report any issues.
First of all, I'm very happy to see this diff. Thanks Philip.
I have been running this diff on my amd64 NFS client/server build
machine since you posted it. So far no issue, so it is stable for
this usage as well.
I'd however appreciate if you could commit the killjobc() and
enter*grp() refactoring first. Because in case of revert this
would be less pain.
That's also for this reason that I introduced a macro for the
NET_LOCK(). So I could enable/disable it without having to revert
N files. No idea if this could be useful there two.
I like the way you annotate the protected elements in the structure.
I'll try to do the same for bpf.
I'm also suggesting you commit the `uidinfolk' bits first. This seems
quite safe. In this exact part, what about introducing a uid_release(),
a wrapper around rw_exit_write(), to be called after uid_find()?
This way you can keep the lock local.
I have other questions, but I'll keep them for the upcoming smaller
diffs :)
> Index: sys/proc.h
> ===
> RCS file: /data/src/openbsd/src/sys/sys/proc.h,v
> retrieving revision 1.240
> diff -u -p -r1.240 proc.h
> --- sys/proc.h29 Aug 2017 02:51:27 - 1.240
> +++ sys/proc.h1 Sep 2017 05:48:15 -
> @@ -44,6 +44,7 @@
> #include /* For struct selinfo */
> #include/* For LOGIN_NAME_MAX */
> #include
> +#include /* For struct rwlock */
> #include /* For struct timeout */
> #include/* For struct klist */
> #include/* For struct mutex */
> @@ -55,16 +56,25 @@
> #endif
>
> /*
> + * Locks used to protect struct members in this file:
> + * I immutable after creation
> + * t proctreelk
> + *
> + * If multiple locks are listed then all are required for writes,
> + * but any one of them is sufficient for reads.
> + */
> +
> +/*
> * One structure allocated per session.
> */
> struct process;
> struct session {
> int s_count;/* Ref cnt; pgrps in session. */
> - struct process *s_leader; /* Session leader. */
> + struct process *s_leader; /* [t] Session leader. */
> struct vnode *s_ttyvp; /* Vnode of controlling terminal. */
> - struct tty *s_ttyp;/* Controlling terminal. */
> - chars_login[LOGIN_NAME_MAX];/* Setlogin() name. */
> - pid_t s_verauthppid;
> + struct tty *s_ttyp;/* [t] Controlling terminal. */
> + chars_login[LOGIN_NAME_MAX];/* setlogin() name. */
> + pid_t s_verauthppid; /* TIOCSETVERAUTH info */
> uid_t s_verauthuid;
> struct timeout s_verauthto;
> };
> @@ -75,10 +85,10 @@ void zapverauth(/* struct session */ voi
> * One structure allocated per process group.
> */
> struct pgrp {
> - LIST_ENTRY(pgrp) pg_hash; /* Hash chain. */
> - LIST_HEAD(, process) pg_members;/* Pointer to pgrp members. */
> - struct session *pg_session;/* Pointer to session. */
> - pid_t pg_id; /* Pgrp id. */
> + LIST_ENTRY(pgrp) pg_hash; /* [t] Hash chain. */
> + LIST_HEAD(, process) pg_members;/* [t] Pointer to pgrp members. */
> + struct session *pg_session;/* [I] Pointer to session. */
> + pid_t pg_id; /* [I] Pgrp id. */
> int pg_jobc;/* # procs qualifying pgrp for job control */
> };
>
> @@ -156,17 +166,17 @@ struct process {
> LIST_ENTRY(process) ps_list;/* List of all processes. */
> TAILQ_HEAD(,proc) ps_threads; /* Threads in this process. */
>
> - LIST_ENTRY(process) ps_pglist; /* List of processes in pgrp. */
> - struct process *ps_pptr; /* Pointer to parent process. */
> - LIST_ENTRY(process) ps_sibling; /* List of sibling processes. */
> - LIST_HEAD(, process) ps_children;/* Pointer to list of children. */
> + LIST_ENTRY(process) ps_pglist; /* [t] List of processes in pgrp. */
> + struct p
proctreelk
The diff below adds proctreelk, an rwlock protecting the links of the
process tree and related bits, as well as uidinfolk, an rwlock protecting
the uidinfo hash table.
Parts of this are based on FreeBSD's proctree_lock, particularly the
reorganization of enterpgrp() into enternewpgrp() and enterthispgrp() and
the splitting out of killjobc() from exit1().
This diff should address the previously reported crashes seen when using
ktrace(2) while creating/exiting processes.
This has been stable for quite a while under my usage; please test and
report any issues.
Philip Guenther
Index: sys/proc.h
===
RCS file: /data/src/openbsd/src/sys/sys/proc.h,v
retrieving revision 1.240
diff -u -p -r1.240 proc.h
--- sys/proc.h 29 Aug 2017 02:51:27 - 1.240
+++ sys/proc.h 1 Sep 2017 05:48:15 -
@@ -44,6 +44,7 @@
#include/* For struct selinfo */
#include /* For LOGIN_NAME_MAX */
#include
+#include /* For struct rwlock */
#include/* For struct timeout */
#include /* For struct klist */
#include /* For struct mutex */
@@ -55,16 +56,25 @@
#endif
/*
+ * Locks used to protect struct members in this file:
+ * I immutable after creation
+ * t proctreelk
+ *
+ * If multiple locks are listed then all are required for writes,
+ * but any one of them is sufficient for reads.
+ */
+
+/*
* One structure allocated per session.
*/
struct process;
struct session {
int s_count;/* Ref cnt; pgrps in session. */
- struct process *s_leader; /* Session leader. */
+ struct process *s_leader; /* [t] Session leader. */
struct vnode *s_ttyvp; /* Vnode of controlling terminal. */
- struct tty *s_ttyp;/* Controlling terminal. */
- chars_login[LOGIN_NAME_MAX];/* Setlogin() name. */
- pid_t s_verauthppid;
+ struct tty *s_ttyp;/* [t] Controlling terminal. */
+ chars_login[LOGIN_NAME_MAX];/* setlogin() name. */
+ pid_t s_verauthppid; /* TIOCSETVERAUTH info */
uid_t s_verauthuid;
struct timeout s_verauthto;
};
@@ -75,10 +85,10 @@ void zapverauth(/* struct session */ voi
* One structure allocated per process group.
*/
struct pgrp {
- LIST_ENTRY(pgrp) pg_hash; /* Hash chain. */
- LIST_HEAD(, process) pg_members;/* Pointer to pgrp members. */
- struct session *pg_session;/* Pointer to session. */
- pid_t pg_id; /* Pgrp id. */
+ LIST_ENTRY(pgrp) pg_hash; /* [t] Hash chain. */
+ LIST_HEAD(, process) pg_members;/* [t] Pointer to pgrp members. */
+ struct session *pg_session;/* [I] Pointer to session. */
+ pid_t pg_id; /* [I] Pgrp id. */
int pg_jobc;/* # procs qualifying pgrp for job control */
};
@@ -156,17 +166,17 @@ struct process {
LIST_ENTRY(process) ps_list;/* List of all processes. */
TAILQ_HEAD(,proc) ps_threads; /* Threads in this process. */
- LIST_ENTRY(process) ps_pglist; /* List of processes in pgrp. */
- struct process *ps_pptr; /* Pointer to parent process. */
- LIST_ENTRY(process) ps_sibling; /* List of sibling processes. */
- LIST_HEAD(, process) ps_children;/* Pointer to list of children. */
+ LIST_ENTRY(process) ps_pglist; /* [t] List of processes in pgrp. */
+ struct process *ps_pptr; /* [t] Pointer to parent process. */
+ LIST_ENTRY(process) ps_sibling; /* [t] List of sibling processes. */
+ LIST_HEAD(, process) ps_children;/* [t] Pointer to list of children. */
LIST_ENTRY(process) ps_hash;/* Hash chain. */
struct sigacts *ps_sigacts;/* Signal actions, state */
struct vnode *ps_textvp; /* Vnode of executable. */
struct filedesc *ps_fd;/* Ptr to open files structure */
struct vmspace *ps_vmspace;/* Address space */
- pid_t ps_pid; /* Process identifier. */
+ pid_t ps_pid; /* [I] Process identifier. */
/* The following fields are all zeroed upon creation in process_new. */
#defineps_startzerops_klist
@@ -180,7 +190,7 @@ struct process {
struct vnode *ps_tracevp; /* Trace to vnode. */
struct ucred *ps_tracecred;/* Creds for writing trace */
- pid_t ps_oppid; /* Save parent pid during ptrace. */
+ pid_t ps_oppid; /* [t] Save parent pid during ptrace. */
int ps_ptmask; /* Ptrace event mask */
struct ptrace_state *ps_ptstat;/* Ptrace state */
@@ -197,7 +207,7 @@ struct process {
/* The following fields are all copied upon creation in process_new. */
#defineps_startcopy
